chore: bigfield audit fixes that change circuits#15205
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the bigfield implementation to simplify conditional operations and centralize limb-based multiplication logic, and updates related tests and scripts to reflect changes in circuit gates and external inputs.
- Simplified
conditional_negateandconditional_selectby leveragingconditional_assign - Introduced
compute_partial_schoolbook_multiplicationhelper and replaced manual max‐value computations - Updated test expectations for gate counts and connected‐component sizes, and bumped the pinned CIVC inputs URL
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| barretenberg/.../bigfield_impl.hpp | Refactor conditional_negate/conditional_select and optimize max computations |
| barretenberg/.../bigfield.hpp | Declare compute_partial_schoolbook_multiplication, switch C arrays to std::array, add helper getters |
| barretenberg/.../ultra_recursive_verifier.test.cpp | Update expected gate count from 874952 to 869846 |
| barretenberg/.../graph_description_ultra_recursive_verifier.test.cpp | Update expected connected‐components count from 4 to 3 |
| barretenberg/.../test_civc_standalone_vks_havent_changed.sh | Update pinned_civc_inputs_url to new checksum |
Comments suppressed due to low confidence (1)
barretenberg/cpp/src/barretenberg/stdlib/primitives/bigfield/bigfield.hpp:1044
- The
@returnannotation saysstd::array<field_t<Builder>, NUM_LIMBS>but the method actually returnsstd::array<uint256_t, NUM_LIMBS>. Please update the doc to match the signature.
* @return std::array<field_t<Builder>, NUM_LIMBS> An array containing the maximum values of the binary basis limbs.
a540ecc to
0f8c4db
Compare
| const auto& x_limb = binary_basis_limbs[i].element; | ||
| const auto& y_limb = other.binary_basis_limbs[i].element; | ||
|
|
||
| x_scaled[i] = { x_limb.witness_index, x_limb.multiplicative_constant }; |
There was a problem hiding this comment.
I have always wondered how we can achieve the situation of a multiplicative constant of any limb being !=1 while the prime limb's is
There was a problem hiding this comment.
Well I agree with you that its highely unlikely that multiplicative_constant of prime_limb is 1 but binary limbs is not 1. I think we will always have either all multiplicative_constants either 1 or ≠1.
I ran all stdlib_primitives_tests to check if this condition is triggered when any of the binary limb's mult const ≠ 1 but both prime limbs mult const = 1. Interestingly, for operator+ this never happens, i.e., all limbs mult const is 1 whenever we enter the if condition.
For operator- though, it does happen for secp256k1 scalar mul methods. We see:
this: limb 0: 0x0000000000000000000000000000000000000000000000000000000000000001
this: limb 1: 0x0000000000000000000000000000000000000000000000000000000000000002
this: limb 2: 0x0000000000000000000000000000000000000000000000000000000000000002
this: limb 3: 0x0000000000000000000000000000000000000000000000000000000000000002
other: limb 0: 0x0000000000000000000000000000000000000000000000000000000000000001
other: limb 1: 0x0000000000000000000000000000000000000000000000000000000000000001
other: limb 2: 0x0000000000000000000000000000000000000000000000000000000000000001
other: limb 3: 0x0000000000000000000000000000000000000000000000000000000000000001
which is very weird. Lets chat tomorrow about this.
| // predicate == 0 ==> lhs | ||
| // predicate == 1 ==> rhs | ||
| // | ||
| field_ct binary_limb_0 = |
There was a problem hiding this comment.
Did the drop in the number of gates in the rollup happen just because of this?
There was a problem hiding this comment.
Yes. The gate count was different for the case when predicate is circuit-constant and this, other are witnesses/constant.
| Predicate | This | Other | Gate Count (Before) | Gate Count (After) |
|---|---|---|---|---|
| constant | constant | constant | 0 | 0 |
| constant | witness | constant | ||
| constant | constant | witness | 0 | 0 |
| constant | witness | witness | ||
| witness | constant | constant | 5 | 5 |
| witness | witness | constant | 5 | 5 |
| witness | constant | witness | 5 | 5 |
| witness | witness | witness | 10 | 10 |
This difference was because previously we were manually computing the constraint as:
field_t(predicate) * (other[i] - this[i]) + this[i]
which indicates at least 5 field multiplications (hence at least 5 gates).
However, field_t::conditional_assign does not add any constraints if predicate is circuit-constant. That's why in the new version, gates added are 0 whenever predicate is circuit-constant.
…assign in bigfield::conditional_select.
…stant checks in a condition.
…ints irrespective of odd and even.
… products. avoids code duplication and fixes a bug with max value calc of product q * p' in unsafe_evaluate_multiple_multiply_add.
get rid of temp NOTEs.
d574f32 to
0940774
Compare
See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). chore: update merge-train documentation feat!: (sequence of) Apps share a transcript until a kernel is encountered (#15313) chore: bigfield todo fixes (#15202) chore: remove insecure logic from ipa recursion (#14102) chore: bigfield audit fixes that change circuits (#15205) feat: stdlib::Proof (#15410) fix: build bb for bench_ivc script (#15429) chore: revert "feat: stdlib::Proof (#15410)" (#15431) chore: unrevert stdlib proof (#15443) chore: package vk hash with vk (#15318) chore: fix avm build (#15448) chore: skip building wasm for bench_ivc when NO_WASM=1 (#15462) chore: add a link to audit PR template in default PR template (#15463) --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: ludamad <adam.domurad@gmail.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.com> Co-authored-by: Lucas Xia <lucasxia01@gmail.com>
See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). chore: update merge-train documentation feat!: (sequence of) Apps share a transcript until a kernel is encountered (AztecProtocol#15313) chore: bigfield todo fixes (AztecProtocol#15202) chore: remove insecure logic from ipa recursion (AztecProtocol#14102) chore: bigfield audit fixes that change circuits (AztecProtocol#15205) feat: stdlib::Proof (AztecProtocol#15410) fix: build bb for bench_ivc script (AztecProtocol#15429) chore: revert "feat: stdlib::Proof (AztecProtocol#15410)" (AztecProtocol#15431) chore: unrevert stdlib proof (AztecProtocol#15443) chore: package vk hash with vk (AztecProtocol#15318) chore: fix avm build (AztecProtocol#15448) chore: skip building wasm for bench_ivc when NO_WASM=1 (AztecProtocol#15462) chore: add a link to audit PR template in default PR template (AztecProtocol#15463) --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: ludamad <adam.domurad@gmail.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.com> Co-authored-by: Lucas Xia <lucasxia01@gmail.com>
Final bigfield audit PR with a few circuit changes.
Modifications that don't change circuits:
MAXIMUM_LIMB_SIZE_THAT_WOULDNT_OVERFLOW = 86from87(explained in the code-comment). This only reduces our safety-margin slightly, but we're already way below this so does not affect anything.compute_partial_schoolbook_multiplicationto compute native limb multiplication with documentationModifications that change circuits:
bigfield::conditional_negatelogic by just usingbigfield::conditional_assignwithout adding new gates. Note: Custom logic ofbigfield::conditional_negatehad a bug in max value calculations.bigfield::conditional_selectusingfield::conditional_assigninstead of manually usingfield_t::maddand subtraction.resolves #14657 #14656 #14661
resolves AztecProtocol/barretenberg#660
resolves #15091
resolves #15088