feat: make token transfer be recursive (#7730)

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.

noir-projects/aztec-nr/value-note/src/filter.nr

@@ -6,6 +6,7 @@ pub fn filter_notes_min_sum(
     min_sum: Field
 ) -> [Option<ValueNote>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] {
     let mut selected = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
+
     let mut sum = U128::from_integer(0);
     for i in 0..notes.len() {
         if notes[i].is_some() & (sum < U128::from_integer(min_sum)) {
@@ -14,5 +15,6 @@ pub fn filter_notes_min_sum(
             sum += U128::from_integer(note.value);
         }
     }
+
     selected
 }

noir-projects/noir-contracts/contracts/token_contract/src/main.nr

@@ -16,7 +16,7 @@ contract Token {
     use dep::compressed_string::FieldCompressedString;
 
     use dep::aztec::{
-        hash::compute_secret_hash,
+        context::{PrivateContext, PrivateCallInterface}, hash::compute_secret_hash,
         prelude::{NoteGetterOptions, Map, PublicMutable, SharedImmutable, PrivateSet, AztecAddress},
         encrypted_logs::{
         encrypted_note_emission::{
@@ -34,6 +34,15 @@ contract Token {
     use crate::types::{transparent_note::TransparentNote, token_note::{TokenNote, TOKEN_NOTE_LEN}, balances_map::BalancesMap};
     // docs:end::imports
 
+    // In the first transfer iteration we are computing a lot of additional information (validating inputs, retrieving
+    // keys, etc.), so the gate count is already relatively high. We therefore only read a few notes to keep the happy
+    // case with few constraints.
+    global INITIAL_TRANSFER_CALL_MAX_NOTES = 2;
+    // All the recursive call does is nullify notes, meaning the gate count is low, but it is all constant overhead. We
+    // therefore read more notes than in the base case to increase the efficiency of the overhead, since this results in
+    // an overall small circuit regardless.
+    global RECURSIVE_TRANSFER_CALL_MAX_NOTES = 8;
+
     #[aztec(event)]
     struct Transfer {
         from: AztecAddress,
@@ -335,13 +344,74 @@ 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, from));
+
+        // We reduce `from`'s balance by amount by recursively removing notes over potentially multiple calls. This
+        // method keeps the gate count for each individual call low - reading too many notes at once could result in
+        // circuits in which proving is not feasible.
+        // Since the sum of the amounts in the notes we nullified was potentially larger than amount, we create a new
+        // note for `from` with the change amount, e.g. if `amount` is 10 and two notes are nullified with amounts 8 and
+        // 5, then the change will be 3 (since 8 + 5 - 10 = 3).
+        let change = subtract_balance(
+            &mut context,
+            storage,
+            from,
+            amount,
+            INITIAL_TRANSFER_CALL_MAX_NOTES
+        );
+
+        storage.balances.add(from, change).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, from_ivpk, from));
+
         storage.balances.add(to, amount).emit(encode_and_encrypt_note_with_keys_unconstrained(&mut context, from_ovpk, to_ivpk, to));
 
+        // We don't constrain encryption of the note log in `transfer` (unlike in `transfer_from`) because the transfer
+        // function is only designed to be used in situations where the event is not strictly necessary (e.g. payment to
+        // another person where the payment is considered to be successful when the other party successfully decrypts a
+        // note).
         Transfer { from, to, amount: amount.to_field() }.emit(encode_and_encrypt_event_with_keys_unconstrained(&mut context, from_ovpk, to_ivpk, to));
     }
     // docs:end:transfer
 
+    #[contract_library_method]
+    fn subtract_balance(
+        context: &mut PrivateContext,
+        storage: Storage<&mut PrivateContext>,
+        account: AztecAddress,
+        amount: U128,
+        max_notes: u32
+    ) -> U128 {
+        let subtracted = storage.balances.try_sub(account, amount, max_notes);
+
+        // Failing to subtract any amount means that the owner was unable to produce more notes that could be nullified.
+        // We could in some cases fail early inside try_sub if we detected that fewer notes than the maximum were
+        // returned and we were still unable to reach the target amount, but that'd make the code more complicated, and
+        // optimizing for the failure scenario is not as important.
+        assert(subtracted > U128::from_integer(0), "Balance too low");
+
+        if subtracted >= amount {
+            // We have achieved our goal of nullifying notes that add up to more than amount, so we return the change
+            subtracted - amount
+        } else {
+            // try_sub failed to nullify enough notes to reach the target amount, so we compute the amount remaining
+            // and try again.
+            let remaining = amount - subtracted;
+            Token::at(context.this_address())._recurse_subtract_balance(account, remaining.to_field()).call(context)
+        }
+    }
+
+    // TODO(#7728): even though the amount should be a U128, we can't have that type in a contract interface due to
+    // serialization issues.
+    #[aztec(internal)]
+    #[aztec(private)]
+    fn _recurse_subtract_balance(account: AztecAddress, amount: Field) -> U128 {
+        subtract_balance(
+            &mut context,
+            storage,
+            account,
+            U128::from_integer(amount),
+            RECURSIVE_TRANSFER_CALL_MAX_NOTES
+        )
+    }
+
     /**
      * Cancel a private authentication witness.
      * @param inner_hash The inner hash of the authwit to cancel.
@@ -427,4 +497,5 @@ contract Token {
     }
     // docs:end:balance_of_private
 }
-// docs:end:token_all
+
+// docs:end:token_all

noir-projects/noir-contracts/contracts/token_contract/src/types/balances_map.nr

@@ -82,14 +82,37 @@ impl<T> BalancesMap<T, &mut PrivateContext> {
     pub fn sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
         self: Self,
         owner: AztecAddress,
-        subtrahend: U128
+        amount: U128
     ) -> OuterNoteEmission<T> where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote + Eq {
+        let subtracted = self.try_sub(owner, amount, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL);
+
+        // try_sub may have substracted more or less than amount. We must ensure that we subtracted at least as much as
+        // we needed, and then create a new note for the owner for the change (if any).
+        assert(subtracted >= amount, "Balance too low");
+        self.add(owner, subtracted - amount)
+    }
+
+    // Attempts to remove 'target_amount' from the owner's balance. try_sub returns how much was actually subtracted
+    // (i.e. the sum of the value of nullified notes), but this subtracted amount may be more or less than the target
+    // amount.
+    // This may seem odd, but is unfortunately unavoidable due to the number of notes available and their amounts being
+    // unknown. What try_sub does is a best-effort attempt to consume as few notes as possible that add up to more than 
+    // `target_amount`.
+    // The `max_notes` parameter is used to fine-tune the number of constraints created by this function. The gate count
+    // scales relatively linearly with `max_notes`, but a lower `max_notes` parameter increases the likelihood of 
+    // `try_sub` subtracting an amount smaller than `target_amount`.
+    pub fn try_sub<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
+        self: Self,
+        owner: AztecAddress,
+        target_amount: U128,
+        max_notes: u32
+    ) -> U128 where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote + Eq {
         // docs:start:get_notes
-        let options = NoteGetterOptions::with_filter(filter_notes_min_sum, subtrahend);
+        let options = NoteGetterOptions::with_filter(filter_notes_min_sum, target_amount).set_limit(max_notes);
         let notes = self.map.at(owner).get_notes(options);
         // docs:end:get_notes
 
-        let mut minuend: U128 = U128::from_integer(0);
+        let mut subtracted = U128::from_integer(0);
         for i in 0..options.limit {
             if i < notes.len() {
                 let note = notes.get_unchecked(i);
@@ -102,26 +125,30 @@ impl<T> BalancesMap<T, &mut PrivateContext> {
                 self.map.at(owner).remove(note);
                 // docs:end:remove
 
-                minuend = minuend + note.get_amount();
+                subtracted = subtracted + note.get_amount();
             }
         }
 
-        // This is to provide a nicer error msg,
-        // without it minuend-subtrahend would still catch it, but more generic error then.
-        // without the == true, it includes 'minuend.ge(subtrahend)' as part of the error.
-        assert(minuend >= subtrahend, "Balance too low");
-
-        self.add(owner, minuend - subtrahend)
+        subtracted
     }
 }
 
+// Computes the partial sum of the notes array, stopping once 'min_sum' is reached. This can be used to minimize the
+// number of notes read that add to some value, e.g. when transferring some amount of tokens.
+// The filter does not check if total sum is larger or equal to 'min_sum' - all it does is remove extra notes if it does
+// reach that value.
+// Note that proper usage of this filter requires for notes to be sorted in descending order.
 pub fn filter_notes_min_sum<T, T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN>(
     notes: [Option<T>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL],
     min_sum: U128
 ) -> [Option<T>; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL] where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote {
     let mut selected = [Option::none(); MAX_NOTE_HASH_READ_REQUESTS_PER_CALL];
     let mut sum = U128::from_integer(0);
     for i in 0..notes.len() {
+        // Because we process notes in retrieved order, notes need to be sorted in descending amount order for this
+        // filter to be useful. Consider a 'min_sum' of 4, and a set of notes with amounts [3, 2, 1, 1, 1, 1, 1]. If
+        // sorted in descending order, the filter will only choose the notes with values 3 and 2, but if sorted in
+        // ascending order it will choose 4 notes of value 1.
         if notes[i].is_some() & sum < min_sum {
             let note = notes[i].unwrap_unchecked();
             selected[i] = Option::some(note);

noir/noir-repo/tooling/nargo/src/errors.rs

@@ -64,31 +64,25 @@ impl<F: AcirField> NargoError<F> {
         &self,
         error_types: &BTreeMap<ErrorSelector, AbiErrorType>,
     ) -> Option<String> {
-        let execution_error = match self {
-            NargoError::ExecutionError(error) => error,
-            _ => return None,
-        };
-
-        match execution_error {
-            ExecutionError::AssertionFailed(payload, _) => match payload {
-                ResolvedAssertionPayload::String(message) => Some(message.to_string()),
-                ResolvedAssertionPayload::Raw(raw) => {
-                    let abi_type = error_types.get(&raw.selector)?;
-                    let decoded = display_abi_error(&raw.data, abi_type.clone());
-                    Some(decoded.to_string())
-                }
-            },
-            ExecutionError::SolvingError(error, _) => match error {
-                OpcodeResolutionError::IndexOutOfBounds { .. }
-                | OpcodeResolutionError::OpcodeNotSolvable(_)
-                | OpcodeResolutionError::UnsatisfiedConstrain { .. }
-                | OpcodeResolutionError::AcirMainCallAttempted { .. }
-                | OpcodeResolutionError::BrilligFunctionFailed { .. }
-                | OpcodeResolutionError::AcirCallOutputsMismatch { .. } => None,
-                OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => {
-                    Some(reason.to_string())
-                }
+        match self {
+            NargoError::ExecutionError(error) => match error {
+                ExecutionError::AssertionFailed(payload, _) => match payload {
+                    ResolvedAssertionPayload::String(message) => Some(message.to_string()),
+                    ResolvedAssertionPayload::Raw(raw) => {
+                        let abi_type = error_types.get(&raw.selector)?;
+                        let decoded = display_abi_error(&raw.data, abi_type.clone());
+                        Some(decoded.to_string())
+                    }
+                },
+                ExecutionError::SolvingError(error, _) => match error {
+                    OpcodeResolutionError::BlackBoxFunctionFailed(_, reason) => {
+                        Some(reason.to_string())
+                    }
+                    _ => None,
+                },
             },
+            NargoError::ForeignCallError(error) => Some(error.to_string()),
+            _ => None,
         }
     }
 }

yarn-project/end-to-end/src/e2e_token_contract/private_transfer_recursion.test.ts

@@ -0,0 +1,90 @@
+import { BatchCall, EventType, Fr } from '@aztec/aztec.js';
+import { TokenContract } from '@aztec/noir-contracts.js';
+
+import { TokenContractTest } from './token_contract_test.js';
+
+describe('e2e_token_contract private transfer recursion', () => {
+  const t = new TokenContractTest('odd_transfer_private');
+  let { asset, accounts, wallets } = t;
+
+  beforeAll(async () => {
+    await t.applyBaseSnapshots();
+    await t.setup();
+    ({ asset, accounts, wallets } = t);
+  });
+
+  afterAll(async () => {
+    await t.teardown();
+  });
+
+  async function mintNotes(noteAmounts: bigint[]): Promise<bigint> {
+    // Mint all notes, 4 at a time
+    for (let mintedNotes = 0; mintedNotes < noteAmounts.length; mintedNotes += 4) {
+      const toMint = noteAmounts.slice(mintedNotes, mintedNotes + 4); // We mint 4 notes at a time
+      const actions = toMint.map(amt => asset.methods.privately_mint_private_note(amt).request());
+      await new BatchCall(wallets[0], actions).send().wait();
+    }
+
+    return noteAmounts.reduce((prev, curr) => prev + curr, 0n);
+  }
+
+  it('transfer full balance', async () => {
+    // We insert 16 notes, which is large enough to guarantee that the token will need to do two recursive calls to
+    // itself to consume them all (since it retrieves 2 notes on the first pass and 8 in each subsequent pass).
+    const totalNotes = 16;
+    const totalBalance = await mintNotes(Array(totalNotes).fill(10n));
+    const tx = await asset.methods.transfer(accounts[1].address, totalBalance).send().wait({ debug: true });
+
+    // We should have nullified all notes, plus an extra nullifier for the transaction
+    expect(tx.debugInfo?.nullifiers.length).toBe(totalNotes + 1);
+    // We should have created a single new note, for the recipient
+    expect(tx.debugInfo?.noteHashes.length).toBe(1);
+
+    const events = await wallets[1].getEvents(EventType.Encrypted, TokenContract.events.Transfer, tx.blockNumber!, 1);
+
+    expect(events[0]).toEqual({
+      from: accounts[0].address,
+      to: accounts[1].address,
+      amount: new Fr(totalBalance),
+    });
+  });
+
+  it('transfer less than full balance and get change', async () => {
+    const noteAmounts = [10n, 10n, 10n, 10n];
+    const expectedChange = 3n; // This will result in one of the notes being partially used
+
+    const totalBalance = await mintNotes(noteAmounts);
+    const toSend = totalBalance - expectedChange;
+
+    const tx = await asset.methods.transfer(accounts[1].address, toSend).send().wait({ debug: true });
+
+    // We should have nullified all notes, plus an extra nullifier for the transaction
+    expect(tx.debugInfo?.nullifiers.length).toBe(noteAmounts.length + 1);
+    // We should have created two new notes, one for the recipient and one for the sender (with the change)
+    expect(tx.debugInfo?.noteHashes.length).toBe(2);
+
+    const senderBalance = await asset.methods.balance_of_private(accounts[0].address).simulate();
+    expect(senderBalance).toEqual(expectedChange);
+
+    const events = await wallets[1].getEvents(EventType.Encrypted, TokenContract.events.Transfer, tx.blockNumber!, 1);
+
+    expect(events[0]).toEqual({
+      from: accounts[0].address,
+      to: accounts[1].address,
+      amount: new Fr(toSend),
+    });
+  });
+
+  describe('failure cases', () => {
+    it('transfer more than balance', async () => {
+      const balance0 = await asset.methods.balance_of_private(accounts[0].address).simulate();
+
+      const amount = balance0 + 1n;
+      expect(amount).toBeGreaterThan(0n);
+
+      await expect(asset.methods.transfer(accounts[1].address, amount).simulate()).rejects.toThrow(
+        'Assertion failed: Balance too low',
+      );
+    });
+  });
+});