Open
Conversation
## Summary Instead of bumping timeouts for slow tests in debug mode, skip them entirely. `HonkRecursionConstraintTest` and `ChonkRecursionConstraintTest` take 400-600s in debug builds, hitting the 600s default timeout. These are heavy recursion constraint tests that build full recursive circuits — the same code paths are already exercised (with debug assertions) by faster tests in the suite. This replaces the timeout-bump approach from PR #22347 with Adam's suggestion to be selective about what runs in debug mode. Supersedes #22427 ClaudeBox log: https://claudebox.work/s/c3fd56d2f55c879c?run=2
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
## Summary When an ACIR `AssertZero` opcode has no variables but a non-zero constant, the circuit is fundamentally unsatisfiable (`nonzero == 0` can never hold). Previously this hit a generic assertion about empty gates with no indication of the real problem. Added an early check in `assert_zero_to_quad_constraints` that detects this case and produces a clear error: "circuit is unsatisfiable. An AssertZero opcode contains no variables but has a non-zero constant, which can never equal zero." ## Test plan - All 60 existing quad/big-quad constraint tests pass - Built and ran `dsl_tests` successfully ClaudeBox log: https://claudebox.work/s/bcd66a8253a37704?run=1
…22461) ## Summary Add `lagrange_first * transcript_accumulator_not_empty = 0` subrelation to `ECCVMTranscriptRelation`. This is a prerequisite for #22334 (masking at top of ECCVM circuit). The audit in #22442 identified that when `lagrange_first` moves to row k (away from the PCS-enforced zero row), `transcript_accumulator_not_empty` is the only shiftable column where a malicious prover can potentially set a non-zero value at the `lagrange_first` row without any existing relation catching it. Setting it to 1 disables `INFINITY_ACC_X/Y`, allowing arbitrary accumulator coordinates to be injected. ## Changes - New subrelation `ACCUMULATOR_NOT_EMPTY_INIT` in `ecc_transcript_relation.hpp` (degree 2) - Gate count updates (+174 gates from the new subrelation): - `ECCVM_RECURSIVE_VERIFIER_GATE_COUNT`: 224336 → 224510 - `CHONK_RECURSION_GATES`: 1491593 → 1491767 ## Test plan - [x] `eccvm_tests` — all 44 tests pass - [x] `stdlib_eccvm_verifier_tests` — `SingleRecursiveVerification` passes with updated gate count - [x] `dsl_tests` — `GateCountChonkRecursion` passes with updated gate count Co-authored-by: notnotraju <raju@aztec-labs.com>
…on coverage (#22389) ## Summary Fixes nightly barretenberg debug build failure (CI run https://github.com/AztecProtocol/aztec-packages/actions/runs/24228497408). **Root cause:** `AvmRecursionInnerCircuitTests.Tampering` from `vm2_tests` timed out at 600s. CI log: http://ci.aztec-labs.com/ee4d306df4cbfd3c **Fix:** 1. Add `AvmRecursionInnerCircuitTests` to the debug build skip list (matching PR #22446 pattern) 2. Reinstate `HonkRecursionConstraintTestWithoutPredicate/1.GenerateVKFromConstraints` (241s, well within timeout) so the debug-only `native_verification_debug` code path in `honk_recursion_constraint.cpp:126` is still exercised **Debug assertion audit of all skipped suites:** - `HonkRecursionConstraintTest`: The only unique debug code is `native_verification_debug` (`#ifndef NDEBUG`, line 126) — a native side-verification sanity check. All tampering tests call `BB_DISABLE_ASSERTS()` so no debug assertions fire there. We keep `/1.GenerateVKFromConstraints` to cover this. - `ChonkRecursionConstraintTest`: All `BB_ASSERT_DEBUG` in `chonk.cpp` are disabled by `BB_DISABLE_ASSERTS()` in the test. `ChonkTests` tampering (separate binary, not skipped) still runs. - `AvmRecursionInnerCircuitTests`: Only `BB_ASSERT_LTE` (circuit size bounds) which is hit on the happy path by `AvmRecursiveTests`. ClaudeBox log: https://claudebox.work/s/7d8cbad767a56122?run=5
**finding 19: guard in `hash` for empty inputs** --- fixed 29e5bbb The guarantee documented in `pedersen.hpp` “hash output is never point at infinity” applies when the input size is greater than 0. When called on an empty input, `hash` was returning the x co-ordinate of point at infinity. This is addressed now by adding a `throw_or_abort` guard in `hash`to catch empty inputs. Tests are also included which verify that hashing an empty input throws an exception. **finding 2: guard to prevent integer underflow in `convert_buffer` on empty input** --- fixed fc40256 `convert_buffer` had an integer underflow when it was passed an empty input. This is addressed by including a `throw_or_abort` guard in `hash_buffer` before the call to `convert_buffer` is made so that an empty input cannot be sent to `convert_buffer`. Tests are also included which verify that hashing an empty input throws an exception. **finding 18: capping `hash_index` `bbapi` Pedersen handlers** --- not fixed `hash_index` is currently not capped. However, as pointed out, it is hardcoded to 0 in the current code and is not reachable. Hence, not fixing this.
… step of keccak (#22404) `create_small_range_constraint(var, range)` constrains `var` to `[0-range]`. To constrain `lo` and `hi`, which are base-11 digits, it was invoked with `range=BASE` where `BASE=11`. This constrained `lo` and `hi` incorrectly to `[0-11]` instead of `[0-10]`. This PR fixes this issue by calling `create_small_range_constraint` with `range=BASE-1` which correctly constrains `lo` and `hi` to `[0-10]`. --------- Co-authored-by: ledwards2225 <l.edwards.d@gmail.com>
**finding 7: inverted assertion in keccak fuzzer** --- fixed 4da6a65 The keccak fuzzer had an inverted assertion in the circuit checker. This has been fixed. **finding 10: delete dead code pertaining to `hash_field_element`** --- fixed 940f6e4 `hash_field_element` and `hash_field_elements` constitute dead code, which was missed to be removed in the internal audit. These have been deleted now.
**finding 11: change `key_array` to use correct element count** --- fixed dfc08b5 `key_array` over-allocated bytes and zero-initialized the additional bytes that are never used. This has been addressed by allocating only as many bytes as needed. **finding 13: input validation consistency between `blake3s` and `blake3s_constexpr`** --- fixed 97ec044 `barretenberg` does not support Blake3 with input lengths greater than 1024 bytes. This is enforced in `blake3s` via `BB_ASSERT_LTE(input.size, 1024)` and such a check is missing in `blake3s_constexpr`. The check was probably omitted because `BB_ASSERT_LTE` is runtime and not compile-time. This has been addressed via introducing a compile-time check. **finding 15: performance optimisation in `blake3_constraint.cpp`** --- fixed f9b13ab `barretenberg` does not support Blake3 with input lengths greater than 1024 bytes. The input size validation check happens after memory is allocated to the input, even for an invalid input. This has been addressed to first check that the input size is less than or equal to 1024 bytes, and then followed by the memory allocation for the input. **finding 22: blake2s increment_counter logic** --- fixed 25eccda `increment_counter` relied on `ranged_less_than<32>` over an untruncated field value to detect 32-bit carry and implicitly assumed the counter words were constants, making the logic fragile and potentially incorrect (for impractically large messages). This has been fixed by explicitly asserting that `t[0]` and `t[1]` are constants, and restoring the earlier version where the carry is computed using native 32-bit truncation, ensuring correct wrap-around semantics. Thanks for noting this.
| # | Finding | Resolution | |---|---------|------------| | 3, 12 | Msgpack NIL produces null `shared_ptr`, dereferenced in ACIR parsers | **Patched.** NIL rejection in deserializer helpers, plus tests | | 6 | `result_infinite` unconstrained in EC gadgets; prover could forge it | **Already fixed** in PR #21865 | | 9 | `assert_valid_variables` debug-only, heap-allocated a vector per call | **Patched.** Updated to use `initializer_list`/`span` to make cheap enough, always-on, (check in `get_variable` too expensive) | | 16 | Sparse witness map amplifies ~10 bytes into ~137 GB allocation | **Accepted risk.** No cap works without potentially rejecting valid inputs | | 20 | UB shift-by-64 in `get_wnaf_bits`; hash tests Ultra-only | **Patched.** Fixed UB, added Mega coverage to 4 test suites | | 21 | Clang under-aligns NTTP objects, UB in `barrett_reduction` | **Patched.** Root cause different than claimed in issue (see `static_assert` guards), Pointer NTTP refactor resolves UB per UBsan |
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
5 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: skip heavy recursion tests in debug builds (#22446)
fix: add clear error for unsatisfiable ACIR AssertZero opcode (#22417)
feat: enforce accumulator_not_empty = 0 at ECCVM lagrange_first row (#22461)
fix: skip heavy recursion tests in debug builds, keep one for assertion coverage (#22389)
fix: external audit fixes for Pedersen (#22434)
chore!: fix BASE off-by-one in create_small_range_constraint in theta step of keccak (#22404)
fix: external audit fixes for Keccak (#22436)
fix: external audit fixes for BLAKE (#22443)
chore: misc hash gadget updates (#22452)
END_COMMIT_OVERRIDE