test(e2e): Trigger public call stack ordering error (#1637)

Fixes #1615

.circleci/config.yml

@@ -833,6 +833,18 @@ jobs:
           command: ./scripts/cond_run_script end-to-end $JOB_NAME ./scripts/run_tests_local e2e_pending_commitments_contract.test.ts
           working_directory: yarn-project/end-to-end
 
+  e2e-ordering:
+    machine:
+      image: ubuntu-2004:202010-01
+    resource_class: large
+    steps:
+      - *checkout
+      - *setup_env
+      - run:
+          name: "Test"
+          command: ./scripts/cond_run_script end-to-end $JOB_NAME ./scripts/run_tests_local e2e_ordering.test.ts
+          working_directory: yarn-project/end-to-end
+
   uniswap-trade-on-l1-from-l2:
     machine:
       image: ubuntu-2004:202010-01
@@ -1312,6 +1324,7 @@ workflows:
       - e2e-account-contracts: *e2e_test
       - e2e-escrow-contract: *e2e_test
       - e2e-pending-commitments-contract: *e2e_test
+      - e2e-ordering: *e2e_test
       - uniswap-trade-on-l1-from-l2: *e2e_test
       - integration-l1-publisher: *e2e_test
       - integration-archiver-l1-to-l2: *e2e_test
@@ -1338,6 +1351,7 @@ workflows:
             - e2e-account-contracts
             - e2e-escrow-contract
             - e2e-pending-commitments-contract
+            - e2e-ordering
             - uniswap-trade-on-l1-from-l2
             - integration-l1-publisher
             - integration-archiver-l1-to-l2

yarn-project/acir-simulator/src/public/index.test.ts

@@ -199,7 +199,7 @@ describe('ACIR public execution simulator', () => {
         );
 
         const childContractAddress = AztecAddress.random();
-        const childValueFn = ChildContractAbi.functions.find(f => f.name === 'pubValue')!;
+        const childValueFn = ChildContractAbi.functions.find(f => f.name === 'pubGetValue')!;
         const childValueFnSelector = generateFunctionSelector(childValueFn.name, childValueFn.parameters);
 
         const initialValue = 3n;

yarn-project/aztec.js/src/utils/abi_types.ts

@@ -1,4 +1,4 @@
 import { Fr } from '@aztec/circuits.js';
 
 /** Any type that can be converted into a field for a contract call. */
-export type FieldLike = Fr | bigint | number | { /** Converts to field */ toField: () => Fr };
+export type FieldLike = Fr | Buffer | bigint | number | { /** Converts to field */ toField: () => Fr };

yarn-project/end-to-end/src/e2e_2_rpc_servers.test.ts

@@ -189,7 +189,7 @@ describe('e2e_2_rpc_servers', () => {
     const newValueToSet = 256n;
 
     const childContractWithWalletB = await ChildContract.at(childAddress, walletB);
-    const tx = childContractWithWalletB.methods.pubStoreValue(newValueToSet).send({ origin: userB.address });
+    const tx = childContractWithWalletB.methods.pubIncValue(newValueToSet).send({ origin: userB.address });
     await tx.isMined({ interval: 0.1 });
 
     const receipt = await tx.getReceipt();

yarn-project/end-to-end/src/e2e_account_contracts.test.ts

@@ -47,7 +47,7 @@ function itShouldBehaveLikeAnAccountContract(getAccountContract: (encryptionKey:
     it('calls a public function', async () => {
       const { logger, aztecRpcServer } = context;
       logger('Calling public function...');
-      const tx = child.methods.pubStoreValue(42).send();
+      const tx = child.methods.pubIncValue(42).send();
       expect(await tx.isMined({ interval: 0.1 })).toBeTruthy();
       expect(toBigInt((await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)))!)).toEqual(42n);
     }, 60_000);

yarn-project/end-to-end/src/e2e_nested_contract.test.ts

@@ -73,7 +73,7 @@ describe('e2e_nested_contract', () => {
 
     it('performs public nested calls', async () => {
       const tx = parentContract.methods
-        .pubEntryPoint(childContract.address, Fr.fromBuffer(childContract.methods.pubValue.selector), 42n)
+        .pubEntryPoint(childContract.address, Fr.fromBuffer(childContract.methods.pubGetValue.selector), 42n)
         .send({ origin: sender });
 
       await tx.isMined({ interval: 0.1 });
@@ -84,7 +84,7 @@ describe('e2e_nested_contract', () => {
 
     it('enqueues a single public call', async () => {
       const tx = parentContract.methods
-        .enqueueCallToChild(childContract.address, Fr.fromBuffer(childContract.methods.pubStoreValue.selector), 42n)
+        .enqueueCallToChild(childContract.address, Fr.fromBuffer(childContract.methods.pubIncValue.selector), 42n)
         .send({ origin: sender });
 
       await tx.isMined({ interval: 0.1 });
@@ -101,7 +101,7 @@ describe('e2e_nested_contract', () => {
       const tx = parentContract.methods
         .enqueueCallToChildTwice(
           addressToField(childContract.address),
-          Fr.fromBuffer(childContract.methods.pubStoreValue.selector).value,
+          Fr.fromBuffer(childContract.methods.pubIncValue.selector).value,
           42n,
         )
         .send({ origin: sender });
@@ -117,7 +117,7 @@ describe('e2e_nested_contract', () => {
       const tx = parentContract.methods
         .enqueueCallToPubEntryPoint(
           childContract.address,
-          Fr.fromBuffer(childContract.methods.pubStoreValue.selector),
+          Fr.fromBuffer(childContract.methods.pubIncValue.selector),
           42n,
         )
         .send({ origin: sender });
@@ -136,7 +136,7 @@ describe('e2e_nested_contract', () => {
       const tx = parentContract.methods
         .enqueueCallsToPubEntryPoint(
           childContract.address,
-          Fr.fromBuffer(childContract.methods.pubStoreValue.selector),
+          Fr.fromBuffer(childContract.methods.pubIncValue.selector),
           42n,
         )
         .send({ origin: sender });
@@ -156,7 +156,7 @@ describe('e2e_nested_contract', () => {
       const tx = parentContract.methods
         .pubEntryPointTwice(
           addressToField(childContract.address),
-          Fr.fromBuffer(childContract.methods.pubStoreValue.selector).value,
+          Fr.fromBuffer(childContract.methods.pubIncValue.selector).value,
           42n,
         )
         .send({ origin: sender });

yarn-project/end-to-end/src/e2e_ordering.test.ts

@@ -0,0 +1,66 @@
+// Test suite for testing proper ordering of side effects
+import { AztecNodeService } from '@aztec/aztec-node';
+import { AztecRPCServer } from '@aztec/aztec-rpc';
+import { BatchCall, Wallet } from '@aztec/aztec.js';
+import { Fr } from '@aztec/circuits.js';
+import { toBigInt } from '@aztec/foundation/serialize';
+import { ChildContract, ParentContract } from '@aztec/noir-contracts/types';
+import { AztecRPC, FunctionCall, L2BlockL2Logs } from '@aztec/types';
+
+import { setup } from './fixtures/utils.js';
+
+// See https://github.com/AztecProtocol/aztec-packages/issues/1601
+describe('e2e_ordering', () => {
+  let aztecNode: AztecNodeService | undefined;
+  let aztecRpcServer: AztecRPC;
+  let wallet: Wallet;
+
+  beforeEach(async () => {
+    ({ aztecNode, aztecRpcServer, wallet } = await setup());
+  }, 100_000);
+
+  afterEach(async () => {
+    await aztecNode?.stop();
+    if (aztecRpcServer instanceof AztecRPCServer) {
+      await aztecRpcServer?.stop();
+    }
+  });
+
+  describe('with parent and child contract', () => {
+    let parent: ParentContract;
+    let child: ChildContract;
+
+    beforeEach(async () => {
+      parent = await ParentContract.deploy(wallet).send().deployed();
+      child = await ChildContract.deploy(wallet).send().deployed();
+    });
+
+    const getChildStoredValue = () =>
+      aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!));
+
+    // Fails since current value at the end of execution is 10, not 20
+    it.skip('orders public function execution requests when nested call is last', async () => {
+      const actions: FunctionCall[] = [
+        child.methods.pubSetValue(10).request(),
+        parent.methods.enqueueCallToChild(child.address, child.methods.pubSetValue.selector, 20).request(),
+      ];
+
+      await new BatchCall(wallet, actions).send().wait();
+      expect(await getChildStoredValue()).toEqual(20n);
+      const logs = await aztecRpcServer.getUnencryptedLogs(1, 10).then(L2BlockL2Logs.unrollLogs);
+      expect(logs).toEqual([[10], [20]].map(Buffer.from));
+    });
+
+    it('orders public function execution requests when nested call is first', async () => {
+      const actions: FunctionCall[] = [
+        parent.methods.enqueueCallToChild(child.address, child.methods.pubSetValue.selector, 10).request(),
+        child.methods.pubSetValue(20).request(),
+      ];
+
+      await new BatchCall(wallet, actions).send().wait();
+      expect(await getChildStoredValue()).toEqual(20n);
+      const logs = await aztecRpcServer.getUnencryptedLogs(1, 10).then(L2BlockL2Logs.unrollLogs);
+      expect(logs).toEqual([[10], [20]].map(Buffer.from));
+    });
+  });
+});

yarn-project/foundation/src/abi/encoder.ts

@@ -27,7 +27,9 @@ class ArgumentEncoder {
         } else if (typeof arg === 'bigint') {
           this.flattened.push(new Fr(arg));
         } else if (typeof arg === 'object') {
-          if (typeof arg.toField === 'function') {
+          if (Buffer.isBuffer(arg)) {
+            this.flattened.push(Fr.fromBuffer(arg));
+          } else if (typeof arg.toField === 'function') {
             this.flattened.push(arg.toField());
           } else if (arg instanceof Fr) {
             this.flattened.push(arg);

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

@@ -10,6 +10,7 @@ contract Child {
         PublicContext
     };
     use crate::storage::Storage;
+    use dep::aztec::oracle::logs::emit_unencrypted_log;
 
     fn constructor(
         inputs: PrivateContextInputs,
@@ -32,7 +33,7 @@ contract Child {
     }
 
     // Returns base_value + 42.
-    open fn pubValue(inputs: PublicContextInputs, base_value: Field) -> pub abi::PublicCircuitPublicInputs {
+    open fn pubGetValue(inputs: PublicContextInputs, base_value: Field) -> pub abi::PublicCircuitPublicInputs {
         let mut context = PublicContext::new(inputs, abi::hash_args([base_value]));
 
         let returnValue = base_value + context.chain_id() + context.version() + context.block_number() + context.timestamp();
@@ -41,20 +42,26 @@ contract Child {
         context.finish()
     }
 
-    // Increments `current_value` by `new_value` and returns `new_value` + 1.
-    open fn pubStoreValue(inputs: PublicContextInputs, new_value: Field) -> pub abi::PublicCircuitPublicInputs {
+    // Sets `current_value` to `new_value`
+    open fn pubSetValue(inputs: PublicContextInputs, new_value: Field) -> pub abi::PublicCircuitPublicInputs {
+        let mut context = PublicContext::new(inputs, abi::hash_args([new_value]));
+
+        let storage = Storage::init();
+        storage.current_value.write(new_value);
+        let _hash = emit_unencrypted_log(new_value);
+        context.return_values.push(new_value);
+        context.finish()
+    }
+
+    // Increments `current_value` by `new_value`
+    open fn pubIncValue(inputs: PublicContextInputs, new_value: Field) -> pub abi::PublicCircuitPublicInputs {
         let mut context = PublicContext::new(inputs, abi::hash_args([new_value]));
 
         let storage = Storage::init();
         let old_value = storage.current_value.read();
-        // Compiler complains if we don't assign the result to anything
-        let _void1 = storage.current_value.write(old_value + new_value);
-        // Compiler fails with "we do not allow private ABI inputs to be returned as public outputs" if we try to
-        // return new_value as-is, but then it also complains if we add `pub` to `new_value` in the args, so we
-        // just assign it to another variable and tweak it so it's not the same value, and the compiler is happy.
-        let return_value = new_value + 1;
-
-        context.return_values.push(return_value);
+        storage.current_value.write(old_value + new_value);
+        let _hash = emit_unencrypted_log(new_value);
+        context.return_values.push(new_value);
         context.finish()
     }
 }

yarn-project/sequencer-client/src/sequencer/public_processor.ts

@@ -138,8 +138,7 @@ export class PublicProcessor {
     this.log(`Executing enqueued public calls for tx ${await tx.getTxHash()}`);
     if (!tx.enqueuedPublicFunctionCalls) throw new Error(`Missing preimages for enqueued public calls`);
 
-    // We execute the requests in order, which means reversing the input as the stack pops from the end of the array
-    const executionStack: (PublicExecution | PublicExecutionResult)[] = [...tx.enqueuedPublicFunctionCalls].reverse();
+    const executionStack: (PublicExecution | PublicExecutionResult)[] = [...tx.enqueuedPublicFunctionCalls];
 
     let kernelOutput = tx.data;
     let kernelProof = tx.proof;