fix: minor fixes pt. 4#22282
Conversation
| } | ||
| BB_ASSERT_EQ( | ||
| G_zero, data.G_zero_from_prover, "G_0 should be equal to G_0 sent in transcript. IPA verification fails."); | ||
| if (G_zero != data.G_zero_from_prover) { |
There was a problem hiding this comment.
Don't see any reason to panic here
There was a problem hiding this comment.
hmm yeah this does seem funny
| claimed_G_zero.assert_equal(computed_G_zero); | ||
| BB_ASSERT_EQ(computed_G_zero.get_value(), claimed_G_zero.get_value(), "G_zero doesn't match received G_zero."); | ||
| // The circuit constraint enforces correctness; mismatched witnesses will produce an unsatisfiable circuit. | ||
| claimed_G_zero.assert_equal(computed_G_zero, "G_zero doesn't match received G_zero."); |
There was a problem hiding this comment.
same as here - i think this assert was disruptive for circuit construction
| ProverEqPolynomial<FF>::construct(accumulator_claim.challenge, bb::numeric::get_msb(circuit_size)); | ||
| polynomials.eq_instance = | ||
| ProverEqPolynomial<FF>::construct(instance_claim.challenge, bb::numeric::get_msb(circuit_size)); | ||
| // Construct eq polynomials with 2^m coefficients (m = log2(circuit_size)), not 2^VIRTUAL_LOG_N. |
There was a problem hiding this comment.
It's efficient and correct, but the implementation is somewhat convoluted
| GTEST_SKIP() << "Native-only test"; | ||
| return; | ||
| } | ||
| if constexpr (!IsRecursive) { |
There was a problem hiding this comment.
checking that an honest prover can prove a merge with shift size 0 - i got puzzled why we have an assert on the verifier side
| } | ||
|
|
||
| // The domain separation IV depends on the input size, therefore, the hashes must not coincide. | ||
| EXPECT_NE(hashes[0], hashes[1]); |
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
LGTM but want to get @federicobarbacovi to weigh in on the merge assert. Seems potentially valuable to keep if it alerts us to unexpected behavior. On the other hand, if we don't need it anymore, should we remove the dummy ops in apps?
| } | ||
| BB_ASSERT_EQ( | ||
| G_zero, data.G_zero_from_prover, "G_0 should be equal to G_0 sent in transcript. IPA verification fails."); | ||
| if (G_zero != data.G_zero_from_prover) { |
There was a problem hiding this comment.
hmm yeah this does seem funny
| // For stdlib: shift_size is FF (we'll get the value later) | ||
| const FF shift_size = transcript->template receive_from_prover<FF>("shift_size"); | ||
| ; | ||
| if constexpr (IsRecursive) { |
There was a problem hiding this comment.
I'm guessing that these checks were intended to flag unexpected behavior rather than invalid behavior. It used to be the case that we always added some ops to everything - I thought we got rid of that but then I would think this would've triggered. Maybe @federicobarbacovi can weigh in here?
There was a problem hiding this comment.
If shift size is zero, proving used to fail because Shplonk tries to access the zero coefficient of the polynomials it's proving, see here.
We are not using Shplonk directly in the merge anymore, so I don't know whether proving would still fail with shift size equal to zero.
Re whether we always add something: yes, we do add ecc ops to each builder during finalisation, see here. At the same time, the first merge in Chonk happens against an empty table. The reason why the first merge doesn't trigger this assertion is that the shift size is computed for the left table. As the first merge is in prepend mode, the left table is the table coming from the first app, which is never zero because of the ops we add.
|
|
||
| // === Verify with unmodified verifier (which has BB_ASSERT_GT for shift_size > 0) === | ||
| // Disable the assert so we can test the math | ||
| BB_DISABLE_ASSERTS(); |
There was a problem hiding this comment.
is this disable needed if we remove the assert on shift size 0?
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
Approving so as to unblock the PR since Federico weighed in. I wonder if we should keep the test demonstrating validity but also keep the ASSERT? One reason we might not want to remove dummy ops from apps even if merge allows it is to avoid commitments that are infinity.
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
4 participants
another batch of claude issues
removed dummy ops from all mega circuits -- allows to bump up the max chonk capacity to 18 circuits (it's really close to 19, ~100 msm rows above 2^15)