fix: better error message for compute_note_hash_and_nullifier. (#3097)

Closes #3012 

# 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).

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

@@ -71,6 +71,19 @@ export interface DBOracle extends CommitmentsDB {
     selector: FunctionSelector,
   ): Promise<FunctionArtifactWithDebugMetadata>;
 
+  /**
+   * Retrieves the artifact of a specified function within a given contract.
+   * The function is identified by its name, which is unique within a contract.
+   *
+   * @param contractAddress - The AztecAddress representing the contract containing the function.
+   * @param functionName - The name of the function.
+   * @returns The corresponding function's artifact as an object.
+   */
+  getFunctionArtifactByName(
+    contractAddress: AztecAddress,
+    functionName: string,
+  ): Promise<FunctionArtifactWithDebugMetadata | undefined>;
+
   /**
    * Retrieves the portal contract address associated with the given contract address.
    * Throws an error if the input contract address is not found or invalid.

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

@@ -18,7 +18,6 @@ import {
   computeCallStackItemHash,
   computeCommitmentNonce,
   computeSecretMessageHash,
-  computeUniqueCommitment,
   computeVarArgsHash,
   siloCommitment,
 } from '@aztec/circuits.js/abis';
@@ -226,35 +225,9 @@ describe('Private Execution test suite', () => {
         throw new Error(`Unknown address ${address}`);
       });
 
-      oracle.getFunctionArtifact.mockImplementation((_, selector) =>
-        Promise.resolve(getFunctionArtifactWithSelector(StatefulTestContractArtifact, selector)),
-      );
-    });
-
-    it('should have an artifact for computing note hash and nullifier', async () => {
-      const storageSlot = Fr.random();
-      const note = buildNote(60n, owner, storageSlot);
-
-      // Should be the same as how we compute the values for the ValueNote in the Aztec.nr library.
-      const valueNoteHash = hashFields(note.preimage);
-      const innerNoteHash = hashFields([storageSlot, valueNoteHash]);
-      const siloedNoteHash = siloCommitment(circuitsWasm, contractAddress, innerNoteHash);
-      const uniqueSiloedNoteHash = computeUniqueCommitment(circuitsWasm, note.nonce, siloedNoteHash);
-      const innerNullifier = hashFields([uniqueSiloedNoteHash, ownerPk.low, ownerPk.high]);
-
-      const result = await acirSimulator.computeNoteHashAndNullifier(
-        contractAddress,
-        note.nonce,
-        storageSlot,
-        note.preimage,
+      oracle.getFunctionArtifactByName.mockImplementation((_, functionName: string) =>
+        Promise.resolve(getFunctionArtifact(StatefulTestContractArtifact, functionName)),
       );
-
-      expect(result).toEqual({
-        innerNoteHash,
-        siloedNoteHash,
-        uniqueSiloedNoteHash,
-        innerNullifier,
-      });
     });
 
     it('should have a constructor with arguments that inserts notes', async () => {
@@ -608,6 +581,9 @@ describe('Private Execution test suite', () => {
       oracle.getFunctionArtifact.mockImplementation((_, selector) =>
         Promise.resolve(getFunctionArtifactWithSelector(PendingCommitmentsContractArtifact, selector)),
       );
+      oracle.getFunctionArtifactByName.mockImplementation((_, functionName: string) =>
+        Promise.resolve(getFunctionArtifact(PendingCommitmentsContractArtifact, functionName)),
+      );
     });
 
     it('should be able to insert, read, and nullify pending commitments in one call', async () => {

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

@@ -0,0 +1,113 @@
+import { CircuitsWasm, CompleteAddress } from '@aztec/circuits.js';
+import { computeUniqueCommitment, siloCommitment } from '@aztec/circuits.js/abis';
+import { pedersenHashInputs } from '@aztec/circuits.js/barretenberg';
+import { ABIParameterVisibility } from '@aztec/foundation/abi';
+import { AztecAddress } from '@aztec/foundation/aztec-address';
+import { Fr, GrumpkinScalar } from '@aztec/foundation/fields';
+import { TokenContractArtifact } from '@aztec/noir-contracts/artifacts';
+
+import { MockProxy, mock } from 'jest-mock-extended';
+
+import { getFunctionArtifact } from '../test/utils.js';
+import { DBOracle, FunctionArtifactWithDebugMetadata } from './db_oracle.js';
+import { AcirSimulator } from './simulator.js';
+
+describe('Simulator', () => {
+  let oracle: MockProxy<DBOracle>;
+  let simulator: AcirSimulator;
+  let circuitsWasm: CircuitsWasm;
+  let ownerCompleteAddress: CompleteAddress;
+  let owner: AztecAddress;
+  const ownerPk = GrumpkinScalar.fromString('2dcc5485a58316776299be08c78fa3788a1a7961ae30dc747fb1be17692a8d32');
+
+  const hashFields = (data: Fr[]) =>
+    Fr.fromBuffer(
+      pedersenHashInputs(
+        circuitsWasm,
+        data.map(f => f.toBuffer()),
+      ),
+    );
+
+  beforeAll(async () => {
+    circuitsWasm = await CircuitsWasm.get();
+
+    ownerCompleteAddress = await CompleteAddress.fromPrivateKeyAndPartialAddress(ownerPk, Fr.random());
+    owner = ownerCompleteAddress.address;
+  });
+
+  beforeEach(() => {
+    oracle = mock<DBOracle>();
+    oracle.getSecretKey.mockResolvedValue(ownerPk);
+    oracle.getCompleteAddress.mockResolvedValue(ownerCompleteAddress);
+
+    simulator = new AcirSimulator(oracle);
+  });
+
+  describe('computeNoteHashAndNullifier', () => {
+    const artifact = getFunctionArtifact(TokenContractArtifact, 'compute_note_hash_and_nullifier');
+    const contractAddress = AztecAddress.random();
+    const nonce = Fr.random();
+    const storageSlot = Fr.random();
+
+    const createPreimage = (amount = 123n) => [new Fr(amount), owner.toField(), Fr.random()];
+
+    it('should compute note hashes and nullifier', async () => {
+      oracle.getFunctionArtifactByName.mockResolvedValue(artifact);
+
+      const preimage = createPreimage();
+      const valueNoteHash = hashFields(preimage);
+      const innerNoteHash = hashFields([storageSlot, valueNoteHash]);
+      const siloedNoteHash = siloCommitment(circuitsWasm, contractAddress, innerNoteHash);
+      const uniqueSiloedNoteHash = computeUniqueCommitment(circuitsWasm, nonce, siloedNoteHash);
+      const innerNullifier = hashFields([uniqueSiloedNoteHash, ownerPk.low, ownerPk.high]);
+
+      const result = await simulator.computeNoteHashAndNullifier(contractAddress, nonce, storageSlot, preimage);
+
+      expect(result).toEqual({
+        innerNoteHash,
+        siloedNoteHash,
+        uniqueSiloedNoteHash,
+        innerNullifier,
+      });
+    });
+
+    it('throw if the contract does not implement "compute_note_hash_and_nullifier"', async () => {
+      oracle.getFunctionArtifactByName.mockResolvedValue(undefined);
+
+      const preimage = createPreimage();
+      await expect(
+        simulator.computeNoteHashAndNullifier(contractAddress, nonce, storageSlot, preimage),
+      ).rejects.toThrowError(/Mandatory implementation of "compute_note_hash_and_nullifier" missing/);
+    });
+
+    it('throw if a note has more fields than "compute_note_hash_and_nullifier" can process', async () => {
+      const preimage = createPreimage();
+      const wrongPreimageLength = preimage.length - 1;
+
+      const modifiedArtifact: FunctionArtifactWithDebugMetadata = {
+        ...artifact,
+        parameters: [
+          ...artifact.parameters.slice(0, -1),
+          {
+            name: 'preimage',
+            type: {
+              kind: 'array',
+              length: wrongPreimageLength,
+              type: {
+                kind: 'field',
+              },
+            },
+            visibility: ABIParameterVisibility.SECRET,
+          },
+        ],
+      };
+      oracle.getFunctionArtifactByName.mockResolvedValue(modifiedArtifact);
+
+      await expect(
+        simulator.computeNoteHashAndNullifier(contractAddress, nonce, storageSlot, preimage),
+      ).rejects.toThrowError(
+        new RegExp(`"compute_note_hash_and_nullifier" can only handle a maximum of ${wrongPreimageLength} fields`),
+      );
+    });
+  });
+});

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

@@ -1,4 +1,4 @@
-import { CallContext, FunctionData, MAX_NOTE_FIELDS_LENGTH } from '@aztec/circuits.js';
+import { CallContext, FunctionData } from '@aztec/circuits.js';
 import { Grumpkin } from '@aztec/circuits.js/barretenberg';
 import { ArrayType, FunctionSelector, FunctionType, encodeArguments } from '@aztec/foundation/abi';
 import { AztecAddress } from '@aztec/foundation/aztec-address';
@@ -161,28 +161,24 @@ export class AcirSimulator {
     storageSlot: Fr,
     notePreimage: Fr[],
   ) {
-    let artifact: FunctionArtifactWithDebugMetadata | undefined = undefined;
-
-    // Brute force
-    for (let i = notePreimage.length; i < MAX_NOTE_FIELDS_LENGTH; i++) {
-      const signature = `compute_note_hash_and_nullifier(Field,Field,Field,[Field;${i}])`;
-      const selector = FunctionSelector.fromSignature(signature);
-      try {
-        artifact = await this.db.getFunctionArtifact(contractAddress, selector);
-        if (artifact !== undefined) break;
-      } catch (e) {
-        // ignore
-      }
+    const artifact: FunctionArtifactWithDebugMetadata | undefined = await this.db.getFunctionArtifactByName(
+      contractAddress,
+      'compute_note_hash_and_nullifier',
+    );
+    if (!artifact) {
+      throw new Error(
+        `Mandatory implementation of "compute_note_hash_and_nullifier" missing in noir contract ${contractAddress.toString()}.`,
+      );
     }
 
-    if (artifact == undefined) {
+    const maxNoteFields = (artifact.parameters[artifact.parameters.length - 1].type as ArrayType).length;
+    if (maxNoteFields < notePreimage.length) {
       throw new Error(
-        `Mandatory implementation of "compute_note_hash_and_nullifier" missing in noir contract ${contractAddress.toString()}.`,
+        `The note being processed has ${notePreimage.length} fields, while "compute_note_hash_and_nullifier" can only handle a maximum of ${maxNoteFields} fields. Please consider increasing the allowed field size to accommodate all notes generated from the contract.`,
       );
     }
 
-    const preimageLen = (artifact.parameters[3].type as ArrayType).length;
-    const extendedPreimage = notePreimage.concat(Array(preimageLen - notePreimage.length).fill(Fr.ZERO));
+    const extendedPreimage = notePreimage.concat(Array(maxNoteFields - notePreimage.length).fill(Fr.ZERO));
 
     const execRequest: FunctionCall = {
       to: AztecAddress.ZERO,

yarn-project/acir-simulator/src/test/utils.ts

@@ -46,30 +46,26 @@ export const getFunctionArtifact = (
   artifact: ContractArtifact,
   functionName: string,
 ): FunctionArtifactWithDebugMetadata => {
-  const functionIndex = artifact.functions.findIndex(f => f.name === functionName);
-  if (functionIndex < 0) {
+  const functionArtifact = artifact.functions.find(f => f.name === functionName);
+  if (!functionArtifact) {
     throw new Error(`Unknown function ${functionName}`);
   }
-  const functionArtifact = artifact.functions[functionIndex];
 
   const debug = getFunctionDebugMetadata(artifact, functionName);
-
   return { ...functionArtifact, debug };
 };
 
 export const getFunctionArtifactWithSelector = (
   artifact: ContractArtifact,
   functionSelector: FunctionSelector,
 ): FunctionArtifactWithDebugMetadata => {
-  const functionIndex = artifact.functions.findIndex(f =>
+  const functionArtifact = artifact.functions.find(f =>
     functionSelector.equals(FunctionSelector.fromNameAndParameters(f.name, f.parameters)),
   );
-  if (functionIndex < 0) {
+  if (!functionArtifact) {
     throw new Error(`Unknown function ${functionSelector}`);
   }
-  const functionArtifact = artifact.functions[functionIndex];
 
   const debug = getFunctionDebugMetadata(artifact, functionArtifact.name);
-
   return { ...functionArtifact, debug };
 };

yarn-project/pxe/src/contract_data_oracle/index.ts

@@ -44,6 +44,19 @@ export class ContractDataOracle {
     return tree.getFunctionArtifact(selector);
   }
 
+  /**
+   * Retrieves the artifact of a specified function within a given contract.
+   * The function is identified by its name, which is unique within a contract.
+   *
+   * @param contractAddress - The AztecAddress representing the contract containing the function.
+   * @param functionName - The name of the function.
+   * @returns The corresponding function's artifact as an object.
+   */
+  public async getFunctionArtifactByName(contractAddress: AztecAddress, functionName: string) {
+    const contract = await this.db.getContract(contractAddress);
+    return contract?.functions.find(f => f.name === functionName);
+  }
+
   /**
    * Retrieves the debug metadata of a specified function within a given contract.
    * The function is identified by its selector, which is a unique code generated from the function's signature.

yarn-project/pxe/src/simulator_oracle/index.ts

@@ -72,6 +72,22 @@ export class SimulatorOracle implements DBOracle {
     };
   }
 
+  async getFunctionArtifactByName(
+    contractAddress: AztecAddress,
+    functionName: string,
+  ): Promise<FunctionArtifactWithDebugMetadata | undefined> {
+    const artifact = await this.contractDataOracle.getFunctionArtifactByName(contractAddress, functionName);
+    if (!artifact) {
+      return;
+    }
+
+    const debug = await this.contractDataOracle.getFunctionDebugMetadata(contractAddress, artifact.selector);
+    return {
+      ...artifact,
+      debug,
+    };
+  }
+
   async getPortalContractAddress(contractAddress: AztecAddress): Promise<EthAddress> {
     return await this.contractDataOracle.getPortalContractAddress(contractAddress);
   }