Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider refactoring Token's transfer function with recursion #7142

Closed
Tracked by #5070 ...
benesjan opened this issue Jun 21, 2024 · 3 comments · Fixed by #7730
Closed
Tracked by #5070 ...

Consider refactoring Token's transfer function with recursion #7142

benesjan opened this issue Jun 21, 2024 · 3 comments · Fixed by #7730
Assignees

Comments

@benesjan
Copy link
Contributor

Current transfer function is very costly because we try to fetch and work with 32 notes. And given (as Nico would say) that circuits are terrible this results in us having huge gate count because all the gates of note logic are present 32 times in the circuit. Mike described an alternative approach on slack here which leverages recursion. This could allow us to only work with 2 notes by default and recurse if needed (making the base case cheap).

@LHerskind
Copy link
Contributor

We might not need to directly do "recursion" in here, but can just have a smaller amount of notes. Then you can use the multicall structure of the wallets to just perform a few transfers to yourself at the start. Otherwise would still prefer that we don't do it as part of the transfer since the act of making a call is still expensive, so would likely still be cheaper if we would have an "accumulate" instead, where you recurse etc 🤷.

@iAmMichaelConnor
Copy link
Contributor

I think in the best case (having sufficient balance in 1 or 2 notes at the start), the approach I've set out is the most efficient, because then the core transfer function only loops over 2 notes.

Then you can use the multicall structure of the wallets to just perform a few transfers to yourself at the start

the act of making a call is still expensive

Isn't this a contradiction? A multicall results in calls. The approach I've set out doesn't result in any calls if the number of required notes is <= 2.

I also don't think the logic to decide how many times to accumulate should bleed into the wallet (via multicalls); I think it should be part of the token contract, as set out in my proposal.

@nventuro
Copy link
Contributor

nventuro commented Jul 19, 2024

Isn't this a contradiction? A multicall results in calls. The approach I've set out doesn't result in any calls if the number of required notes is <= 2.

I think Lasse was referring to the fact that the entrypoint will include the gate count for those calls anyway, so might as well use them.

@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Aug 6, 2024
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue 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
Labels
None yet
Projects
Archived in project
4 participants