Add faster STARK configuration for testing purposes #739
Conversation
github-actions
bot
added
the
crate: evm_arithmetization
github-actions
bot
added
the
crate: zero_bin
sai-deng
requested review from
muursh,
Nashtare,
atanmarko,
wborgeaud and
LindaGuiga
as code owners
This reverts commit d098b24.
| // When using test configurations, the block circuit's degree is less than the | ||
| // 2-to-1 circuit's degree. Therefore, we also need to ensure its size meets | ||
| // the 2-to-1 circuit's recursion threshold degree bits. | ||
| while log2_ceil(builder.num_gates()) < TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS { |
There was a problem hiding this comment.
What if we want to test another circuit that doesn't require the same number of degree bits? I don't think TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS should be constant, because even though now, we're only using this to test the 2-to-1 circuit in the CI, we should still be able to use this for other circuits that might not have the same size...
Also, I'm actually surprised TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS is greater than common.degree_bits(). Do you know why?
There was a problem hiding this comment.
TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS is the smallest degree bits we can get with a low-security STARK config for the 2-1-block circuit. We will not use this value for other circuits.
We need it in the block wrapper circuit because we need to match its degree with the 2-1-block circuit’s degree, but we build this circuit before the 2-1-block circuit so we need to know this value in advance.
There was a problem hiding this comment.
But why does the common circuit data not contain a high enough degree then? Shouldn't the common data and the circuit we're building agree on the degree_bits?
Also, if we keep this, maybe we should have something like:
while log2_ceil(builder.num_gates()) < max(TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS, block.circuit.common.degree_bits())
to not have two different while loops doing the same thing?
There was a problem hiding this comment.
The previous check log2_ceil(builder.num_gates()) < block.circuit.common.degree_bits() is a hack, as described in the comments.
The degree_bits is needed for this circuit is determined in the next layer of recursion, while the common data is from the last layer of recursion. Therefore, they may not align. We could use a trial-and-error loop to find this number, but it would be much slower and seems excessive for now.
There was a problem hiding this comment.
I’ve completed the refactoring to keep only one while. PTAL
|
|
||
| let all_circuits = AllRecursiveCircuits::new( | ||
| &all_stark, | ||
| &[16..17, 8..9, 12..13, 8..9, 8..9, 6..7, 17..18, 16..17, 7..8], | ||
| &config, | ||
| Some(&TEST_RECURSION_CONFIG), | ||
| Some(&TEST_RECURSION_CONFIG), | ||
| None, |
There was a problem hiding this comment.
Why isn't it TWO_TO_ONE_BLOCK_CIRCUIT_TEST_THRESHOLD_DEGREE_BITS?
There was a problem hiding this comment.
This is only for 2-to-1 block circuit's threshold.
There was a problem hiding this comment.
Yes, we should use TEST_THRESHOLD_DEGREE_BITS. Thanks for the catch!
There was a problem hiding this comment.
Now, we have two_to_one_block aggregation test passing in 38 seconds on my MacBook. We can confidently remove [ignored] from it now.
| rate_bits: 3, | ||
| cap_height: 4, |
There was a problem hiding this comment.
non-blocking, but is this the fastest test setup? I think increasing the cap size would just increase proof size, and reduce recursive verification? (though this may not matter much as you're adding a shrinking layer by default if the threshold is too small when testing, just wondering)
There was a problem hiding this comment.
I did not increase the cap_height; it was 4 before.
| shrinking_circuit_config: Option<&CircuitConfig>, | ||
| recursion_circuit_config: Option<&CircuitConfig>, | ||
| threshold_degree_bits: Option<usize>, |
There was a problem hiding this comment.
nit: quid of grouping this into a RecursionParams struct or similar?
There was a problem hiding this comment.
Sure, I will work on it in a separate PR.
#676 Enables fast STARK configurations in integration tests. Achieved up to 82% test time savings. Detailed results are provided below.
Next, I will work on enabling it in Zero Bin.
With the big test time savings for the
two_to_one_block_aggregationtest, I have removed[ignore]from it.