Open
Conversation
#### L1: ACIR `num_bits` Validation
Status: won't fix
It is true that `create_logic_constraint` uses `BB_ASSERT` (which
aborts) rather than throwing an exception when `num_bits` is invalid.
`BB_ASSERT` is the standard pattern throughout barretenberg for
structural invariant checks. Additionally, the `num_bits` value
originates from Noir's `IntegerBitSize` enum, which restricts values to
{1, 8, 16, 32, 64, 128} at the compiler level, so invalid values cannot
reach this code through normal operation.
#### L2: OriginTag Provenance
Status: fixed 0f3ca1c
`create_logic_constraint` was not propagating `OriginTag` when
converting constant inputs to witnesses, or when returning the final
result. Tags are now copied onto the witness before recursing (for both
the left-constant and right-constant paths), and the final accumulated
result is tagged with the merged tag of both operands. Note we do not
propagate tags to the chunks because these are intermediate circuit
variables that are not used outside of logic.
#### L3: Builder Context Validation
Status: fixed 249bfdc
When both inputs are witnesses, the code extracted the builder context
from only one operand without verifying both refer to the same builder.
Added `validate_context<Builder>(a.get_context(), b.get_context())` to
assert consistency, matching the pattern used in other stdlib
primitives.
#### L4: Write-VK Mode Guard for Constant Inputs
Status: fixed 5b3d4b8
In write-VK mode, when both inputs are constants,
`create_logic_constraint` returns a constant `field_t`. The ACIR bridge
then attempts `assert_equal` against a result witness that holds a dummy
zero, causing a spurious failure. The fix uses `builder.set_variable()`
to patch the result witness with the correct computed value before the
equality check.
#### I1: Witness Bounds Checks Are Debug-Only
Status: acknowledged, will be fixed later
The witness index bounds checks in `get_variable()` use
`BB_ASSERT_DEBUG`, which is compiled out in release builds. This is
known and it affects every circuit component that calls
`get_variable()`, not just the logic gadget. We are discussing this the
noir team if it can be exploited in any meaningful way. Regardless, we
will enable these asserts in release mode soon after ensuring no
significant hit in performance because of the asserts.
#### I2: Oversized Plookup Table
Status: won't fix
The code always uses `UINT32_XOR`/`UINT32_AND` plookup tables even when
the last chunk is smaller than 32 bits. Smaller tables (UINT8, UINT16)
exist and would use fewer sub-lookups. However, this is a minor
optimization that would change the circuit structure, which we want to
avoid at this point. The existing explicit range constraint on the last
chunk ensures correctness regardless of table size.
#### I3: Operator Precedence Bug in Test
Status: fixed — 2a597fb
The test computed `1 << num_bits - 1` intending "all bits set" (`(1 <<
num_bits) - 1`), but C++ precedence evaluates it as `1 << (num_bits -
1)` (a single bit). For `num_bits = 8`, this gives `128` instead of
`255`. Corrected the test.
#### I4: Redundant Range Constraints
Status: won't fix
Yes there could be cases of redundant range constraints (on the noir
side as well as barretenberg) but we prefer to have redundancy over
missing a range constraint. This was a good find but we decided to not
use the optimisation.
## Summary - Remove incorrect `const` from the first parameter of the 5 `asm_self_*` field functions (mul, sqr, add, sub, reduce_once). These functions modify the parameter in-place via inline assembly but declared it `const field&`. ## Test plan - [x] `ecc_tests` — 836/836 passed Co-authored-by: notnotraju <raju@aztec-labs.com>
## Summary Addresses findings from the polynomials/ module security audit (AztecProtocol/barretenberg-claude#2383). - **F1** (Low): Harden file-backed polynomial memory: restrict temp file permissions to `0600`, add `O_EXCL` to prevent race conditions, use `MAP_PRIVATE` instead of `MAP_SHARED` - **F4** (Info): Guard `compute_linear_polynomial_product` against `n=0` to prevent underflow - **F5** (Info): Clarify `UnivariateCoefficientBasis` docstring to distinguish Karatsuba precomputation from polynomial coefficients **Already fixed upstream:** - **F2** (Low): `factor_roots` empty polynomial guard (in #22282) - **F3** (Info): `get_scratch_space` thread safety via mutex (in #22306)
Fixes the following from UB meta [issue](AztecProtocol/barretenberg-claude#2414): - F11 (zk_sumcheck_data.hpp) — Signed 1 << n is UB when n≥31; changed to 1UL << - F10 (rom_table.cpp, ram_table.cpp, twin_rom_table.cpp, databus.cpp) — OOB vector access after context->failure() soft fail; added early returns - F9 (field_declarations.hpp, field_impl_x64.hpp) — asm_self_* functions write through const& which is UB; changed to mutable ref
…22347) ## Summary Fixes nightly barretenberg debug build failure (CI run https://github.com/AztecProtocol/aztec-packages/actions/runs/24066248159). **Root cause:** `HonkRecursionConstraintTestWithoutPredicate/2.GenerateVKFromConstraints` (root rollup circuit, the heaviest test variant) timed out at 601s against the 600s default timeout. Exit code 124. **Fix:** - Bump timeout from 600s to 900s for `HonkRecursionConstraintTest` and `ChonkRecursionConstraintTest` suites - Add `ChonkRecursionConstraintTest` to the CPUS=4:MEM=8g resource group Several other tests in these suites are also close to the 600s limit (503s, 519s), so this prevents future timeouts as debug build overhead grows. **Note:** PR #22346 (from earlier session) is a no-op — the logderivative fix it applies already exists on `next` via merge-train/barretenberg (#22324). The actual failure was this timeout. ClaudeBox log: https://claudebox.work/s/5f7d3bb56d7d756f?run=1
Addresses missing origin tags identified in the meta issue [here](AztecProtocol/barretenberg-claude#2456). These were all about adding tagging that was missing. Some of them led to failures once proper tests were added, others aren't triggered in current usage.
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
fix: addresses logic audit findings (#22304)
chore: fix asm_self_* field arithmetic signatures (#22353)
fix: polynomials module audit response (#22361)
chore: UB updates (#22308)
fix: increase test timeout for heavy recursion tests in debug build (#22347)
chore: origin tag updates (#22307)
END_COMMIT_OVERRIDE