feat: merge-train/barretenberg#21406
Merged
Merged
Conversation
Templates tests for common ecc operations in `ecc/groups` instead of duplicated tests for each curve.
- Killed dead code - Changed a few names to be more explanatory - had many iterations with Claude to improve the documentation, which includes proofs that the bounds were correct etc. --------- Co-authored-by: notnotraju <raju@aztec-labs.com>
No real content. Co-authored-by: notnotraju <raju@aztec-labs.com>
## 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.hpp` and `barretenberg/cpp/src/barretenberg/commitment_schemes_recursion/ipa_recursive.test.cpp`. ## Commits ### `6b4582d` — Fix `BB_ASSERT` syntax in IPA prover The `BB_ASSERT` in `compute_opening_proof` used `&&` 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 in `accumulate()` A `BB_ASSERT_EQ` after `compute_opening_proof` repeated the identical check (`challenge_poly.evaluate(...) == evaluation`) that already exists a few lines above. Removed the redundant copy. ### `21e9b99` — Add `static_assert` for `log_poly_length >= 1` `evaluate_challenge_poly` computes `log_poly_length - 1` in an unsigned loop bound — if `log_poly_length` were 0 this would underflow. Added a compile-time guard. (The default value `CONST_ECCVM_LOG_N` is safe, but this protects against future misuse of the template parameter.) ### `0422766` — Fix `emplace_back` on pre-sized vectors in recursive verifier In `reduce_verify_internal_recursive`, `msm_elements` and `msm_scalars` are constructed with size `2 * log_poly_length + 2`, but the last two entries were written via `emplace_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 in `full_verify_recursive` The native IPA verifier slices the SRS to `poly_length` elements before the MSM, but `full_verify_recursive` passed the full SRS vector to `batch_mul`, which asserts `scalars.size() == base_points.size()`. If the SRS ever has more points than `poly_length`, this would fail. Added `BB_ASSERT_GTE` + `resize(poly_length)` to match native verifier behavior. ### `dec6be7` — Remove duplicate comment and inaccessible link Removed 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 `accumulate` docstring. ### `f72bdd4` — Fix return value of `reduce_verify_internal_recursive` The return statement accessed `.verifier_accumulator` on the result, but `reduce_verify_internal_recursive` already returns a `VerifierAccumulator` directly. Removed the erroneous member access. ### `a6a3b92` — Fix documentation of step 7 in `reduce_verify_internal_recursive` Corrected 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 to `AccumulationAndFullRecursiveVerifier` test The test only verified `running_truth_value` (a witness boolean) but never checked that circuit constraints — including the `claimed_G_zero.assert_equal(computed_G_zero)` constraint from `full_verify_recursive` — are actually satisfied. Added `EXPECT_TRUE(CircuitChecker::check(root_rollup))`. --------- Co-authored-by: notnotraju <raju@aztec-labs.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This PR switches the batched hiding Translator to use committed sumcheck. --------- Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: iakovenkos <sergey.s.yakovenko@gmail.com>
One edit per commit. --------- Co-authored-by: notnotraju <raju@aztec-labs.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Adds a comment above `process_RAM_array` documenting the current ~3.25 gate cost per RAM interaction and the potential optimization to reduce it to ~2.25 gates by deferring memory gate creation to circuit finalisation and embedding the timestamp relation directly. Just documenting [522](AztecProtocol/barretenberg#522) Co-authored-by: notnotraju <raju@aztec-labs.com>
Renames the `MSGPACK_FIELDS` macro to `SERIALIZATION_FIELDS` throughout the barretenberg codebase and the Noir ACIR codegen, since the macro is also used by `read()` and `write()` serialization — not just msgpack. 50 C++ files updated (306 replacements) plus corresponding references in the Noir ACIR C++ codegen (`acvm-repo/acir/src/lib.rs`). ClaudeBox log: http://ci.aztec-labs.com/23152dd90800b8e9-1
johnathan79717
requested review from
IlyasRidhuan,
fcarreiro and
jeanmon
as code owners
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
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
6 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.
BEGIN_COMMIT_OVERRIDE
chore: unify tests in ecc/curves and ecc/groups (#21392)
chore:
ecc/fieldsASM audit (#20892)chore: audit the
ecc/fieldextension 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