Skip to content

chore: simplify uint logic by removing witness_status#15976

Merged
suyash67 merged 4 commits into
merge-train/barretenbergfrom
sb/remove-uint-witness-status
Jul 25, 2025
Merged

chore: simplify uint logic by removing witness_status#15976
suyash67 merged 4 commits into
merge-train/barretenbergfrom
sb/remove-uint-witness-status

Conversation

@suyash67

@suyash67 suyash67 commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

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:

// N.B. We assume that additive_constant is nonzero ONLY if value is constant
const uint256_t lhs = get_value();
const uint256_t rhs = other.get_value();
const uint256_t constants = (additive_constant + other.additive_constant) & MASK;
const uint256_t sum = lhs + rhs;
const uint256_t overflow = sum >> width;
const uint256_t remainder = sum & MASK;
const add_quad_<FF> gate{
is_constant() ? ctx->zero_idx : witness_index,
other.is_constant() ? ctx->zero_idx : other.witness_index,
ctx->add_variable(remainder),
ctx->add_variable(overflow),
FF::one(),
FF::one(),
FF::neg_one(),
-FF(CIRCUIT_UINT_MAX_PLUS_ONE),
constants,
};
ctx->create_balanced_add_gate(gate);

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 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.

@suyash67 suyash67 changed the title Sb/remove uint witness status chore: simplify uint logic by removing witness_status Jul 24, 2025
Base automatically changed from merge-train/barretenberg to next July 25, 2025 03:26
@suyash67 suyash67 force-pushed the sb/remove-uint-witness-status branch from af65231 to b0166fb Compare July 25, 2025 08:04
@suyash67 suyash67 marked this pull request as ready for review July 25, 2025 08:04
@suyash67 suyash67 changed the base branch from next to merge-train/barretenberg July 25, 2025 08:04
@suyash67 suyash67 force-pushed the sb/remove-uint-witness-status branch from 84284b0 to b9711c0 Compare July 25, 2025 08:28
@suyash67 suyash67 requested a review from iakovenkos July 25, 2025 09:51
@suyash67 suyash67 merged commit 96868aa into merge-train/barretenberg Jul 25, 2025
4 checks passed
@suyash67 suyash67 deleted the sb/remove-uint-witness-status branch July 25, 2025 14:13
suyash67 added a commit that referenced this pull request Jul 25, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant