hack(ordering): public state action ordering (#1685)

Resolves #1617
Resolves #1622 
Resolves #1623 

Patches the `PublicDataReads` and `PublicDataUpdateRequests` coming from
the public kernel to reorder them properly until public state is
properly reordered (#757).

---------

circuits/cpp/src/aztec3/circuits/abis/c_bind.cpp

@@ -452,6 +452,16 @@ CBIND(abis__compute_block_hash_with_globals, aztec3::circuits::compute_block_has
  */
 CBIND(abis__compute_globals_hash, aztec3::circuits::compute_globals_hash<NT>);
 
+/**
+ * @brief Compute the value to be inserted into the public data tree
+ */
+CBIND(abis__compute_public_data_tree_value, aztec3::circuits::compute_public_data_tree_value<NT>);
+
+/**
+ * @brief Compute the index for inserting a value into the public data tree
+ */
+CBIND(abis__compute_public_data_tree_index, aztec3::circuits::compute_public_data_tree_index<NT>);
+
 /**
  * @brief Generates a signed tx request hash from it's pre-image
  * This is a WASM-export that can be called from Typescript.

circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp

@@ -224,7 +224,7 @@ void perform_static_call_checks(Builder& builder, KernelInput const& public_kern
 }
 
 /**
- * @brief Proagates valid (i.e. non-empty) update requests from this iteration to the circuit output
+ * @brief Propagates valid (i.e. non-empty) update requests from this iteration to the circuit output
  * @tparam The type of kernel input
  * @param public_kernel_inputs The inputs to this iteration of the kernel circuit
  * @param circuit_outputs The circuit outputs to be populated

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

@@ -105,5 +105,5 @@ export function collectEnqueuedPublicFunctionCalls(execResult: ExecutionResult):
   return [
     ...execResult.enqueuedPublicFunctionCalls,
     ...[...execResult.nestedExecutions].flatMap(collectEnqueuedPublicFunctionCalls),
-  ].sort((a, b) => b.order! - a.order!);
+  ].sort((a, b) => b.sideEffectCounter! - a.sideEffectCounter!); // REVERSE SORT!
 }

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

@@ -798,7 +798,7 @@ describe('Private Execution test suite', () => {
           isDelegateCall: false,
           isStaticCall: false,
         }),
-        order: 0,
+        sideEffectCounter: 0,
       });
 
       const publicCallRequestHash = computeCallStackItemHash(

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

@@ -28,9 +28,6 @@ import { AcirSimulator, ExecutionResult, NewNoteData, NewNullifierData } from '.
 import { ClientTxExecutionContext } from './client_execution_context.js';
 import { acvmFieldMessageToString, oracleDebugCallToFormattedStr } from './debug.js';
 
-/** Orderings of side effects */
-type Orderings = { /** Public call stack execution requests */ publicCall: number };
-
 /**
  * The private function execution class.
  */
@@ -43,7 +40,7 @@ export class PrivateFunctionExecution {
     private argsHash: Fr,
     private callContext: CallContext,
     private curve: Grumpkin,
-    private orderings: Orderings = { publicCall: 0 },
+    private sideEffectCounter: number = 0,
     private log = createDebugLogger('aztec:simulator:secret_execution'),
   ) {}
 
@@ -142,11 +139,10 @@ export class PrivateFunctionExecution {
           frToSelector(fromACVMField(acvmFunctionSelector)),
           this.context.packedArgsCache.unpack(fromACVMField(acvmArgsHash)),
           this.callContext,
-          this.orderings,
         );
 
         this.log(
-          `Enqueued call to public function #${enqueuedRequest.order} ${acvmContractAddress}:${acvmFunctionSelector}`,
+          `Enqueued call to public function (with side-effect counter #${enqueuedRequest.sideEffectCounter}) ${acvmContractAddress}:${acvmFunctionSelector}`,
         );
         enqueuedPublicFunctionCalls.push(enqueuedRequest);
         return toAcvmEnqueuePublicFunctionResult(enqueuedRequest);
@@ -279,7 +275,7 @@ export class PrivateFunctionExecution {
       targetArgsHash,
       derivedCallContext,
       curve,
-      this.orderings,
+      this.sideEffectCounter,
       this.log,
     );
 
@@ -294,27 +290,29 @@ export class PrivateFunctionExecution {
    * @param targetFunctionSelector - The function selector of the function to call.
    * @param targetArgs - The arguments to pass to the function.
    * @param callerContext - The call context of the caller.
-   * @param orderings - Orderings.
    * @returns The public call stack item with the request information.
    */
   private async enqueuePublicFunctionCall(
     targetContractAddress: AztecAddress,
     targetFunctionSelector: Buffer,
     targetArgs: Fr[],
     callerContext: CallContext,
-    orderings: Orderings,
   ): Promise<PublicCallRequest> {
     const targetAbi = await this.context.db.getFunctionABI(targetContractAddress, targetFunctionSelector);
     const derivedCallContext = await this.deriveCallContext(callerContext, targetContractAddress, false, false);
-    const order = orderings.publicCall++;
 
     return PublicCallRequest.from({
       args: targetArgs,
       callContext: derivedCallContext,
       functionData: FunctionData.fromAbi(targetAbi),
       contractAddress: targetContractAddress,
-      order,
+      sideEffectCounter: this.sideEffectCounter++, // update after assigning current value to call
     });
+
+    // TODO($846): if enqueued public calls are associated with global
+    // side-effect counter, that will leak info about how many other private
+    // side-effects occurred in the TX. Ultimately the private kernel should
+    // just outut everything in the proper order without any counters.
   }
 
   /**

yarn-project/acir-simulator/src/public/execution.ts

@@ -5,7 +5,11 @@ import {
   ContractStorageUpdateRequest,
   Fr,
   FunctionData,
+  PublicDataRead,
+  PublicDataUpdateRequest,
 } from '@aztec/circuits.js';
+import { computePublicDataTreeIndex, computePublicDataTreeValue } from '@aztec/circuits.js/abis';
+import { IWasmModule } from '@aztec/foundation/wasm';
 import { FunctionL2Logs } from '@aztec/types';
 
 /**
@@ -59,3 +63,87 @@ export function isPublicExecutionResult(
 ): input is PublicExecutionResult {
   return !!(input as PublicExecutionResult).execution;
 }
+
+/**
+ * Collect all public storage reads across all nested executions
+ * and convert them to PublicDataReads (to match kernel output).
+ * @param wasm - A module providing low-level wasm access.
+ * @param execResult - The topmost execution result.
+ * @returns All public data reads (in execution order).
+ */
+export function collectPublicDataReads(wasm: IWasmModule, execResult: PublicExecutionResult): PublicDataRead[] {
+  // HACK(#1622): part of temporary hack - may be able to remove this function after public state ordering is fixed
+  const contractAddress = execResult.execution.contractAddress;
+
+  const thisExecPublicDataReads = execResult.contractStorageReads.map(read =>
+    contractStorageReadToPublicDataRead(wasm, read, contractAddress),
+  );
+  const unsorted = [
+    ...thisExecPublicDataReads,
+    ...[...execResult.nestedExecutions].flatMap(result => collectPublicDataReads(wasm, result)),
+  ];
+  return unsorted.sort((a, b) => a.sideEffectCounter! - b.sideEffectCounter!);
+}
+
+/**
+ * Collect all public storage update requests across all nested executions
+ * and convert them to PublicDataUpdateRequests (to match kernel output).
+ * @param wasm - A module providing low-level wasm access.
+ * @param execResult - The topmost execution result.
+ * @returns All public data reads (in execution order).
+ */
+export function collectPublicDataUpdateRequests(
+  wasm: IWasmModule,
+  execResult: PublicExecutionResult,
+): PublicDataUpdateRequest[] {
+  // HACK(#1622): part of temporary hack - may be able to remove this function after public state ordering is fixed
+  const contractAddress = execResult.execution.contractAddress;
+
+  const thisExecPublicDataUpdateRequests = execResult.contractStorageUpdateRequests.map(update =>
+    contractStorageUpdateRequestToPublicDataUpdateRequest(wasm, update, contractAddress),
+  );
+  const unsorted = [
+    ...thisExecPublicDataUpdateRequests,
+    ...[...execResult.nestedExecutions].flatMap(result => collectPublicDataUpdateRequests(wasm, result)),
+  ];
+  return unsorted.sort((a, b) => a.sideEffectCounter! - b.sideEffectCounter!);
+}
+
+/**
+ * Convert a Contract Storage Read to a Public Data Read.
+ * @param wasm - A module providing low-level wasm access.
+ * @param read - the contract storage read to convert
+ * @param contractAddress - the contract address of the read
+ * @returns The public data read.
+ */
+function contractStorageReadToPublicDataRead(
+  wasm: IWasmModule,
+  read: ContractStorageRead,
+  contractAddress: AztecAddress,
+): PublicDataRead {
+  return new PublicDataRead(
+    computePublicDataTreeIndex(wasm, contractAddress.toField(), read.storageSlot),
+    computePublicDataTreeValue(wasm, read.currentValue),
+    read.sideEffectCounter!,
+  );
+}
+
+/**
+ * Convert a Contract Storage Update Request to a Public Data Update Request.
+ * @param wasm - A module providing low-level wasm access.
+ * @param update - the contract storage update request to convert
+ * @param contractAddress - the contract address of the data update request.
+ * @returns The public data update request.
+ */
+function contractStorageUpdateRequestToPublicDataUpdateRequest(
+  wasm: IWasmModule,
+  update: ContractStorageUpdateRequest,
+  contractAddress: AztecAddress,
+): PublicDataUpdateRequest {
+  return new PublicDataUpdateRequest(
+    computePublicDataTreeIndex(wasm, contractAddress.toField(), update.storageSlot),
+    computePublicDataTreeValue(wasm, update.oldValue),
+    computePublicDataTreeValue(wasm, update.newValue),
+    update.sideEffectCounter!,
+  );
+}

yarn-project/acir-simulator/src/public/executor.ts

@@ -43,7 +43,7 @@ export class PublicExecutor {
     private readonly contractsDb: PublicContractsDB,
     private readonly commitmentsDb: CommitmentsDB,
     private readonly blockData: HistoricBlockData,
-
+    private sideEffectCounter: number = 0,
     private log = createDebugLogger('aztec:simulator:public-executor'),
   ) {}
 
@@ -97,7 +97,7 @@ export class PublicExecutor {
         const values = [];
         for (let i = 0; i < Number(numberOfElements); i++) {
           const storageSlot = new Fr(startStorageSlot.value + BigInt(i));
-          const value = await storageActions.read(storageSlot);
+          const value = await storageActions.read(storageSlot, this.sideEffectCounter++); // update the sideEffectCounter after assigning its current value to storage action
           this.log(`Oracle storage read: slot=${storageSlot.toString()} value=${value.toString()}`);
           values.push(value);
         }
@@ -109,7 +109,7 @@ export class PublicExecutor {
         for (let i = 0; i < values.length; i++) {
           const storageSlot = new Fr(startStorageSlot.value + BigInt(i));
           const newValue = fromACVMField(values[i]);
-          await storageActions.write(storageSlot, newValue);
+          await storageActions.write(storageSlot, newValue, this.sideEffectCounter++); // update the sideEffectCounter after assigning its current value to storage action
           await this.stateDb.storageWrite(execution.contractAddress, storageSlot, newValue);
           this.log(`Oracle storage write: slot=${storageSlot.toString()} value=${newValue.toString()}`);
           newValues.push(newValue);
@@ -164,6 +164,11 @@ export class PublicExecutor {
     const { returnValues } = extractPublicCircuitPublicInputs(partialWitness, acir);
 
     const [contractStorageReads, contractStorageUpdateRequests] = storageActions.collect();
+    this.log(
+      `Contract storage reads: ${contractStorageReads
+        .map(r => r.toFriendlyJSON() + ` - sec: ${r.sideEffectCounter}`)
+        .join(', ')}`,
+    );
 
     return {
       execution,

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

@@ -89,7 +89,7 @@ describe('ACIR public execution simulator', () => {
 
         const storageSlot = computeSlotForMapping(new Fr(1n), recipient.toField(), circuitsWasm);
         expect(result.contractStorageUpdateRequests).toEqual([
-          { storageSlot, oldValue: previousBalance, newValue: expectedBalance },
+          { storageSlot, oldValue: previousBalance, newValue: expectedBalance, sideEffectCounter: 1 }, // 0th is a read
         ]);
 
         expect(result.contractStorageReads).toEqual([]);
@@ -157,8 +157,18 @@ describe('ACIR public execution simulator', () => {
         expect(result.returnValues[0]).toEqual(expectedRecipientBalance);
 
         expect(result.contractStorageUpdateRequests).toEqual([
-          { storageSlot: senderStorageSlot, oldValue: senderBalance, newValue: expectedSenderBalance },
-          { storageSlot: recipientStorageSlot, oldValue: recipientBalance, newValue: expectedRecipientBalance },
+          {
+            storageSlot: senderStorageSlot,
+            oldValue: senderBalance,
+            newValue: expectedSenderBalance,
+            sideEffectCounter: 2,
+          }, // 0th, 1st are reads
+          {
+            storageSlot: recipientStorageSlot,
+            oldValue: recipientBalance,
+            newValue: expectedRecipientBalance,
+            sideEffectCounter: 3,
+          },
         ]);
 
         expect(result.contractStorageReads).toEqual([]);

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

@@ -1,3 +1,9 @@
 export * from './db.js';
-export { PublicExecution, PublicExecutionResult, isPublicExecutionResult } from './execution.js';
+export {
+  PublicExecution,
+  PublicExecutionResult,
+  isPublicExecutionResult,
+  collectPublicDataReads,
+  collectPublicDataUpdateRequests,
+} from './execution.js';
 export { PublicExecutor } from './executor.js';

yarn-project/acir-simulator/src/public/state_actions.ts

@@ -11,12 +11,19 @@ import { PublicStateDB } from './db.js';
  */
 export class ContractStorageActionsCollector {
   // Map from slot to first read value
-  private readonly contractStorageReads: Map<bigint, { /** The value read. */ currentValue: Fr }> = new Map();
+  private readonly contractStorageReads: Map<
+    bigint,
+    { /** The value read. */ currentValue: Fr; /** Side effect counter. */ sideEffectCounter: number }
+  > = new Map();
 
   // Map from slot to first read value and latest updated value
   private readonly contractStorageUpdateRequests: Map<
     bigint,
-    { /** The old value. */ oldValue: Fr; /** The updated value. */ newValue: Fr }
+    {
+      /** The old value. */ oldValue: Fr;
+      /** The updated value. */ newValue: Fr;
+      /** Side effect counter. */ sideEffectCounter: number;
+    }
   > = new Map();
 
   constructor(private db: PublicStateDB, private address: AztecAddress) {}
@@ -26,42 +33,44 @@ export class ContractStorageActionsCollector {
    * falling back to the public db. Collects the operation in storage reads,
    * as long as there is no existing update request.
    * @param storageSlot - Slot to check.
+   * @param sideEffectCounter - Side effect counter associated with this storage action.
    * @returns The current value as affected by all update requests so far.
    */
-  public async read(storageSlot: Fr): Promise<Fr> {
+  public async read(storageSlot: Fr, sideEffectCounter: number): Promise<Fr> {
     const slot = storageSlot.value;
     const updateRequest = this.contractStorageUpdateRequests.get(slot);
     if (updateRequest) return updateRequest.newValue;
     const read = this.contractStorageReads.get(slot);
     if (read) return read.currentValue;
     const value = await this.db.storageRead(this.address, storageSlot);
-    this.contractStorageReads.set(slot, { currentValue: value });
+    this.contractStorageReads.set(slot, { currentValue: value, sideEffectCounter });
     return value;
   }
 
   /**
    * Sets a new value for a slot in the internal update requests cache,
    * clearing any previous storage read or update operation for the same slot.
    * @param storageSlot - Slot to write to.
-   * @param newValue - Balue to write to it.
+   * @param newValue - Value to write to it.
+   * @param sideEffectCounter - Side effect counter associated with this storage action.
    */
-  public async write(storageSlot: Fr, newValue: Fr): Promise<void> {
+  public async write(storageSlot: Fr, newValue: Fr, sideEffectCounter: number): Promise<void> {
     const slot = storageSlot.value;
     const updateRequest = this.contractStorageUpdateRequests.get(slot);
     if (updateRequest) {
-      this.contractStorageUpdateRequests.set(slot, { oldValue: updateRequest.oldValue, newValue });
+      this.contractStorageUpdateRequests.set(slot, { oldValue: updateRequest.oldValue, newValue, sideEffectCounter });
       return;
     }
 
     const read = this.contractStorageReads.get(slot);
     if (read) {
       this.contractStorageReads.delete(slot);
-      this.contractStorageUpdateRequests.set(slot, { oldValue: read.currentValue, newValue });
+      this.contractStorageUpdateRequests.set(slot, { oldValue: read.currentValue, newValue, sideEffectCounter });
       return;
     }
 
     const oldValue = await this.db.storageRead(this.address, storageSlot);
-    this.contractStorageUpdateRequests.set(slot, { oldValue, newValue });
+    this.contractStorageUpdateRequests.set(slot, { oldValue, newValue, sideEffectCounter });
     return;
   }
 
@@ -70,17 +79,17 @@ export class ContractStorageActionsCollector {
    * @returns All storage read and update requests.
    */
   public collect(): [ContractStorageRead[], ContractStorageUpdateRequest[]] {
-    const reads = Array.from(this.contractStorageReads.entries()).map(([slot, value]) =>
+    const reads = Array.from(this.contractStorageReads.entries()).map(([slot, valueAndCounter]) =>
       ContractStorageRead.from({
         storageSlot: new Fr(slot),
-        ...value,
+        ...valueAndCounter,
       }),
     );
 
-    const updateRequests = Array.from(this.contractStorageUpdateRequests.entries()).map(([slot, values]) =>
+    const updateRequests = Array.from(this.contractStorageUpdateRequests.entries()).map(([slot, valuesAndCounter]) =>
       ContractStorageUpdateRequest.from({
         storageSlot: new Fr(slot),
-        ...values,
+        ...valuesAndCounter,
       }),
     );