Skip to content

fix: handle missing nullifiers in nonce discovery #21328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nchamo wants to merge 8 commits into from
Closed

fix: handle missing nullifiers in nonce discovery #21328

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f6f8005
fix: handle missing nullifiers in nonce discovery by skipping notes i…
nchamo Mar 10, 2026
669fd2b
chore(aztec-nr): reflow comment to avoid nargo fmt line break
nchamo Mar 10, 2026
0365281
Apply suggestions from code review
nchamo Mar 12, 2026
4a640f2
chore(aztec-nr): include nonce in nullifier-skip warning log
nchamo Mar 12, 2026
144cfc2
Merge remote-tracking branch 'origin/merge-train/fairies' into fix/no…
nchamo Mar 13, 2026
c2ea115
Merge remote-tracking branch 'origin/merge-train/fairies' into fix/no…
nchamo Mar 13, 2026
0f40a4e
fix(aztec-nr): check nullifier availability only after confirming not…
nchamo Mar 13, 2026
9503d5a
chore(aztec-nr): clean up nonce discovery warning messages
nchamo Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 121 additions & 44 deletions noir-projects/aztec-nr/aztec/src/messages/discovery/nonce_discovery.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -35,6 +35,8 @@ pub(crate) unconstrained fn attempt_note_nonce_discovery<Env>(
packed_note: BoundedVec<Field, MAX_NOTE_PACKED_LEN>,
) -> BoundedVec<DiscoveredNoteInfo, MAX_NOTE_HASHES_PER_TX> {
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}",
Expand All @@ -51,58 +53,82 @@ pub(crate) unconstrained fn attempt_note_nonce_discovery<Env>(
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,
note_type_id,
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()],
Comment on lines +118 to +130
Copy link
Contributor

@nventuro nventuro Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure. This is the second time we run into this - might as well do it properly if we're going to be doing so much error handling.

We could have a local implementation of Result (Noir doesn't have it yet), which is the same as Option except the none case has a type:

struct Result<T, E> {
  value: T;
  error: E;
  is_ok: bool;
}

impl<T, E> Result<T, E> {
   fn ok(value: T) -> Self {
       Self { value, error: std::mem::zeroed(), is_ok: true }
   }

   fn err(error: E) -> Self {
       Self { value: std::mem::zeroed(), error,  is_ok: false }
   }
  
   fn is_ok(self) -> bool {
     ..
   }

   fn is_err(self) -> bool {
     ..
   }
  
    fn and_then(self) -> bool {
     ..
   }
}

See https://doc.rust-lang.org/std/result/ and https://doc.rust-lang.org/std/result/enum.Result.html.

wdyt?

Copy link
Contributor

@benesjan benesjan Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it but seems like something that should be in noir::std. Shall we ping Noir people if they are willing to add it?

@nchamo once this discussion gets resolved feel free to re-request review from me (just trying to keep my review work clean by un-requesting when it doesn't make sense for me to imminently review). Thanks

);
Comment on lines +118 to +131
Copy link
Contributor Author

@nchamo nchamo Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @benesjan mentioned before, we were emitting warnings inside the loop. The problem is that Option::none could be returned for reasons unrelated to the nonce, so we would end up logging the warning for every nonce. So I moved this here

}
}

Expand Down Expand Up @@ -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<Field, MAX_NOTE_PACKED_LEN>,
owner: AztecAddress,
storage_slot: Field,
note_type_id: Field,
contract_address: AztecAddress,
randomness: Field,
note_nonce: Field,
) -> Option<NoteHashAndNullifier> {
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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
Loading