fix: Broadcasting unconstrained function with empty sibling (#5429)

We were accidentally removing zero elements from the beginning of the
sibling path when broadcasting unconstrained functions. Looking at the
[logs from failed
runs](https://app.circleci.com/pipelines/github/AztecProtocol/aztec-packages/30898/workflows/4010907e-6c6c-41f7-bcc1-48e886fb30cb/jobs/1462699/parallel-runs/0),
it seems to be the culprit:

```
2024-03-25 14:15:00 sandbox_1     |   aztec:circuits:artifact_hash TRACE Computed artifact hash artifactHash=0x05ec36f43999f40c957a0ed86659191680430fc1c9f2c451a1cfea8b71f0d7e0 privateFunctionRoot=0x2183eee0e130f895c25e3cda8b7f3b367baeb0bca0ddf5689ac6979da1e8cdb0 unconstrainedFunctionRoot=0x2b65d4d40fae120ec8738725cec71ffb4e340d1a17f678e7b03c1ab05e646210 metadataHash=0x229d43c7daac528d0aefd72bee59385d7e9ff06ea477b673389e1f65168cba9f +0ms

2024-03-25 14:15:17 end-to-end_1  |   aztec:circuits:function_membership_proof TRACE Computed proof for unconstrained function with selector 0xcff0a264 functionArtifactHash=0x26d3bd465a183b367291552b08bd192043bebb5d7b07b6f8b7d43432d74af09b functionMetadataHash=0x1a8b89c75f0ad79c19ba53a8bdfcb25bd4f8d33bfa11f1f52528a8ee58fc2543 artifactMetadataHash=0x229d43c7daac528d0aefd72bee59385d7e9ff06ea477b673389e1f65168cba9f artifactFunctionTreeSiblingPath=0x0000000000000000000000000000000000000000000000000000000000000000,0x01c398cebbd80cad3c89750c7523a853883c54d2248d7f5c634b50b1cf282422 privateFunctionsArtifactTreeRoot=0x2183eee0e130f895c25e3cda8b7f3b367baeb0bca0ddf5689ac6979da1e8cdb0 +0ms

2024-03-25 14:15:23 sandbox_1     |   aztec:circuits:function_membership_proof TRACE Artifact hash mismatch expected=0x05ec36f43999f40c957a0ed86659191680430fc1c9f2c451a1cfea8b71f0d7e0 computedArtifactHash=0x16fcaee22ed912eb011b05ec01cc8bb34a189fe162f122918cc1aa141ec3cc77 computedFunctionArtifactHash=0x26d3bd465a183b367291552b08bd192043bebb5d7b07b6f8b7d43432d74af09b computedArtifactFunctionTreeRoot=0x22266dee73fdd82ff937cd96df83d2d3f96a833978c639289146aa82eceaf305 privateFunctionsArtifactTreeRoot=0x2183eee0e130f895c25e3cda8b7f3b367baeb0bca0ddf5689ac6979da1e8cdb0 metadataHash=0x229d43c7daac528d0aefd72bee59385d7e9ff06ea477b673389e1f65168cba9f artifactFunctionTreeSiblingPath=0x01c398cebbd80cad3c89750c7523a853883c54d2248d7f5c634b50b1cf282422 +0ms
```

Note how the `artifactFunctionTreeSiblingPath` starts with a zero when
computing the proof, but that's missing when verifying it.

yarn-project/circuits.js/scripts/copy-contracts.sh

@@ -3,3 +3,4 @@ set -euo pipefail
 mkdir -p ./fixtures
 
 cp "../../noir-projects/noir-contracts/target/benchmarking_contract-Benchmarking.json" ./fixtures/Benchmarking.test.json
+cp "../../noir-projects/noir-contracts/target/test_contract-Test.json" ./fixtures/Test.test.json

yarn-project/circuits.js/src/contract/artifact_hash.test.ts

@@ -1,9 +1,9 @@
-import { getSampleContractArtifact } from '../tests/fixtures.js';
+import { getBenchmarkContractArtifact } from '../tests/fixtures.js';
 import { computeArtifactHash } from './artifact_hash.js';
 
 describe('ArtifactHash', () => {
   it('calculates the artifact hash', () => {
-    const artifact = getSampleContractArtifact();
+    const artifact = getBenchmarkContractArtifact();
     expect(computeArtifactHash(artifact).toString()).toMatchInlineSnapshot(
       `"0x1cf6d98fcb8e56b65f077265ebc3f10ec7ce9fe85c8603a5a0ce09434d94dd53"`,
     );

yarn-project/circuits.js/src/contract/contract_class.test.ts

@@ -1,12 +1,12 @@
 import { FunctionSelector, FunctionType } from '@aztec/foundation/abi';
 import { Fr } from '@aztec/foundation/fields';
 
-import { getSampleContractArtifact } from '../tests/fixtures.js';
+import { getBenchmarkContractArtifact } from '../tests/fixtures.js';
 import { getContractClassFromArtifact } from './contract_class.js';
 
 describe('ContractClass', () => {
   it('creates a contract class from a contract compilation artifact', () => {
-    const artifact = getSampleContractArtifact();
+    const artifact = getBenchmarkContractArtifact();
     const contractClass = getContractClassFromArtifact({
       ...artifact,
       artifactHash: Fr.fromString('0x1234'),

yarn-project/circuits.js/src/contract/events/unconstrained_function_broadcasted_event.test.ts

@@ -1,7 +1,14 @@
+import { FunctionSelector } from '@aztec/foundation/abi';
+import { randomBytes } from '@aztec/foundation/crypto';
+import { Fr } from '@aztec/foundation/fields';
+import { Tuple } from '@aztec/foundation/serialize';
 import { setupCustomSnapshotSerializers } from '@aztec/foundation/testing';
 
 import { getSampleUnconstrainedFunctionBroadcastedEventPayload } from '../../tests/fixtures.js';
-import { UnconstrainedFunctionBroadcastedEvent } from './unconstrained_function_broadcasted_event.js';
+import {
+  BroadcastedUnconstrainedFunction,
+  UnconstrainedFunctionBroadcastedEvent,
+} from './unconstrained_function_broadcasted_event.js';
 
 describe('UnconstrainedFunctionBroadcastedEvent', () => {
   beforeAll(() => setupCustomSnapshotSerializers(expect));
@@ -10,4 +17,18 @@ describe('UnconstrainedFunctionBroadcastedEvent', () => {
     const event = UnconstrainedFunctionBroadcastedEvent.fromLogData(data);
     expect(event).toMatchSnapshot();
   });
+
+  it('filters out zero-elements at the end of the artifcat tree sibling path', () => {
+    const siblingPath: Tuple<Fr, 5> = [Fr.ZERO, new Fr(1), Fr.ZERO, new Fr(2), Fr.ZERO];
+    const event = new UnconstrainedFunctionBroadcastedEvent(
+      Fr.random(),
+      Fr.random(),
+      Fr.random(),
+      siblingPath,
+      0,
+      new BroadcastedUnconstrainedFunction(FunctionSelector.random(), Fr.random(), randomBytes(32)),
+    );
+    const filtered = event.toFunctionWithMembershipProof().artifactTreeSiblingPath;
+    expect(filtered).toEqual([Fr.ZERO, new Fr(1), Fr.ZERO, new Fr(2)]);
+  });
 });

yarn-project/circuits.js/src/contract/events/unconstrained_function_broadcasted_event.ts

@@ -1,6 +1,7 @@
 import { FunctionSelector, bufferFromFields } from '@aztec/foundation/abi';
 import { AztecAddress } from '@aztec/foundation/aztec-address';
 import { toBigIntBE } from '@aztec/foundation/bigint-buffer';
+import { removeArrayPaddingEnd } from '@aztec/foundation/collection';
 import { Fr } from '@aztec/foundation/fields';
 import { BufferReader, Tuple } from '@aztec/foundation/serialize';
 import { UnconstrainedFunction, UnconstrainedFunctionWithMembershipProof } from '@aztec/types/contracts';
@@ -85,13 +86,18 @@ export class UnconstrainedFunctionBroadcastedEvent {
   }
 
   toFunctionWithMembershipProof(): UnconstrainedFunctionWithMembershipProof {
+    // We should be able to safely remove the zero elements that pad the variable-length sibling path,
+    // since a sibling with value zero can only occur on the tree leaves, so the sibling path will never end
+    // in a zero. The only exception is a tree with depth 2 with one non-zero leaf, where the sibling path would
+    // be a single zero element, but in that case the artifact tree should be just the single leaf.
+    const artifactTreeSiblingPath = removeArrayPaddingEnd(this.artifactFunctionTreeSiblingPath, Fr.isZero);
     return {
       ...this.unconstrainedFunction,
       bytecode: this.unconstrainedFunction.bytecode,
       functionMetadataHash: this.unconstrainedFunction.metadataHash,
       artifactMetadataHash: this.artifactMetadataHash,
       privateFunctionsArtifactTreeRoot: this.privateFunctionsArtifactTreeRoot,
-      artifactTreeSiblingPath: this.artifactFunctionTreeSiblingPath.filter(fr => !fr.isZero()),
+      artifactTreeSiblingPath,
       artifactTreeLeafIndex: this.artifactFunctionTreeLeafIndex,
     };
   }

yarn-project/circuits.js/src/contract/private_function_membership_proof.test.ts

@@ -2,7 +2,7 @@ import { ContractArtifact, FunctionArtifact, FunctionSelector, FunctionType } fr
 import { Fr } from '@aztec/foundation/fields';
 import { ContractClass } from '@aztec/types/contracts';
 
-import { getSampleContractArtifact } from '../tests/fixtures.js';
+import { getBenchmarkContractArtifact } from '../tests/fixtures.js';
 import { computeVerificationKeyHash, getContractClassFromArtifact } from './contract_class.js';
 import { ContractClassIdPreimage } from './contract_class_id.js';
 import {
@@ -18,7 +18,7 @@ describe('private_function_membership_proof', () => {
   let selector: FunctionSelector;
 
   beforeAll(() => {
-    artifact = getSampleContractArtifact();
+    artifact = getBenchmarkContractArtifact();
     contractClass = getContractClassFromArtifact(artifact);
     privateFunction = artifact.functions.findLast(fn => fn.functionType === FunctionType.SECRET)!;
     vkHash = computeVerificationKeyHash(privateFunction.verificationKey!);

yarn-project/circuits.js/src/contract/public_bytecode.test.ts

@@ -1,13 +1,13 @@
 import { ContractArtifact } from '@aztec/foundation/abi';
 
-import { getSampleContractArtifact } from '../tests/fixtures.js';
+import { getBenchmarkContractArtifact } from '../tests/fixtures.js';
 import { getContractClassFromArtifact } from './contract_class.js';
 import { packBytecode, unpackBytecode } from './public_bytecode.js';
 
 describe('PublicBytecode', () => {
   let artifact: ContractArtifact;
   beforeAll(() => {
-    artifact = getSampleContractArtifact();
+    artifact = getBenchmarkContractArtifact();
   });
 
   it('packs and unpacks public bytecode', () => {

yarn-project/circuits.js/src/contract/unconstrained_function_membership_proof.test.ts

@@ -2,7 +2,7 @@ import { ContractArtifact, FunctionArtifact, FunctionSelector, FunctionType } fr
 import { Fr } from '@aztec/foundation/fields';
 import { ContractClass } from '@aztec/types/contracts';
 
-import { getSampleContractArtifact } from '../tests/fixtures.js';
+import { getTestContractArtifact } from '../tests/fixtures.js';
 import { getContractClassFromArtifact } from './contract_class.js';
 import { ContractClassIdPreimage } from './contract_class_id.js';
 import {
@@ -17,16 +17,34 @@ describe('unconstrained_function_membership_proof', () => {
   let vkHash: Fr;
   let selector: FunctionSelector;
 
-  beforeAll(() => {
-    artifact = getSampleContractArtifact();
+  beforeEach(() => {
+    artifact = getTestContractArtifact();
     contractClass = getContractClassFromArtifact(artifact);
     unconstrainedFunction = artifact.functions.findLast(fn => fn.functionType === FunctionType.UNCONSTRAINED)!;
     selector = FunctionSelector.fromNameAndParameters(unconstrainedFunction);
   });
 
+  const isUnconstrained = (fn: { functionType: FunctionType }) => fn.functionType === FunctionType.UNCONSTRAINED;
+
   it('computes and verifies a proof', () => {
+    expect(unconstrainedFunction).toBeDefined();
+    const proof = createUnconstrainedFunctionMembershipProof(selector, artifact);
+    const fn = { ...unconstrainedFunction, ...proof, selector };
+    expect(isValidUnconstrainedFunctionMembershipProof(fn, contractClass)).toBeTruthy();
+  });
+
+  it('handles a contract with a single function', () => {
+    // Remove all unconstrained functions from the contract but one
+    const unconstrainedFns = artifact.functions.filter(isUnconstrained);
+    artifact.functions = artifact.functions.filter(fn => !isUnconstrained(fn) || fn === unconstrainedFns[0]);
+    expect(artifact.functions.filter(isUnconstrained).length).toBe(1);
+
+    const unconstrainedFunction = unconstrainedFns[0];
     const proof = createUnconstrainedFunctionMembershipProof(selector, artifact);
+    expect(proof.artifactTreeSiblingPath.length).toBe(0);
+
     const fn = { ...unconstrainedFunction, ...proof, selector };
+    const contractClass = getContractClassFromArtifact(artifact);
     expect(isValidUnconstrainedFunctionMembershipProof(fn, contractClass)).toBeTruthy();
   });
 

yarn-project/circuits.js/src/tests/fixtures.ts

@@ -7,7 +7,14 @@ import { dirname, resolve } from 'path';
 import { fileURLToPath } from 'url';
 
 // Copied from the build output for the contract `Benchmarking` in noir-contracts
-export function getSampleContractArtifact(): ContractArtifact {
+export function getBenchmarkContractArtifact(): ContractArtifact {
+  const path = getPathToFixture('Benchmarking.test.json');
+  const content = JSON.parse(readFileSync(path).toString()) as NoirCompiledContract;
+  return loadContractArtifact(content);
+}
+
+// Copied from the build output for the contract `Benchmarking` in noir-contracts
+export function getTestContractArtifact(): ContractArtifact {
   const path = getPathToFixture('Benchmarking.test.json');
   const content = JSON.parse(readFileSync(path).toString()) as NoirCompiledContract;
   return loadContractArtifact(content);

yarn-project/foundation/src/collection/array.test.ts

@@ -1,4 +1,4 @@
-import { times } from './array.js';
+import { removeArrayPaddingEnd, times } from './array.js';
 
 describe('times', () => {
   it('should return an array with the result from all executions', () => {
@@ -11,3 +11,25 @@ describe('times', () => {
     expect(result).toEqual([]);
   });
 });
+
+describe('removeArrayPaddingEnd', () => {
+  it('removes padding from the end of the array', () => {
+    expect(removeArrayPaddingEnd([0, 1, 2, 0, 3, 4, 0, 0], i => i === 0)).toEqual([0, 1, 2, 0, 3, 4]);
+  });
+
+  it('does not change array if no padding', () => {
+    expect(removeArrayPaddingEnd([0, 1, 2, 0, 3, 4], i => i === 0)).toEqual([0, 1, 2, 0, 3, 4]);
+  });
+
+  it('handles no empty items ', () => {
+    expect(removeArrayPaddingEnd([1, 2, 3, 4], i => i === 0)).toEqual([1, 2, 3, 4]);
+  });
+
+  it('handles empty array', () => {
+    expect(removeArrayPaddingEnd([], i => i === 0)).toEqual([]);
+  });
+
+  it('handles array with empty items', () => {
+    expect(removeArrayPaddingEnd([0, 0, 0], i => i === 0)).toEqual([]);
+  });
+});

yarn-project/foundation/src/collection/array.ts

@@ -15,6 +15,12 @@ export function padArrayEnd<T, N extends number>(arr: T[], elem: T, length: N):
   return [...arr, ...Array(length - arr.length).fill(elem)] as Tuple<T, N>;
 }
 
+/** Removes the right-padding for an array. Does not modify original array. */
+export function removeArrayPaddingEnd<T>(arr: T[], isEmpty: (item: T) => boolean): T[] {
+  const lastNonEmptyIndex = arr.reduce((last, item, i) => (isEmpty(item) ? last : i), -1);
+  return lastNonEmptyIndex === -1 ? [] : arr.slice(0, lastNonEmptyIndex + 1);
+}
+
 /**
  * Pads an array to the target length by prepending elements at the beginning. Throws if target length exceeds the input array length. Does not modify the input array.
  * @param arr - Array with elements to pad.