chore: Fix some instances of missing unsafe blocks (#8232)

I added a couple `unsafe {}` blocks in some constrained functions that
were constraining oracle results, in some cases updating the wording to
better explain what is going on. These were all fairly trivial to sort
out - most of the remaining instances are actually fairly interesting
and I imagine will spark discussions, such as whether AVM code should
all be unconstrained, whether tests are constrained or unconstrained,
etc.

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

@@ -12,13 +12,15 @@ trait ProveNoteInclusion {
 
 impl ProveNoteInclusion for Header {
     fn prove_note_inclusion<Note, N, M>(self, note: Note) where Note: NoteInterface<N, M> {
-        // 1) Compute note_hash
         let note_hash = compute_note_hash_for_nullify(note);
 
-        // 2) Get the membership witness of the note in the note hash tree
-        let witness = get_note_hash_membership_witness(self.global_variables.block_number as u32, note_hash);
+        let witness = unsafe {
+            get_note_hash_membership_witness(self.global_variables.block_number as u32, note_hash)
+        };
 
-        // 3) Prove that the commitment is in the note hash tree
+        // Note inclusion is fairly straightforward, since all we need to prove is that a note exists in the note tree -
+        // we don't even care _where_ in the tree it is stored. This is because entries in the note hash tree are
+        // unique.
         assert_eq(
             self.state.partial.note_hash_tree.root, root_from_sibling_path(note_hash, witness.index, witness.path), "Proving note inclusion failed"
         );

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

@@ -13,21 +13,21 @@ trait ProveNullifierInclusion {
 impl ProveNullifierInclusion for Header {
     fn prove_nullifier_inclusion(self, nullifier: Field) {
         // 1) Get the membership witness of the nullifier
-        let witness = get_nullifier_membership_witness(self.global_variables.block_number as u32, nullifier);
+        let witness = unsafe {
+            get_nullifier_membership_witness(self.global_variables.block_number as u32, nullifier)
+        };
 
-        // 2) Check that the witness we obtained matches the nullifier
-        assert(witness.leaf_preimage.nullifier == nullifier, "Nullifier does not match value in witness");
-
-        // 3) Compute the nullifier tree leaf
-        let nullifier_leaf = witness.leaf_preimage.hash();
-
-        // 4) Prove that the nullifier is in the nullifier tree
+        // 2) First we prove that the tree leaf in the witness is present in the nullifier tree. This is expected to be
+        // the leaf that contains the nullifier we're proving inclusion for.
         assert(
             self.state.partial.nullifier_tree.root
-            == root_from_sibling_path(nullifier_leaf, witness.index, witness.path), "Proving nullifier inclusion failed"
+            == root_from_sibling_path(witness.leaf_preimage.hash(), witness.index, witness.path), "Proving nullifier inclusion failed"
         );
-        // --> Now we have traversed the trees all the way up to archive root and verified that the nullifier
-        //     was included in the nullifier tree.
+
+        // 3) Then we simply check that the value in the leaf is the expected one. Note that we don't need to perform
+        // any checks on the rest of the values in the leaf preimage (the next index or next nullifier), since all we
+        // care about is showing that the tree contains an entry with the expected nullifier.
+        assert(witness.leaf_preimage.nullifier == nullifier, "Nullifier does not match value in witness");
     }
 }
 

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

@@ -12,30 +12,31 @@ trait ProveNullifierNonInclusion {
 impl ProveNullifierNonInclusion for Header {
     fn prove_nullifier_non_inclusion(self, nullifier: Field) {
         // 1) Get the membership witness of a low nullifier of the nullifier
-        let witness = get_low_nullifier_membership_witness(self.global_variables.block_number as u32, nullifier);
-
-        // 2) Prove that the nullifier is not included in the nullifier tree
-
-        // 2.a) Compute the low nullifier leaf and prove that it is in the nullifier tree
-        let low_nullifier_leaf = witness.leaf_preimage.hash();
+        let witness = unsafe {
+            get_low_nullifier_membership_witness(self.global_variables.block_number as u32, nullifier)
+        };
+
+        // 2) First we prove that the tree leaf in the witness is present in the nullifier tree. This is expected to be
+        // the 'low leaf', i.e. the leaf that would come immediately before the nullifier's leaf, if the nullifier were
+        // to be in the tree.
+        let low_nullifier_leaf = witness.leaf_preimage;
         assert(
             self.state.partial.nullifier_tree.root
-            == root_from_sibling_path(low_nullifier_leaf, witness.index, witness.path), "Proving nullifier non-inclusion failed: Could not prove low nullifier inclusion"
+            == root_from_sibling_path(low_nullifier_leaf.hash(), witness.index, witness.path), "Proving nullifier non-inclusion failed: Could not prove low nullifier inclusion"
         );
 
-        // 2.b) Prove that the low nullifier is smaller than the nullifier
+        // 3) Prove that the low leaf is indeed smaller than the nullifier
         assert(
-            full_field_less_than(witness.leaf_preimage.nullifier, nullifier), "Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed"
+            full_field_less_than(low_nullifier_leaf.nullifier, nullifier), "Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed"
         );
 
-        // 2.c) Prove that the low nullifier is pointing "over" the nullifier to prove that the nullifier is not
-        // included in the nullifier tree (or to 0 if the to-be-inserted nullifier is the largest of all)
+        // 4) Prove that the low leaf is pointing "over" the nullifier, which means that the nullifier is not included
+        // in the nullifier tree, since if it were it'd need to be between the low leaf and the next leaf. Note the
+        // special case in which the low leaf is the largest of all entries, in which case there's no 'next' entry.
         assert(
-            full_field_greater_than(witness.leaf_preimage.next_nullifier, nullifier)
-            | (witness.leaf_preimage.next_index == 0), "Proving nullifier non-inclusion failed: low_nullifier.next_value > nullifier.value check failed"
+            full_field_greater_than(low_nullifier_leaf.next_nullifier, nullifier)
+            | (low_nullifier_leaf.next_index == 0), "Proving nullifier non-inclusion failed: low_nullifier.next_value > nullifier.value check failed"
         );
-        // --> Now we have traversed the trees all the way up to archive root and verified that the nullifier
-        //     was not yet included in the nullifier tree.
     }
 }
 

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

@@ -12,24 +12,36 @@ trait PublicStorageHistoricalRead {
 
 impl PublicStorageHistoricalRead for Header {
     fn public_storage_historical_read(self, storage_slot: Field, contract_address: AztecAddress) -> Field {
-        // 1) Compute the leaf slot by siloing the storage slot with the contract address
+        // 1) Compute the leaf index by siloing the storage slot with the contract address
         let public_data_tree_index = poseidon2_hash_with_separator(
             [contract_address.to_field(), storage_slot],
             GENERATOR_INDEX__PUBLIC_LEAF_INDEX
         );
 
-        // 2) Get the membership witness of the slot
-        let witness = get_public_data_witness(
-            self.global_variables.block_number as u32,
-            public_data_tree_index
+        // 2) Get the membership witness for the tree index.
+        let witness = unsafe {
+            get_public_data_witness(
+                self.global_variables.block_number as u32,
+                public_data_tree_index
+            )
+        };
+
+        // 3) The witness is made up of two parts: the preimage of the leaf and the proof that it exists in the tree.
+        // We first prove that the witness is indeed valid for the public data tree, i.e. that the preimage is of a
+        // value present in the tree. Note that `hash` returns not just the hash of the value but also the metadata
+        // (slot, next index and next slot).
+        assert(
+            self.state.partial.public_data_tree.root
+            == root_from_sibling_path(witness.leaf_preimage.hash(), witness.index, witness.path), "Proving public value inclusion failed"
         );
 
-        // 3) Extract the value from the witness leaf and check that the storage slot is correct
+        // 4) Now that we know the preimage is valid, we determine the value that's represented by this tree entry. Here
+        // we have two scenarios:
+        // 1. The tree entry is initialized, and the value is the same as the one in the witness
+        // 2. The entry was never initialized, and the value is default zero (the default)
+        // The code below is based on the same checks in `validate_public_data_reads` in `base_rollup_inputs`.
         let preimage = witness.leaf_preimage;
 
-        // Here we have two cases. Code based on same checks in `validate_public_data_reads` in `base_rollup_inputs`
-        // 1. The value is the same as the one in the witness
-        // 2. The value was never initialized and is zero
         let is_less_than_slot = full_field_less_than(preimage.slot, public_data_tree_index);
         let is_next_greater_than = full_field_less_than(public_data_tree_index, preimage.next_slot);
         let is_max = ((preimage.next_index == 0) & (preimage.next_slot == 0));
@@ -42,13 +54,6 @@ impl PublicStorageHistoricalRead for Header {
             preimage.value
         };
 
-        // 4) Prove that the leaf exists in the public data tree. Note that `hash` returns not just the hash of the value
-        // but also the metadata (slot, next index and next slot).
-        assert(
-            self.state.partial.public_data_tree.root
-            == root_from_sibling_path(preimage.hash(), witness.index, witness.path), "Proving public value inclusion failed"
-        );
-
         value
     }
 }

noir-projects/aztec-nr/aztec/src/keys/getters/mod.nr

@@ -24,9 +24,7 @@ global KEY_REGISTRY_UPDATE_BLOCKS = 5;
 
 global KEY_REGISTRY_STORAGE_SLOT = 1;
 
-// A helper function since requesting nsk_app is very common
-// TODO(#6543)
-pub fn get_nsk_app(npk_m_hash: Field) -> Field {
+unconstrained pub fn get_nsk_app(npk_m_hash: Field) -> Field {
     get_key_validation_request(npk_m_hash, NULLIFIER_INDEX).sk_app
 }
 

noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr

@@ -80,13 +80,17 @@ pub fn get_note<Note, let N: u32, let M: u32>(
     context: &mut PrivateContext,
     storage_slot: Field
 ) -> (Note, Field) where Note: NoteInterface<N, M> {
-    let note = get_note_internal(storage_slot);
+    let note = unsafe {
+        get_note_internal(storage_slot)
+    };
 
+    // Constraining that we got a valid note from the oracle is fairly straightforward: all we need to do is check that
+    // the metadata is correct, and that the note exists.
     check_note_header(*context, storage_slot, note);
 
     let note_hash_for_read_request = compute_note_hash_for_read_request(note);
-
     context.push_note_hash_read_request(note_hash_for_read_request);
+
     (note, note_hash_for_read_request)
 }
 
@@ -95,8 +99,12 @@ pub fn get_notes<Note, let N: u32, let M: u32, PREPROCESSOR_ARGS, FILTER_ARGS>(
     storage_slot: Field,
     options: NoteGetterOptions<Note, N, M, PREPROCESSOR_ARGS, FILTER_ARGS>
 ) -> (BoundedVec<Note, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL>, BoundedVec<Field, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL>) where Note: NoteInterface<N, M> + Eq {
-    let opt_notes = get_notes_internal(storage_slot, options);
+    let opt_notes = unsafe {
+        get_notes_internal(storage_slot, options)
+    };
 
+    // We apply the constraints in a separate function instead of inlining them here to make it easier to test that
+    // these checks correctly reject bad notes.
     constrain_get_notes_internal(context, storage_slot, opt_notes, options)
 }
 

noir-projects/aztec-nr/aztec/src/oracle/get_contract_instance.nr

@@ -19,8 +19,13 @@ unconstrained pub fn get_contract_instance_internal_avm(address: AztecAddress) -
 }
 
 pub fn get_contract_instance(address: AztecAddress) -> ContractInstance {
-    let instance = ContractInstance::deserialize(get_contract_instance_internal(address));
-    assert(instance.to_address().eq(address));
+    let instance = unsafe {
+        ContractInstance::deserialize(get_contract_instance_internal(address))
+    };
+    // The to_address function combines all values in the instance object to produce an address, so by checking that we
+    // get the expected address we validate the entire struct.
+    assert_eq(instance.to_address(), address);
+
     instance
 }
 

noir-projects/aztec-nr/aztec/src/oracle/header.nr

@@ -34,7 +34,9 @@ pub fn get_header_at(block_number: u32, context: PrivateContext) -> Header {
         );
 
         // 3) Get the header hint of a given block from an oracle
-        let historical = get_header_at_internal(block_number);
+        let historical = unsafe {
+            get_header_at_internal(block_number)
+        };
 
         // 4) We make sure that the header hint we received from the oracle exists in the state tree and is the actual header
         // at the desired block number
@@ -60,7 +62,9 @@ fn constrain_get_header_at_internal(
     let block_hash = header_hint.hash();
 
     // 2) Get the membership witness of the block in the archive tree
-    let witness = get_archive_membership_witness(last_archive_block_number, block_hash);
+    let witness = unsafe {
+        get_archive_membership_witness(last_archive_block_number, block_hash)
+    };
 
     // 3) Check that the block is in the archive (i.e. the witness is valid)
     assert(

noir-projects/aztec-nr/aztec/src/oracle/key_validation_request.nr

@@ -14,7 +14,9 @@ unconstrained fn get_key_validation_request_internal(
     KeyValidationRequest::deserialize(result)
 }
 
-pub fn get_key_validation_request(pk_m_hash: Field, key_index: Field) -> KeyValidationRequest {
+unconstrained pub fn get_key_validation_request(
+    pk_m_hash: Field,
+    key_index: Field
+) -> KeyValidationRequest {
     get_key_validation_request_internal(pk_m_hash, key_index)
 }
-

noir-projects/aztec-nr/aztec/src/oracle/keys.nr

@@ -4,12 +4,8 @@ use dep::protocol_types::{address::{AztecAddress, PartialAddress}, point::Point}
 #[oracle(getPublicKeysAndPartialAddress)]
 unconstrained fn get_public_keys_and_partial_address_oracle(_address: AztecAddress) -> [Field; 13] {}
 
-unconstrained fn get_public_keys_and_partial_address_oracle_wrapper(address: AztecAddress) -> [Field; 13] {
-    get_public_keys_and_partial_address_oracle(address)
-}
-
-pub fn get_public_keys_and_partial_address(address: AztecAddress) -> (PublicKeys, PartialAddress) {
-    let result = get_public_keys_and_partial_address_oracle_wrapper(address);
+unconstrained pub fn get_public_keys_and_partial_address(address: AztecAddress) -> (PublicKeys, PartialAddress) {
+    let result = get_public_keys_and_partial_address_oracle(address);
 
     let keys = PublicKeys {
         npk_m: NpkM { inner: Point { x: result[0], y: result[1], is_infinite: result[2] as bool } },

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

@@ -70,13 +70,15 @@ impl<Note, N, M> PrivateMutable<Note, &mut PrivateContext> where Note: NoteInter
     // docs:end:replace
 
     pub fn initialize_or_replace(self, note: &mut Note) -> NoteEmission<Note> {
-        let is_initialized = check_nullifier_exists(self.compute_initialization_nullifier());
+        let is_initialized = unsafe {
+            check_nullifier_exists(self.compute_initialization_nullifier())
+        };
 
-        // check_nullifier_exists() is an unconstrained function - we can constrain a true value by providing an 
-        // inclusion proof of the nullifier, but cannot constrain a false value since a non-inclusion proof would only 
+        // check_nullifier_exists() is an unconstrained function - we can constrain a true value by providing an
+        // inclusion proof of the nullifier, but cannot constrain a false value since a non-inclusion proof would only
         // be valid if done in public.
         // Ultimately, this is not an issue ginen that we'll either:
-        //  - initialize the state variable, which would fail if it was already initialized due to the duplicate 
+        //  - initialize the state variable, which would fail if it was already initialized due to the duplicate
         //    nullifier, or
         //  - replace the current value, which would fail if it was not initialized since we wouldn't be able to produce
         //    an inclusion proof for the current note

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

@@ -97,27 +97,28 @@ impl<T, let INITIAL_DELAY: u32, Context> SharedMutable<T, INITIAL_DELAY, Context
         // Instead, we get an oracle to provide us the correct values for both the value and delay changes, and instead
         // prove inclusion of their hash, which is both a much smaller proof (a single slot), and also independent of
         // the size of T.
-        let (value_change_hint, delay_change_hint) = get_public_storage_hints(address, self.storage_slot, historical_block_number);
+        let (value_change_hint, delay_change_hint) = unsafe {
+            get_public_storage_hints(address, self.storage_slot, historical_block_number)
+        };
 
         // Ideally the following would be simply public_storage::read_historical, but we can't implement that yet.
         let hash = header.public_storage_historical_read(self.get_hash_storage_slot(), address);
 
-        // @todo This is written strangely to bypass a formatting issue with the if that is breaking ci.
-        let (a, b, c) = if hash != 0 {
-            let a = SharedMutable::hash_scheduled_data(value_change_hint, delay_change_hint);
-            (a, value_change_hint, delay_change_hint)
+        if hash != 0 {
+            assert_eq(
+                hash, SharedMutable::hash_scheduled_data(value_change_hint, delay_change_hint), "Hint values do not match hash"
+            );
         } else {
             // The hash slot can only hold a zero if it is uninitialized, meaning no value or delay change was ever
             // scheduled. Therefore, the hints must then correspond to uninitialized scheduled changes.
-            let b = ScheduledValueChange::deserialize(zeroed());
-            let c = ScheduledDelayChange::deserialize(zeroed());
-            (hash, b, c)
+            assert_eq(
+                value_change_hint, ScheduledValueChange::deserialize(zeroed()), "Non-zero value change for zero hash"
+            );
+            assert_eq(
+                delay_change_hint, ScheduledDelayChange::deserialize(zeroed()), "Non-zero delay change for zero hash"
+            );
         };
 
-        assert_eq(hash, a, "Hint values do not match hash");
-        assert_eq(value_change_hint, b, "Non-zero value change for zero hash");
-        assert_eq(delay_change_hint, c, "Non-zero delay change for zero hash");
-
         (value_change_hint, delay_change_hint, historical_block_number)
     }
 }

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

@@ -6,14 +6,19 @@ use dep::protocol_types::traits::Eq;
 // this returns
 //   collapsed: [3, 1]
 pub fn collapse_array<T, let N: u32>(input: [Option<T>; N]) -> BoundedVec<T, N> where T: Eq {
+    // Computing the collpased BoundedVec would result in a very large number of constraints, since we'd need to loop
+    // over the input array and conditionally write to a dynamic vec index, which is a very unfriendly pattern to the
+    // proving backend.
+    // Instead, we use an unconstrained function to produce the final collapsed array, along with some hints, and then
+    // verify that the input and collapsed arrays are equivalent.
     let (collapsed, collapsed_to_input_index_mapping) = unsafe {
         get_collapse_hints(input)
     };
     verify_collapse_hints(input, collapsed, collapsed_to_input_index_mapping);
     collapsed
 }
 
-pub fn verify_collapse_hints<T, let N: u32>(
+fn verify_collapse_hints<T, let N: u32>(
     input: [Option<T>; N],
     collapsed: BoundedVec<T, N>,
     collapsed_to_input_index_mapping: BoundedVec<u32, N>

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

@@ -3,4 +3,4 @@ mod comparison;
 mod point;
 mod test;
 
-use crate::utils::collapse_array::{collapse_array, verify_collapse_hints};
+use crate::utils::collapse_array::collapse_array;