refactor: showcase recursion case for transfer #7271
Closed
Conversation
This file contains 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
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @LHerskind and the rest of your teammates on |
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
| subtrahend: U128, | ||
| limit: u32 | ||
| ) -> (U128, U128) where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote { | ||
| // docs:start:get_notes |
There was a problem hiding this comment.
dangling doc comments here and below btw
This was referenced Jul 2, 2024
LHerskind
force-pushed
the
lh/private-call-stack-item-hash
branch
2 times, most recently
from
752252f
to
da132ed
Compare
LHerskind
force-pushed
the
lh/private-call-stack-item-hash
branch
from
da132ed
to
9ed5785
Compare
LHerskind
changed the base branch from
lh/private-call-stack-item-hash
to
lh/public-call-stack-item-hash
LHerskind
force-pushed
the
lh/public-call-stack-item-hash
branch
from
411f896
to
b790ce8
Compare
LHerskind
force-pushed
the
lh/public-call-stack-item-hash
branch
from
02f62f0
to
a77b618
Compare
Thunkar
requested changes
Jul 8, 2024
benesjan
approved these changes
Jul 8, 2024
| @@ -113,6 +113,39 @@ impl<T> BalancesMap<T, &mut PrivateContext> { | |||
|
|
|||
| self.add(owner, minuend - subtrahend) | |||
| } | |||
|
|
|||
| pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>( | |||
There was a problem hiding this comment.
Suggested change
| pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>( | |
| /// Performs a subtraction operation on the owner's balance, allowing for partial subtraction. | |
| /// | |
| /// This function attempts to subtract a given amount (subtrahend) from the owner's balance, | |
| /// but can handle cases where the balance is insufficient. It returns either the remaining | |
| /// balance after subtraction or the deficit amount. | |
| /// | |
| /// # Arguments | |
| /// * `self` - The current instance of the contract. | |
| /// * `owner` - The AztecAddress of the account owner. | |
| /// * `subtrahend` - The amount to be subtracted. | |
| /// * `limit` - The maximum number of notes to consider when calculating the balance. | |
| /// | |
| /// # Returns | |
| /// A tuple `(U128, U128)` where: | |
| /// - The first element is the remaining balance if subtraction was successful, or zero if not. | |
| /// - The second element is zero if subtraction was successful, or the deficit amount if not. | |
| /// | |
| /// # Type Parameters | |
| /// * `T_SERIALIZED_LEN` - The serialized length of the note. | |
| /// * `T_SERIALIZED_BYTES_LEN` - The serialized bytes length of the note. | |
| /// * `T` - The type implementing NoteInterface and OwnedNote traits. | |
| /// | |
| /// # Panics | |
| /// Panics if no notes are found for the given owner (i.e., if the balance is zero). | |
| pub fn odd_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>( |
Claude ai generated nice docs here.
BTW agree with Grego that odd_sub is not that great of a name. Maybe sub_with_deficit would be better?
| } | ||
|
|
||
| if minuend >= subtrahend { | ||
| (minuend - subtrahend, U128::zero()) |
There was a problem hiding this comment.
Suggested change
| (minuend - subtrahend, U128::zero()) | |
| // If balance is sufficient, return remaining balance and zero deficit | |
| (minuend - subtrahend, U128::zero()) |
| if minuend >= subtrahend { | ||
| (minuend - subtrahend, U128::zero()) | ||
| } else { | ||
| (U128::zero(), subtrahend - minuend) |
There was a problem hiding this comment.
Suggested change
| (U128::zero(), subtrahend - minuend) | |
| // If balance is insufficient, return zero remaining and the deficit amount | |
| (U128::zero(), subtrahend - minuend) |
| @@ -335,13 +336,38 @@ contract Token { | |||
| let to_ivpk = header.get_ivpk_m(&mut context, to); | |||
|
|
|||
| let amount = U128::from_integer(amount); | |||
| storage.balances.sub(from, amount).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk)); | |||
| let mut (change, missing) = storage.balances.odd_sub(from, amount, 2); | |||
There was a problem hiding this comment.
Could you explain here why we try to fetch only 2 notes? It would be very non-obvious to outsiders reading the code unless they are very knowledgeable about circuit optimization and we want the repo contracts to be an example.
| change = call.call(&mut context); | ||
| } | ||
|
|
||
| storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk)); |
There was a problem hiding this comment.
Suggested change
| storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk)); | |
| // We don't constrain encryption of the note log in `transfer` (unlike in `transfer_from`) because transfer | |
| // function is only designed to be used in situations where it's not necessary (e.g. payment to another person | |
| // where the payment is considered to be successful when the other party successfully decrypts a note). | |
| storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk)); |
|
I'm splitting this into a couple smaller PRs since there's quite a few things going on at the same time. We''ll still do this, but I want to keep the unrelated changes separate if at all possible. Created #7621 for zero note retrieval (which by itself has a bunch of repercussions). |
|
Superceded by #7730 |
nventuro
added a commit
that referenced
this pull request
Aug 6, 2024
Replaces #7271 Closes #7142 Closes #7362 This is quite similar to the implementation in #7271: `transfer` consumes two notes, and if their amount is insufficient for the desired transfer it calls a second internal function which recursively consumes 8 notes per iteration until either the amount is reached, or no more notes are found. If the total amount consumed exceeds the transfer amount, a change note is created. This results in a much smaller transfer function for the scenario in which two notes suffice, since we're using a `limit` value of 2 and therefore only doing two iterations of note constraining (the expensive part). Two notes is a good amount as it provides a way for change notes to be consumed in follow-up calls. The recursive call has a much lower gate count, since it doesn't need to fetch keys, create notes, emit events, etc., and so we can afford to read more notes here while still resulting in a relatively small circuit. I created a new e2e test in which the number of notes and their amounts are quite controlled in order to properly test this. The tests are unfortunately currently interdependent, but given the relative simplicity of the test suite it should be hopefully simple to maintain and expand, and maybe eventually fix.
AztecBot
pushed a commit
to AztecProtocol/aztec-nr
that referenced
this pull request
Aug 7, 2024
Replaces AztecProtocol/aztec-packages#7271 Closes AztecProtocol/aztec-packages#7142 Closes AztecProtocol/aztec-packages#7362 This is quite similar to the implementation in #7271: `transfer` consumes two notes, and if their amount is insufficient for the desired transfer it calls a second internal function which recursively consumes 8 notes per iteration until either the amount is reached, or no more notes are found. If the total amount consumed exceeds the transfer amount, a change note is created. This results in a much smaller transfer function for the scenario in which two notes suffice, since we're using a `limit` value of 2 and therefore only doing two iterations of note constraining (the expensive part). Two notes is a good amount as it provides a way for change notes to be consumed in follow-up calls. The recursive call has a much lower gate count, since it doesn't need to fetch keys, create notes, emit events, etc., and so we can afford to read more notes here while still resulting in a relatively small circuit. I created a new e2e test in which the number of notes and their amounts are quite controlled in order to properly test this. The tests are unfortunately currently interdependent, but given the relative simplicity of the test suite it should be hopefully simple to maintain and expand, and maybe eventually fix.
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.
Fixes #7142 and #7362.
Instead of doing direct recursion on the
transferI have altered thebalances_mapslightly to include a newsubfunction that allow a reduction of notes WITHOUT inserting the change note. We can then recurse over this new sub function to spend notes until we get what we need and then insert a change note after that.Notice that the transfer is consuming at most 2 notes, but that the
_accumulatemight consume up to 8!The reason for this is fairly simple, the
transferwould be inserting notes that are instantly consumed but spend plenty of juice on constraining the encryption first. Instead we can just spend a lot of notes with_accumulatewithout creating any new notes, and then return a final change value for the transfer to add for thefrom.Since there are less "overhead" and we are just consuming notes in
_accumulatewe can spend the same number of constraints just consuming notes as we usually spend on consuming, encrypting and updating state.During the testing figured something interesting, that might be an issue with compiler optimisation.
note_getterthat asserts the length of the bounded vec is non-zero. Since that allow us for more meaningful error messages.