Commit b5f7017
authored
fix: addresses logic audit findings (#22304)
#### 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.1 parent ba14c17 commit b5f7017
File tree
4 files changed
+76
-3
lines changed- barretenberg/cpp/src/barretenberg
- dsl/acir_format
- stdlib/primitives/logic
4 files changed
+76
-3
lines changedLines changed: 9 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
26 | 35 | | |
27 | 36 | | |
28 | 37 | | |
| |||
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
73 | 73 | | |
74 | 74 | | |
75 | 75 | | |
76 | | - | |
77 | | - | |
| 76 | + | |
| 77 | + | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| |||
Lines changed: 4 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
| 61 | + | |
61 | 62 | | |
62 | 63 | | |
63 | 64 | | |
64 | 65 | | |
65 | 66 | | |
66 | 67 | | |
67 | 68 | | |
| 69 | + | |
68 | 70 | | |
69 | 71 | | |
70 | 72 | | |
71 | | - | |
| 73 | + | |
72 | 74 | | |
73 | 75 | | |
74 | 76 | | |
| |||
111 | 113 | | |
112 | 114 | | |
113 | 115 | | |
| 116 | + | |
114 | 117 | | |
115 | 118 | | |
116 | 119 | | |
| |||
Lines changed: 61 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
143 | 143 | | |
144 | 144 | | |
145 | 145 | | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
0 commit comments