Noir contract cleanup and small fixes. (#1138)

* Try comparing all note hashes to find nonce.

* Check contract address and storage slot for return notes.

* Rename set.has to set.assert_contains.

* Add same note for singleton.getNote.

* Use initialisation_nullifier generator index properly.

* Recompile contracts.

* Fix note header checker.

yarn-project/acir-simulator/src/client/private_execution.test.ts

@@ -343,27 +343,23 @@ describe('Private Execution test suite', () => {
     });
 
     it('Should be able to claim a note by providing the correct secret', async () => {
-      const contractAddress = AztecAddress.random();
       const amount = 100n;
       const secret = Fr.random();
       const abi = ZkTokenContractAbi.functions.find(f => f.name === 'claim')!;
       const storageSlot = new Fr(2n);
+      const nonce = Fr.ZERO;
 
       oracle.getNotes.mockResolvedValue([
         {
           contractAddress,
           storageSlot,
-          nonce: Fr.ZERO,
+          nonce,
           preimage: [new Fr(amount), secret],
-          index: BigInt(1),
+          index: 1n,
         },
       ]);
 
-      const customNoteHash = hash([toBufferBE(amount, 32), secret.toBuffer()]);
-      const innerNoteHash = Fr.fromBuffer(hash([storageSlot.toBuffer(), customNoteHash]));
-
       const result = await runSimulator({
-        origin: contractAddress,
         abi,
         args: [amount, secret, recipient],
       });
@@ -374,7 +370,8 @@ describe('Private Execution test suite', () => {
 
       // Check the read request was inserted successfully.
       const readRequests = result.callStackItem.publicInputs.readRequests.filter(field => !field.equals(Fr.ZERO));
-      const nonce = Fr.ZERO;
+      const customNoteHash = hash([toBufferBE(amount, 32), secret.toBuffer()]);
+      const innerNoteHash = Fr.fromBuffer(hash([storageSlot.toBuffer(), customNoteHash]));
       const uniqueNoteHash = computeUniqueCommitment(circuitsWasm, nonce, innerNoteHash);
       expect(readRequests).toEqual([uniqueNoteHash]);
     });

yarn-project/aztec-rpc/src/note_processor/note_processor.ts

@@ -102,7 +102,6 @@ export class NoteProcessor {
           indexOfTxInABlock * MAX_NEW_COMMITMENTS_PER_TX,
           (indexOfTxInABlock + 1) * MAX_NEW_COMMITMENTS_PER_TX,
         );
-        let commitmentStartIndex = 0;
         // Note: Each tx generates a `TxL2Logs` object and for this reason we can rely on its index corresponding
         //       to the index of a tx in a block.
         const txFunctionLogs = txLogs[indexOfTxInABlock].functionLogs;
@@ -118,7 +117,6 @@ export class NoteProcessor {
               userPertainingTxIndices.add(indexOfTxInABlock);
               const { index, nonce, nullifier } = await this.findNoteIndexAndNullifier(
                 dataStartIndexForTx,
-                commitmentStartIndex,
                 newCommitments,
                 newNullifiers[0],
                 noteSpendingInfo,
@@ -130,10 +128,6 @@ export class NoteProcessor {
                 nullifier,
                 publicKey: this.publicKey,
               });
-              // Since the order of the logs are always aligned with the order of their corresponding commitments,
-              // we can assume the log we will be processing next belongs to either the commitment for the current log,
-              // or one of the commitments after the current one.
-              commitmentStartIndex = Number(index) - dataStartIndexForTx;
             }
           }
         }
@@ -160,23 +154,21 @@ export class NoteProcessor {
    * contract address, and note preimage associated with the noteSpendingInfo.
    * This method assists in identifying spent commitments in the private state.
    * @param dataStartIndex - First index of the commitments in the tx in the private data tree.
-   * @param commitmentStartIndex - Index of the commitment in the tx this function should start checking.
    * @param commitments - Commitments in the tx. One of them should be the note's commitment.
    * @param firstNullifier - First nullifier in the tx.
    * @param noteSpendingInfo - An instance of NoteSpendingInfo containing transaction details.
    * @returns A Fr instance representing the computed nullifier.
    */
   private async findNoteIndexAndNullifier(
     dataStartIndex: number,
-    commitmentStartIndex: number,
     commitments: Fr[],
     firstNullifier: Fr,
     { contractAddress, storageSlot, notePreimage }: NoteSpendingInfo,
   ) {
     const wasm = await CircuitsWasm.get();
+    let commitmentIndex = 0;
     let nonce: Fr | undefined;
     let innerNullifier: Fr | undefined;
-    let commitmentIndex = commitmentStartIndex;
     for (; commitmentIndex < commitments.length; ++commitmentIndex) {
       const commitment = commitments[commitmentIndex];
       if (commitment.equals(Fr.ZERO)) break;

yarn-project/noir-contracts/src/contracts/zk_token_contract/src/main.nr

@@ -140,7 +140,7 @@ contract ZkToken {
 
         // Remove the claim note if it exists in the set.
         let mut note = ClaimNote::new(amount, secret);
-        let (new_context, note_with_header) = storage.claims.has(context, note);
+        let (new_context, note_with_header) = storage.claims.assert_contains(context, note);
         context = new_context;
         note = note_with_header;
         context = storage.claims.remove(context, note);

yarn-project/noir-libs/noir-aztec/src/abi.nr

@@ -161,7 +161,6 @@ struct PrivateCircuitPublicInputs {
     private_call_stack: [Field; MAX_PRIVATE_CALL_STACK_LENGTH_PER_CALL],
     public_call_stack: [Field; MAX_PUBLIC_CALL_STACK_LENGTH_PER_CALL],
     new_l2_to_l1_msgs: [Field; MAX_NEW_L2_TO_L1_MSGS_PER_CALL],
-    // TODO #588, relevant issue: https://github.com/AztecProtocol/aztec-packages/issues/588
     // Explore introducing a new type like uint256 (similar to Point), so it's more explicit that
     // we're talking about a single number backed by two field elements.
     encrypted_logs_hash: [Field; NUM_FIELDS_PER_SHA256],

yarn-project/noir-libs/noir-aztec/src/note/note_getter.nr

@@ -1,15 +1,30 @@
+use crate::constants_gen::{
+    MAX_READ_REQUESTS_PER_CALL,
+    GET_NOTE_ORACLE_RETURN_LENGTH,
+    GET_NOTES_ORACLE_RETURN_LENGTH,
+    MAX_NOTES_PER_PAGE,
+    VIEW_NOTE_ORACLE_RETURN_LENGTH,
+};
 use crate::context::Context;
 use crate::note::{
     note_getter_options::NoteGetterOptions,
     note_interface::NoteInterface,
     utils::compute_unique_note_hash,
 };
 use crate::oracle;
-use crate::constants_gen::MAX_READ_REQUESTS_PER_CALL;
-use crate::constants_gen::GET_NOTE_ORACLE_RETURN_LENGTH;
-use crate::constants_gen::GET_NOTES_ORACLE_RETURN_LENGTH;
-use crate::constants_gen::MAX_NOTES_PER_PAGE;
-use crate::constants_gen::VIEW_NOTE_ORACLE_RETURN_LENGTH;
+
+fn check_note_header<Note, N>(
+    context: Context,
+    storage_slot: Field,
+    note_interface: NoteInterface<Note, N>,
+    note: Note,
+) {
+    let get_header = note_interface.get_header;
+    let header = get_header(note);
+    let contract_address = context.inputs.call_context.storage_contract_address;
+    assert(header.contract_address == contract_address);
+    assert(header.storage_slot == storage_slot);
+}
 
 fn ensure_note_exists<Note, N>(
     mut context: Context,
@@ -21,14 +36,18 @@ fn ensure_note_exists<Note, N>(
     let mut unique_note_hash = 0;
     let is_dummy = note_interface.is_dummy;
     if is_dummy(note) == false {
-        let zero_fields = [0; GET_NOTE_ORACLE_RETURN_LENGTH];
-        let saved_note = oracle::notes::get_note(storage_slot, note_interface, zero_fields);
+        // Get a note from oracle and early out if it doesn't exist.
+        let saved_note = get_note_internal(storage_slot, note_interface);
         assert(is_dummy(saved_note) == false);
 
+        // Only copy over the header to the original note to make sure the preimage is the same.
         let get_header = note_interface.get_header;
         let set_header = note_interface.set_header;
-        let note_header = get_header(saved_note);
-        note_with_header = set_header(note, note_header);
+        let header = get_header(saved_note);
+        note_with_header = set_header(note, header);
+
+        check_note_header(context, storage_slot, note_interface, note_with_header);
+
         unique_note_hash = compute_unique_note_hash(note_interface, note_with_header);
     };
     context = context.push_read_request(unique_note_hash);
@@ -40,11 +59,11 @@ fn get_note<Note, N>(
     storage_slot: Field,
     note_interface: NoteInterface<Note, N>,
 ) -> (Context, Note) {
-    let zero_fields = [0; GET_NOTE_ORACLE_RETURN_LENGTH];
-    let note = oracle::notes::get_note(storage_slot, note_interface, zero_fields);
+    let note = get_note_internal(storage_slot, note_interface);
     let mut unique_note_hash = 0;
     let is_dummy = note_interface.is_dummy;
     if is_dummy(note) == false {
+        check_note_header(context, storage_slot, note_interface, note);
         unique_note_hash = compute_unique_note_hash(note_interface, note);
     };
     context = context.push_read_request(unique_note_hash);
@@ -63,13 +82,34 @@ fn get_notes<Note, N, S, P>(
         let note = notes[i];
         let mut unique_note_hash = 0;
         if is_dummy(note) == false {
+            check_note_header(context, storage_slot, note_interface, note);
             unique_note_hash = compute_unique_note_hash(note_interface, note);
         };
         context = context.push_read_request(unique_note_hash);
     };
     (context, notes)
 }
 
+// TODO: Should be unconstrained.
+fn get_note_internal<Note, N>(
+    storage_slot: Field,
+    note_interface: NoteInterface<Note, N>,
+) -> Note {
+    let dummy = note_interface.dummy;
+    let dummy_note = [dummy()];
+    let zero_fields = [0; GET_NOTE_ORACLE_RETURN_LENGTH];
+    oracle::notes::get_notes(
+        storage_slot,
+        note_interface,
+        [],
+        [],
+        1, // limit
+        0, // offset
+        dummy_note,
+        zero_fields,
+    )[0]
+}
+
 // TODO: Should be unconstrained.
 fn get_notes_internal<Note, N, S, P>(
     storage_slot: Field,
@@ -84,7 +124,6 @@ fn get_notes_internal<Note, N, S, P>(
         sort_by_indices[i] = sort_by[i].field_index;
         sort_order[i] = sort_by[i].order;
     };
-    let offset = options.offset;
     let dummy_notes = [dummy(); MAX_READ_REQUESTS_PER_CALL];
     let zero_fields = [0; GET_NOTES_ORACLE_RETURN_LENGTH];
     let notes = oracle::notes::get_notes(
@@ -93,7 +132,7 @@ fn get_notes_internal<Note, N, S, P>(
         sort_by_indices,
         sort_order,
         MAX_READ_REQUESTS_PER_CALL as u32,
-        offset,
+        options.offset,
         dummy_notes,
         zero_fields,
     );
@@ -103,6 +142,7 @@ fn get_notes_internal<Note, N, S, P>(
     filter(notes, filter_args)
 }
 
+// TODO: Should be unconstrained.
 fn view_notes<Note, N>(
     storage_slot: Field,
     note_interface: NoteInterface<Note, N>,

yarn-project/noir-libs/noir-aztec/src/oracle/notes.nr

@@ -55,37 +55,12 @@ unconstrained fn get_notes_oracle_wrapper<N, S>(
     get_notes_oracle(storage_slot, sort_by, sort_order, limit, offset, return_size, fields)
 }
 
-// TODO: The following functions should all be unconstrained.
-
-fn get_note<Note, N, NS>(
-    storage_slot: Field,
-    note_interface: NoteInterface<Note, N>,
-    zero_fields: [Field; NS],
-) -> Note {
-    let fields = get_notes_oracle_wrapper(storage_slot, [], [], 1, 0, zero_fields);
-    let has_note = fields[0] == 1;
-    let contract_address = fields[1];
-    if (has_note) {
-        let contract_address = fields[1];
-        let nonce = fields[2];
-        let header = NoteHeader { contract_address, nonce, storage_slot };
-
-        let deserialise = note_interface.deserialise;
-        let note = deserialise(arr_copy_slice(fields, [0; N], 3));
-
-        let set_header = note_interface.set_header;
-        set_header(note, header)
-    } else {
-        let dummy = note_interface.dummy;
-        dummy()
-    }
-}
-
-fn get_notes<Note, N, S, NS>(
+// TODO: Should be unconstrained.
+fn get_notes<Note, N, M, S, NS>(
     storage_slot: Field,
     note_interface: NoteInterface<Note, N>,
-    sort_by: [u8; N],
-    sort_order: [u8; N],
+    sort_by: [u8; M],
+    sort_order: [u8; M],
     limit: u32,
     offset: u32,
     mut notes: [Note; S], // TODO: Remove it and use `limit` to initialise the note array.

yarn-project/noir-libs/noir-aztec/src/state_vars/immutable_singleton.nr

@@ -1,3 +1,4 @@
+use dep::std::hash::pedersen_with_separator;
 use crate::context::Context;
 use crate::note::lifecycle::create_note;
 use crate::note::note_getter::get_note;
@@ -30,11 +31,7 @@ impl<Note, N> ImmutableSingleton<Note, N> {
     }
 
     fn compute_initialisation_nullifier(self) -> Field {
-        let storage_slot = self.storage_slot;
-        dep::std::hash::pedersen([
-            GENERATOR_INDEX__INITIALISATION_NULLIFIER,
-            storage_slot,
-        ])[0]
+        pedersen_with_separator([self.storage_slot], GENERATOR_INDEX__INITIALISATION_NULLIFIER)[0]
     }
 
     fn get_note(self, mut context: Context) -> (Context, Note) {

yarn-project/noir-libs/noir-aztec/src/state_vars/set.nr

@@ -27,7 +27,7 @@ impl<Note, N> Set<Note, N> {
         create_commitment(note_hash);
     }
 
-    fn has(self, context: Context, note: Note) -> (Context, Note) {
+    fn assert_contains(self, context: Context, note: Note) -> (Context, Note) {
         ensure_note_exists(context, self.storage_slot, self.note_interface, note)
     }
 

yarn-project/noir-libs/noir-aztec/src/state_vars/singleton.nr

@@ -1,3 +1,4 @@
+use dep::std::hash::pedersen_with_separator;
 use crate::context::Context;
 use crate::oracle;
 use crate::note::lifecycle::create_note;
@@ -31,11 +32,7 @@ impl<Note, N> Singleton<Note, N> {
     }
 
     fn compute_initialisation_nullifier(self) -> Field {
-        let storage_slot = self.storage_slot;
-        dep::std::hash::pedersen([
-            GENERATOR_INDEX__INITIALISATION_NULLIFIER,
-            storage_slot,
-        ])[0]
+        pedersen_with_separator([self.storage_slot], GENERATOR_INDEX__INITIALISATION_NULLIFIER)[0]
     }
 
     fn replace(self, mut context: Context, new_note: Note) -> (Context, Note) {
@@ -51,16 +48,16 @@ impl<Note, N> Singleton<Note, N> {
         (context, prev_note)
     }
 
-    fn get_note(self, mut context: Context, replicate: fn (Note) -> Note) -> (Context, Note) {
+    fn get_note(self, mut context: Context) -> (Context, Note) {
         let (new_context, note) = get_note(context, self.storage_slot, self.note_interface);
         context = new_context;
 
-        // Nullify current note.
+        // Nullify current note to make sure it's reading the latest note.
         context = destroy_note(context, self.storage_slot, note, self.note_interface);
 
-        // Add replica.
-        let new_note = replicate(note);
-        context = create_note(context, self.storage_slot, new_note, self.note_interface);
+        // Add the same note again.
+        // Because a nonce is added to every note in the kernel, its nullifier will be different.
+        context = create_note(context, self.storage_slot, note, self.note_interface);
 
         (context, note)
     }