diff --git a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr index a9a406549990..a7d3c276aa67 100644 --- a/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr +++ b/noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr @@ -43,14 +43,15 @@ pub unconstrained fn attempt_note_nonce_discovery( // We need to find nonces (typically just one) that result in a note hash that, once siloed into a unique note // hash, is one of the note hashes created by the transaction. - unique_note_hashes_in_tx.for_eachi(|i, expected_unique_note_hash| { - // Nonces are computed by hashing the first nullifier in the transaction with the index of the note in the new - // note hashes array. We therefore know for each note in every transaction what its nonce is. - let candidate_nonce = compute_note_hash_nonce(first_nullifier_in_tx, i); - - // Given note nonce, note content and metadata, we can compute the note hash and silo it to check if it matches - // the note hash at the array index we're currently processing. TODO(#11157): handle failed - // note_hash_and_nullifier computation + // The nonce is meant to be derived from the index of the note hash in the transaction effects array. However, due + // to an issue in the kernels the nonce might actually use any of the possible note hash indices - not necessarily + // the one that corresponds to the note hash. Hence, we need to try them all. + for i in 0..MAX_NOTE_HASHES_PER_TX { + let nonce_for_i = compute_note_hash_nonce(first_nullifier_in_tx, i); + + // Given note nonce, note content and metadata, we can compute the note hash and silo it to check if + // the resulting unique note matches any in the transaction. + // TODO(#11157): handle failed note_hash_and_nullifier computation let hashes = compute_note_hash_and_nullifier( packed_note, owner, @@ -58,23 +59,37 @@ pub unconstrained fn attempt_note_nonce_discovery( note_type_id, contract_address, randomness, - candidate_nonce, + nonce_for_i, ) .expect(f"Failed to compute a note hash for note type {note_type_id}"); - let siloed_note_hash = compute_siloed_note_hash(contract_address, hashes.note_hash); - let unique_note_hash = compute_unique_note_hash(candidate_nonce, siloed_note_hash); + let siloed_note_hash_for_i = compute_siloed_note_hash(contract_address, hashes.note_hash); + let unique_note_hash_for_i = compute_unique_note_hash(nonce_for_i, siloed_note_hash_for_i); - if unique_note_hash == expected_unique_note_hash { - // Note that while we did check that the note hash is the preimage of the expected unique note hash, we - // perform no validations on the nullifier - we fundamentally cannot, since only the application knows how - // to compute nullifiers. We simply trust it to have provided the correct one: if it hasn't, then PXE may - // fail to realize that a given note has been nullified already, and calls to the application could result - // in invalid transactions (with duplicate nullifiers). This is not a concern because an application - // already has more direct means of making a call to it fail the transaction. + let matching_notes = bvec_filter( + unique_note_hashes_in_tx, + |unique_note_hash_in_tx| unique_note_hash_in_tx == unique_note_hash_for_i, + ); + if matching_notes.len() > 1 { + let identical_note_hashes = matching_notes.len(); + // Note that we don't actually check that the note hashes array contains unique values, only that the note + // we found is unique. We don't expect for this to ever happen (it'd indicate a malicious node or PXE, + // which + // are both assumed to be cooperative) so testing for it just in case is unnecessary, but we _do_ need to + // handle it if we find a duplicate. + panic( + f"Received {identical_note_hashes} identical note hashes for a transaction - these should all be unique", + ) + } else if matching_notes.len() == 1 { + // Note that while we did check that the note hash is the preimage of a unique note hash, we perform no + // validations on the nullifier - we fundamentally cannot, since only the application knows how to compute + // nullifiers. We simply trust it to have provided the correct one: if it hasn't, then PXE may fail to + // realize that a given note has been nullified already, and calls to the application could result in + // invalid transactions (with duplicate nullifiers). This is not a concern because an application already + // has more direct means of making a call to it fail the transaction. discovered_notes.push( DiscoveredNoteInfo { - note_nonce: candidate_nonce, + note_nonce: nonce_for_i, note_hash: hashes.note_hash, // TODO: The None case will be handled in a followup PR. // https://linear.app/aztec-labs/issue/F-265/store-external-notes @@ -88,7 +103,7 @@ pub unconstrained fn attempt_note_nonce_discovery( // multiple times in the same transaction with different nonces. This typically doesn't happen due to notes // containing random values in order to hide their contents. } - }); + } debug_log_format( "Found valid nonces for a total of {0} notes", @@ -98,6 +113,22 @@ pub unconstrained fn attempt_note_nonce_discovery( *discovered_notes } +// There is no BoundedVec::filter in the stdlib, so we use this until that is implemented. +unconstrained fn bvec_filter( + bvec: BoundedVec, + filter: fn[Env](T) -> bool, +) -> BoundedVec { + let filtered = &mut BoundedVec::new(); + + bvec.for_each(|value| { + if filter(value) { + filtered.push(value); + } + }); + + *filtered +} + mod test { use crate::{ messages::{discovery::NoteHashAndNullifier, logs::note::MAX_NOTE_PACKED_LEN}, @@ -309,4 +340,96 @@ mod test { & (discovered_note.inner_nullifier == second_note_and_data.inner_nullifier) })); } + + #[test] + unconstrained fn single_note_misaligned_nonce() { + let note_index_in_tx = 2; + let note_and_data = construct_note(VALUE, note_index_in_tx); + + let mut unique_note_hashes_in_tx = BoundedVec::from_array([ + random(), random(), random(), random(), random(), random(), random(), + ]); + + // The note is not at the correct index + unique_note_hashes_in_tx.set(note_index_in_tx + 1, note_and_data.unique_note_hash); + + let discovered_notes = attempt_note_nonce_discovery( + unique_note_hashes_in_tx, + FIRST_NULLIFIER_IN_TX, + compute_note_hash_and_nullifier, + CONTRACT_ADDRESS, + OWNER, + STORAGE_SLOT, + RANDOMNESS, + MockNote::get_id(), + BoundedVec::from_array(note_and_data.note.pack()), + ); + + assert_eq(discovered_notes.len(), 1); + let discovered_note = discovered_notes.get(0); + + assert_eq(discovered_note.note_nonce, note_and_data.note_nonce); + assert_eq(discovered_note.note_hash, note_and_data.note_hash); + assert_eq(discovered_note.inner_nullifier, note_and_data.inner_nullifier); + } + + #[test] + unconstrained fn single_note_nonce_with_index_past_note_hashes_in_tx() { + let mut unique_note_hashes_in_tx = BoundedVec::from_array([ + random(), random(), random(), random(), random(), random(), random(), + ]); + + // The nonce is computed with an index that does not exist in the tx + let note_index_in_tx = unique_note_hashes_in_tx.len() + 5; + let note_and_data = construct_note(VALUE, note_index_in_tx); + + // The note is inserted at an arbitrary index - its true index is out of the array's bounds + unique_note_hashes_in_tx.set(2, note_and_data.unique_note_hash); + + let discovered_notes = attempt_note_nonce_discovery( + unique_note_hashes_in_tx, + FIRST_NULLIFIER_IN_TX, + compute_note_hash_and_nullifier, + CONTRACT_ADDRESS, + OWNER, + STORAGE_SLOT, + RANDOMNESS, + MockNote::get_id(), + BoundedVec::from_array(note_and_data.note.pack()), + ); + + assert_eq(discovered_notes.len(), 1); + let discovered_note = discovered_notes.get(0); + + assert_eq(discovered_note.note_nonce, note_and_data.note_nonce); + assert_eq(discovered_note.note_hash, note_and_data.note_hash); + assert_eq(discovered_note.inner_nullifier, note_and_data.inner_nullifier); + } + + #[test(should_fail_with = "identical note hashes for a transaction")] + unconstrained fn duplicate_unique_note_hashes() { + let note_index_in_tx = 2; + let note_and_data = construct_note(VALUE, note_index_in_tx); + + let mut unique_note_hashes_in_tx = BoundedVec::from_array([ + random(), random(), random(), random(), random(), random(), random(), + ]); + + // The same unique note hash is present in two indices in the array, which is not allowed. Note that we don't + // test all note hashes for uniqueness, only those that we actually find. + unique_note_hashes_in_tx.set(note_index_in_tx, note_and_data.unique_note_hash); + unique_note_hashes_in_tx.set(note_index_in_tx + 1, note_and_data.unique_note_hash); + + let _ = attempt_note_nonce_discovery( + unique_note_hashes_in_tx, + FIRST_NULLIFIER_IN_TX, + compute_note_hash_and_nullifier, + CONTRACT_ADDRESS, + OWNER, + STORAGE_SLOT, + RANDOMNESS, + MockNote::get_id(), + BoundedVec::from_array(note_and_data.note.pack()), + ); + } }