feat: merge-train/barretenberg#23269
Open
AztecBot wants to merge 3 commits into
Open
Conversation
…g build (#23229) ## Why The nightly Barretenberg debug build (`Nightly Debug Build` workflow, run [25781836067](https://github.com/AztecProtocol/aztec-packages/actions/runs/25781836067)) on `next` @ `2b1d9cf22d` failed in `circuit_checker_tests`: ``` [ RUN ] UltraCircuitBuilderArithmetic.QArith3Gate C++ exception with description "Assertion failed: (block_selectors[idx].size() == nominal_size) Actual : 1 Expected: 2" thrown in the test body. [ FAILED ] UltraCircuitBuilderArithmetic.QArith3Gate ``` CI log: http://ci.aztec-labs.com/e0ba10c734d2dd7b The assertion fires inside `UltraCircuitBuilder_::check_selector_length_consistency` (`stdlib_circuit_builders/ultra_circuit_builder.hpp`), which is gated by `#if NDEBUG ... #else` and so runs only in debug builds — that's why no other CI mode caught it. `QArith3Gate` builds two arithmetic gates manually by pushing each selector individually. Every in-tree gate constructor (`create_arithmetic_gate`, `fix_witness`, `create_add_gate`, etc.) follows `q_4().emplace_back(...)` with `q_5().emplace_back(0)`; this test was the only manual-construction site that omitted the `q_5` push. Pre-merge-train, the consistency check used a per-flavour curated `get_selectors()` for `UltraTraceArithmeticBlock` that did not include `q_5`, so the omission was invisible. PR #23137 (`feat: merge-train/barretenberg`) refactored `ExecutionTraceBlock` so `get_selectors()` returns `non_gate_selectors` (7 entries including `q_5`) concatenated with `gate_selectors`, expanding the consistency check to cover `q_5`. The test has been silently broken w.r.t. the new check ever since that landed last night. ## What Add the missing `builder.blocks.arithmetic.q_5().emplace_back(0)` to both manually-constructed gates in `QArith3Gate`, matching the existing convention. ## Verification ``` cd barretenberg/cpp rm -rf build-debug AVM=0 AVM_TRANSPILER=0 cmake --preset debug -DAVM=OFF cmake --build build-debug --target circuit_checker_tests ./build-debug/bin/circuit_checker_tests --gtest_filter='UltraCircuitBuilderArithmetic.QArith3Gate' # -> [ OK ] UltraCircuitBuilderArithmetic.QArith3Gate ``` Full `circuit_checker_tests` (79 tests) and a sanity-run of `polynomials_tests`, `common_tests`, `relations_tests` all pass after the fix. Full analysis: https://gist.github.com/AztecBot/74d9233cda80ad6c3108d670a019afea ClaudeBox log: https://claudebox.work/s/3064b2e43c917621?run=1 ClaudeBox log: https://claudebox.work/s/3064b2e43c917621?run=1 Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com>
## Summary Addresses [findings](https://cantina.xyz/code/3dc4ffe5-40f6-4d08-926c-b17315a5bedb/findings) from the ecc/groups audit. ## Commits - **finding 1** — extend `batch_affine_double_impl` slope formula to include `a` for curves with `a ≠ 0` - **finding 2** — ~reject non-canonical x-coordinate encodings in `affine_element::from_compressed`~ (Opened its own [PR](#22908)) - **finding 3** — route ECDSA / Schnorr secret-nonce scalar mul through a new constant-time `element::mul_const_time` (Montgomery ladder + Coron blinding) - **finding 4** — handle `rhs = ∞` in mixed-coordinate `operator+=` - **finding 5** — zero `y` (and `z`) in `self_set_inf` so the infinity representation is canonical - **finding 6** — fix `pairing.FinalExponentiation` reference helper to iterate the full 256-bit exponent - **finding 7** — extend the G1/G2 SRS defenses from #22858 across the bb.js boundary: in `SrsInitSrs::execute`, SHA-256-verify every fully-present compressed-G1 chunk against `BN254_G1_CHUNK_HASHES` before decompression, pin `g1_points[0]` to the canonical generator and `g1_points[1]` to `τ·G` after parsing, and SHA-256-pin the 128-byte G2 input. The subgroup check, on-disk G2 hash pin, infinity rejection, and `is_in_prime_subgroup` implementation itself already landed in #22858. - **finding 8** — make `field::is_zero` constant-time ## Test plan - `ecc_tests` — pass (audit-related filter: 110 pass, 8 skipped, 0 failures) - `srs_tests` (`CrsFactory.*`) — pass - `bbapi_tests` — pass (30/30) - New regression tests in `affine_element.test.cpp`, `element.test.cpp`, `pairing.test.cpp`, `prime_field.test.cpp`
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
## Summary
Adds focused DSL tests for the `MultiScalarMul` ACIR opcode covering
Grumpkin scalars that lie **outside the Grumpkin scalar field** (i.e.
`>= bb::fq::modulus`), and writes up the resulting safety analysis. No
production code changes — this is test coverage plus a small refactor to
share the existing single-term MSM circuit helpers.
### What changed
`barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.test.cpp`:
- Extracted the single-term MSM circuit helpers (`push_point`,
`push_scalar`, `make_msm`, `run_circuit`, CRS setup) out of
`MultiScalarMulInfinityTests` into a shared base fixture
`MsmSingleTermFixture<Builder>`. `MultiScalarMulInfinityTests` now
derives from it; behaviour and test names are unchanged.
- New suite `MultiScalarMulScalarBoundsTests` (run for both
`UltraCircuitBuilder` and `MegaCircuitBuilder`), driving the opcode
through the normal ACIR → circuit path with `predicate = 1`:
| Test | Scalar value `v` | Expectation |
|---|---|---|
| `ScalarEqualToModulusFails` | `r` | circuit unsatisfiable |
| `ScalarModulusPlusOneFails` | `r + 1` | circuit unsatisfiable |
| `ScalarModulusMinusOneProves` | `r - 1` (largest in-field scalar) |
proves (baseline) |
| `MaxRepresentableScalarFails` | `2^254 - 1` (limbs in range, value `>
r`) | circuit unsatisfiable |
| `ScalarWithOversizedHiLimbFails` | `2^254` (`hi` limb `= 2^126`, 127
bits) | circuit unsatisfiable |
| `AddingGrumpkinModulusDoesNotReproveSameOutput` | `5` vs `5 + r` | `5`
proves the output; `5 + r` does not, even though `(5+r)·P = 5·P` |
Here `r == bb::fq::modulus` is the Grumpkin scalar field modulus, which
is also the order of the Grumpkin group (Grumpkin's scalar field is
BN254's base field). For the rejected cases the test claims the
mathematically-correct reduced result `(v mod r)·P`, so the rejection is
purely from the scalar-validity constraints, not a mismatched output.
## Test commands and results
```
cd barretenberg/cpp
cmake --preset default
cd build && ninja dsl_tests
./bin/dsl_tests --gtest_filter='*MultiScalarMulScalarBoundsTests*:*MultiScalarMulInfinityTests*'
```
Result: **18/18 passed** (6 new tests × 2 builders, plus the 6
pre-existing infinity tests confirming the refactor is
behaviour-preserving). For the rejected-scalar cases the circuit checker
reports `circuit contains invalid witnesses: field_t::range_constraint`
— the borrow-comparison range checks added by `cycle_scalar`'s
`validate_scalar_is_in_field`.
Running the full `*MultiScalarMul*` filter also surfaces 8 failures in
`MultiScalarMul{None,Points,Scalars,Both}Constant.GenerateVKFromConstraints`
— these are **pre-existing and unrelated**: they throw `bn254 g1 data
not found at /aztec-packages-private/.bb-crs` because the BN254 G1 CRS
is not downloaded in this environment (VK generation needs it;
`CircuitChecker`-based tests don't). They fail identically without this
change.
## Safety report: out-of-field Grumpkin scalars in the MSM opcode
### Background
The `MultiScalarMul` opcode (`dsl/acir_format/multi_scalar_mul.cpp`)
computes `sum(scalars[i] · points[i])` on Grumpkin. Each scalar is
supplied as two field-element limbs — `scalar_lo` (low 128 bits) and
`scalar_hi` (next 126 bits) — and reconstructed as `v = lo + hi ·
2^128`. The limb encoding can represent any value in `[0, 2^254)`, which
strictly contains `[0, r)` (`r ≈ 2^253.2`), so non-canonical scalars are
expressible.
`to_grumpkin_scalar` (`dsl/acir_format/witness_constant.cpp`) builds a
`cycle_scalar` from the two limbs via the public `cycle_scalar(lo, hi)`
constructor, which calls `validate_scalar_is_in_field()`. That adds
in-circuit constraints (via `validate_split_in_field_unsafe`, a
borrow-subtraction comparison) enforcing `v < r`. Separately,
`cycle_group::batch_mul` range-constrains the limbs to `lo < 2^128` and
`hi < 2^126` as part of the MSM algorithm — and
`validate_split_in_field_unsafe` relies on exactly those range
constraints being applied.
### Answers
1. **Is it impossible to prove a circuit which is passed an invalid
Grumpkin scalar?** Yes. Any scalar `v >= r` makes the circuit
unsatisfiable: `validate_scalar_is_in_field` is violated (its hi/lo
borrow range checks cannot all hold), so no satisfying witness exists —
neither an honest nor a dishonest prover can produce a valid proof. A
value `>= 2^254` additionally violates the `hi < 2^126` limb range
constraint applied by `batch_mul`. The check is sound because the limb
range constraints it depends on are applied unconditionally inside the
MSM. (When `predicate` is witness-false the scalar is first replaced by
the constant `1`, and during VK generation the limbs are overwritten
with a dummy in-field value — so neither path can smuggle an
out-of-field scalar past the check.)
2. **Can adding the Grumpkin modulus to a scalar prove the same
output?** No. Although `s·P = (s + r)·P` in the group, the circuit
enforces a canonical scalar `< r`, so `s + r` (which is `>= r`) is
rejected. There is no representable witness that both equals `s + r` and
passes `validate_scalar_is_in_field`. Caller-side malleability of the
scalar therefore cannot produce an alternate proof of an existing MSM
output. `AddingGrumpkinModulusDoesNotReproveSameOutput` demonstrates
both halves of this.
3. **Other edge cases / boundary values.** `r - 1` is the largest
accepted scalar and proves correctly; `r`, `r + 1`, and `2^254 - 1` are
all rejected — the last shows the limb range constraints alone aren't
enough and the explicit `< r` check does the work; `2^254` is rejected
by both the limb range constraint and the field check. `0` is accepted
and yields the point at infinity (already covered by the existing
`ResultIsInfinity` test).
### Conclusion
Allowing invalid Grumpkin scalars through to barretenberg is **safe** in
the soundness sense. If a caller (Noir, hand-written ACIR, or a
buggy/malicious frontend) hands the MSM opcode a scalar outside `[0,
r)`, the worst that happens is the circuit becomes unsatisfiable and
proving fails — fail-closed. There is no soundness gap: an out-of-field
scalar is not silently reduced into a different valid statement, and `s
+ r` cannot stand in for `s`. So canonicalising scalars does not have to
be done entirely on the caller side; barretenberg's in-circuit
`validate_scalar_is_in_field` is a real backstop. The only caveat is a
liveness one — a frontend that emits a non-canonical scalar will produce
circuits that cannot be proven, which is the desired behaviour.
---
*Created by
[claudebox](https://claudebox.work/v2/sessions/bebfc4a2ecc4b733) ·
group: `cli-startup-research`*
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.
BEGIN_COMMIT_OVERRIDE
fix(bb): push missing q_5 in QArith3Gate test to unbreak nightly debug build (#23229)
fix: ecc/groups audit findings (#22864)
chore: add tests around MSM invalid scalar handling (#23176)
END_COMMIT_OVERRIDE