fix: ensure_note_hash_exists (#2256)

Closes #1142 #1029 

- Rename `oracle.getCommitment` to `oracle.checkNoteHashExists`.
- Change `Set.assert_contains_and_remove(note, nonce)` to take a nonce
in addition to a note. We calculate the inner note hash from the
provided note. And although the nonce can be set to the header of the
note, by making it a required parameter of this method makes it clearer
that nonce is needed to check the existence of a note hash. (An example
will be created in a later PR to show how a recipient can learn about
the nonce if they don't have the encrypted data.)
- Change the api on CommitmentDb to only return an index of a note hash:
the index is all we need to know if a note hash exists, and to use it to
get `readRequestMembershipWitnesses` later in the kernel prover.
- (A tiny change that is not really related to this PR): We don't have
to emit storage slot when notifying the simulator about a new nullifier.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: David Banks <47112877+dbanks12@users.noreply.github.com>

yarn-project/acir-simulator/src/acvm/acvm.ts

@@ -38,6 +38,7 @@ type ORACLE_NAMES =
   | 'getSecretKey'
   | 'getNote'
   | 'getNotes'
+  | 'checkNoteHashExists'
   | 'getRandomField'
   | 'notifyCreatedNote'
   | 'notifyNullifiedNote'
@@ -46,7 +47,6 @@ type ORACLE_NAMES =
   | 'enqueuePublicFunctionCall'
   | 'storageRead'
   | 'storageWrite'
-  | 'getCommitment'
   | 'getL1ToL2Message'
   | 'getPortalContractAddress'
   | 'emitEncryptedLog'

yarn-project/acir-simulator/src/acvm/serialize.ts

@@ -9,7 +9,7 @@ import {
 } from '@aztec/circuits.js';
 import { Fr } from '@aztec/foundation/fields';
 
-import { CommitmentDataOracleInputs, MessageLoadOracleInputs } from '../client/db_oracle.js';
+import { MessageLoadOracleInputs } from '../client/db_oracle.js';
 import { ACVMField, toACVMField } from './acvm.js';
 
 // Utilities to write TS classes to ACVM Field arrays
@@ -160,23 +160,6 @@ export function toAcvmL1ToL2MessageLoadOracleInputs(
   ];
 }
 
-/**
- * Converts the result of loading commitments to ACVM fields.
- * @param commitmentLoadOracleInputs - The result of loading messages to convert.
- * @param l1ToL2MessagesTreeRoot - The L1 to L2 messages tree root
- * @returns The Message Oracle Fields.
- */
-export function toAcvmCommitmentLoadOracleInputs(
-  messageLoadOracleInputs: CommitmentDataOracleInputs,
-  l1ToL2MessagesTreeRoot: Fr,
-): ACVMField[] {
-  return [
-    toACVMField(messageLoadOracleInputs.commitment),
-    toACVMField(messageLoadOracleInputs.index),
-    toACVMField(l1ToL2MessagesTreeRoot),
-  ];
-}
-
 /**
  * Inserts a list of ACVM fields to a witness.
  * @param witnessStartIndex - The index where to start inserting the fields.

yarn-project/acir-simulator/src/client/client_execution_context.ts

@@ -6,9 +6,10 @@ import { createDebugLogger } from '@aztec/foundation/log';
 
 import {
   ACVMField,
+  ONE_ACVM_FIELD,
+  ZERO_ACVM_FIELD,
   fromACVMField,
   toACVMField,
-  toAcvmCommitmentLoadOracleInputs,
   toAcvmL1ToL2MessageLoadOracleInputs,
 } from '../acvm/index.js';
 import { PackedArgsCache } from '../common/packed_args_cache.js';
@@ -156,7 +157,7 @@ export class ClientTxExecutionContext {
     // Nullified pending notes are already removed from the list.
     const pendingNotes = this.noteCache.getNotes(contractAddress, storageSlotField);
 
-    const pendingNullifiers = this.noteCache.getNullifiers(contractAddress, storageSlotField);
+    const pendingNullifiers = this.noteCache.getNullifiers(contractAddress);
     const dbNotes = await this.db.getNotes(contractAddress, storageSlotField);
     const dbNotesFiltered = dbNotes.filter(n => !pendingNullifiers.has((n.siloedNullifier as Fr).value));
 
@@ -253,23 +254,12 @@ export class ClientTxExecutionContext {
    * Adding a siloed nullifier into the current set of all pending nullifiers created
    * within the current transaction/execution.
    * @param contractAddress - The contract address.
-   * @param storageSlot - The storage slot.
    * @param innerNullifier - The pending nullifier to add in the list (not yet siloed by contract address).
    * @param innerNoteHash - The inner note hash of the new note.
    * @param contractAddress - The contract address
    */
-  public async handleNullifiedNote(
-    contractAddress: AztecAddress,
-    storageSlot: ACVMField,
-    innerNullifier: ACVMField,
-    innerNoteHash: ACVMField,
-  ) {
-    await this.noteCache.nullifyNote(
-      contractAddress,
-      fromACVMField(storageSlot),
-      fromACVMField(innerNullifier),
-      fromACVMField(innerNoteHash),
-    );
+  public async handleNullifiedNote(contractAddress: AztecAddress, innerNullifier: ACVMField, innerNoteHash: ACVMField) {
+    await this.noteCache.nullifyNote(contractAddress, fromACVMField(innerNullifier), fromACVMField(innerNoteHash));
   }
 
   /**
@@ -285,20 +275,39 @@ export class ClientTxExecutionContext {
   /**
    * Fetches a path to prove existence of a commitment in the db, given its contract side commitment (before silo).
    * @param contractAddress - The contract address.
-   * @param commitment - The commitment.
-   * @returns The commitment data.
+   * @param nonce - The nonce of the note.
+   * @param innerNoteHash - The inner note hash of the note.
+   * @returns 1 if (persistent or transient) note hash exists, 0 otherwise. Value is in ACVMField form.
    */
-  public async getCommitment(contractAddress: AztecAddress, commitment: ACVMField) {
-    const innerNoteHash = fromACVMField(commitment);
-    // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): only works
-    // for noteHashes/commitments created by public functions! Once public kernel or
-    // base rollup circuit injects nonces, this can be used with commitments created by
-    // private functions as well.
-    const commitmentInputs = await this.db.getCommitmentOracle(contractAddress, innerNoteHash);
-    // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1029): support pending commitments here
+  public async checkNoteHashExists(
+    contractAddress: AztecAddress,
+    nonce: ACVMField,
+    innerNoteHash: ACVMField,
+  ): Promise<ACVMField> {
+    const nonceField = fromACVMField(nonce);
+    const innerNoteHashField = fromACVMField(innerNoteHash);
+    if (nonceField.isZero()) {
+      // If nonce is 0, we are looking for a new note created in this transaction.
+      const exists = this.noteCache.checkNoteExists(contractAddress, innerNoteHashField);
+      if (exists) {
+        return ONE_ACVM_FIELD;
+      }
+      // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386)
+      // Currently it can also be a note created from public if nonce is 0.
+      // If we can't find a matching new note, keep looking for the match from the notes created in previous transactions.
+    }
+
+    // If nonce is zero, SHOULD only be able to reach this point if note was publicly created
     const wasm = await CircuitsWasm.get();
-    const siloedNoteHash = siloCommitment(wasm, contractAddress, innerNoteHash);
-    this.gotNotes.set(siloedNoteHash.value, commitmentInputs.index);
-    return toAcvmCommitmentLoadOracleInputs(commitmentInputs, this.historicBlockData.privateDataTreeRoot);
+    let noteHashToLookUp = siloCommitment(wasm, contractAddress, innerNoteHashField);
+    if (!nonceField.isZero()) {
+      noteHashToLookUp = computeUniqueCommitment(wasm, nonceField, noteHashToLookUp);
+    }
+
+    const index = await this.db.getCommitmentIndex(noteHashToLookUp);
+    if (index !== undefined) {
+      this.gotNotes.set(noteHashToLookUp.value, index);
+    }
+    return index !== undefined ? ONE_ACVM_FIELD : ZERO_ACVM_FIELD;
   }
 }

yarn-project/acir-simulator/src/client/db_oracle.ts

@@ -45,22 +45,6 @@ export interface MessageLoadOracleInputs {
   index: bigint;
 }
 
-/**
- * The format noir uses to get commitments.
- */
-export interface CommitmentDataOracleInputs {
-  /** The siloed commitment. */
-  commitment: Fr;
-  /**
-   * The path in the merkle tree to the commitment.
-   */
-  siblingPath: Fr[];
-  /**
-   * The index of the message commitment in the merkle tree.
-   */
-  index: bigint;
-}
-
 /**
  * A function ABI with optional debug metadata
  */

yarn-project/acir-simulator/src/client/execution_note_cache.ts

@@ -5,41 +5,32 @@ import { Fr } from '@aztec/foundation/fields';
 
 import { NoteData } from './db_oracle.js';
 
-/**
- * Generate a key with the given contract address and storage slot.
- * @param contractAddress - Contract address.
- * @param storageSlot - Storage slot.
- */
-const generateKeyForContractStorageSlot = (contractAddress: AztecAddress, storageSlot: Fr) =>
-  `${contractAddress.toShortString}${storageSlot}`;
-
 /**
  * Data that's accessible by all the function calls in an execution.
  */
 export class ExecutionNoteCache {
   /**
-   * Notes created during execution.
-   * The key of the mapping is a value representing a contract address and storage slot combination.
+   * New notes created in this transaction.
+   * This mapping maps from a contract address to the notes in the contract.
    */
-  private newNotes: Map<string, NoteData[]> = new Map();
+  private newNotes: Map<bigint, NoteData[]> = new Map();
 
   /**
    * The list of nullifiers created in this transaction.
-   * The key of the mapping is a value representing a contract address and storage slot combination.
+   * This mapping maps from a contract address to the nullifiers emitted from the contract.
    * The note which is nullified might be new or not (i.e., was generated in a previous transaction).
    * Note that their value (bigint representation) is used because Frs cannot be looked up in Sets.
    */
-  private nullifiers: Map<string, Set<bigint>> = new Map();
+  private nullifiers: Map<bigint, Set<bigint>> = new Map();
 
   /**
    * Add a new note to cache.
    * @param note - New note created during execution.
    */
   public addNewNote(note: NoteData) {
-    const key = generateKeyForContractStorageSlot(note.contractAddress, note.storageSlot);
-    const notes = this.newNotes.get(key) ?? [];
+    const notes = this.newNotes.get(note.contractAddress.toBigInt()) ?? [];
     notes.push(note);
-    this.newNotes.set(key, notes);
+    this.newNotes.set(note.contractAddress.toBigInt(), notes);
   }
 
   /**
@@ -50,32 +41,22 @@ export class ExecutionNoteCache {
    * @param innerNoteHash - Inner note hash of the note. If this value equals EMPTY_NULLIFIED_COMMITMENT, it means the
    * note being nullified is from a previous transaction (and thus not a new note).
    */
-  public async nullifyNote(contractAddress: AztecAddress, storageSlot: Fr, innerNullifier: Fr, innerNoteHash: Fr) {
+  public async nullifyNote(contractAddress: AztecAddress, innerNullifier: Fr, innerNoteHash: Fr) {
     const wasm = await CircuitsWasm.get();
     const siloedNullifier = siloNullifier(wasm, contractAddress, innerNullifier);
-    const nullifiers = this.getNullifiers(contractAddress, storageSlot);
-    if (nullifiers.has(siloedNullifier.value)) {
-      throw new Error('Attemp to nullify the same note twice.');
-    }
-
+    const nullifiers = this.getNullifiers(contractAddress);
     nullifiers.add(siloedNullifier.value);
-    const key = generateKeyForContractStorageSlot(contractAddress, storageSlot);
-    this.nullifiers.set(key, nullifiers);
+    this.nullifiers.set(contractAddress.toBigInt(), nullifiers);
 
     // Find and remove the matching new note if the emitted innerNoteHash is not empty.
     if (!innerNoteHash.equals(new Fr(EMPTY_NULLIFIED_COMMITMENT))) {
-      const notes = this.newNotes.get(key) ?? [];
-      /**
-       * The identifier used to determine matching is the inner note hash value.
-       * However, we adopt a defensive approach and ensure that the contract address
-       * and storage slot do match.
-       */
+      const notes = this.newNotes.get(contractAddress.toBigInt()) ?? [];
       const noteIndexToRemove = notes.findIndex(n => n.innerNoteHash.equals(innerNoteHash));
       if (noteIndexToRemove === -1) {
         throw new Error('Attemp to remove a pending note that does not exist.');
       }
       notes.splice(noteIndexToRemove, 1);
-      this.newNotes.set(key, notes);
+      this.newNotes.set(contractAddress.toBigInt(), notes);
     }
   }
 
@@ -86,17 +67,26 @@ export class ExecutionNoteCache {
    * @param storageSlot - Storage slot of the notes.
    **/
   public getNotes(contractAddress: AztecAddress, storageSlot: Fr) {
-    const key = generateKeyForContractStorageSlot(contractAddress, storageSlot);
-    return this.newNotes.get(key) ?? [];
+    const notes = this.newNotes.get(contractAddress.toBigInt()) ?? [];
+    return notes.filter(n => n.storageSlot.equals(storageSlot));
   }
 
   /**
-   * Return all nullifiers of a storage slot.
-   * @param contractAddress - Contract address of the notes.
-   * @param storageSlot - Storage slot of the notes.
+   * Check if a note exists in the newNotes array.
+   * @param contractAddress - Contract address of the note.
+   * @param storageSlot - Storage slot of the note.
+   * @param innerNoteHash - Inner note hash of the note.
+   **/
+  public checkNoteExists(contractAddress: AztecAddress, innerNoteHash: Fr) {
+    const notes = this.newNotes.get(contractAddress.toBigInt()) ?? [];
+    return notes.some(n => n.innerNoteHash.equals(innerNoteHash));
+  }
+
+  /**
+   * Return all nullifiers emitted from a contract.
+   * @param contractAddress - Address of the contract.
    */
-  public getNullifiers(contractAddress: AztecAddress, storageSlot: Fr): Set<bigint> {
-    const key = generateKeyForContractStorageSlot(contractAddress, storageSlot);
-    return this.nullifiers.get(key) || new Set();
+  public getNullifiers(contractAddress: AztecAddress): Set<bigint> {
+    return this.nullifiers.get(contractAddress.toBigInt()) ?? new Set();
   }
 }

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

@@ -365,7 +365,7 @@ describe('Private Execution test suite', () => {
       expect(changeNote.preimage[0]).toEqual(new Fr(balance - amountToTransfer));
     });
 
-    it('Should be able to claim a note by providing the correct secret', async () => {
+    it('Should be able to claim a note by providing the correct secret and nonce', async () => {
       const amount = 100n;
       const secret = Fr.random();
       const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'claim');
@@ -374,22 +374,11 @@ describe('Private Execution test suite', () => {
       const nonce = new Fr(1n);
       const customNoteHash = hash([toBufferBE(amount, 32), secret.toBuffer()]);
       const innerNoteHash = Fr.fromBuffer(hash([storageSlot.toBuffer(), customNoteHash]));
-
-      oracle.getNotes.mockResolvedValue([
-        {
-          contractAddress,
-          storageSlot,
-          nonce,
-          preimage: [new Fr(amount), secret],
-          innerNoteHash,
-          siloedNullifier: new Fr(0),
-          index: 1n,
-        },
-      ]);
+      oracle.getCommitmentIndex.mockResolvedValue(2n);
 
       const result = await runSimulator({
         abi,
-        args: [amount, secret, recipient],
+        args: [amount, secret, recipient, nonce],
       });
 
       // Check a nullifier has been inserted.
@@ -751,20 +740,7 @@ describe('Private Execution test suite', () => {
       const storageSlot = new Fr(2);
       const innerNoteHash = hash([storageSlot.toBuffer(), noteHash.toBuffer()]);
       const siloedNoteHash = siloCommitment(wasm, contractAddress, Fr.fromBuffer(innerNoteHash));
-      // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1386): should insert
-      // uniqueSiloedNoteHash in tree and that should be what is expected in Noir
-      //const uniqueSiloedNoteHash = computeUniqueCommitment(wasm, nonce, Fr.fromBuffer(innerNoteHash));
-
-      const tree = await insertLeaves([siloedNoteHash]);
-
-      oracle.getCommitmentOracle.mockImplementation(async () => {
-        // Check the calculated commitment is correct
-        return Promise.resolve({
-          commitment: siloedNoteHash,
-          siblingPath: (await tree.getSiblingPath(0n, false)).toFieldArray(),
-          index: 0n,
-        });
-      });
+      oracle.getCommitmentIndex.mockResolvedValue(0n);
 
       const result = await runSimulator({
         abi,

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

@@ -99,10 +99,12 @@ export class PrivateFunctionExecution {
         this.context.handleNewNote(this.contractAddress, storageSlot, preimage, innerNoteHash);
         return Promise.resolve(ZERO_ACVM_FIELD);
       },
-      notifyNullifiedNote: async ([slot], [innerNullifier], [innerNoteHash]) => {
-        await this.context.handleNullifiedNote(this.contractAddress, slot, innerNullifier, innerNoteHash);
+      notifyNullifiedNote: async ([innerNullifier], [innerNoteHash]) => {
+        await this.context.handleNullifiedNote(this.contractAddress, innerNullifier, innerNoteHash);
         return Promise.resolve(ZERO_ACVM_FIELD);
       },
+      checkNoteHashExists: ([nonce], [innerNoteHash]) =>
+        this.context.checkNoteHashExists(this.contractAddress, nonce, innerNoteHash),
       callPrivateFunction: async ([acvmContractAddress], [acvmFunctionSelector], [acvmArgsHash]) => {
         const contractAddress = fromACVMField(acvmContractAddress);
         const functionSelector = fromACVMField(acvmFunctionSelector);
@@ -125,7 +127,6 @@ export class PrivateFunctionExecution {
       getL1ToL2Message: ([msgKey]) => {
         return this.context.getL1ToL2Message(fromACVMField(msgKey));
       },
-      getCommitment: ([commitment]) => this.context.getCommitment(this.contractAddress, commitment),
       debugLog: (...args) => {
         this.log(oracleDebugCallToFormattedStr(args));
         return Promise.resolve(ZERO_ACVM_FIELD);

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

@@ -72,13 +72,14 @@ export class UnconstrainedFunctionExecution {
           +offset,
           +returnSize,
         ),
+      checkNoteHashExists: ([nonce], [innerNoteHash]) =>
+        this.context.checkNoteHashExists(this.contractAddress, nonce, innerNoteHash),
       getRandomField: () => Promise.resolve(toACVMField(Fr.random())),
       debugLog: (...params) => {
         this.log(oracleDebugCallToFormattedStr(params));
         return Promise.resolve(ZERO_ACVM_FIELD);
       },
       getL1ToL2Message: ([msgKey]) => this.context.getL1ToL2Message(fromACVMField(msgKey)),
-      getCommitment: ([commitment]) => this.context.getCommitment(this.contractAddress, commitment),
       storageRead: async ([slot], [numberOfElements]) => {
         if (!aztecNode) {
           const errMsg = `Aztec node is undefined, cannot read storage`;