chore: simplify uint logic by removing witness_status [take 2]#16014
Merged
Conversation
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
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.
TLDR:
uintarithmetic operators+and-had a coding error and as a result, we weren't actually supporting lazy arithmetic over integers. This PR simplifies theuintclass to now allow any "unbounded" values.The Issue
In the current
uintclass, we allow "unbounded" values, for example, auint32_ctcan contain a value > 32 bits. This was done to allow lazy arithmetic before such values were "normalized". This is because a call tonormalize()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.$\textsf{uint}x$ values $a$ and $b$ (where $x \in [8, 16, 32, 64]$ ), we currently do:
On adding two
aztec-packages/barretenberg/cpp/src/barretenberg/stdlib/primitives/uint/arithmetic.cpp
Lines 27 to 47 in 5c2c217
Assume$a, b$ are both witnesses, the
create_balanced_add_gatecreates the following constraint:where the quotient$q$ and remainder $r$ are computed as:
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 ofget_value()(on lines 27 and 28 inoperator+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_statusmember of theuintclass as it tracks if auintneeds to be "normalized". As a consequence, we now need to "normalize" in every constructor where we weren't constraining the accumulators (i.e.,byte_arrayandstd::vector<bool_t>). Further, inoperator+andoperator-we normalize the result. Also, removed theget_unbounded_value()as it isn't being used anywhere.