feat: merge-train/fairies

docs/docs-developers/docs/resources/migration_notes.md

@@ -9,6 +9,16 @@ Aztec is in active development. Each version may introduce breaking changes that
 
 ## TBD
 
+### [Aztec.nr] Made `compute_note_hash_for_nullification` unconstrained
+
+This function shouldn't have been constrained in the first place, as constrained computation of `HintedNote` nullifiers is dangerous (constrained computation of nullifiers can be performed only on the `ConfirmedNote` type). If you were calling this from a constrained function, consider using `compute_confirmed_note_hash_for_nullification` instead. Unconstrained usage is safe.
+
+### [Aztec.nr] Changes to standard note hash computation
+
+Note hashes used to be computed with the storage slot being the last value of the preimage, it is now the first. This is to make it easier to ensure all note hashes have proper domain separation.
+
+This change requires no input from your side unless you were testing or relying on hardcoded note hashes.
+
 ### [Aztec.js] `getPublicEvents` now returns an object instead of an array
 
 `getPublicEvents` now returns a `GetPublicEventsResult<T>` object with `events` and `maxLogsHit` fields instead of a plain array. This enables pagination through large result sets using the new `afterLog` filter option.

noir-projects/aztec-nr/aztec/src/history/nullifier.nr

@@ -26,7 +26,7 @@ mod test;
 ///
 /// ## Cost
 ///
-/// This function performs a single merkle tree inclusion proof, which is in the order of 4k gates.
+/// This function performs a single merkle tree inclusion proof, which is ~4k gates.
 ///
 /// If you don't need to assert existence at a _specific_ past block, consider using
 /// [`PrivateContext::assert_nullifier_exists`](crate::context::PrivateContext::assert_nullifier_exists) instead, which
@@ -82,7 +82,7 @@ pub fn assert_nullifier_existed_by(block_header: BlockHeader, siloed_nullifier:
 ///
 /// ## Cost
 ///
-/// This function performs a single merkle tree inclusion proof, which is in the order of 4k gates.
+/// This function performs a single merkle tree inclusion proof, which is ~4k gates.
 pub fn assert_nullifier_did_not_exist_by(block_header: BlockHeader, siloed_nullifier: Field) {
     // 1) Get the membership witness of a low nullifier of the nullifier.
     // Safety: The witness is only used as a "magical value" that makes the proof below pass. Hence it's safe.

noir-projects/aztec-nr/aztec/src/macros/notes.nr

@@ -78,8 +78,8 @@ comptime fn generate_note_hash_trait_impl(s: TypeDefinition) -> Quoted {
     quote {
         impl aztec::note::note_interface::NoteHash for $name {
             fn compute_note_hash(self, owner: aztec::protocol::address::AztecAddress, storage_slot: Field, randomness: Field) -> Field {
-                let inputs = aztec::protocol::traits::Packable::pack(self).concat( [aztec::protocol::traits::ToField::to_field(owner), storage_slot, randomness]);
-                aztec::protocol::hash::poseidon2_hash_with_separator(inputs, aztec::protocol::constants::DOM_SEP__NOTE_HASH)
+                let data = aztec::protocol::traits::Packable::pack(self).concat([aztec::protocol::traits::ToField::to_field(owner), randomness]);
+                aztec::note::utils::compute_note_hash(storage_slot, data)
             }
 
             fn compute_nullifier(
@@ -93,10 +93,7 @@ comptime fn generate_note_hash_trait_impl(s: TypeDefinition) -> Quoted {
                 // in the quote to avoid "trait not in scope" compiler warnings.
                 let owner_npk_m_hash = aztec::protocol::traits::Hash::hash(owner_npk_m);
                 let secret = context.request_nhk_app(owner_npk_m_hash);
-                aztec::protocol::hash::poseidon2_hash_with_separator(
-                    [note_hash_for_nullification, secret],
-                    aztec::protocol::constants::DOM_SEP__NOTE_NULLIFIER as Field,
-                )
+                aztec::note::utils::compute_note_nullifier(note_hash_for_nullification, [secret])
             }
 
             unconstrained fn compute_nullifier_unconstrained(
@@ -113,10 +110,7 @@ comptime fn generate_note_hash_trait_impl(s: TypeDefinition) -> Quoted {
                     // in the quote to avoid "trait not in scope" compiler warnings.
                     let owner_npk_m_hash = aztec::protocol::traits::Hash::hash(owner_npk_m);
                     let secret = aztec::keys::getters::get_nhk_app(owner_npk_m_hash);
-                    aztec::protocol::hash::poseidon2_hash_with_separator(
-                        [note_hash_for_nullification, secret],
-                        aztec::protocol::constants::DOM_SEP__NOTE_NULLIFIER as Field,
-                    )
+                    aztec::note::utils::compute_note_nullifier(note_hash_for_nullification, [secret])
                 })
             }
         }

noir-projects/aztec-nr/aztec/src/note/utils.nr

@@ -1,6 +1,40 @@
 use crate::{context::NoteExistenceRequest, note::{ConfirmedNote, HintedNote, note_interface::NoteHash}};
 
-use crate::protocol::hash::{compute_siloed_note_hash, compute_unique_note_hash};
+use crate::protocol::{
+    constants::{DOM_SEP__NOTE_HASH, DOM_SEP__NOTE_NULLIFIER},
+    hash::{compute_siloed_note_hash, compute_unique_note_hash, poseidon2_hash_with_separator},
+};
+
+/// Computes a domain-separated note hash.
+///
+/// Receives the `storage_slot` of the [`crate::state_vars::StateVariable`] that holds the note, plus any arbitrary
+/// note `data`. This typically includes randomness, owner, and domain specific values (e.g. numeric amount, address,
+/// id, etc.).
+///
+/// Usage of this function guarantees that different state variables will never produce colliding note hashes, even if
+/// their underlying notes have different implementations.
+pub fn compute_note_hash<let N: u32>(storage_slot: Field, data: [Field; N]) -> Field {
+    // All state variables have different storage slots, so by placing this at a fixed first position in the preimage
+    // we prevent collisions.
+    poseidon2_hash_with_separator([storage_slot].concat(data), DOM_SEP__NOTE_HASH)
+}
+
+/// Computes a domain-separated note nullifier.
+///
+/// Receives the `note_hash_for_nullification` of the note (usually returned by
+/// [`compute_confirmed_note_hash_for_nullification`]), plus any arbitrary note `data`. This typically includes
+/// secrets, such as the app-siloed nullifier hiding key of the note's owner.
+///
+/// Usage of this function guarantees that different state variables will never produce colliding note nullifiers, even
+/// if their underlying notes have different implementations.
+pub fn compute_note_nullifier<let N: u32>(note_hash_for_nullification: Field, data: [Field; N]) -> Field {
+    // All notes have different note hashes for nullification (i.e. transient or settled), so by placing this at a
+    // fixed first position in the preimage we prevent collisions.
+    poseidon2_hash_with_separator(
+        [note_hash_for_nullification].concat(data),
+        DOM_SEP__NOTE_NULLIFIER,
+    )
+}
 
 /// Returns the [`NoteExistenceRequest`] used to prove a note exists.
 pub fn compute_note_existence_request<Note>(hinted_note: HintedNote<Note>) -> NoteExistenceRequest
@@ -26,21 +60,27 @@ where
     }
 }
 
-/// Returns the note hash that must be used to compute a note's nullifier when calling `NoteHash::compute_nullifier` or
-/// `NoteHash::compute_nullifier_unconstrained`.
-pub fn compute_note_hash_for_nullification<Note>(hinted_note: HintedNote<Note>) -> Field
+/// Unconstrained variant of [`compute_confirmed_note_hash_for_nullification`].
+pub unconstrained fn compute_note_hash_for_nullification<Note>(hinted_note: HintedNote<Note>) -> Field
 where
     Note: NoteHash,
 {
+    // Creating a ConfirmedNote like we do here is typically unsafe, as we've not confirmed existence. We can do it
+    // here because this is an unconstrained function, so the returned value should not make its way to a constrained
+    // function. This lets us reuse the `compute_confirmed_note_hash_for_nullification` implementation.
     compute_confirmed_note_hash_for_nullification(ConfirmedNote::new(
         hinted_note,
         compute_note_existence_request(hinted_note).note_hash(),
     ))
 }
 
-/// Same as `compute_note_hash_for_nullification`, except it takes the note hash used in a read request (i.e. what
-/// `compute_note_existence_request` would return). This is useful in scenarios where that hash has already been
-/// computed to reduce constraints by reusing this value.
+/// Returns the note hash to use when computing its nullifier.
+///
+/// The `note_hash_for_nullification` parameter [`NoteHash::compute_nullifier`] takes depends on the note's stage, e.g.
+/// settled notes use the unique note hash, but pending notes cannot as they have no nonce. This function returns the
+/// correct note hash to use.
+///
+/// Use [`compute_note_hash_for_nullification`] when computing this value in unconstrained functions.
 pub fn compute_confirmed_note_hash_for_nullification<Note>(confirmed_note: ConfirmedNote<Note>) -> Field {
     // There is just one instance in which the note hash for nullification does not match the note hash used for a read
     // request, which is when dealing with pending previous phase notes. These had their existence proven using their

noir-projects/aztec-nr/aztec/src/state_vars/delayed_public_mutable.nr

@@ -401,6 +401,10 @@ where
 {
     /// Returns the current value.
     ///
+    /// This sets the transaction's `expiration_timestamp` (see
+    /// [`PrivateContext::set_expiration_timestamp`](crate::context::PrivateContext::set_expiration_timestamp)),
+    /// invalidating the transaction if not included before that time. See the 'Privacy' section below to learn more.
+    ///
     /// If [`schedule_value_change`](DelayedPublicMutable::schedule_value_change) has never been called, then this
     /// returns the default empty public storage value, which is all zeroes - equivalent to `let t =
     /// T::unpack(std::mem::zeroed());`.
@@ -410,18 +414,16 @@ where
     ///
     /// ## Privacy
     ///
-    /// This function does leak some privacy, though in a subtle way. Understanding this is key to understanding how to
-    /// use `DelayedPublicMutable` in a privacy-preserving way.
+    /// This function leaks information via the transaction's expiration timestamp. The degree of leakage depends on
+    /// the delay that is set, and on whether a value change is currently scheduled.
     ///
     /// Private reads are based on a historical public storage read at the anchor block (i.e.
     /// [`crate::history::storage::public_storage_historical_read`]). `DelayedPublicMutable` is able to provide
-    /// guarantees about values read in the past remaining the state variable's current value into the future due to
+    /// guarantees that values read in the past remain the state variable's current value into the future due to
     /// the existence of delays when scheduling writes. It then sets the `expiration_timestamp` property of the current
-    /// transaction (see
-    /// [`PrivateContext::set_expiration_timestamp`](crate::context::PrivateContext::set_expiration_timestamp)) to
-    /// ensure that the transaction can only be included in a block **prior** to the state variable's value changing.
-    /// In other words, it knows some facts about the near future up until some time horizon, and then makes sure that
-    /// it doesn't act on this knowledge past said moment.
+    /// transaction to ensure that the transaction can only be included in a block **prior** to the state variable's
+    /// value changing. In other words, it knows some facts about the near future up until some time horizon, and then
+    /// makes sure that it doesn't act on this knowledge past said moment.
     ///
     /// Because the `expiration_timestamp` property is part of the transaction's public information, any mutation to
     /// this value could result in transaction fingerprinting. Note that multiple contracts may set this value during a
@@ -470,7 +472,7 @@ where
     ///
     /// ## Cost
     ///
-    /// This function performs a historical public storage read (which is in the order of 4k gates), **regardless of
+    /// This function performs a historical public storage read (which is ~4k gates), **regardless of
     /// `T`'s packed length**. This is because [`DelayedPublicMutable::schedule_value_change`] stores not just the
     /// value but also its hash: this function obtains the preimage from an oracle and proves that it matches the hash
     /// from public storage.

noir-projects/aztec-nr/aztec/src/state_vars/public_mutable.nr

@@ -2,6 +2,8 @@ use crate::context::{PublicContext, UtilityContext};
 use crate::protocol::traits::Packable;
 use crate::state_vars::StateVariable;
 
+mod test;
+
 /// Mutable public values.
 ///
 /// This is one of the most basic public state variables. It is equivalent to a non-`immutable` non-`constant` Solidity

noir-projects/aztec-nr/aztec/src/state_vars/public_mutable/test.nr

@@ -1,4 +1,4 @@
-use crate::{context::{PublicContext, UtilityContext}, state_vars::PublicMutable};
+use crate::{context::{PublicContext, UtilityContext}, state_vars::{PublicMutable, StateVariable}};
 use crate::test::{helpers::test_environment::TestEnvironment, mocks::MockStruct};
 use std::mem::zeroed;
 

noir-projects/aztec-nr/aztec/src/state_vars/single_use_claim.nr

@@ -1,5 +1,6 @@
 use crate::protocol::{
-    address::AztecAddress, constants::DOM_SEP__NOTE_HASH, hash::poseidon2_hash_with_separator, traits::Hash,
+    address::AztecAddress, constants::DOM_SEP__SINGLE_USE_CLAIM_NULLIFIER, hash::poseidon2_hash_with_separator,
+    traits::Hash,
 };
 
 use crate::{
@@ -62,7 +63,6 @@ mod test;
 ///
 /// Public effects you emit alongside a claim (e.g. a  public function call to update a tally) may still let observers
 /// infer who likely exercised the claim, so consider that when designing flows.
-/// ```
 pub struct SingleUseClaim<Context> {
     context: Context,
     storage_slot: Field,
@@ -82,8 +82,10 @@ impl<Context> SingleUseClaim<Context> {
     /// This function is primarily used internally by functions [`SingleUseClaim::claim`],
     /// [`SingleUseClaim::assert_claimed`] and [`SingleUseClaim::has_claimed`] to coherently write and read state.
     fn compute_nullifier(self, owner_nhk_app: Field) -> Field {
-        // TODO(F-180): make sure we follow the nullifier convention
-        poseidon2_hash_with_separator([owner_nhk_app, self.storage_slot], DOM_SEP__NOTE_HASH)
+        poseidon2_hash_with_separator(
+            [owner_nhk_app, self.storage_slot],
+            DOM_SEP__SINGLE_USE_CLAIM_NULLIFIER,
+        )
     }
 }
 

noir-projects/aztec-nr/aztec/src/test/mocks/mock_note.nr

@@ -5,11 +5,9 @@ use crate::{
     note::{HintedNote, note_interface::{NoteHash, NoteType}, note_metadata::NoteMetadata},
 };
 
-use crate::protocol::{
-    address::AztecAddress,
-    constants::{DOM_SEP__NOTE_HASH, DOM_SEP__NOTE_NULLIFIER},
-    hash::poseidon2_hash_with_separator,
-    traits::{Packable, ToField},
+use crate::{
+    note::utils::{compute_note_hash, compute_note_nullifier},
+    protocol::{address::AztecAddress, traits::{Packable, ToField}},
 };
 
 #[derive(Eq, Packable)]
@@ -26,8 +24,8 @@ impl NoteType for MockNote {
 
 impl NoteHash for MockNote {
     fn compute_note_hash(self, owner: AztecAddress, storage_slot: Field, randomness: Field) -> Field {
-        let input = self.pack().concat([owner.to_field(), storage_slot, randomness]);
-        poseidon2_hash_with_separator(input, DOM_SEP__NOTE_HASH)
+        let data = self.pack().concat([owner.to_field(), randomness]);
+        compute_note_hash(storage_slot, data)
     }
 
     fn compute_nullifier(
@@ -38,10 +36,7 @@ impl NoteHash for MockNote {
     ) -> Field {
         // We don't use any kind of secret here since this is only a mock note and having it here would make tests more
         // cumbersome
-        poseidon2_hash_with_separator(
-            [note_hash_for_nullification],
-            DOM_SEP__NOTE_NULLIFIER as Field,
-        )
+        compute_note_nullifier(note_hash_for_nullification, [])
     }
 
     unconstrained fn compute_nullifier_unconstrained(
@@ -51,12 +46,7 @@ impl NoteHash for MockNote {
     ) -> Option<Field> {
         // We don't use any kind of secret here since this is only a mock note and having it here would make tests more
         // cumbersome
-        Option::some(
-            poseidon2_hash_with_separator(
-                [note_hash_for_nullification],
-                DOM_SEP__NOTE_NULLIFIER as Field,
-            ),
-        )
+        Option::some(compute_note_nullifier(note_hash_for_nullification, []))
     }
 }
 

noir-projects/aztec-nr/uint-note/src/uint_note.nr

@@ -4,14 +4,11 @@ use aztec::{
     keys::getters::{get_nhk_app, get_public_keys, try_get_public_keys},
     macros::notes::custom_note,
     messages::logs::partial_note::compute_partial_note_private_content_log,
-    note::note_interface::{NoteHash, NoteType},
+    note::{note_interface::{NoteHash, NoteType}, utils::compute_note_nullifier},
     oracle::random::random,
     protocol::{
         address::AztecAddress,
-        constants::{
-            DOM_SEP__NOTE_HASH, DOM_SEP__NOTE_NULLIFIER, DOM_SEP__PARTIAL_NOTE_VALIDITY_COMMITMENT,
-            PRIVATE_LOG_SIZE_IN_FIELDS,
-        },
+        constants::{DOM_SEP__NOTE_HASH, DOM_SEP__PARTIAL_NOTE_VALIDITY_COMMITMENT, PRIVATE_LOG_SIZE_IN_FIELDS},
         hash::{compute_siloed_nullifier, poseidon2_hash_with_separator},
         traits::{Deserialize, FromField, Hash, Packable, Serialize, ToField},
     },
@@ -65,10 +62,7 @@ impl NoteHash for UintNote {
         let owner_npk_m = get_public_keys(owner).npk_m;
         let owner_npk_m_hash = owner_npk_m.hash();
         let secret = context.request_nhk_app(owner_npk_m_hash);
-        poseidon2_hash_with_separator(
-            [note_hash_for_nullification, secret],
-            DOM_SEP__NOTE_NULLIFIER,
-        )
+        compute_note_nullifier(note_hash_for_nullification, [secret])
     }
 
     unconstrained fn compute_nullifier_unconstrained(
@@ -80,10 +74,7 @@ impl NoteHash for UintNote {
             let owner_npk_m = public_keys.npk_m;
             let owner_npk_m_hash = owner_npk_m.hash();
             let secret = get_nhk_app(owner_npk_m_hash);
-            poseidon2_hash_with_separator(
-                [note_hash_for_nullification, secret],
-                DOM_SEP__NOTE_NULLIFIER,
-            )
+            compute_note_nullifier(note_hash_for_nullification, [secret])
         })
     }
 }

noir-projects/noir-contracts/contracts/app/claim_contract/src/main.nr

@@ -7,7 +7,10 @@ pub contract Claim {
     // docs:end:history_import
     use aztec::{
         macros::{functions::{external, initializer}, storage::storage},
-        note::{HintedNote, note_interface::NoteHash, utils::compute_note_hash_for_nullification},
+        note::{
+            HintedNote, note_interface::NoteHash,
+            utils::compute_confirmed_note_hash_for_nullification,
+        },
         protocol::address::AztecAddress,
         state_vars::PublicImmutable,
     };
@@ -48,7 +51,7 @@ pub contract Claim {
         // 3) Prove that the note hash exists in the note hash tree
         // docs:start:prove_note_inclusion
         let header = self.context.get_anchor_block_header();
-        let _ = assert_note_existed_by(header, hinted_note);
+        let confirmed_note = assert_note_existed_by(header, hinted_note);
         // docs:end:prove_note_inclusion
 
         // 4) Compute and emit a nullifier which is unique to the note and this contract to ensure the reward can be
@@ -58,7 +61,8 @@ pub contract Claim {
         // the address of a contract it was emitted from.
         // TODO(#7775): manually computing the hash and passing it to compute_nullifier func is not great as note could
         // handle it on its own or we could make assert_note_existed_by return note_hash_for_nullification.
-        let note_hash_for_nullification = compute_note_hash_for_nullification(hinted_note);
+        let note_hash_for_nullification =
+            compute_confirmed_note_hash_for_nullification(confirmed_note);
         let nullifier = hinted_note.note.compute_nullifier(
             self.context,
             hinted_note.owner,