From be2d7c9c6d302ef564f6f985775a518cc831e3c5 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 18 Aug 2023 18:00:12 +0000 Subject: [PATCH 1/8] test that checks public state update order --- .../src/client/execution_result.ts | 3 +- .../end-to-end/src/e2e_ordering.test.ts | 62 ++++++++++++++++--- .../src/contracts/child_contract/src/main.nr | 32 +++++++++- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/yarn-project/acir-simulator/src/client/execution_result.ts b/yarn-project/acir-simulator/src/client/execution_result.ts index f4b07dfff6b..80aa97d8fbf 100644 --- a/yarn-project/acir-simulator/src/client/execution_result.ts +++ b/yarn-project/acir-simulator/src/client/execution_result.ts @@ -100,7 +100,8 @@ export function collectUnencryptedLogs(execResult: ExecutionResult): FunctionL2L * @returns All enqueued public function calls. */ export function collectEnqueuedPublicFunctionCalls(execResult: ExecutionResult): PublicCallRequest[] { - // without the reverse sort, the logs will be in a queue like fashion which is wrong as the kernel processes it like a stack. + // without the reverse sort, the logs will be in a queue like fashion which is wrong + // as the kernel processes it like a stack, popping items off and pushing them to output return [ ...execResult.enqueuedPublicFunctionCalls, ...[...execResult.nestedExecutions].flatMap(collectEnqueuedPublicFunctionCalls), diff --git a/yarn-project/end-to-end/src/e2e_ordering.test.ts b/yarn-project/end-to-end/src/e2e_ordering.test.ts index 1978797c968..94ce78dd90a 100644 --- a/yarn-project/end-to-end/src/e2e_ordering.test.ts +++ b/yarn-project/end-to-end/src/e2e_ordering.test.ts @@ -5,7 +5,9 @@ import { 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, L2BlockL2Logs } from '@aztec/types'; +import { AztecRPC, L2BlockL2Logs, TxStatus } from '@aztec/types'; + +import { toBigIntBE } from '@aztec/foundation/bigint-buffer'; import { setup } from './fixtures/utils.js'; @@ -14,9 +16,19 @@ describe('e2e_ordering', () => { let aztecNode: AztecNodeService | undefined; let aztecRpcServer: AztecRPC; let wallet: Wallet; + //let logger: DebugLogger; + + const expectLogsFromLastBlockToBe = async (logMessages: bigint[]) => { + const l2BlockNum = await aztecRpcServer.getBlockNum(); + const unencryptedLogs = await aztecRpcServer.getUnencryptedLogs(l2BlockNum, 1); + const unrolledLogs = L2BlockL2Logs.unrollLogs(unencryptedLogs); + const bigintLogs = unrolledLogs.map((log: Buffer) => toBigIntBE(log)); + + expect(bigintLogs).toStrictEqual(logMessages); + }; beforeEach(async () => { - ({ aztecNode, aztecRpcServer, wallet } = await setup()); + ({ aztecNode, aztecRpcServer, wallet, /*logger*/ } = await setup()); }, 100_000); afterEach(async () => { @@ -42,8 +54,8 @@ describe('e2e_ordering', () => { const directValue = 20n; const expectedOrders = { - enqueueCallsToChildWithNestedFirst: [nestedValue, directValue], - enqueueCallsToChildWithNestedLast: [directValue, nestedValue], + enqueueCallsToChildWithNestedFirst: [nestedValue, directValue] as bigint[], + enqueueCallsToChildWithNestedLast: [directValue, nestedValue] as bigint[], } as const; it.each(['enqueueCallsToChildWithNestedFirst', 'enqueueCallsToChildWithNestedLast'] as const)( @@ -66,15 +78,49 @@ describe('e2e_ordering', () => { expect(enqueuedPublicCalls.map(c => c.args[0].toBigInt())).toEqual([...expectedOrder].reverse()); // Logs are emitted in the expected order - const logs = await aztecRpcServer.getUnencryptedLogs(1, 10).then(L2BlockL2Logs.unrollLogs); - const expectedLogs = expectedOrder.map(x => Buffer.from([Number(x)])); - expect(logs).toEqual(expectedLogs); + await expectLogsFromLastBlockToBe(expectedOrder); // The final value of the child is the last one set const value = await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!)); - expect(value).toEqual(expectedOrder[1]); + expect(value).toEqual(expectedOrder[1]); // final state should match last value set }, ); }); + + describe('public state update ordering, and final state value check', () => { + const nestedValue = 10n; + const directValue = 20n; + + const expectedOrders = { + setValueTwiceWithNestedFirst: [nestedValue, directValue] as bigint[], + setValueTwiceWithNestedLast: [directValue, nestedValue] as bigint[], + } as const; + + // TODO(dbanks12): implement public state ordering, or... + // TODO(#1622): hack around this error by removing public kernel checks + // that public state updates are in valid sequence + // Full explanation: + // Setting public storage twice (first in a nested call, then directly) leads + // to an error in public kernel because it sees the public state updates + // in reverse order. More info in this thread: https://discourse.aztec.network/t/identifying-the-ordering-of-state-access-across-contract-calls/382/12#transition-counters-for-private-calls-2 + // + // Once fixed, re-include the `setValueTwiceWithNestedFirst` test + //it.each(['setValueTwiceWithNestedFirst', 'setValueTwiceWithNestedLast'] as const)( + it.each(['setValueTwiceWithNestedLast'] as const)( + 'orders public state updates in %s (and ensures final state value is correct)', + async method => { + const expectedOrder = expectedOrders[method]; + + const tx = child.methods[method]().send(); + const receipt = await tx.wait(); + expect(receipt.status).toBe(TxStatus.MINED); + + // Logs are emitted in the expected order + await expectLogsFromLastBlockToBe(expectedOrder); + + const value = await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!)); + expect(value).toEqual(expectedOrder[1]); // final state should match last value set + }); + }); }); }); diff --git a/yarn-project/noir-contracts/src/contracts/child_contract/src/main.nr b/yarn-project/noir-contracts/src/contracts/child_contract/src/main.nr index 7eb6a969bd9..222090acc6c 100644 --- a/yarn-project/noir-contracts/src/contracts/child_contract/src/main.nr +++ b/yarn-project/noir-contracts/src/contracts/child_contract/src/main.nr @@ -35,7 +35,7 @@ contract Child { // Returns base_value + 42. 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(); context.return_values.push(returnValue); @@ -64,4 +64,34 @@ contract Child { context.return_values.push(new_value); context.finish() } + + open fn setValueTwiceWithNestedFirst( + inputs: PublicContextInputs, + ) -> pub abi::PublicCircuitPublicInputs { + let mut context = PublicContext::new(inputs, abi::hash_args([])); + + let pubSetValueSelector = 0x5b0f91b0; + let _ret = context.call_public_function(context.this_address(), pubSetValueSelector, [10]); + + let storage = Storage::init(); + storage.current_value.write(20); + let _hash = emit_unencrypted_log(20); + + context.finish() + } + + open fn setValueTwiceWithNestedLast( + inputs: PublicContextInputs, + ) -> pub abi::PublicCircuitPublicInputs { + let mut context = PublicContext::new(inputs, abi::hash_args([])); + + let storage = Storage::init(); + storage.current_value.write(20); + let _hash = emit_unencrypted_log(20); + + let pubSetValueSelector = 0x5b0f91b0; + let _ret = context.call_public_function(context.this_address(), pubSetValueSelector, [10]); + + context.finish() + } } From 15b546ea729e9939f1880437c83734c4291d39a1 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 18 Aug 2023 18:04:04 +0000 Subject: [PATCH 2/8] format --- yarn-project/end-to-end/src/e2e_ordering.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_ordering.test.ts b/yarn-project/end-to-end/src/e2e_ordering.test.ts index 94ce78dd90a..c512e078b56 100644 --- a/yarn-project/end-to-end/src/e2e_ordering.test.ts +++ b/yarn-project/end-to-end/src/e2e_ordering.test.ts @@ -3,12 +3,11 @@ import { AztecNodeService } from '@aztec/aztec-node'; import { AztecRPCServer } from '@aztec/aztec-rpc'; import { Wallet } from '@aztec/aztec.js'; import { Fr } from '@aztec/circuits.js'; +import { toBigIntBE } from '@aztec/foundation/bigint-buffer'; import { toBigInt } from '@aztec/foundation/serialize'; import { ChildContract, ParentContract } from '@aztec/noir-contracts/types'; import { AztecRPC, L2BlockL2Logs, TxStatus } from '@aztec/types'; -import { toBigIntBE } from '@aztec/foundation/bigint-buffer'; - import { setup } from './fixtures/utils.js'; // See https://github.com/AztecProtocol/aztec-packages/issues/1601 @@ -28,7 +27,7 @@ describe('e2e_ordering', () => { }; beforeEach(async () => { - ({ aztecNode, aztecRpcServer, wallet, /*logger*/ } = await setup()); + ({ aztecNode, aztecRpcServer, wallet /*, logger*/ } = await setup()); }, 100_000); afterEach(async () => { @@ -120,7 +119,8 @@ describe('e2e_ordering', () => { const value = await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!)); expect(value).toEqual(expectedOrder[1]); // final state should match last value set - }); + }, + ); }); }); }); From b7e27354b56c7427e532e5f47166c1e4816743a8 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sun, 20 Aug 2023 23:11:10 +0000 Subject: [PATCH 3/8] ordering hack for public state transitions --- .../cpp/src/aztec3/circuits/abis/c_bind.cpp | 10 ++ .../aztec3/circuits/kernel/public/common.hpp | 2 +- .../src/client/execution_result.ts | 2 +- .../src/client/private_execution.test.ts | 2 +- .../src/client/private_execution.ts | 21 ++- .../acir-simulator/src/public/execution.ts | 73 ++++++++++ .../acir-simulator/src/public/executor.ts | 6 +- .../acir-simulator/src/public/index.ts | 2 +- .../src/public/state_actions.ts | 28 ++-- .../src/aztec_rpc_server/aztec_rpc_server.ts | 6 +- yarn-project/circuits.js/src/abis/abis.ts | 28 ++++ .../circuits.js/src/cbind/circuits.gen.ts | 6 + .../kernel/combined_accumulated_data.ts | 8 ++ .../src/structs/public_call_request.ts | 6 +- .../structs/public_circuit_public_inputs.ts | 41 +++--- .../end-to-end/src/e2e_ordering.test.ts | 28 ++-- .../src/sequencer/public_processor.ts | 130 +++++++++++++++--- 17 files changed, 321 insertions(+), 78 deletions(-) diff --git a/circuits/cpp/src/aztec3/circuits/abis/c_bind.cpp b/circuits/cpp/src/aztec3/circuits/abis/c_bind.cpp index dfba68deb24..81f84e0407c 100644 --- a/circuits/cpp/src/aztec3/circuits/abis/c_bind.cpp +++ b/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); +/** + * @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); + +/** + * @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); + /** * @brief Generates a signed tx request hash from it's pre-image * This is a WASM-export that can be called from Typescript. diff --git a/circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp b/circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp index b41c9c980d2..a661a0db034 100644 --- a/circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp +++ b/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 diff --git a/yarn-project/acir-simulator/src/client/execution_result.ts b/yarn-project/acir-simulator/src/client/execution_result.ts index 80aa97d8fbf..8effcb4649b 100644 --- a/yarn-project/acir-simulator/src/client/execution_result.ts +++ b/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! } diff --git a/yarn-project/acir-simulator/src/client/private_execution.test.ts b/yarn-project/acir-simulator/src/client/private_execution.test.ts index 729d3e92bed..5c9c02c2b00 100644 --- a/yarn-project/acir-simulator/src/client/private_execution.test.ts +++ b/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( diff --git a/yarn-project/acir-simulator/src/client/private_execution.ts b/yarn-project/acir-simulator/src/client/private_execution.ts index bc6d0796bf4..e3528219eff 100644 --- a/yarn-project/acir-simulator/src/client/private_execution.ts +++ b/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); @@ -285,7 +281,7 @@ export class PrivateFunctionExecution { targetArgsHash, derivedCallContext, curve, - this.orderings, + this.sideEffectCounter, this.log, ); @@ -300,7 +296,6 @@ 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( @@ -308,19 +303,23 @@ export class PrivateFunctionExecution { targetFunctionSelector: Buffer, targetArgs: Fr[], callerContext: CallContext, - orderings: Orderings, ): Promise { 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. + } /** diff --git a/yarn-project/acir-simulator/src/public/execution.ts b/yarn-project/acir-simulator/src/public/execution.ts index 1bbbb48307a..11063068bc9 100644 --- a/yarn-project/acir-simulator/src/public/execution.ts +++ b/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,72 @@ 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 read + * @returns The public data read. + */ +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! + ); +} diff --git a/yarn-project/acir-simulator/src/public/executor.ts b/yarn-project/acir-simulator/src/public/executor.ts index c9d58d810b2..1ab6f5bb37f 100644 --- a/yarn-project/acir-simulator/src/public/executor.ts +++ b/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 after assigning 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 after assigning 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); diff --git a/yarn-project/acir-simulator/src/public/index.ts b/yarn-project/acir-simulator/src/public/index.ts index 167df4f9d9a..a8ff40ef13c 100644 --- a/yarn-project/acir-simulator/src/public/index.ts +++ b/yarn-project/acir-simulator/src/public/index.ts @@ -1,3 +1,3 @@ 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'; diff --git a/yarn-project/acir-simulator/src/public/state_actions.ts b/yarn-project/acir-simulator/src/public/state_actions.ts index f8891270ff2..26d2a8c0a89 100644 --- a/yarn-project/acir-simulator/src/public/state_actions.ts +++ b/yarn-project/acir-simulator/src/public/state_actions.ts @@ -11,12 +11,12 @@ import { PublicStateDB } from './db.js'; */ export class ContractStorageActionsCollector { // Map from slot to first read value - private readonly contractStorageReads: Map = new Map(); + private readonly contractStorageReads: Map = 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,16 +26,17 @@ 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 { + public async read(storageSlot: Fr, sideEffectCounter: number): Promise { 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; } @@ -43,25 +44,26 @@ export class ContractStorageActionsCollector { * 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 { + public async write(storageSlot: Fr, newValue: Fr, sideEffectCounter: number): Promise { 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 +72,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, }), ); diff --git a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts index c2b5ad24974..e9d867ca30b 100644 --- a/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts +++ b/yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts @@ -421,6 +421,7 @@ export class AztecRPCServer implements AztecRPC { const enqueuedPublicFunctions = collectEnqueuedPublicFunctionCalls(executionResult); // HACK(#1639): Manually patches the ordering of the public call stack + // TODO(#757): Enforce proper ordering of enqueued public calls await this.patchPublicCallStackOrdering(publicInputs, enqueuedPublicFunctions); return new Tx( @@ -433,12 +434,13 @@ export class AztecRPCServer implements AztecRPC { ); } - // This is a hack to fix ordering of public calls enqueued in the call stack. Since the private kernel cannot - // keep track of side effects that happen after or before a nested call, we override the public call stack + // HACK(#1639): this is a hack to fix ordering of public calls enqueued in the call stack. Since the private kernel + // cannot keep track of side effects that happen after or before a nested call, we override the public call stack // it emits with whatever we got from the simulator collected enqueued calls. As a sanity check, we at least verify // that the elements are the same, so we are only tweaking their ordering. // See yarn-project/end-to-end/src/e2e_ordering.test.ts // See https://github.com/AztecProtocol/aztec-packages/issues/1615 + // TODO(#757): Enforce proper ordering of enqueued public calls private async patchPublicCallStackOrdering( publicInputs: KernelCircuitPublicInputs, enqueuedPublicCalls: PublicCallRequest[], diff --git a/yarn-project/circuits.js/src/abis/abis.ts b/yarn-project/circuits.js/src/abis/abis.ts index 0e49a4a078d..806adc2a2d7 100644 --- a/yarn-project/circuits.js/src/abis/abis.ts +++ b/yarn-project/circuits.js/src/abis/abis.ts @@ -10,6 +10,8 @@ import { abisComputeCommitmentNonce, abisComputeGlobalsHash, abisComputeUniqueCommitment, + abisComputePublicDataTreeValue, + abisComputePublicDataTreeIndex, abisSiloCommitment, abisSiloNullifier, } from '../cbind/circuits.gen.js'; @@ -377,6 +379,32 @@ export function computeGlobalsHash(wasm: IWasmModule, globals: GlobalVariables): return abisComputeGlobalsHash(wasm, globals); } +/** + * Computes a public data tree value ready for insertion. + * @param wasm - A module providing low-level wasm access. + * @param value - Raw public data tree value to hash into a tree-insertion-ready value. + * @returns Value hash into a tree-insertion-ready value. + + */ +export function computePublicDataTreeValue(wasm: IWasmModule, value: Fr): Fr { + wasm.call('pedersen__init'); + return abisComputePublicDataTreeValue(wasm, value); +} + +/** + * Computes a public data tree index from contract address and storage slot. + * @param wasm - A module providing low-level wasm access. + * @param contractAddress - Contract where insertion is occurring. + * @param storageSlot - Storage slot where insertion is occuring. + * @returns Public data tree index computed from contract address and storage slot. + + */ +export function computePublicDataTreeIndex(wasm: IWasmModule, contractAddress: Fr, storageSlot: Fr): Fr { + wasm.call('pedersen__init'); + return abisComputePublicDataTreeIndex(wasm, contractAddress, storageSlot); +} + + const ARGS_HASH_CHUNK_SIZE = 32; const ARGS_HASH_CHUNK_COUNT = 16; diff --git a/yarn-project/circuits.js/src/cbind/circuits.gen.ts b/yarn-project/circuits.js/src/cbind/circuits.gen.ts index 943da3019e6..27f59b8bff4 100644 --- a/yarn-project/circuits.js/src/cbind/circuits.gen.ts +++ b/yarn-project/circuits.js/src/cbind/circuits.gen.ts @@ -1557,6 +1557,12 @@ export function abisComputeBlockHashWithGlobals( export function abisComputeGlobalsHash(wasm: IWasmModule, arg0: GlobalVariables): Fr { return Fr.fromBuffer(callCbind(wasm, 'abis__compute_globals_hash', [fromGlobalVariables(arg0)])); } +export function abisComputePublicDataTreeValue(wasm: IWasmModule, arg0: Fr): Fr { + return Fr.fromBuffer(callCbind(wasm, 'abis__compute_public_data_tree_value', [toBuffer(arg0)])); +} +export function abisComputePublicDataTreeIndex(wasm: IWasmModule, arg0: Fr, arg1: Fr): Fr { + return Fr.fromBuffer(callCbind(wasm, 'abis__compute_public_data_tree_index', [toBuffer(arg0), toBuffer(arg1)])); +} export function privateKernelDummyPreviousKernel(wasm: IWasmModule): PreviousKernelData { return toPreviousKernelData(callCbind(wasm, 'private_kernel__dummy_previous_kernel', [])); } diff --git a/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts b/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts index 0099fa7605c..8b5ef26a4ba 100644 --- a/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts +++ b/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts @@ -186,6 +186,10 @@ export class PublicDataRead { * Returned value from the public data tree. */ public readonly value: Fr, + /** + * Optional side effect counter tracking position of this event in tx execution. + */ + public readonly sideEffectCounter?: number, ) {} static from(args: { @@ -236,6 +240,10 @@ export class PublicDataUpdateRequest { * New value of the leaf. */ public readonly newValue: Fr, + /** + * Optional side effect counter tracking position of this event in tx execution. + */ + public readonly sideEffectCounter?: number, ) {} static from(args: { diff --git a/yarn-project/circuits.js/src/structs/public_call_request.ts b/yarn-project/circuits.js/src/structs/public_call_request.ts index 21fc00e70a6..80655711c8a 100644 --- a/yarn-project/circuits.js/src/structs/public_call_request.ts +++ b/yarn-project/circuits.js/src/structs/public_call_request.ts @@ -36,9 +36,9 @@ export class PublicCallRequest { */ public args: Fr[], /** - * Optional ordering within a tx execution. + * Optional side effect counter tracking position of this event in tx execution. */ - public order?: number, + public sideEffectCounter?: number, ) {} /** @@ -79,7 +79,7 @@ export class PublicCallRequest { * @returns The array. */ static getFields(fields: FieldsOf) { - return [fields.contractAddress, fields.functionData, fields.callContext, fields.args, fields.order] as const; + return [fields.contractAddress, fields.functionData, fields.callContext, fields.args, fields.sideEffectCounter] as const; } /** diff --git a/yarn-project/circuits.js/src/structs/public_circuit_public_inputs.ts b/yarn-project/circuits.js/src/structs/public_circuit_public_inputs.ts index 04955b697fa..9bbcfe872d3 100644 --- a/yarn-project/circuits.js/src/structs/public_circuit_public_inputs.ts +++ b/yarn-project/circuits.js/src/structs/public_circuit_public_inputs.ts @@ -34,6 +34,10 @@ export class ContractStorageRead { * Value read from the storage slot. */ public readonly currentValue: Fr, + /** + * Optional side effect counter tracking position of this event in tx execution. + */ + public readonly sideEffectCounter?: number, ) {} static from(args: { @@ -91,24 +95,11 @@ export class ContractStorageUpdateRequest { * New value of the storage slot. */ public readonly newValue: Fr, - ) {} - - static from(args: { - /** - * Storage slot we are updating. - */ - storageSlot: Fr; /** - * Old value of the storage slot. + * Optional side effect counter tracking position of this event in tx execution. */ - oldValue: Fr; - /** - * New value of the storage slot. - */ - newValue: Fr; - }) { - return new ContractStorageUpdateRequest(args.storageSlot, args.oldValue, args.newValue); - } + public readonly sideEffectCounter?: number, + ) {} toBuffer() { return serializeToBuffer(this.storageSlot, this.oldValue, this.newValue); @@ -119,6 +110,24 @@ export class ContractStorageUpdateRequest { return new ContractStorageUpdateRequest(reader.readFr(), reader.readFr(), reader.readFr()); } + /** + * Create PublicCallRequest from a fields dictionary. + * @param fields - The dictionary. + * @returns A PublicCallRequest object. + */ + static from(fields: FieldsOf): ContractStorageUpdateRequest { + return new ContractStorageUpdateRequest(...ContractStorageUpdateRequest.getFields(fields)); + } + + /** + * Serialize into a field array. Low-level utility. + * @param fields - Object with fields. + * @returns The array. + */ + static getFields(fields: FieldsOf) { + return [fields.storageSlot, fields.oldValue, fields.newValue, fields.sideEffectCounter] as const; + } + static empty() { return new ContractStorageUpdateRequest(Fr.ZERO, Fr.ZERO, Fr.ZERO); } diff --git a/yarn-project/end-to-end/src/e2e_ordering.test.ts b/yarn-project/end-to-end/src/e2e_ordering.test.ts index c512e078b56..ae2eea8f6a3 100644 --- a/yarn-project/end-to-end/src/e2e_ordering.test.ts +++ b/yarn-project/end-to-end/src/e2e_ordering.test.ts @@ -95,18 +95,29 @@ describe('e2e_ordering', () => { setValueTwiceWithNestedLast: [directValue, nestedValue] as bigint[], } as const; - // TODO(dbanks12): implement public state ordering, or... - // TODO(#1622): hack around this error by removing public kernel checks - // that public state updates are in valid sequence + it.each(['setValueTwiceWithNestedFirst', 'setValueTwiceWithNestedLast'] as const)( + 'orders public state updates in %s (and ensures final state value is correct)', + async method => { + const expectedOrder = expectedOrders[method]; + + const tx = child.methods[method]().send(); + const receipt = await tx.wait(); + expect(receipt.status).toBe(TxStatus.MINED); + + const value = await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!)); + expect(value).toEqual(expectedOrder[1]); // final state should match last value set + }, + ); + + // TODO(#838): Public kernel outputs logs in wrong order! // Full explanation: - // Setting public storage twice (first in a nested call, then directly) leads - // to an error in public kernel because it sees the public state updates + // Emitting logs twice (first in a nested call, then directly) leads + // to a misordering of them by the public kernel because it sees them // in reverse order. More info in this thread: https://discourse.aztec.network/t/identifying-the-ordering-of-state-access-across-contract-calls/382/12#transition-counters-for-private-calls-2 - // // Once fixed, re-include the `setValueTwiceWithNestedFirst` test //it.each(['setValueTwiceWithNestedFirst', 'setValueTwiceWithNestedLast'] as const)( it.each(['setValueTwiceWithNestedLast'] as const)( - 'orders public state updates in %s (and ensures final state value is correct)', + 'orders unencrypted logs in %s', async method => { const expectedOrder = expectedOrders[method]; @@ -116,9 +127,6 @@ describe('e2e_ordering', () => { // Logs are emitted in the expected order await expectLogsFromLastBlockToBe(expectedOrder); - - const value = await aztecRpcServer.getPublicStorageAt(child.address, new Fr(1)).then(x => toBigInt(x!)); - expect(value).toEqual(expectedOrder[1]); // final state should match last value set }, ); }); diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.ts index 8574a9083fd..7300513358f 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.ts @@ -1,4 +1,4 @@ -import { PublicExecution, PublicExecutionResult, PublicExecutor, isPublicExecutionResult } from '@aztec/acir-simulator'; +import { PublicExecution, PublicExecutionResult, PublicExecutor, isPublicExecutionResult, collectPublicDataReads, collectPublicDataUpdateRequests } from '@aztec/acir-simulator'; import { AztecAddress, CircuitsWasm, @@ -13,20 +13,24 @@ import { MAX_NEW_NULLIFIERS_PER_CALL, MAX_PUBLIC_CALL_STACK_LENGTH_PER_CALL, MAX_PUBLIC_DATA_READS_PER_CALL, + MAX_PUBLIC_DATA_READS_PER_TX, MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_CALL, + MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, MembershipWitness, PreviousKernelData, Proof, PublicCallData, PublicCallStackItem, PublicCircuitPublicInputs, + PublicDataRead, + PublicDataUpdateRequest, PublicKernelInputs, PublicKernelPublicInputs, RETURN_VALUES_LENGTH, VK_TREE_HEIGHT, } from '@aztec/circuits.js'; import { computeCallStackItemHash, computeVarArgsHash } from '@aztec/circuits.js/abis'; -import { isArrayEmpty, padArrayEnd, padArrayStart } from '@aztec/foundation/collection'; +import { arrayNonEmptyLength, isArrayEmpty, padArrayEnd, padArrayStart } from '@aztec/foundation/collection'; import { createDebugLogger } from '@aztec/foundation/log'; import { Tuple, mapTuple, to2Fields } from '@aztec/foundation/serialize'; import { ContractDataSource, FunctionL2Logs, L1ToL2MessageSource, MerkleTreeId, Tx } from '@aztec/types'; @@ -138,24 +142,40 @@ 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`); - const executionStack: (PublicExecution | PublicExecutionResult)[] = [...tx.enqueuedPublicFunctionCalls]; - let kernelOutput = tx.data; let kernelProof = tx.proof; const newUnencryptedFunctionLogs: FunctionL2Logs[] = []; - while (executionStack.length) { - const current = executionStack.pop()!; - const isExecutionRequest = !isPublicExecutionResult(current); - const result = isExecutionRequest ? await this.publicExecutor.execute(current, this.globalVariables) : current; - newUnencryptedFunctionLogs.push(result.unencryptedLogs); - const functionSelector = result.execution.functionData.functionSelectorBuffer.toString('hex'); - this.log(`Running public kernel circuit for ${functionSelector}@${result.execution.contractAddress.toString()}`); - executionStack.push(...result.nestedExecutions); - const preimages = await this.getPublicCallStackPreimages(result); - const callData = await this.getPublicCallData(result, preimages, isExecutionRequest); - - [kernelOutput, kernelProof] = await this.runKernelCircuit(callData, kernelOutput, kernelProof); + // TODO(#1684): Should multiple separately enqueued public calls be treated as + // separate public callstacks to be proven by separate public kernel sequences + // and submitted separately to the base rollup? + + // TODO(dbanks12): why must these be reversed? + const enqueuedCallsReversed = tx.enqueuedPublicFunctionCalls.slice().reverse(); + for (const enqueuedCall of enqueuedCallsReversed) { + const executionStack: (PublicExecution | PublicExecutionResult)[] = [enqueuedCall]; + + // Keep track of which result is for the top/enqueued call + let enqueuedExecutionResult: PublicExecutionResult | undefined; + + while (executionStack.length) { + const current = executionStack.pop()!; + const isExecutionRequest = !isPublicExecutionResult(current); + const result = isExecutionRequest ? await this.publicExecutor.execute(current, this.globalVariables) : current; + newUnencryptedFunctionLogs.push(result.unencryptedLogs); + const functionSelector = result.execution.functionData.functionSelectorBuffer.toString('hex'); + this.log(`Running public kernel circuit for ${functionSelector}@${result.execution.contractAddress.toString()}`); + executionStack.push(...result.nestedExecutions); + const preimages = await this.getPublicCallStackPreimages(result); + const callData = await this.getPublicCallData(result, preimages, isExecutionRequest); + + [kernelOutput, kernelProof] = await this.runKernelCircuit(callData, kernelOutput, kernelProof); + + if (!enqueuedExecutionResult) enqueuedExecutionResult = result; + } + // HACK(#1622): Manually patches the ordering of public state actions + // TODO(#757): Enforce proper ordering of public state actions + await this.patchPublicStorageActionOrdering(kernelOutput, enqueuedExecutionResult!); } return [kernelOutput, kernelProof, newUnencryptedFunctionLogs]; @@ -286,4 +306,82 @@ export class PublicProcessor { const proof = await this.publicProver.getPublicCircuitProof(callStackItem.publicInputs); return new PublicCallData(callStackItem, preimages, proof, portalContractAddress, bytecodeHash); } + + // HACK(#1622): this is a hack to fix ordering of public state in the call stack. Since the private kernel + // cannot keep track of side effects that happen after or before a nested call, we override the public + // state actions it emits with whatever we got from the simulator. As a sanity check, we at least verify + // that the elements are the same, so we are only tweaking their ordering. + // See yarn-project/end-to-end/src/e2e_ordering.test.ts + // See https://github.com/AztecProtocol/aztec-packages/issues/1616 + // TODO(#757): Enforce proper ordering of public state actions + /** + * Patch the ordering of storage actions output from the public kernel. + * @param publicInputs - to be patched here: public inputs to the kernel iteration up to this point + * @param execResult - result of the top/first execution for this enqueued public call + */ + private async patchPublicStorageActionOrdering( + publicInputs: KernelCircuitPublicInputs, + execResult: PublicExecutionResult, + ) { + // Convert ContractStorage* objects to PublicData* objects and sort them in execution order + const wasm = await CircuitsWasm.get(); + const simPublicDataReads = collectPublicDataReads(wasm, execResult); + const simPublicDataUpdateRequests = collectPublicDataUpdateRequests(wasm, execResult); + + const { publicDataReads, publicDataUpdateRequests } = publicInputs.end; // from kernel + + // Validate all items in enqueued public calls are in the kernel emitted stack + const readsAreEqual = simPublicDataReads.reduce( + (accum, read) => accum && !!publicDataReads.find(item => item.leafIndex.equals(read.leafIndex) && item.value.equals(read.value)), + true, + ); + const updatesAreEqual = simPublicDataUpdateRequests.reduce( + (accum, update) => accum && !!publicDataUpdateRequests.find(item => item.leafIndex.equals(update.leafIndex) && item.oldValue.equals(update.oldValue) && item.newValue.equals(update.newValue)), + true, + ); + + if (!readsAreEqual) { + throw new Error( + `Public data reads from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataReads + .map(p => p.toFriendlyJSON()) + .join(', ')}\nFrom public kernel: ${publicDataReads.map(i => i.toFriendlyJSON()).join(', ')}`, + ); + } + if (!updatesAreEqual) { + throw new Error( + `Public data update requests from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataUpdateRequests + .map(p => p.toFriendlyJSON()) + .join(', ')}\nFrom public kernel: ${publicDataUpdateRequests.map(i => i.toFriendlyJSON()).join(', ')}`, + ); + } + + // Assume that kernel public inputs has the right number of items. + // We only want to reorder the items from the public inputs of the + // most recently processed top/enqueued call. + const numTotalReadsInKernel = arrayNonEmptyLength(publicInputs.end.publicDataReads, f => f.leafIndex.equals(Fr.ZERO) && f.value.equals(Fr.ZERO)); + const numTotalUpdatesInKernel = arrayNonEmptyLength(publicInputs.end.publicDataUpdateRequests, f => f.leafIndex.equals(Fr.ZERO) && f.oldValue.equals(Fr.ZERO) && f.newValue.equals(Fr.ZERO)); + const numReadsBeforeThisEnqueuedCall = numTotalReadsInKernel - simPublicDataReads.length; + const numUpdatesBeforeThisEnqueuedCall = numTotalUpdatesInKernel - simPublicDataUpdateRequests.length; + + // Override kernel output + publicInputs.end.publicDataReads = padArrayEnd( + [ + // do not mess with items from previous top/enqueued calls in kernel output + ...publicDataReads.slice(0, numReadsBeforeThisEnqueuedCall), + ...simPublicDataReads + ], + PublicDataRead.empty(), + MAX_PUBLIC_DATA_READS_PER_TX, + ); + // Override kernel output + publicInputs.end.publicDataUpdateRequests = padArrayEnd( + [ + // do not mess with items from previous top/enqueued calls in kernel output + ...publicDataUpdateRequests.slice(0, numUpdatesBeforeThisEnqueuedCall), + ...simPublicDataUpdateRequests + ], + PublicDataUpdateRequest.empty(), + MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, + ); + } } From 3b25fb0be92be6a08e479b68001b4f7cefc8d319 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Sun, 20 Aug 2023 23:11:24 +0000 Subject: [PATCH 4/8] formatting fix --- .../end-to-end/src/e2e_ordering.test.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_ordering.test.ts b/yarn-project/end-to-end/src/e2e_ordering.test.ts index ae2eea8f6a3..a93149cb151 100644 --- a/yarn-project/end-to-end/src/e2e_ordering.test.ts +++ b/yarn-project/end-to-end/src/e2e_ordering.test.ts @@ -116,19 +116,16 @@ describe('e2e_ordering', () => { // in reverse order. More info in this thread: https://discourse.aztec.network/t/identifying-the-ordering-of-state-access-across-contract-calls/382/12#transition-counters-for-private-calls-2 // Once fixed, re-include the `setValueTwiceWithNestedFirst` test //it.each(['setValueTwiceWithNestedFirst', 'setValueTwiceWithNestedLast'] as const)( - it.each(['setValueTwiceWithNestedLast'] as const)( - 'orders unencrypted logs in %s', - async method => { - const expectedOrder = expectedOrders[method]; + it.each(['setValueTwiceWithNestedLast'] as const)('orders unencrypted logs in %s', async method => { + const expectedOrder = expectedOrders[method]; - const tx = child.methods[method]().send(); - const receipt = await tx.wait(); - expect(receipt.status).toBe(TxStatus.MINED); + const tx = child.methods[method]().send(); + const receipt = await tx.wait(); + expect(receipt.status).toBe(TxStatus.MINED); - // Logs are emitted in the expected order - await expectLogsFromLastBlockToBe(expectedOrder); - }, - ); + // Logs are emitted in the expected order + await expectLogsFromLastBlockToBe(expectedOrder); + }); }); }); }); From 253651689b5c49a9dd56cdb8d78de13559ddc0f8 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Mon, 21 Aug 2023 02:10:02 +0000 Subject: [PATCH 5/8] formatting, mention hack PR in comment --- .../src/client/private_execution.ts | 1 - .../acir-simulator/src/public/execution.ts | 29 +++++++++--- .../acir-simulator/src/public/index.ts | 8 +++- .../src/public/state_actions.ts | 13 ++++-- yarn-project/circuits.js/src/abis/abis.ts | 5 +-- .../src/structs/public_call_request.ts | 8 +++- .../src/sequencer/public_processor.ts | 45 ++++++++++++++----- 7 files changed, 82 insertions(+), 27 deletions(-) diff --git a/yarn-project/acir-simulator/src/client/private_execution.ts b/yarn-project/acir-simulator/src/client/private_execution.ts index e3528219eff..b8b1bc4e9c0 100644 --- a/yarn-project/acir-simulator/src/client/private_execution.ts +++ b/yarn-project/acir-simulator/src/client/private_execution.ts @@ -319,7 +319,6 @@ export class PrivateFunctionExecution { // 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. - } /** diff --git a/yarn-project/acir-simulator/src/public/execution.ts b/yarn-project/acir-simulator/src/public/execution.ts index 11063068bc9..75f7dadd994 100644 --- a/yarn-project/acir-simulator/src/public/execution.ts +++ b/yarn-project/acir-simulator/src/public/execution.ts @@ -75,7 +75,9 @@ export function collectPublicDataReads(wasm: IWasmModule, execResult: PublicExec // 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 thisExecPublicDataReads = execResult.contractStorageReads.map(read => + contractStorageReadToPublicDataRead(wasm, read, contractAddress), + ); const unsorted = [ ...thisExecPublicDataReads, ...[...execResult.nestedExecutions].flatMap(result => collectPublicDataReads(wasm, result)), @@ -90,11 +92,16 @@ export function collectPublicDataReads(wasm: IWasmModule, execResult: PublicExec * @param execResult - The topmost execution result. * @returns All public data reads (in execution order). */ -export function collectPublicDataUpdateRequests(wasm: IWasmModule, execResult: PublicExecutionResult): PublicDataUpdateRequest[] { +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 thisExecPublicDataUpdateRequests = execResult.contractStorageUpdateRequests.map(update => + contractStorageUpdateRequestToPublicDataUpdateRequest(wasm, update, contractAddress), + ); const unsorted = [ ...thisExecPublicDataUpdateRequests, ...[...execResult.nestedExecutions].flatMap(result => collectPublicDataUpdateRequests(wasm, result)), @@ -109,11 +116,15 @@ export function collectPublicDataUpdateRequests(wasm: IWasmModule, execResult: P * @param contractAddress - the contract address of the read * @returns The public data read. */ -function contractStorageReadToPublicDataRead(wasm: IWasmModule, read: ContractStorageRead, contractAddress: AztecAddress): PublicDataRead { +function contractStorageReadToPublicDataRead( + wasm: IWasmModule, + read: ContractStorageRead, + contractAddress: AztecAddress, +): PublicDataRead { return new PublicDataRead( computePublicDataTreeIndex(wasm, contractAddress.toField(), read.storageSlot), computePublicDataTreeValue(wasm, read.currentValue), - read.sideEffectCounter! + read.sideEffectCounter!, ); } @@ -124,11 +135,15 @@ function contractStorageReadToPublicDataRead(wasm: IWasmModule, read: ContractSt * @param contractAddress - the contract address of the read * @returns The public data read. */ -function contractStorageUpdateRequestToPublicDataUpdateRequest(wasm: IWasmModule, update: ContractStorageUpdateRequest, contractAddress: AztecAddress): PublicDataUpdateRequest { +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! + update.sideEffectCounter!, ); } diff --git a/yarn-project/acir-simulator/src/public/index.ts b/yarn-project/acir-simulator/src/public/index.ts index a8ff40ef13c..c5297f5ad00 100644 --- a/yarn-project/acir-simulator/src/public/index.ts +++ b/yarn-project/acir-simulator/src/public/index.ts @@ -1,3 +1,9 @@ export * from './db.js'; -export { PublicExecution, PublicExecutionResult, isPublicExecutionResult, collectPublicDataReads, collectPublicDataUpdateRequests } from './execution.js'; +export { + PublicExecution, + PublicExecutionResult, + isPublicExecutionResult, + collectPublicDataReads, + collectPublicDataUpdateRequests, +} from './execution.js'; export { PublicExecutor } from './executor.js'; diff --git a/yarn-project/acir-simulator/src/public/state_actions.ts b/yarn-project/acir-simulator/src/public/state_actions.ts index 26d2a8c0a89..b9297d2793c 100644 --- a/yarn-project/acir-simulator/src/public/state_actions.ts +++ b/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 = 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, /** Side effect counter. */ sideEffectCounter: number } + { + /** The old value. */ oldValue: Fr; + /** The updated value. */ newValue: Fr; + /** Side effect counter. */ sideEffectCounter: number; + } > = new Map(); constructor(private db: PublicStateDB, private address: AztecAddress) {} @@ -36,7 +43,7 @@ export class ContractStorageActionsCollector { 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, sideEffectCounter, }); + this.contractStorageReads.set(slot, { currentValue: value, sideEffectCounter }); return value; } diff --git a/yarn-project/circuits.js/src/abis/abis.ts b/yarn-project/circuits.js/src/abis/abis.ts index 806adc2a2d7..b9acda0c2de 100644 --- a/yarn-project/circuits.js/src/abis/abis.ts +++ b/yarn-project/circuits.js/src/abis/abis.ts @@ -9,9 +9,9 @@ import { abisComputeBlockHashWithGlobals, abisComputeCommitmentNonce, abisComputeGlobalsHash, - abisComputeUniqueCommitment, - abisComputePublicDataTreeValue, abisComputePublicDataTreeIndex, + abisComputePublicDataTreeValue, + abisComputeUniqueCommitment, abisSiloCommitment, abisSiloNullifier, } from '../cbind/circuits.gen.js'; @@ -404,7 +404,6 @@ export function computePublicDataTreeIndex(wasm: IWasmModule, contractAddress: F return abisComputePublicDataTreeIndex(wasm, contractAddress, storageSlot); } - const ARGS_HASH_CHUNK_SIZE = 32; const ARGS_HASH_CHUNK_COUNT = 16; diff --git a/yarn-project/circuits.js/src/structs/public_call_request.ts b/yarn-project/circuits.js/src/structs/public_call_request.ts index 80655711c8a..b4ba51f8c58 100644 --- a/yarn-project/circuits.js/src/structs/public_call_request.ts +++ b/yarn-project/circuits.js/src/structs/public_call_request.ts @@ -79,7 +79,13 @@ export class PublicCallRequest { * @returns The array. */ static getFields(fields: FieldsOf) { - return [fields.contractAddress, fields.functionData, fields.callContext, fields.args, fields.sideEffectCounter] as const; + return [ + fields.contractAddress, + fields.functionData, + fields.callContext, + fields.args, + fields.sideEffectCounter, + ] as const; } /** diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.ts index 7300513358f..f6a9c011c12 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.ts @@ -1,4 +1,11 @@ -import { PublicExecution, PublicExecutionResult, PublicExecutor, isPublicExecutionResult, collectPublicDataReads, collectPublicDataUpdateRequests } from '@aztec/acir-simulator'; +import { + PublicExecution, + PublicExecutionResult, + PublicExecutor, + collectPublicDataReads, + collectPublicDataUpdateRequests, + isPublicExecutionResult, +} from '@aztec/acir-simulator'; import { AztecAddress, CircuitsWasm, @@ -164,7 +171,9 @@ export class PublicProcessor { const result = isExecutionRequest ? await this.publicExecutor.execute(current, this.globalVariables) : current; newUnencryptedFunctionLogs.push(result.unencryptedLogs); const functionSelector = result.execution.functionData.functionSelectorBuffer.toString('hex'); - this.log(`Running public kernel circuit for ${functionSelector}@${result.execution.contractAddress.toString()}`); + this.log( + `Running public kernel circuit for ${functionSelector}@${result.execution.contractAddress.toString()}`, + ); executionStack.push(...result.nestedExecutions); const preimages = await this.getPublicCallStackPreimages(result); const callData = await this.getPublicCallData(result, preimages, isExecutionRequest); @@ -173,7 +182,7 @@ export class PublicProcessor { if (!enqueuedExecutionResult) enqueuedExecutionResult = result; } - // HACK(#1622): Manually patches the ordering of public state actions + // HACK(#1685): Manually patches the ordering of public state actions // TODO(#757): Enforce proper ordering of public state actions await this.patchPublicStorageActionOrdering(kernelOutput, enqueuedExecutionResult!); } @@ -332,19 +341,27 @@ export class PublicProcessor { // Validate all items in enqueued public calls are in the kernel emitted stack const readsAreEqual = simPublicDataReads.reduce( - (accum, read) => accum && !!publicDataReads.find(item => item.leafIndex.equals(read.leafIndex) && item.value.equals(read.value)), + (accum, read) => + accum && !!publicDataReads.find(item => item.leafIndex.equals(read.leafIndex) && item.value.equals(read.value)), true, ); const updatesAreEqual = simPublicDataUpdateRequests.reduce( - (accum, update) => accum && !!publicDataUpdateRequests.find(item => item.leafIndex.equals(update.leafIndex) && item.oldValue.equals(update.oldValue) && item.newValue.equals(update.newValue)), + (accum, update) => + accum && + !!publicDataUpdateRequests.find( + item => + item.leafIndex.equals(update.leafIndex) && + item.oldValue.equals(update.oldValue) && + item.newValue.equals(update.newValue), + ), true, ); if (!readsAreEqual) { throw new Error( `Public data reads from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataReads - .map(p => p.toFriendlyJSON()) - .join(', ')}\nFrom public kernel: ${publicDataReads.map(i => i.toFriendlyJSON()).join(', ')}`, + .map(p => p.toFriendlyJSON()) + .join(', ')}\nFrom public kernel: ${publicDataReads.map(i => i.toFriendlyJSON()).join(', ')}`, ); } if (!updatesAreEqual) { @@ -358,8 +375,14 @@ export class PublicProcessor { // Assume that kernel public inputs has the right number of items. // We only want to reorder the items from the public inputs of the // most recently processed top/enqueued call. - const numTotalReadsInKernel = arrayNonEmptyLength(publicInputs.end.publicDataReads, f => f.leafIndex.equals(Fr.ZERO) && f.value.equals(Fr.ZERO)); - const numTotalUpdatesInKernel = arrayNonEmptyLength(publicInputs.end.publicDataUpdateRequests, f => f.leafIndex.equals(Fr.ZERO) && f.oldValue.equals(Fr.ZERO) && f.newValue.equals(Fr.ZERO)); + const numTotalReadsInKernel = arrayNonEmptyLength( + publicInputs.end.publicDataReads, + f => f.leafIndex.equals(Fr.ZERO) && f.value.equals(Fr.ZERO), + ); + const numTotalUpdatesInKernel = arrayNonEmptyLength( + publicInputs.end.publicDataUpdateRequests, + f => f.leafIndex.equals(Fr.ZERO) && f.oldValue.equals(Fr.ZERO) && f.newValue.equals(Fr.ZERO), + ); const numReadsBeforeThisEnqueuedCall = numTotalReadsInKernel - simPublicDataReads.length; const numUpdatesBeforeThisEnqueuedCall = numTotalUpdatesInKernel - simPublicDataUpdateRequests.length; @@ -368,7 +391,7 @@ export class PublicProcessor { [ // do not mess with items from previous top/enqueued calls in kernel output ...publicDataReads.slice(0, numReadsBeforeThisEnqueuedCall), - ...simPublicDataReads + ...simPublicDataReads, ], PublicDataRead.empty(), MAX_PUBLIC_DATA_READS_PER_TX, @@ -378,7 +401,7 @@ export class PublicProcessor { [ // do not mess with items from previous top/enqueued calls in kernel output ...publicDataUpdateRequests.slice(0, numUpdatesBeforeThisEnqueuedCall), - ...simPublicDataUpdateRequests + ...simPublicDataUpdateRequests, ], PublicDataUpdateRequest.empty(), MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, From 8728fa3510508a5f0ac677482851308f4a01668a Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Mon, 21 Aug 2023 02:52:31 +0000 Subject: [PATCH 6/8] fix acir sim tests --- .../acir-simulator/src/public/executor.ts | 5 +++++ .../acir-simulator/src/public/index.test.ts | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/yarn-project/acir-simulator/src/public/executor.ts b/yarn-project/acir-simulator/src/public/executor.ts index 1ab6f5bb37f..3ecd300b0b7 100644 --- a/yarn-project/acir-simulator/src/public/executor.ts +++ b/yarn-project/acir-simulator/src/public/executor.ts @@ -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, diff --git a/yarn-project/acir-simulator/src/public/index.test.ts b/yarn-project/acir-simulator/src/public/index.test.ts index cf45331d690..75df0ff093c 100644 --- a/yarn-project/acir-simulator/src/public/index.test.ts +++ b/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([]); From e3c051046f87ee413ee279f5b61d8166a54df7d2 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Mon, 21 Aug 2023 03:06:56 +0000 Subject: [PATCH 7/8] remove commented out logger --- yarn-project/end-to-end/src/e2e_ordering.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_ordering.test.ts b/yarn-project/end-to-end/src/e2e_ordering.test.ts index a93149cb151..cbfaaf37d19 100644 --- a/yarn-project/end-to-end/src/e2e_ordering.test.ts +++ b/yarn-project/end-to-end/src/e2e_ordering.test.ts @@ -15,7 +15,6 @@ describe('e2e_ordering', () => { let aztecNode: AztecNodeService | undefined; let aztecRpcServer: AztecRPC; let wallet: Wallet; - //let logger: DebugLogger; const expectLogsFromLastBlockToBe = async (logMessages: bigint[]) => { const l2BlockNum = await aztecRpcServer.getBlockNum(); @@ -27,7 +26,7 @@ describe('e2e_ordering', () => { }; beforeEach(async () => { - ({ aztecNode, aztecRpcServer, wallet /*, logger*/ } = await setup()); + ({ aztecNode, aztecRpcServer, wallet } = await setup()); }, 100_000); afterEach(async () => { From fb0858785bfc1c07a2a5c12ea1d6f2f2895f4b3c Mon Sep 17 00:00:00 2001 From: Suyash Bagad Date: Mon, 21 Aug 2023 15:23:03 +0000 Subject: [PATCH 8/8] Address Jean's comments. --- yarn-project/acir-simulator/src/public/execution.ts | 4 ++-- yarn-project/acir-simulator/src/public/executor.ts | 4 ++-- .../sequencer-client/src/sequencer/public_processor.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/yarn-project/acir-simulator/src/public/execution.ts b/yarn-project/acir-simulator/src/public/execution.ts index 75f7dadd994..e26dba8831d 100644 --- a/yarn-project/acir-simulator/src/public/execution.ts +++ b/yarn-project/acir-simulator/src/public/execution.ts @@ -132,8 +132,8 @@ function contractStorageReadToPublicDataRead( * 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 read - * @returns The public data read. + * @param contractAddress - the contract address of the data update request. + * @returns The public data update request. */ function contractStorageUpdateRequestToPublicDataUpdateRequest( wasm: IWasmModule, diff --git a/yarn-project/acir-simulator/src/public/executor.ts b/yarn-project/acir-simulator/src/public/executor.ts index 9bc701b7797..b9cf8f9f546 100644 --- a/yarn-project/acir-simulator/src/public/executor.ts +++ b/yarn-project/acir-simulator/src/public/executor.ts @@ -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, this.sideEffectCounter++); // update after assigning current value to storage action + 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, this.sideEffectCounter++); // update after assigning current value to storage action + 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); diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.ts index f6a9c011c12..5c561cc544b 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.ts @@ -182,7 +182,7 @@ export class PublicProcessor { if (!enqueuedExecutionResult) enqueuedExecutionResult = result; } - // HACK(#1685): Manually patches the ordering of public state actions + // HACK(#1622): Manually patches the ordering of public state actions // TODO(#757): Enforce proper ordering of public state actions await this.patchPublicStorageActionOrdering(kernelOutput, enqueuedExecutionResult!); }