refactor(storage-proofs): make base and expansion parents compile time constants

[dignifiedquire]

This is the first step to pulling in optimizations from r2.

Important changes:

• All graphs are now using the degrees 5 + 8, including all tests and exposed sector sizes.
• degrees are no inputs anymore for the circuits, need to validate this is okay
• This will require new params
• Adds compile-time feature allow-small-degree which allows graphs to be constructed with base and expansion degrees smaller than 5 and 8 respectively. This feature is enabled in the CI for the ignored storage-proofs tests.

[porcuquine]

Personally, I think these should still be public parameters. Just because we want to make them constants as a compile-time optimization doesn't mean that has to change. I would rather see them remain public parameters with a run-time check that the compiled constants agree with the public parameters.

In general, I'm okay with these optimizations, but it would be better to make them in a way which preserves the flexible structure of the proofs, wherever possible. Doing as I suggest here would make clear that making these global constants is an optimization. This will allow public parameters to retain their status as the the sole source of theoretically adjustable parameters to the proof construction.

[schomatis]

+1 to that, not very impactful to my work but i usually reduce the -m and --expansion parameters to speed up certain memory benchmarks and also to test the impact of things like the parents cache

[dignifiedquire]

+1 to that, not very impactful to my work but i usually reduce the -m and --expansion parameters to speed up certain memory benchmarks and also to test the impact of things like the parents cache

This will not be possible if we want to get the performance improvements in from r2 (except for changing the compile time constants). What @porcuquine is suggesting, simply enforces recording the values in the circuits

[dignifiedquire]

At least that is how I understand the comment. Anything else is not really possible, if we want to get the improved hashing and use arrays.

[porcuquine]

@dignifiedquire Not just in the circuits, but as public parameters for the proofs generally, including circuits.

@schomatis Your use case could perhaps be accomplished by creating a compile-time feature flag for reduced/fast parameters in development mode.

[dignifiedquire]

@porcuquine I disagree with that, this makes dev life harder, without providing real benefits.

…

On 27. Aug 2019, 00:45 +0200, porcuquine ***@***.***>, wrote: @dignifiedquire Not just in the circuits, but as public parameters for the proofs generally, including circuits. @schomatis Your use case could perhaps be accomplished by creating a compile-time feature flag for reduced/fast parameters in development mode. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[porcuquine]

I don't think having well-defined public parameters distinct from constants in the code is 'without real benefits'.

How does this make devs life harder?

[schomatis]

Your use case could perhaps be accomplished by creating a compile-time feature flag for reduced/fast parameters in development mode.

understood, then +1 to this

[dignifiedquire]

@porcuquine we need to be able to write things like [u64; PARENTS_SIZE], in order to make this change worthwile. So while we can keep some paramters around, they wouldn't be actually used, and be just for show.

To make sure we can change things we should have a second set of constants for tests and debugging, which can be switched to by using a feature flag.

[porcuquine]

@dignifiedquire I understand what you are trying to accomplish and why. I accept that this may be the easiest way to do so for now, also.

I don't agree that retaining the public parameters as such would be 'just for show', though. I believe that these parameters have semantic meaning at the protocol level, and that they should not be changed as the side effect of an optimization we want to perform.

Having well-defined parameters will help with connecting implementation, specification, and ubercalc.

It's also not clear to me that we need to remove the parameters from the circuits. This makes them much less useful as proof-construction building blocks or as reference implementations. Retaining the existing parameter structure would make it easier to retain them and make this a smaller change set. Do you believe that converting to constants in the circuits provides a significant performance benefit when synthesizing them? Because it does come at a price.

[dignifiedquire]

alright, I talked with @porcuquine about this and we have come to the following agreement:

• revert changes such that circuits do not change and the params stay the same
• add asserts that the params in the circuits, that they match the constants
• change the vanilla params such that they only can be created with the values of the constants, and assert that they match as part of proofing and verifying

[DrPeterVanNostrand]

@dignifiedquire ok, sounds good

[schomatis]

Can the default values of the zigzag example also draw from theses constants?

rust-fil-proofs/filecoin-proofs/examples/zigzag.rs

Lines 407 to 419 in d07e03a

	.arg(
	Arg::with_name("m")
	.help("The size of m")
	.long("m")
	.default_value("5")
	.takes_value(true),
	)
	.arg(
	Arg::with_name("exp")
	.help("Expansion degree")
	.long("expansion")
	.default_value("8")
	.takes_value(true),

[DrPeterVanNostrand]

@schomatis yup, np

The requested changes are out of date with the PR (but the requested changes have been made)

[dignifiedquire]

we should validate this in the base graph as well

[DrPeterVanNostrand]

@dignifiedquire ok, I added degree validation to ZigZagBucketGraph's and BucketGraph's constructors and changed all the places in the tests where BucketGraph was being instantiated with a degree not equal to BASE_DEGREE.

[dignifiedquire]

lgtm,