chore: combined uint audit#16030
Merged
Merged
Conversation
### 🧾 Audit Context Audit of the `uint` module in stdlib, specifically the methods: `operator>>`, `operator<<` and `ror` (bitwise operations). ### 🛠️ Changes Made - All three methods were manually "slicing" a field element which was very error-prone. I replaced it with `split_at` method (wrote a generic method `split_at`[^1] that splits a field_t at a given bit index and returns the lower and higher slices). - Cleaned up and optimised `uint` constructor from `byte_array` and `std::vector<bool_t>` (resolving old TODO) - Added static assert on `width` so that we restrict `uint` to 8, 16, 32, and 64 bits only. ### ✅ Checklist - [x] Audited all methods of the relevant module/class - [x] Audited the interface of the module/class with other (relevant) components - [x] Documented existing functionality and any changes made (as per Doxygen requirements) - [x] Resolved and/or closed all issues/TODOs pertaining to the audited files - [x] Confirmed and documented any security or other issues found (if applicable) - [ ] Verified that tests cover all critical paths (and added tests if necessary) - [ ] Updated audit tracking for the files audited (check the start of each file you audited) ### 📌 Notes for Reviewers Please review the `split_at` function carefully, and check if we are testing it thoroughly. We're soon going to remove the function `slice(msb, lsb)` from field_t since we no longer need it (used only in `logic` module). Instead, the new function `split_at` will replace the use of `slice` in `logic` module: https://github.com/AztecProtocol/aztec-packages/blob/6434f3193683748d31d94c787e87252234c6a6fd/barretenberg/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp#L99 will be changed to: `field_pt a_slice = a.split_at(0, num_bits).second;` Note: we are not going to merge any PRs that change anything in `field` or `bigfield` as they're undergoing external audits at the moment. Thus, this PR will be merged after the external audit of `field` (and its fixes) are done. [^1]: The existing `slice(msb, lsb)` method could be used but there were three issues in doing so: (a) found a [bug](eb3f3ec) in range-constraint on the `hi` limb in it, (b) `slice(...)` returns three limbs: hi, mid and lo so it naturally requires three range constraints. We only need two parts of a field element. (c) `slice(...)` assumes a 252-bit field element which results in (unnecessary) costly range-constraints. We'd have to change the function to accept `num_bits` as a parameter (or template parameter).
TLDR: `uint` arithmetic operators `+` and `-` had a coding error and as
a result, we weren't actually supporting lazy arithmetic over integers.
This PR simplifies the `uint` class to now allow any "unbounded" values.
#### The Issue
In the current `uint` class, we allow "unbounded" values, for example, a
`uint32_ct` can contain a value > 32 bits. This was done to allow lazy
arithmetic before such values were "normalized". This is because a call
to `normalize()` is expensive: it decomposes the value in 12-bit slices
and range-constrains each slice.
In practice though, the addition and subtraction operator actually
didn't allow any overflow due to a coding error.
On adding two $\textsf{uint}x$ values $a$ and $b$ (where $x \in [8, 16,
32, 64]$), we currently do:
https://github.com/AztecProtocol/aztec-packages/blob/5c2c217a2f1b05ae226a16ee19a99079dbba8fec/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/arithmetic.cpp#L27-L47
Assume $a, b$ are both witnesses, the `create_balanced_add_gate` creates
the following constraint:
$$a + b = q \cdot \textcolor{grey}{2^x} + r$$
where the quotient $q$ and remainder $r$ are computed as:
$$q := \frac{(a \textsf{ mod } 2^x) + (b \textsf{ mod } 2^x)}{2^x},
\quad r := \left((a \textsf{ mod } 2^x) + (b \textsf{ mod } 2^x)\right)
\textsf{ mod } 2^x.$$
In other words, the quotient and remainder are computed from the
"truncated" values of $a$ and $b$ when it should have been from the
"unbounded" values. Effectively, this means we are not actually
supporting lazy arithmetic (i.e., arithmetic operations expect inputs to
be "normalized"). I wrote a test
[here](https://github.com/AztecProtocol/aztec-packages/blob/ace0afdb4fb773cfc50af92930ecb94993ab72a5/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/uint.test.cpp#L243-L271)
that fails when, ideally, it should have passed. This confirmed the
coding error.
#### Solution(s)
One way to fix this is to actually use `get_unbounded_value()` in place
of `get_value()` (on lines 27 and 28 in `operator+` above). But we never
really were using the benefits of lazy addition (because of this silly
error). So we decided its better to remove functionality related to
"unbounded" uint values.
Thus, we remove the `witness_status` member of the `uint` class as it
tracks if a `uint` needs to be "normalized". As a consequence, we now
need to "normalize" in every constructor where we weren't constraining
the accumulators (i.e., `byte_array` and `std::vector<bool_t>`).
Further, in `operator+` and `operator-` we normalize the result. Also,
removed the `get_unbounded_value()` as it isn't being used anywhere.
### 🧾 Audit Context Remainder audit of `uint`. ### 🛠️ Changes Made - Removed `at` function as it has a bug. The bug is explained [here](https://github.com/AztecProtocol/aztec-packages/blob/3941e2f537a5257f4d19c0e018a73ab687151024/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/uint.cpp#L244-L249). - Added assertions and minor comments. - No circuit changes. ### ✅ Checklist - [x] Audited all methods of the relevant module/class - [x] Audited the interface of the module/class with other (relevant) components - [x] Documented existing functionality and any changes made (as per Doxygen requirements) - [x] Resolved and/or closed all issues/TODOs pertaining to the audited files - [x] Confirmed and documented any security or other issues found (if applicable) - [x] Verified that tests cover all critical paths (and added tests if necessary) - [x] Updated audit tracking for the files audited (check the start of each file you audited) ### 📌 Notes for Reviewers NA
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jul 30, 2025
See [merge-train-readme.md](https://github.com/AztecProtocol/aztec-packages/blob/next/.github/workflows/merge-train-readme.md). BEGIN_COMMIT_OVERRIDE chore: remove `logic` operations from `uint` (#15975) chore: Modify the `MergeVerifier` so that it gets the subtable commitments as input and returns the commitment to the merged table (#15949) fix: delete tar generated by test-vk-havent-changed script (#15988) chore: replace q_arith with q_3 in memory relation (#15953) chore: simplify `uint` logic by removing `witness_status` (#15976) Revert "chore: remove `logic` operations from `uint`" (#15997) Revert "chore: simplify `uint` logic by removing `witness_status`" (#16000) feat: Add the last merged table to the public inputs of the hiding circuit (#15829) chore: combined `uint` audit (#16030) chore: delete and ignore barretenberg/src/honk/keys/ (#16042) fix: Fix tube proof construction (#16052) feat: Link successive recursive Merge verifications (#16032) chore: Package inputs to Merge verifier into a single struct (#16075) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: federicobarbacovi <171914500+federicobarbacovi@users.noreply.github.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.com> Co-authored-by: ledwards2225 <98505400+ledwards2225@users.noreply.github.com> Co-authored-by: sergei iakovenko <105737703+iakovenkos@users.noreply.github.com> Co-authored-by: ludamad <adam.domurad@gmail.com> Co-authored-by: Raju Krishnamoorthy <krishnamoorthy@gmail.com> Co-authored-by: notnotraju <raju@aztec-labs.com> Co-authored-by: Lucas Xia <lucasxia01@gmail.com> Co-authored-by: Khashayar Barooti <khashayar@aztecprotocol.com> Co-authored-by: Jean M <132435771+jeanmon@users.noreply.github.com> Co-authored-by: Alex Gherghisan <alexghr@users.noreply.github.com> Co-authored-by: Santiago Palladino <spalladino@users.noreply.github.com> Co-authored-by: Santiago Palladino <santiago@aztec-labs.com> Co-authored-by: ludamad <domuradical@gmail.com> Co-authored-by: maramihali <mara@aztecprotocol.com> Co-authored-by: Sarkoxed <75146596+Sarkoxed@users.noreply.github.com>
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
1 participant
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.
All
uintaudit-related changes. Note we are going to removeuintmodule altogether, this PR/commit should serve as a fallback if we ever need to useuintin future.