diff --git a/noir-projects/aztec-nr/value-note/src/filter.nr b/noir-projects/aztec-nr/value-note/src/filter.nr index af5b3ee03e02..8024414d4e52 100644 --- a/noir-projects/aztec-nr/value-note/src/filter.nr +++ b/noir-projects/aztec-nr/value-note/src/filter.nr @@ -6,6 +6,7 @@ pub fn filter_notes_min_sum( min_sum: Field ) -> [Option; 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 } diff --git a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/token_contract/src/main.nr index e12673267f4c..4da6180d5fc5 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/main.nr +++ b/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 \ No newline at end of file + +// docs:end:token_all diff --git a/noir-projects/noir-contracts/contracts/token_contract/src/types/balances_map.nr b/noir-projects/noir-contracts/contracts/token_contract/src/types/balances_map.nr index 63a0092ea5c9..b357b93bf1a9 100644 --- a/noir-projects/noir-contracts/contracts/token_contract/src/types/balances_map.nr +++ b/noir-projects/noir-contracts/contracts/token_contract/src/types/balances_map.nr @@ -82,14 +82,37 @@ impl BalancesMap { pub fn sub( self: Self, owner: AztecAddress, - subtrahend: U128 + amount: U128 ) -> OuterNoteEmission where T: NoteInterface + 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( + self: Self, + owner: AztecAddress, + target_amount: U128, + max_notes: u32 + ) -> U128 where T: NoteInterface + 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,19 +125,19 @@ impl BalancesMap { 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( notes: [Option; MAX_NOTE_HASH_READ_REQUESTS_PER_CALL], min_sum: U128 @@ -122,6 +145,10 @@ pub fn filter_notes_min_sum( 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); diff --git a/noir/noir-repo/tooling/nargo/src/errors.rs b/noir/noir-repo/tooling/nargo/src/errors.rs index b2248605cb53..6cfcfc4fc516 100644 --- a/noir/noir-repo/tooling/nargo/src/errors.rs +++ b/noir/noir-repo/tooling/nargo/src/errors.rs @@ -64,31 +64,25 @@ impl NargoError { &self, error_types: &BTreeMap, ) -> Option { - 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, } } } diff --git a/yarn-project/end-to-end/src/e2e_token_contract/private_transfer_recursion.test.ts b/yarn-project/end-to-end/src/e2e_token_contract/private_transfer_recursion.test.ts new file mode 100644 index 000000000000..25535f55b1f5 --- /dev/null +++ b/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 { + // 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', + ); + }); + }); +});