feat: merge-train/barretenberg#15978
Merged
Merged
Conversation
In `stdlib_uint` we no longer need logical operations because the only places they were used in, i.e., std/turbo version of sha256, blake2s, blake3s, have been removed. So its best to reduce complexity of the `uint` class and keep it minimal. Removed the following functions from the `uint` class: ```cpp operator^ operator& operator| operator~ operator>> operator<< ror rol logic_operator ```
…ments as input and returns the commitment to the merged table (#15949) We modify the `MergeVerifier` so that it gets the subtable commitments as input and returns the commitment to the merged table. The reason for this change is that given the new structure of `ClientIVC` following [#15704](#15704), we can't access the merged table commitments from inside `complete_hiding_circuit_logic`. This PR is in preparation for [#15829](#15829) --------- Co-authored-by: AztecBot <tech@aztecprotocol.com>
federicobarbacovi
requested review from
IlyasRidhuan,
fcarreiro and
jeanmon
as code owners
The tar is not supposed to be checked in according to 50a1bd3#r2229436867
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.
…rcuit (#15829) We make the merged table received by the Merge verifier in the hiding circuit a public input to the hiding circuit. This is needed because the Merge verifier will soon receive `t_commitments`, `T_prev_commitments` as inputs rather than reading them from the proof. **EDIT:** To complete the work on the consistency checks, and to ensure the soundness of the Goblin verification, the merged table received by the Merge verifier in the last step of a Goblin accumulation must be set to be a public input of the circuit that performs the verification, so that the verifier can extract that public input and use it as the commitment to the previous table in the Merge verification. For example, in ClientIVC the last Merge verification before the final Goblin verification happens in the HidingKernel, so we need to add the merged table commitments received by the Merge verifier inside the HidingKernel to be public inputs of the HidingKernel. After this PR, `MegaVerifier = UltraVerifier<MegaFlavor>` always expects the inputs to be `PairingInputs` + commitments to ECC op tables. These inputs are produced by the class `HidingKernelIO` (even though in the future we might consider changing this name) The PR required changes to various tests to accommodate the new structure of the public inputs. --------- Co-authored-by: AztecBot <tech@aztecprotocol.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: Suyash Bagad <suyash@aztecprotocol.com> Co-authored-by: Jonathan Hao <jonathan@aztec-labs.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>
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
These are auto-generated by bootstrap.
Update the `prove_tube` function to take into account the new public inputs of the Hiding kernel
We address AztecProtocol/barretenberg#1351: _Merge protocol table commitment consistency checks_. More precisely, we ensure the Merge recursive verification at step $i+1$ uses as commitments to the previous state of the op queue the output of the Merge recursive verification at step $i$. Instead of performing a consistency check between the output at step $i$ and the input at step $i+1$, we pass the output at step $i$ as the input at step $i+1$. This PR also closes AztecProtocol/barretenberg#1473.
We package the inputs to the Merge verifier (two at the moment: `t_commitments` and `T_prev_commitments`) into a single struct to reduce the number of function arguments and prevent incorrect ordering of the arguments. This PR closes AztecProtocol/barretenberg#1492.
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
5 participants
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.
See merge-train-readme.md.
BEGIN_COMMIT_OVERRIDE
chore: remove
logicoperations fromuint(#15975)chore: Modify the
MergeVerifierso 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
uintlogic by removingwitness_status(#15976)Revert "chore: remove
logicoperations fromuint" (#15997)Revert "chore: simplify
uintlogic by removingwitness_status" (#16000)feat: Add the last merged table to the public inputs of the hiding circuit (#15829)
chore: combined
uintaudit (#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