Merge feb719e into ef78eb5

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',
+      );
+    });
+  });
+});