Skip to content

fix: handle bad note lengths on compute_note_hash_and_nullifier #21271

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

Merged
nchamo merged 10 commits into from
Mar 11, 2026
Merged

fix: handle bad note lengths on compute_note_hash_and_nullifier #21271

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
351ae91
fix: return Option::none() instead of panicking on packed note length…
nchamo Mar 9, 2026
c0b1461
chore: nargo fmt
nchamo Mar 9, 2026
4677c6b
build: new test contract for compute_note_hash_and_nullifier and add …
nchamo Mar 10, 2026
a5259d7
test: verify inner nullifier propagation in compute_note_hash_and_nul…
nchamo Mar 10, 2026
f6cfecd
refactor: remove unnecessary dummy storage from test contract
nchamo Mar 10, 2026
5736e66
refactor: move tests into contract package for CI compatibility
nchamo Mar 10, 2026
64636c5
fix: remove unused TestNote import in contract module
nchamo Mar 10, 2026
3932efa
chore: improve comments in test note and warning message
nchamo Mar 10, 2026
b894c47
docs: fix inaccuracies in ComputeNoteHashAndNullifier doc comment
nchamo Mar 10, 2026
834b523
address pr review
nchamo Mar 11, 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
72 changes: 44 additions & 28 deletions noir-projects/aztec-nr/aztec/src/macros/aztec.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,34 +256,50 @@ comptime fn generate_contract_library_method_compute_note_hash_and_nullifier() -
// unpack function on it.
let expected_len = <$typ as $crate::protocol::traits::Packable>::N;
let actual_len = packed_note.len();
assert(
actual_len == expected_len,
f"Expected packed note of length {expected_len} but got {actual_len} for note type id {note_type_id}"
);

let note = $unpack(aztec::utils::array::subarray(packed_note.storage(), 0));

let note_hash = $compute_note_hash(note, owner, storage_slot, randomness);

// The message discovery process finds settled notes, that is, notes that were created in prior transactions and are therefore already part of the note hash tree. We therefore compute the nullification note hash by treating the note as a settled note with the provided note nonce.
let note_hash_for_nullification = aztec::note::utils::compute_note_hash_for_nullification(
aztec::note::HintedNote{
if actual_len != expected_len {
aztec::protocol::logging::warn_log_format(
"[aztec-nr] Packed note length mismatch: expected {0}, got {1} for note type id {2}",
[expected_len as Field, actual_len as Field, note_type_id],
);
Option::none()
} else {
let note = $unpack(aztec::utils::array::subarray(packed_note.storage(), 0));

let note_hash = $compute_note_hash(note, owner, storage_slot, randomness);

// The message discovery process finds settled notes, that is, notes that were created in
// prior transactions and are therefore already part of the note hash tree. We therefore
// compute the nullification note hash by treating the note as a settled note with the
// provided note nonce.
let note_hash_for_nullification =
aztec::note::utils::compute_note_hash_for_nullification(
aztec::note::HintedNote {
note,
contract_address,
owner,
randomness,
storage_slot,
metadata:
aztec::note::note_metadata::SettledNoteMetadata::new(
note_nonce,
)
.into(),
},
);

let inner_nullifier = $compute_nullifier_unconstrained(
note,
contract_address,
owner,
randomness,
storage_slot,
metadata: aztec::note::note_metadata::SettledNoteMetadata::new(note_nonce).into()
}
);

let inner_nullifier = $compute_nullifier_unconstrained(note, owner, note_hash_for_nullification);

Option::some(
aztec::messages::discovery::NoteHashAndNullifier {
note_hash, inner_nullifier
}
)
note_hash_for_nullification,
);

Option::some(
aztec::messages::discovery::NoteHashAndNullifier {
note_hash,
inner_nullifier,
},
)
}
}
},
);
Expand All @@ -298,7 +314,7 @@ comptime fn generate_contract_library_method_compute_note_hash_and_nullifier() -
///
/// This function is automatically injected by the `#[aztec]` macro.
#[contract_library_method]
unconstrained fn _compute_note_hash_and_nullifier(
pub(crate) unconstrained fn _compute_note_hash_and_nullifier(

@nchamo nchamo Mar 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made pub(crate) so we could test it

@benesjan benesjan Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that the tests are already in a contract crate this can be avoided by just having a simple wrapper function in the test contract that is exposed and just calls this.

Would say that is preferrable.

@nchamo nchamo Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, that would be much better 🫡

packed_note: BoundedVec<Field, aztec::messages::logs::note::MAX_NOTE_PACKED_LEN>,
owner: aztec::protocol::address::AztecAddress,
storage_slot: Field,
Expand All @@ -321,7 +337,7 @@ comptime fn generate_contract_library_method_compute_note_hash_and_nullifier() -
///
/// This function is automatically injected by the `#[aztec]` macro.
#[contract_library_method]
unconstrained fn _compute_note_hash_and_nullifier(
pub(crate) unconstrained fn _compute_note_hash_and_nullifier(
_packed_note: BoundedVec<Field, aztec::messages::logs::note::MAX_NOTE_PACKED_LEN>,
_owner: aztec::protocol::address::AztecAddress,
_storage_slot: Field,
Expand Down
32 changes: 18 additions & 14 deletions noir-projects/aztec-nr/aztec/src/messages/discovery/mod.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,23 +55,27 @@ pub struct NoteHashAndNullifier {
/// ```noir
/// |packed_note, owner, storage_slot, note_type_id, contract_address, randomness, note_nonce| {
/// if note_type_id == MyNoteType::get_id() {
/// assert(packed_note.len() == MY_NOTE_TYPE_SERIALIZATION_LENGTH);
/// if packed_note.len() != MY_NOTE_TYPE_SERIALIZATION_LENGTH {
/// Option::none()
/// } else {
/// let note = MyNoteType::unpack(aztec::utils::array::subarray(packed_note.storage(), 0));
///
/// let note = MyNoteType::unpack(aztec::utils::array::subarray(packed_note.storage(), 0));
/// let note_hash = note.compute_note_hash(owner, storage_slot, randomness);
/// let note_hash_for_nullification = aztec::note::utils::compute_note_hash_for_nullification(
/// HintedNote {

@nchamo nchamo Mar 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment since now storage_slot goes inside HintedNote

/// note, contract_address, owner, randomness, storage_slot,
/// metadata: SettledNoteMetadata::new(note_nonce).into(),
/// },
/// );
///
/// let note_hash = note.compute_note_hash(owner, storage_slot, randomness);
/// let note_hash_for_nullification = aztec::note::utils::compute_note_hash_for_nullification(
/// HintedNote{ note, contract_address, metadata: SettledNoteMetadata::new(note_nonce).into() },
/// storage_slot
/// );
/// let inner_nullifier = note.compute_nullifier_unconstrained(owner, note_hash_for_nullification);
///
/// let inner_nullifier = note.compute_nullifier_unconstrained(owner, note_hash_for_nullification);
///
/// Option::some(
/// aztec::messages::discovery::NoteHashAndNullifier {
/// note_hash, inner_nullifier
/// }
/// )
/// Option::some(
/// aztec::messages::discovery::NoteHashAndNullifier {
/// note_hash, inner_nullifier
/// }
/// )
/// }
/// } else if note_type_id == MyOtherNoteType::get_id() {
/// ... // Similar to above but calling MyOtherNoteType::unpack_content
/// } else {
Expand Down
1 change: 1 addition & 0 deletions noir-projects/noir-contracts/contracts/test/test_contract/src/test.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod compute_note_hash_and_nullifier;
mod macro_id_allocation;
mod deployment_proofs;
mod note_delivery;
48 changes: 48 additions & 0 deletions ...s/noir-contracts/contracts/test/test_contract/src/test/compute_note_hash_and_nullifier.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
use crate::{Test, test_note::TestNote};

@benesjan benesjan Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test contract is an old pile of mess so ideally would not add to that and just create a new test contract for this.

use aztec::note::note_interface::{NoteHash, NoteType};
use aztec::protocol::address::AztecAddress;

#[test]
unconstrained fn returns_none_for_bad_note_length() {
// TestNote has Packable N=1, but we provide 2 fields
let packed_note = BoundedVec::from_array([42, 99]);

let result = Test::_compute_note_hash_and_nullifier(
packed_note,
AztecAddress::zero(),
0,
TestNote::get_id(),
AztecAddress::zero(),
0,
0,
);

assert(result.is_none());
}

#[test]
unconstrained fn returns_correct_note_hash_and_nullifier() {
// TestNote has Packable N=1
let packed_note = BoundedVec::from_array([42]);

let owner = AztecAddress::zero();
let storage_slot = 0;
let randomness = 0;

let result = Test::_compute_note_hash_and_nullifier(
packed_note,
owner,
storage_slot,
TestNote::get_id(),
AztecAddress::zero(),
randomness,
1,
);

let note_hash_and_nullifier = result.unwrap();
let note = TestNote { value: 42 };
let expected_note_hash = note.compute_note_hash(owner, storage_slot, randomness);
assert_eq(note_hash_and_nullifier.note_hash, expected_note_hash);
// TestNote::compute_nullifier_unconstrained always returns None
assert(note_hash_and_nullifier.inner_nullifier.is_none());
}
Loading