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 a9aaa6c84928..42cc62251a31 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 @@ -1,6 +1,6 @@ use crate::messages::{discovery::ComputeNoteHashAndNullifier, logs::note::MAX_NOTE_PACKED_LEN}; -use crate::logging::aztecnr_debug_log_format; +use crate::logging::{aztecnr_debug_log_format, aztecnr_warn_log_format}; use crate::protocol::{ address::AztecAddress, constants::MAX_NOTE_HASHES_PER_TX, @@ -35,6 +35,8 @@ pub(crate) unconstrained fn attempt_note_nonce_discovery( packed_note: BoundedVec, ) -> BoundedVec { let discovered_notes = &mut BoundedVec::new(); + let mut had_hash_failure = false; + let mut had_nullifier_failure = false; aztecnr_debug_log_format!( "Attempting nonce discovery on {0} potential notes on contract {1} for storage slot {2}", @@ -51,9 +53,10 @@ pub(crate) unconstrained fn attempt_note_nonce_discovery( 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( + // the resulting unique note matches any in the transaction. The contract may not know how to handle this data + // (e.g. unrecognized note_type_id, incorrect packed_note length), in which case it returns None and we + // skip the candidate. + let maybe_hashes = compute_note_hash_and_nullifier( packed_note, owner, storage_slot, @@ -61,48 +64,71 @@ pub(crate) unconstrained fn attempt_note_nonce_discovery( contract_address, randomness, nonce_for_i, - ) - .expect(f"Failed to compute a note hash for note type {note_type_id}"); + ); - 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 maybe_hashes.is_none() { + had_hash_failure = true; + } else { + let hashes = maybe_hashes.unwrap(); - 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: 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 - inner_nullifier: hashes.inner_nullifier.expect( - f"Failed to compute nullifier for note type {note_type_id}", - ), - }, + 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); + + 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. + if hashes.inner_nullifier.is_some() { + discovered_notes.push( + DiscoveredNoteInfo { + note_nonce: nonce_for_i, + note_hash: hashes.note_hash, + inner_nullifier: hashes.inner_nullifier.unwrap(), + }, + ); + + // We don't exit the loop - it is possible (though rare) for the exact same note content to + // be present 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. + } else { + // TODO(F-265): consider the case for external notes even when nullifier is not computable + had_nullifier_failure = true; + } + } + } + } - // We don't exit the loop - it is possible (though rare) for the exact same note content to be present - // 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. + if discovered_notes.len() == 0 { + if had_hash_failure { + aztecnr_warn_log_format!( + "Could not compute note hash for note type {0} on contract {1}, discarding note", + )( + [note_type_id, contract_address.to_field()], + ); + } + if had_nullifier_failure { + aztecnr_warn_log_format!( + "Could not compute nullifier for note type {0} on contract {1}, discarding note", + )( + [note_type_id, contract_address.to_field()], + ); } } @@ -183,6 +209,30 @@ mod test { } } + // Like compute_note_hash_and_nullifier but returns None for the inner nullifier. + unconstrained fn compute_note_hash_only( + packed_note: BoundedVec, + owner: AztecAddress, + storage_slot: Field, + note_type_id: Field, + contract_address: AztecAddress, + randomness: Field, + note_nonce: Field, + ) -> Option { + compute_note_hash_and_nullifier( + packed_note, + owner, + storage_slot, + note_type_id, + contract_address, + randomness, + note_nonce, + ) + .map(|hashes: NoteHashAndNullifier| { + NoteHashAndNullifier { note_hash: hashes.note_hash, inner_nullifier: Option::none() } + }) + } + global VALUE: Field = 7; global FIRST_NULLIFIER_IN_TX: Field = 47; global CONTRACT_ADDRESS: AztecAddress = AztecAddress::from_field(13); @@ -210,8 +260,8 @@ mod test { assert_eq(discovered_notes.len(), 0); } - #[test(should_fail_with = "Failed to compute a note hash")] - unconstrained fn failed_hash_computation() { + #[test] + unconstrained fn unknown_note_type_is_discarded() { let unique_note_hashes_in_tx = BoundedVec::from_array([random()]); let packed_note = BoundedVec::new(); let note_type_id = 0; // This note type id is unknown to compute_note_hash_and_nullifier @@ -293,6 +343,33 @@ mod test { assert_eq(discovered_note.inner_nullifier, note_and_data.inner_nullifier); } + #[test] + unconstrained fn note_without_nullifier_is_discarded() { + 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(), + ]); + unique_note_hashes_in_tx.set(note_index_in_tx, note_and_data.unique_note_hash); + + // Simulates a note without a nullifier: the note hash is computed correctly but the nullifier is None. + let discovered_notes = attempt_note_nonce_discovery( + unique_note_hashes_in_tx, + FIRST_NULLIFIER_IN_TX, + compute_note_hash_only, + CONTRACT_ADDRESS, + OWNER, + STORAGE_SLOT, + RANDOMNESS, + MockNote::get_id(), + BoundedVec::from_array(note_and_data.note.pack()), + ); + + // The note hash matches but the nullifier couldn't be computed, so the note should be discarded. + assert_eq(discovered_notes.len(), 0); + } + #[test] unconstrained fn multiple_notes_same_preimage() { let first_note_index_in_tx = 3;