fix: uint64_ct tests in stdlib#15582
Conversation
AztecBot
force-pushed
the
merge-train/barretenberg
branch
from
42b65c1 to
8369b3f
Compare
AztecBot
requested review from
IlyasRidhuan,
fcarreiro and
jeanmon
as code owners
ludamad
force-pushed
the
merge-train/barretenberg
branch
from
2abe998 to
3330e1c
Compare
ludamad
requested review from
LHerskind,
LeilaWang,
Maddiaa0,
charlielye,
dbanks12,
just-mitch and
sirasistant
as code owners
suyash67
requested review from
Rumata888
and removed request for
LHerskind,
LeilaWang,
Maddiaa0,
charlielye,
dbanks12,
just-mitch and
sirasistant
| MultiTableId::UINT32_XOR, field_t<Builder>(*this), field_t<Builder>(other), true); | ||
| if constexpr (std::is_same_v<Native, uint64_t>) { | ||
| // use the 64-bit XOR lookup | ||
| lookup = plookup_read<Builder>::get_lookup_accumulators( |
There was a problem hiding this comment.
I'd add a static assert here or in class definition to ensure that 32-bit and 64-bit uints are the only available options
There was a problem hiding this comment.
Well for uint8_ct, uin16_ct, uint32_ct the 32-bit lookup works fine. So previously everything for these three types worked. But uint64_ct operations obviously failed with 32-bit lookup tables. Adding this explanation with examples for clarity.
Rumata888
left a comment
Rumata888
left a comment
There was a problem hiding this comment.
No security problems, but we might be wasting gates
| * @details Given a uint_ct a and a constant const_b, this allows to create a | ||
| * uint_ct b having a desired relation to a (either >. = or <). | ||
| */ | ||
| static uint_native impose_comparison(uint_native const_a, |
There was a problem hiding this comment.
Maybe this function need a slightly better explanation, since you touched it. It's not clear why we have pairs of values
There was a problem hiding this comment.
have a rework/cleanup of tests in next PR.
| // so when looking them up in the lookup tables, we need to ensure that the values are range-constrained | ||
| // to the appropriate bit-size. This is done by using the `field_t<Builder>` operator, which normalizes | ||
| // (i.e., creates range constraint on the accumulators) before looking them up in the tables. | ||
| const field_t<Builder> key_left = field_t<Builder>(*this); |
There was a problem hiding this comment.
I looked at the normalize function and all it does is perform the range constraint (I thought that it might be removing top bits). If that is the case, won't the multitable automatically constrain the value? If that's the case, we are wasting gates here
There was a problem hiding this comment.
Good point, I think you are right. We don't need to range-constrain as the lookup constraint should fail if we pass a value greater than the permissible multi-table input range (verified this for confirming my own understanding). Corrected the comment, and avoided the normalize() call in converting to field_t.
…, AND. all tests now pass.
See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). BEGIN_COMMIT_OVERRIDE feat(bbapi): CIVC end to end (#15777) fix: `uint64_ct` tests in stdlib (#15582) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: ludamad <adam.domurad@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com>
2 participants
The
stdlib::uintmodule was only being tested foruint32_ct. This PR makes the tests also run foruint8_ct, uint16_ct, uint64_ct.The tests were failing for
uint64_ctbecause the XOR and AND lookup tables only supported 32-bit inputs. If we attempted to input more than 32-bits, it naturally led to the error message1:'C++ exception with description "Last key slice greater than 4" thrown in the test body."'We slice the 64-bit number in 6-bit slices, starting from the least significant slice:
such that the most-significant slice is 4 bits. Since we allowed only 32-bit XOR and AND lookup tables, it read the first 5 slices$(a_0, \dots, a_4)$ , i.e., 30 bits, and when it needs to read $a_5$ slice, obviously the remainder $r = (a \gg 30)$ is greater than the allowed value of the last slice (i.e., $r > 4$ ).
Solution: simply added a 64-bit multi-table for XOR and AND operation of two 64-bit numbers. Note that we still use the 6-bit basic table repeated 10-times and a new 4-bit table for the last slice. Hence, one 64-bit XOR and AND operation would cost 11 lookup gates.
resolves AztecProtocol/barretenberg#1229
Footnotes
Side note: the comment in the code noted the error message as
C++ exception with description "Last key slice greater than 64" thrown in the test bodybecause it was referring to an error before the bug in uint32 table was fixed. ↩