chore: responding to external IPA audit#20334
Merged
Merged
Conversation
…S__ instead of &&. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ck before compute_opening_proof. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…w in evaluate_challenge_poly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…index assignment instead. The vectors were constructed with size 2*log_poly_length+2 but emplace_back appended past the pre-allocated slots, leaving 2 default-zero entries that wasted circuit constraints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ative verifier behavior. batch_mul asserts strict size equality between points and scalars. The native verifier passes only poly_length elements to pippenger, but full_verify_recursive was passing all SRS points, which would abort if the VK contained more points than needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n accumulate docstring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
notnotraju
added
ci-full
notnotraju
commented
Feb 10, 2026
| msm_elements.emplace_back(-Commitment::one(builder)); | ||
| msm_scalars.emplace_back(a_zero); | ||
| msm_scalars.emplace_back(generator_challenge * a_zero.madd(b_zero, { -opening_claim.opening_pair.evaluation })); | ||
| msm_elements[(2 * log_poly_length)] = -G_zero; |
Contributor
Author
There was a problem hiding this comment.
we've only filled in the elements up to 2 * log_poly_length - 1, but we've allocated 2 * log_poly_length + 2, so emplace_back is not what we naively have in mind. (of course, default values are neutral, so the value of the MSM is the same, but wrongheaded nonetheless.)
notnotraju
commented
Feb 10, 2026
| const std::vector<Commitment> srs_elements = vk.get_monomial_points(); | ||
| // In the native verifier, this uses pippenger. Here we use batch_mul. | ||
| std::vector<Commitment> srs_elements = vk.get_monomial_points(); | ||
| BB_ASSERT_GTE(srs_elements.size(), poly_length, "Not enough SRS points for IPA!"); |
Contributor
Author
There was a problem hiding this comment.
allow for more srs elements than the poly_length, makes a bit less brittle.
notnotraju
commented
Feb 10, 2026
|
|
||
| IPA<NativeCurve, log_poly_length>::compute_opening_proof( | ||
| ck, { challenge_poly, opening_pair }, prover_transcript); | ||
| BB_ASSERT_EQ(challenge_poly.evaluate(bb::fq(output_claim.opening_pair.challenge.get_value())), |
Contributor
Author
There was a problem hiding this comment.
duplicated a few lines above.
Collaborator
Compile (Noir contracts)TypeScript validationAction required: Please fix the docs examples or update them to match the current API. cc @AztecProtocol/devrel |
suyash67
approved these changes
Feb 26, 2026
suyash67
left a comment
suyash67
left a comment
Contributor
There was a problem hiding this comment.
Thanks for such a neat PR, looks good!
added 5 commits
February 27, 2026 14:25
…erAccumulator (rather than accessing .verifier_accumulator)
…ztec-packages into rk/ipa-external-audit-1
…ztec-packages into rk/ipa-external-audit-1
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 12, 2026
BEGIN_COMMIT_OVERRIDE chore: unify tests in ecc/curves and ecc/groups (#21392) chore: `ecc/fields` ASM audit (#20892) chore: audit the `ecc/field` extension field (#21409) chore: responding to external IPA audit (#20334) feat: committed sumcheck for batched Hiding Translator (#21376) chore: documentation fixes for ECCVM after first external audit (#20348) chore: Document RAM gate cost optimization opportunity (#21426) refactor: rename MSGPACK_FIELDS to SERIALIZATION_FIELDS (#21175) END_COMMIT_OVERRIDE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses findings from the Veridise external audit of the IPA (Inner Product Argument) implementation. All changes are in
barretenberg/cpp/src/barretenberg/commitment_schemes/ipa/ipa.hppandbarretenberg/cpp/src/barretenberg/commitment_schemes_recursion/ipa_recursive.test.cpp.Commits
6b4582d— FixBB_ASSERTsyntax in IPA proverThe
BB_ASSERTincompute_opening_proofused&&to attach the error message string, which meant the string was part of the boolean expression (always truthy) rather than being passed as the diagnostic message via__VA_ARGS__. Changed&&to,so the message is correctly forwarded to the assertion handler.ecacc13— Remove duplicate assertion inaccumulate()A
BB_ASSERT_EQaftercompute_opening_proofrepeated the identical check (challenge_poly.evaluate(...) == evaluation) that already exists a few lines above. Removed the redundant copy.21e9b99— Addstatic_assertforlog_poly_length >= 1evaluate_challenge_polycomputeslog_poly_length - 1in an unsigned loop bound — iflog_poly_lengthwere 0 this would underflow. Added a compile-time guard. (The default valueCONST_ECCVM_LOG_Nis safe, but this protects against future misuse of the template parameter.)0422766— Fixemplace_backon pre-sized vectors in recursive verifierIn
reduce_verify_internal_recursive,msm_elementsandmsm_scalarsare constructed with size2 * log_poly_length + 2, but the last two entries were written viaemplace_back— appending beyond the pre-allocated size and wasting circuit constraints on the default-constructed elements. Replaced with direct index assignment at[2 * log_poly_length]and[2 * log_poly_length + 1].bbfb5de— Slice SRS elements infull_verify_recursiveThe native IPA verifier slices the SRS to
poly_lengthelements before the MSM, butfull_verify_recursivepassed the full SRS vector tobatch_mul, which assertsscalars.size() == base_points.size(). If the SRS ever has more points thanpoly_length, this would fail. AddedBB_ASSERT_GTE+resize(poly_length)to match native verifier behavior.dec6be7— Remove duplicate comment and inaccessible linkRemoved a duplicate comment block (lines 178-180 were copies of lines 175-177) in the IPA prover documentation. Also removed an inaccessible HackMD link from the
accumulatedocstring.f72bdd4— Fix return value ofreduce_verify_internal_recursiveThe return statement accessed
.verifier_accumulatoron the result, butreduce_verify_internal_recursivealready returns aVerifierAccumulatordirectly. Removed the erroneous member access.a6a3b92— Fix documentation of step 7 inreduce_verify_internal_recursiveCorrected the comment for the MSM computation to accurately reflect the formula: removed
C'and fixed the sign within the last parenthesis, matching the actual code logic.9e137aa— Add circuit satisfaction check toAccumulationAndFullRecursiveVerifiertestThe test only verified
running_truth_value(a witness boolean) but never checked that circuit constraints — including theclaimed_G_zero.assert_equal(computed_G_zero)constraint fromfull_verify_recursive— are actually satisfied. AddedEXPECT_TRUE(CircuitChecker::check(root_rollup)).