Commit 2166a06
authored
fix: bigfield veridise audit fixes (#16842)
Summary of the fixes based on the audit of the `bigfield` class by
Veridise (on commit
[480f49d](https://github.com/AztecProtocol/aztec-packages/tree/480f49dad314ea4e753ff1e5180992a16926d2b3)):
#### [V-AZT-VUL-001: Missing zero-check for division by constant]()
Added an assertion that triggers when the divisor is a constant zero and
the flag `check_for_zero` is set to true. Also added a test that
validates this behaviour.
Response: Fixed in
bbf2f73
#### [V-AZT-VUL-002: Incomplete ‘assert_is_not_equal‘ assumes random
distribution]()
The function `assert_is_not_equal` is no longer exposed as a public
function (via noir). In fact, none of the non-native field methods (and
group operations) are exposed to developers via noir. Noir instead uses
the natively written
[noir-bignum](https://github.com/noir-lang/noir-bignum) library. (The
`bigint_constraint.cpp` file that contains the interface for non-native
field methods between noir and barretenberg is yet to be removed from
the repository, this will be done in an upcoming PR.)
Response: Acknowledged
#### [V-AZT-VUL-003: borrow_lo rounds down]()
Updated the computation of `borrow_lo_value` to use ceiling instead of
floor:
```cpp
uint256_t borrow_lo_value = (max_remainders_lo + ((uint256_t(1) << (2 * NUM_LIMB_BITS)) - 1)) >> (2 * NUM_LIMB_BITS);
```
Response: Fixed in
c1bdd88
#### [V-AZT-VUL-004: Unsafe default for `msub_div()`]()
Changed the default value of `enable_divisor_nz_check` to `true` and
explicitly pass it as `false` in its usage in the `biggroup` class. In a
few instances, passing this parameter as `false` can lead to incorrect
handling of the point at infinity, added TODOs at such places. These
instances will be analysed separately to avoid any unexpected behaviour.
Response: Fixed in
25e820c,
usage of `msub_div` will be analysed separately
#### [V-AZT-VUL-005: Limbs with a maximum value of 0 trigger compilation
failure]()
In the function `decompose_into_default_range` we have an assertion:
```cpp
ASSERT(num_bits > 0)
```
because number of bits (of a circuit variable) being 0 does not really
make sense. The only case when `carry_hi_msb` or `carry_lo_msb` can be 0
is when:
```cpp
template <typename Builder, typename T>
void bigfield<Builder, T>::unsafe_evaluate_multiply_add(const bigfield& input_left,
const bigfield& input_to_mul,
const std::vector<bigfield>& to_add,
const bigfield& input_quotient,
const std::vector<bigfield>& input_remainders)
```
is called with `input_to_mul` as constant 0 and/or `input_quotient` as
constant 0. In the usage of the function `unsafe_evaluate_multiply_add`
(or `unsafe_evaluate_multiple_multiply_add`) the `input_quotient` is
always a circuit witness. This ensures that the `carry_lo_msb > 0` and
`carry_hi_msb > 0` (added assertions to verify this empirically).
Response: We think it does not make sense to support `num_bits = 0` in
range constraint functions, but open to discussion on this if there's
any way `carry_lo_msb` or `carry_hi_msb` can actually be 0 in practice.
#### [V-AZT-VUL-006: Off-by-one comparison of
maximum_unreduced_limb_value]()
Changed the definition of `get_maximum_unreduced_limb_value()` to $2^Q -
1$ (from $2^Q$).
Response: Fixed in
3da35ba
#### [V-AZT-VUL-007: Maintainability Improvements]()
1. Removed unused constants:
- `bigfield.prime_basis_maximum_limb`
- `bigfield.shift_right_1`
- `bigfield.shift_right_2`
2. Renamed `negative_prime_modulus_mod_binary_basis` to
`negative_prime_modulus_mod_native_basis` and use that while checking
the prime limb consistency (in `evaluate_polynomial_identity`)
3. Used `NUM_LIMBS` instead of hard-coded 4.
4. Fixed comments (using $L$ instead of $t$)
5. Changed the definition of `get_maximum_unreduced_value()` to a form
$(2^N - 1)$ and use the comparison operators correctly
6. Fixed `static constexpr uint64_t MAX_UNREDUCED_LIMB_BITS =
NUM_LIMB_BITS + MAX_ADDITION_LOG`
7. Got rid of variable `neg_prime`
8. Used `is_constant()` instead of iterating over each limb to check
constant-ness in `operator+` and `operator-`.
9. Removed duplicate/redundant assertion: `ASSERT(input_left.size() ==
input_right.size() && input_left.size() < 1024);`
10. Fixed misc comments
Response: Fixed in
e2024df
#### [V-AZT-VUL-008: Incorrect term used during calculation on bound of
upper limb]()
Corrected the calculation of the resultant maximum bound of the upper
and lower limbs:
$$D_{\textsf{hi}} < 2^{2Q + L + \textcolor{red}{3}},$$
$$D_{\textsf{lo}} < 2^{2Q + L + \textcolor{red}{2}}.$$
Response: Fixed in
f41813e
#### [V-AZT-VUL-009: Optimization Improvements]()
1. In the function `add_two`, we could add another `if` condition when
two of the three inputs are constant and reduced by the prime. But the
current implementation should handle that implicitly on calling
`add_two` on each limb. Note that no additional gates would be added
since each `add_two` call on limbs would just update additive constants
on the witness.
2. Agree with the observation but we think its fine to have redundant
reductions in constant case as that would cause negligible impact on
prover performance.
3. It is a good suggestion to use a tighter bound for range constraint
on the quotient when reduction is not necessary. However, while creating
the quotient witness using the constructor:
```cpp
template <typename Builder, typename T>
bigfield<Builder, T> bigfield<Builder,
T>::create_from_u512_as_witness(Builder* ctx,
const uint512_t& value,
const bool can_overflow,
const size_t maximum_bitlength)
```
when `can_overflow` is set to `false`, `maximum_bitlength` must be
greater than $3L$. This assertion breaks if we use tighter range
constraint on the quotient. This needs to be investigated further.1 parent 0d1e742 commit 2166a06
File tree
8 files changed
+268
-128
lines changed- barretenberg/cpp
- scripts
- src/barretenberg/stdlib
- eccvm_verifier
- honk_verifier
- primitives
- bigfield
- biggroup
8 files changed
+268
-128
lines changedLines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
139 | 139 | | |
140 | 140 | | |
141 | 141 | | |
142 | | - | |
| 142 | + | |
143 | 143 | | |
144 | 144 | | |
145 | 145 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
293 | 293 | | |
294 | 294 | | |
295 | 295 | | |
296 | | - | |
| 296 | + | |
297 | 297 | | |
298 | 298 | | |
299 | 299 | | |
| |||
Lines changed: 6 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
413 | 413 | | |
414 | 414 | | |
415 | 415 | | |
416 | | - | |
417 | | - | |
| 416 | + | |
| 417 | + | |
418 | 418 | | |
419 | 419 | | |
420 | 420 | | |
421 | 421 | | |
422 | 422 | | |
423 | 423 | | |
424 | | - | |
| 424 | + | |
425 | 425 | | |
426 | 426 | | |
427 | 427 | | |
428 | 428 | | |
429 | 429 | | |
430 | 430 | | |
431 | | - | |
| 431 | + | |
432 | 432 | | |
433 | 433 | | |
434 | 434 | | |
435 | 435 | | |
436 | 436 | | |
437 | 437 | | |
438 | 438 | | |
439 | | - | |
| 439 | + | |
440 | 440 | | |
441 | 441 | | |
442 | 442 | | |
443 | | - | |
| 443 | + | |
444 | 444 | | |
445 | 445 | | |
446 | 446 | | |
| |||
Lines changed: 25 additions & 26 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
58 | | - | |
| 58 | + | |
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| |||
326 | 326 | | |
327 | 327 | | |
328 | 328 | | |
329 | | - | |
330 | | - | |
331 | 329 | | |
332 | 330 | | |
333 | 331 | | |
334 | 332 | | |
335 | 333 | | |
336 | 334 | | |
337 | | - | |
338 | | - | |
339 | | - | |
340 | | - | |
341 | | - | |
342 | | - | |
343 | | - | |
344 | | - | |
345 | | - | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
346 | 342 | | |
347 | | - | |
348 | | - | |
349 | | - | |
350 | | - | |
351 | | - | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
352 | 348 | | |
353 | 349 | | |
354 | 350 | | |
| |||
568 | 564 | | |
569 | 565 | | |
570 | 566 | | |
571 | | - | |
| 567 | + | |
572 | 568 | | |
573 | 569 | | |
574 | 570 | | |
| |||
770 | 766 | | |
771 | 767 | | |
772 | 768 | | |
773 | | - | |
| 769 | + | |
774 | 770 | | |
775 | 771 | | |
776 | 772 | | |
| |||
787 | 783 | | |
788 | 784 | | |
789 | 785 | | |
790 | | - | |
| 786 | + | |
791 | 787 | | |
792 | 788 | | |
793 | 789 | | |
| |||
915 | 911 | | |
916 | 912 | | |
917 | 913 | | |
918 | | - | |
919 | | - | |
920 | | - | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
921 | 917 | | |
922 | 918 | | |
923 | | - | |
| 919 | + | |
924 | 920 | | |
925 | 921 | | |
926 | 922 | | |
927 | 923 | | |
928 | | - | |
| 924 | + | |
929 | 925 | | |
930 | 926 | | |
931 | 927 | | |
932 | 928 | | |
933 | 929 | | |
934 | 930 | | |
935 | | - | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
936 | 935 | | |
937 | 936 | | |
938 | 937 | | |
| |||
Lines changed: 97 additions & 10 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
80 | 80 | | |
81 | 81 | | |
82 | 82 | | |
83 | | - | |
| 83 | + | |
84 | 84 | | |
85 | 85 | | |
86 | 86 | | |
| |||
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
| 92 | + | |
92 | 93 | | |
93 | 94 | | |
94 | 95 | | |
| |||
126 | 127 | | |
127 | 128 | | |
128 | 129 | | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
134 | 135 | | |
135 | 136 | | |
136 | 137 | | |
137 | 138 | | |
138 | | - | |
| 139 | + | |
139 | 140 | | |
140 | 141 | | |
141 | 142 | | |
| |||
144 | 145 | | |
145 | 146 | | |
146 | 147 | | |
147 | | - | |
| 148 | + | |
148 | 149 | | |
149 | 150 | | |
150 | 151 | | |
| |||
176 | 177 | | |
177 | 178 | | |
178 | 179 | | |
179 | | - | |
| 180 | + | |
180 | 181 | | |
181 | 182 | | |
182 | 183 | | |
183 | 184 | | |
184 | 185 | | |
185 | 186 | | |
186 | | - | |
| 187 | + | |
187 | 188 | | |
188 | 189 | | |
189 | 190 | | |
| |||
444 | 445 | | |
445 | 446 | | |
446 | 447 | | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
447 | 529 | | |
448 | 530 | | |
449 | 531 | | |
| |||
512 | 594 | | |
513 | 595 | | |
514 | 596 | | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
0 commit comments