From 874778080dec661b2bc6749b929c6191ad392edc Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Fri, 8 Mar 2024 10:00:25 -0500 Subject: [PATCH 1/9] Comment proposed changes to open draft PR --- yarn-project/simulator/src/public/db.ts | 26 ++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/yarn-project/simulator/src/public/db.ts b/yarn-project/simulator/src/public/db.ts index fedbfe55c155..de081447a009 100644 --- a/yarn-project/simulator/src/public/db.ts +++ b/yarn-project/simulator/src/public/db.ts @@ -33,11 +33,27 @@ export interface PublicStateDB { */ commit(): Promise; - /** - * Rollback the pending changes. - * @returns Nothing. - */ - rollback(): Promise; + // Proposed interface changes: + // /** + // * Mark the uncommitted changes in this TX as a checkpoint. + // */ + // checkpoint(): Promise; + + // /** + // * Rollback to the last checkpoint. + // */ + // rollbackToCheckpoint(): Promise; + + // /** + // * Commit the changes in this TX. Includes all changes since the last commit, + // * even if they haven't been covered by a checkpoint. + // */ + // commit(): Promise; + + // /** + // * Rollback to the last commit. + // */ + // rollbackToCommit(): Promise; } /** From 200ec74300adb436a7590986971229407e7af349 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Fri, 8 Mar 2024 10:12:48 -0500 Subject: [PATCH 2/9] amend previous. --- yarn-project/simulator/src/public/db.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/yarn-project/simulator/src/public/db.ts b/yarn-project/simulator/src/public/db.ts index de081447a009..dd10a486aae4 100644 --- a/yarn-project/simulator/src/public/db.ts +++ b/yarn-project/simulator/src/public/db.ts @@ -33,6 +33,12 @@ export interface PublicStateDB { */ commit(): Promise; + /** + * Rollback the pending changes. + * @returns Nothing. + */ + rollback(): Promise; + // Proposed interface changes: // /** // * Mark the uncommitted changes in this TX as a checkpoint. From 44164e7e50697a833a48630a4992a0ddcf5ee84a Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 11 Mar 2024 10:11:06 -0400 Subject: [PATCH 3/9] public processor unit tests passing --- .../src/sequencer/abstract_phase_manager.ts | 29 ++- .../src/sequencer/app_logic_phase_manager.ts | 26 +- .../src/sequencer/public_processor.test.ts | 233 +++++++++++++++++- .../src/sequencer/setup_phase_manager.ts | 23 +- .../src/sequencer/tail_phase_manager.ts | 17 +- .../src/sequencer/teardown_phase_manager.ts | 23 +- .../src/simulator/public_executor.ts | 13 +- .../simulator/world_state_public_db.test.ts | 24 +- yarn-project/simulator/src/public/db.ts | 39 +-- 9 files changed, 315 insertions(+), 112 deletions(-) diff --git a/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts index e42d79569a68..65e2241e5c36 100644 --- a/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts +++ b/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts @@ -59,7 +59,6 @@ import { getVerificationKeys } from '../mocks/verification_keys.js'; import { PublicProver } from '../prover/index.js'; import { PublicKernelCircuitSimulator } from '../simulator/index.js'; import { HintsBuilder } from './hints_builder.js'; -import { FailedTx } from './processed_tx.js'; import { lastSideEffectCounter } from './utils.js'; export enum PublicKernelPhase { @@ -115,7 +114,6 @@ export abstract class AbstractPhaseManager { */ revertReason: SimulationError | undefined; }>; - abstract rollback(tx: Tx, err: unknown): Promise; public static extractEnqueuedPublicCallsByPhase( publicInputs: PrivateKernelTailCircuitPublicInputs, @@ -220,8 +218,18 @@ export abstract class AbstractPhaseManager { const result = isExecutionRequest ? await simulator(current, this.globalVariables) : current; - newUnencryptedFunctionLogs.push(result.unencryptedLogs); const functionSelector = result.execution.functionData.selector.toString(); + if (result.reverted && !PhaseIsRevertible[this.phase]) { + this.log.debug( + `Simulation error on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${ + result.revertReason + }`, + ); + throw result.revertReason; + } + + newUnencryptedFunctionLogs.push(result.unencryptedLogs); + this.log.debug( `Running public kernel circuit for ${result.execution.contractAddress.toString()}:${functionSelector}`, ); @@ -230,14 +238,23 @@ export abstract class AbstractPhaseManager { [kernelOutput, kernelProof] = await this.runKernelCircuit(kernelOutput, kernelProof, callData); - if (kernelOutput.reverted && this.phase === PublicKernelPhase.APP_LOGIC) { + // sanity check. Note we can't expect them to just be equal, because e.g. + // if the simulator reverts in app logic, it "resets" and result.reverted will be false when we run teardown, + // but the kernel carries the reverted flag forward. But if the simulator reverts, so should the kernel. + if (result.reverted && !kernelOutput.reverted) { + throw new Error( + `Public kernel circuit did not revert on ${result.execution.contractAddress.toString()}:${functionSelector}, but simulator did.`, + ); + } + + // We know the phase is revertible due to the above check. + // So safely return the revert reason and the kernel output (which has had its revertible side effects dropped) + if (result.reverted) { this.log.debug( `Reverting on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${ result.revertReason }`, ); - // halt immediately if the public kernel circuit has reverted. - // return no logs, as they should not go on-chain. return [kernelOutput, kernelProof, [], result.revertReason]; } diff --git a/yarn-project/sequencer-client/src/sequencer/app_logic_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/app_logic_phase_manager.ts index 8ddbb329df95..6c0ab1d6f56b 100644 --- a/yarn-project/sequencer-client/src/sequencer/app_logic_phase_manager.ts +++ b/yarn-project/sequencer-client/src/sequencer/app_logic_phase_manager.ts @@ -7,7 +7,6 @@ import { PublicProver } from '../prover/index.js'; import { PublicKernelCircuitSimulator } from '../simulator/index.js'; import { ContractsDataSourcePublicDB } from '../simulator/public_executor.js'; import { AbstractPhaseManager, PublicKernelPhase } from './abstract_phase_manager.js'; -import { FailedTx } from './processed_tx.js'; /** * The phase manager responsible for performing the fee preparation phase. @@ -39,27 +38,22 @@ export class AppLogicPhaseManager extends AbstractPhaseManager { this.log(`Processing tx ${tx.getTxHash()}`); await this.publicContractsDB.addNewContracts(tx); const [publicKernelOutput, publicKernelProof, newUnencryptedFunctionLogs, revertReason] = - await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof); + await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof).catch( + // if we throw for any reason other than simulation, we need to rollback and drop the TX + async err => { + await this.publicStateDB.rollbackToCommit(); + throw err; + }, + ); if (revertReason) { - await this.rollback(tx, revertReason); + await this.publicContractsDB.removeNewContracts(tx); + await this.publicStateDB.rollbackToCheckpoint(); } else { tx.unencryptedLogs.addFunctionLogs(newUnencryptedFunctionLogs); - await this.publicStateDB.commit(); + await this.publicStateDB.checkpoint(); } return { publicKernelOutput, publicKernelProof, revertReason }; } - - async rollback(tx: Tx, err: unknown): Promise { - this.log.warn(`Rolling back changes from ${tx.getTxHash()}`); - // remove contracts on failure - await this.publicContractsDB.removeNewContracts(tx); - // rollback any state updates from this failed transaction - await this.publicStateDB.rollback(); - return { - tx, - error: err instanceof Error ? err : new Error('Unknown error'), - }; - } } diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts index 338b91e515e4..f64d0585d357 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts @@ -164,7 +164,7 @@ describe('public_processor', () => { expect(failed[0].tx).toEqual(tx); expect(failed[0].error).toEqual(new SimulationError(`Failed`, [])); expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(0); - expect(publicWorldStateDB.rollback).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(1); }); }); @@ -233,8 +233,8 @@ describe('public_processor', () => { expect(processed).toEqual([expectedTxByHash(tx)]); expect(failed).toHaveLength(0); expect(publicExecutor.simulate).toHaveBeenCalledTimes(2); - expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(2); - expect(publicWorldStateDB.rollback).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(0); }); it('runs a tx with an enqueued public call with nested execution', async function () { @@ -277,8 +277,10 @@ describe('public_processor', () => { expect(processed).toEqual([expectedTxByHash(tx)]); expect(failed).toHaveLength(0); expect(publicExecutor.simulate).toHaveBeenCalledTimes(1); - expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(2); - expect(publicWorldStateDB.rollback).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.checkpoint).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.rollbackToCheckpoint).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(0); }); it('rolls back app logic db updates on failed public execution, but persists setup/teardown', async function () { @@ -378,8 +380,10 @@ describe('public_processor', () => { expect(appLogicSpy).toHaveBeenCalledTimes(2); expect(teardownSpy).toHaveBeenCalledTimes(2); expect(publicExecutor.simulate).toHaveBeenCalledTimes(3); - expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(3); - expect(publicWorldStateDB.rollback).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.checkpoint).toHaveBeenCalledTimes(2); + expect(publicWorldStateDB.rollbackToCheckpoint).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(0); expect(arrayNonEmptyLength(processed[0].data.combinedData.publicCallStack, i => i.isEmpty())).toEqual(0); @@ -395,6 +399,215 @@ describe('public_processor', () => { expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(0); }); + it('fails a transaction that reverts in setup', async function () { + const baseContractAddressSeed = 0x200; + const baseContractAddress = makeAztecAddress(baseContractAddressSeed); + const callRequests: PublicCallRequest[] = [ + baseContractAddressSeed, + baseContractAddressSeed, + baseContractAddressSeed, + ].map(makePublicCallRequest); + callRequests[0].callContext.startSideEffectCounter = 2; + callRequests[1].callContext.startSideEffectCounter = 3; + callRequests[2].callContext.startSideEffectCounter = 4; + + const kernelOutput = makePrivateKernelTailCircuitPublicInputs(0x10); + kernelOutput.end.unencryptedLogsHash = [Fr.ZERO, Fr.ZERO]; + + addKernelPublicCallStack(kernelOutput, { + setupCalls: [callRequests[0]], + appLogicCalls: [callRequests[2]], + teardownCall: callRequests[1], + }); + + const tx = new Tx( + kernelOutput, + proof, + TxL2Logs.empty(), + TxL2Logs.empty(), + // reverse because `enqueuedPublicFunctions` expects the last element to be the front of the queue + callRequests.slice().reverse(), + [ExtendedContractData.random()], + ); + + const contractSlotA = fr(0x100); + const contractSlotB = fr(0x150); + const contractSlotC = fr(0x200); + + let simulatorCallCount = 0; + const simulatorResults: PublicExecutionResult[] = [ + // Setup + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[0], + contractStorageUpdateRequests: [new ContractStorageUpdateRequest(contractSlotA, fr(0x101))], + nestedExecutions: [ + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + contractStorageUpdateRequests: [ + new ContractStorageUpdateRequest(contractSlotA, fr(0x102)), + new ContractStorageUpdateRequest(contractSlotB, fr(0x151)), + ], + }).build(), + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + revertReason: new SimulationError('Simulation Failed', []), + }).build(), + ], + }).build(), + + // App Logic + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[2], + }).build(), + + // Teardown + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[1], + nestedExecutions: [ + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + contractStorageUpdateRequests: [new ContractStorageUpdateRequest(contractSlotC, fr(0x201))], + }).build(), + ], + }).build(), + ]; + + publicExecutor.simulate.mockImplementation(execution => { + if (simulatorCallCount < simulatorResults.length) { + return Promise.resolve(simulatorResults[simulatorCallCount++]); + } else { + throw new Error(`Unexpected execution request: ${execution}, call count: ${simulatorCallCount}`); + } + }); + + const setupSpy = jest.spyOn(publicKernel, 'publicKernelCircuitSetup'); + const appLogicSpy = jest.spyOn(publicKernel, 'publicKernelCircuitAppLogic'); + const teardownSpy = jest.spyOn(publicKernel, 'publicKernelCircuitTeardown'); + + const [processed, failed] = await processor.process([tx]); + + expect(processed).toHaveLength(0); + expect(failed).toHaveLength(1); + expect(failed[0].tx.getTxHash()).toEqual(tx.getTxHash()); + + expect(setupSpy).toHaveBeenCalledTimes(1); + expect(appLogicSpy).toHaveBeenCalledTimes(0); + expect(teardownSpy).toHaveBeenCalledTimes(0); + expect(publicExecutor.simulate).toHaveBeenCalledTimes(1); + + expect(publicWorldStateDB.checkpoint).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.rollbackToCheckpoint).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(1); + }); + + it('fails a transaction that reverts in teardown', async function () { + const baseContractAddressSeed = 0x200; + const baseContractAddress = makeAztecAddress(baseContractAddressSeed); + const callRequests: PublicCallRequest[] = [ + baseContractAddressSeed, + baseContractAddressSeed, + baseContractAddressSeed, + ].map(makePublicCallRequest); + callRequests[0].callContext.startSideEffectCounter = 2; + callRequests[1].callContext.startSideEffectCounter = 3; + callRequests[2].callContext.startSideEffectCounter = 4; + + const kernelOutput = makePrivateKernelTailCircuitPublicInputs(0x10); + kernelOutput.end.unencryptedLogsHash = [Fr.ZERO, Fr.ZERO]; + + addKernelPublicCallStack(kernelOutput, { + setupCalls: [callRequests[0]], + appLogicCalls: [callRequests[2]], + teardownCall: callRequests[1], + }); + + const tx = new Tx( + kernelOutput, + proof, + TxL2Logs.empty(), + TxL2Logs.empty(), + // reverse because `enqueuedPublicFunctions` expects the last element to be the front of the queue + callRequests.slice().reverse(), + [ExtendedContractData.random()], + ); + + const contractSlotA = fr(0x100); + const contractSlotB = fr(0x150); + const contractSlotC = fr(0x200); + + let simulatorCallCount = 0; + const simulatorResults: PublicExecutionResult[] = [ + // Setup + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[0], + contractStorageUpdateRequests: [new ContractStorageUpdateRequest(contractSlotA, fr(0x101))], + nestedExecutions: [ + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + contractStorageUpdateRequests: [ + new ContractStorageUpdateRequest(contractSlotA, fr(0x102)), + new ContractStorageUpdateRequest(contractSlotB, fr(0x151)), + ], + }).build(), + ], + }).build(), + + // App Logic + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[2], + }).build(), + + // Teardown + PublicExecutionResultBuilder.fromPublicCallRequest({ + request: callRequests[1], + nestedExecutions: [ + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + revertReason: new SimulationError('Simulation Failed', []), + }).build(), + PublicExecutionResultBuilder.fromFunctionCall({ + from: callRequests[1].contractAddress, + tx: makeFunctionCall(baseContractAddress, makeSelector(5)), + contractStorageUpdateRequests: [new ContractStorageUpdateRequest(contractSlotC, fr(0x201))], + }).build(), + ], + }).build(), + ]; + + publicExecutor.simulate.mockImplementation(execution => { + if (simulatorCallCount < simulatorResults.length) { + return Promise.resolve(simulatorResults[simulatorCallCount++]); + } else { + throw new Error(`Unexpected execution request: ${execution}, call count: ${simulatorCallCount}`); + } + }); + + const setupSpy = jest.spyOn(publicKernel, 'publicKernelCircuitSetup'); + const appLogicSpy = jest.spyOn(publicKernel, 'publicKernelCircuitAppLogic'); + const teardownSpy = jest.spyOn(publicKernel, 'publicKernelCircuitTeardown'); + + const [processed, failed] = await processor.process([tx]); + + expect(processed).toHaveLength(0); + expect(failed).toHaveLength(1); + expect(failed[0].tx.getTxHash()).toEqual(tx.getTxHash()); + + expect(setupSpy).toHaveBeenCalledTimes(2); + expect(appLogicSpy).toHaveBeenCalledTimes(1); + expect(teardownSpy).toHaveBeenCalledTimes(2); + expect(publicExecutor.simulate).toHaveBeenCalledTimes(3); + expect(publicWorldStateDB.checkpoint).toHaveBeenCalledTimes(2); + expect(publicWorldStateDB.rollbackToCheckpoint).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(1); + }); + it('runs a tx with setup and teardown phases', async function () { const baseContractAddressSeed = 0x200; const baseContractAddress = makeAztecAddress(baseContractAddressSeed); @@ -487,8 +700,10 @@ describe('public_processor', () => { expect(appLogicSpy).toHaveBeenCalledTimes(1); expect(teardownSpy).toHaveBeenCalledTimes(3); expect(publicExecutor.simulate).toHaveBeenCalledTimes(3); - expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(4); - expect(publicWorldStateDB.rollback).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.checkpoint).toHaveBeenCalledTimes(3); + expect(publicWorldStateDB.rollbackToCheckpoint).toHaveBeenCalledTimes(0); + expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(1); + expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(0); const txEffect = toTxEffect(processed[0]); expect(arrayNonEmptyLength(txEffect.publicDataWrites, PublicDataWrite.isEmpty)).toEqual(3); diff --git a/yarn-project/sequencer-client/src/sequencer/setup_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/setup_phase_manager.ts index 070783c1127a..f30c50ee3e49 100644 --- a/yarn-project/sequencer-client/src/sequencer/setup_phase_manager.ts +++ b/yarn-project/sequencer-client/src/sequencer/setup_phase_manager.ts @@ -7,7 +7,6 @@ import { PublicProver } from '../prover/index.js'; import { PublicKernelCircuitSimulator } from '../simulator/index.js'; import { ContractsDataSourcePublicDB } from '../simulator/public_executor.js'; import { AbstractPhaseManager, PublicKernelPhase } from './abstract_phase_manager.js'; -import { FailedTx } from './processed_tx.js'; /** * The phase manager responsible for performing the fee preparation phase. @@ -34,21 +33,15 @@ export class SetupPhaseManager extends AbstractPhaseManager { ) { this.log(`Processing tx ${tx.getTxHash()}`); const [publicKernelOutput, publicKernelProof, newUnencryptedFunctionLogs, revertReason] = - await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof); + await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof).catch( + // the abstract phase manager throws if simulation gives error in a non-revertible phase + async err => { + await this.publicStateDB.rollbackToCommit(); + throw err; + }, + ); tx.unencryptedLogs.addFunctionLogs(newUnencryptedFunctionLogs); - - // commit the state updates from this transaction - await this.publicStateDB.commit(); - + await this.publicStateDB.checkpoint(); return { publicKernelOutput, publicKernelProof, revertReason }; } - - async rollback(tx: Tx, err: unknown): Promise { - this.log.warn(`Error processing tx ${tx.getTxHash()}: ${err}`); - await this.publicStateDB.rollback(); - return { - tx, - error: err instanceof Error ? err : new Error('Unknown error'), - }; - } } diff --git a/yarn-project/sequencer-client/src/sequencer/tail_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/tail_phase_manager.ts index ffb1e018ba62..804623c13c14 100644 --- a/yarn-project/sequencer-client/src/sequencer/tail_phase_manager.ts +++ b/yarn-project/sequencer-client/src/sequencer/tail_phase_manager.ts @@ -7,7 +7,6 @@ import { PublicProver } from '../prover/index.js'; import { PublicKernelCircuitSimulator } from '../simulator/index.js'; import { ContractsDataSourcePublicDB } from '../simulator/public_executor.js'; import { AbstractPhaseManager, PublicKernelPhase } from './abstract_phase_manager.js'; -import { FailedTx } from './processed_tx.js'; export class TailPhaseManager extends AbstractPhaseManager { constructor( @@ -26,10 +25,15 @@ export class TailPhaseManager extends AbstractPhaseManager { async handle(tx: Tx, previousPublicKernelOutput: PublicKernelCircuitPublicInputs, previousPublicKernelProof: Proof) { this.log(`Processing tx ${tx.getTxHash()}`); - this.log(`Executing tail circuit for tx ${tx.getTxHash()}`); const [publicKernelOutput, publicKernelProof] = await this.runKernelCircuit( previousPublicKernelOutput, previousPublicKernelProof, + ).catch( + // the abstract phase manager throws if simulation gives error in non-revertible phase + async err => { + await this.publicStateDB.rollbackToCommit(); + throw err; + }, ); // commit the state updates from this transaction @@ -37,13 +41,4 @@ export class TailPhaseManager extends AbstractPhaseManager { return { publicKernelOutput, publicKernelProof, revertReason: undefined }; } - - async rollback(tx: Tx, err: unknown): Promise { - this.log.warn(`Error processing tx ${tx.getTxHash()}: ${err}`); - await this.publicStateDB.rollback(); - return { - tx, - error: err instanceof Error ? err : new Error('Unknown error'), - }; - } } diff --git a/yarn-project/sequencer-client/src/sequencer/teardown_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/teardown_phase_manager.ts index f466218fa590..f263806caf59 100644 --- a/yarn-project/sequencer-client/src/sequencer/teardown_phase_manager.ts +++ b/yarn-project/sequencer-client/src/sequencer/teardown_phase_manager.ts @@ -7,7 +7,6 @@ import { PublicProver } from '../prover/index.js'; import { PublicKernelCircuitSimulator } from '../simulator/index.js'; import { ContractsDataSourcePublicDB } from '../simulator/public_executor.js'; import { AbstractPhaseManager, PublicKernelPhase } from './abstract_phase_manager.js'; -import { FailedTx } from './processed_tx.js'; /** * The phase manager responsible for performing the fee preparation phase. @@ -34,21 +33,15 @@ export class TeardownPhaseManager extends AbstractPhaseManager { ) { this.log(`Processing tx ${tx.getTxHash()}`); const [publicKernelOutput, publicKernelProof, newUnencryptedFunctionLogs, revertReason] = - await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof); + await this.processEnqueuedPublicCalls(tx, previousPublicKernelOutput, previousPublicKernelProof).catch( + // the abstract phase manager throws if simulation gives error in a non-revertible phase + async err => { + await this.publicStateDB.rollbackToCommit(); + throw err; + }, + ); tx.unencryptedLogs.addFunctionLogs(newUnencryptedFunctionLogs); - - // commit the state updates from this transaction - await this.publicStateDB.commit(); - + await this.publicStateDB.checkpoint(); return { publicKernelOutput, publicKernelProof, revertReason }; } - - async rollback(tx: Tx, err: unknown): Promise { - this.log.warn(`Error processing tx ${tx.getTxHash()}: ${err}`); - await this.publicStateDB.rollback(); - return { - tx, - error: err instanceof Error ? err : new Error('Unknown error'), - }; - } } diff --git a/yarn-project/sequencer-client/src/simulator/public_executor.ts b/yarn-project/sequencer-client/src/simulator/public_executor.ts index 44556483d27b..48a21c7adca2 100644 --- a/yarn-project/sequencer-client/src/simulator/public_executor.ts +++ b/yarn-project/sequencer-client/src/simulator/public_executor.ts @@ -131,6 +131,7 @@ export class ContractsDataSourcePublicDB implements PublicContractsDB { */ export class WorldStatePublicDB implements PublicStateDB { private commitedWriteCache: Map = new Map(); + private checkpointedWriteCache: Map = new Map(); private uncommitedWriteCache: Map = new Map(); constructor(private db: MerkleTreeOperations) {} @@ -185,17 +186,25 @@ export class WorldStatePublicDB implements PublicStateDB { for (const [k, v] of this.uncommitedWriteCache) { this.commitedWriteCache.set(k, v); } - return this.rollback(); + return this.rollbackToCommit(); } /** * Rollback the pending changes. * @returns Nothing. */ - rollback(): Promise { + rollbackToCommit(): Promise { this.uncommitedWriteCache = new Map(); return Promise.resolve(); } + + checkpoint(): Promise { + return Promise.resolve(); + } + + rollbackToCheckpoint(): Promise { + return Promise.resolve(); + } } /** diff --git a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts index 885abc0b192d..3f7ff6977d2e 100644 --- a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts +++ b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts @@ -104,14 +104,14 @@ describe('world_state_public_db', () => { // commit the data await publicStateDb.commit(); - // should read back the commited value + // should read back the committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // other slots should be unchanged expect(await publicStateDb.storageRead(addresses[1], slots[1])).toEqual(dbValues[1]); }); - it('will not rollback a commited value', async function () { + it('will not rollback a committed value', async function () { const publicStateDb = new WorldStatePublicDB(db); expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(dbValues[0]); @@ -123,12 +123,12 @@ describe('world_state_public_db', () => { // commit the data await publicStateDb.commit(); - // should read back the commited value + // should read back the committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); - await publicStateDb.rollback(); + await publicStateDb.rollbackToCommit(); - // should still read back the commited value + // should still read back the committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); }); @@ -145,7 +145,7 @@ describe('world_state_public_db', () => { expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // now rollback - await publicStateDb.rollback(); + await publicStateDb.rollbackToCommit(); // should now read the original value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(dbValues[0]); @@ -163,7 +163,7 @@ describe('world_state_public_db', () => { // commit the data await publicStateDb.commit(); - // should read back the commited value + // should read back the committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // other slots should be unchanged @@ -178,7 +178,7 @@ describe('world_state_public_db', () => { expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue2); }); - it('rolls back to previously commited value', async function () { + it('rolls back to previously committed value', async function () { const publicStateDb = new WorldStatePublicDB(db); expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(dbValues[0]); @@ -190,7 +190,7 @@ describe('world_state_public_db', () => { // commit the data await publicStateDb.commit(); - // should read back the commited value + // should read back the committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // other slots should be unchanged @@ -201,13 +201,13 @@ describe('world_state_public_db', () => { // write a new value to our first value await publicStateDb.storageWrite(addresses[0], slots[0], newValue2); - // should read back the uncommited value + // should read back the uncommitted value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue2); // rollback - await publicStateDb.rollback(); + await publicStateDb.rollbackToCommit(); - // should read back the previously commited value + // should read back the previously committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); }); }); diff --git a/yarn-project/simulator/src/public/db.ts b/yarn-project/simulator/src/public/db.ts index dd10a486aae4..e126f7326952 100644 --- a/yarn-project/simulator/src/public/db.ts +++ b/yarn-project/simulator/src/public/db.ts @@ -28,38 +28,25 @@ export interface PublicStateDB { storageWrite(contract: AztecAddress, slot: Fr, newValue: Fr): Promise; /** - * Commit the pending changes to the DB. - * @returns Nothing. + * Mark the uncommitted changes in this TX as a checkpoint. */ - commit(): Promise; + checkpoint(): Promise; /** - * Rollback the pending changes. - * @returns Nothing. + * Rollback to the last checkpoint. */ - rollback(): Promise; - - // Proposed interface changes: - // /** - // * Mark the uncommitted changes in this TX as a checkpoint. - // */ - // checkpoint(): Promise; - - // /** - // * Rollback to the last checkpoint. - // */ - // rollbackToCheckpoint(): Promise; + rollbackToCheckpoint(): Promise; - // /** - // * Commit the changes in this TX. Includes all changes since the last commit, - // * even if they haven't been covered by a checkpoint. - // */ - // commit(): Promise; + /** + * Commit the changes in this TX. Includes all changes since the last commit, + * even if they haven't been covered by a checkpoint. + */ + commit(): Promise; - // /** - // * Rollback to the last commit. - // */ - // rollbackToCommit(): Promise; + /** + * Rollback to the last commit. + */ + rollbackToCommit(): Promise; } /** From bcdaf0b33bb340fc973e624f6ed08faa2d28c390 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 11 Mar 2024 10:47:03 -0400 Subject: [PATCH 4/9] db checkpointing unit test --- cspell.json | 1 + .../src/simulator/public_executor.ts | 42 ++++++++++++------ .../simulator/world_state_public_db.test.ts | 44 +++++++++++++++++++ 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/cspell.json b/cspell.json index 299b4ccf2036..b489357e0e55 100644 --- a/cspell.json +++ b/cspell.json @@ -38,6 +38,7 @@ "chainsafe", "cheatcode", "cheatcodes", + "checkpointed", "checksummed", "cimg", "ciphertext", diff --git a/yarn-project/sequencer-client/src/simulator/public_executor.ts b/yarn-project/sequencer-client/src/simulator/public_executor.ts index 48a21c7adca2..8641bbf0a6dc 100644 --- a/yarn-project/sequencer-client/src/simulator/public_executor.ts +++ b/yarn-project/sequencer-client/src/simulator/public_executor.ts @@ -130,9 +130,9 @@ export class ContractsDataSourcePublicDB implements PublicContractsDB { * Implements the PublicStateDB using a world-state database. */ export class WorldStatePublicDB implements PublicStateDB { - private commitedWriteCache: Map = new Map(); + private committedWriteCache: Map = new Map(); private checkpointedWriteCache: Map = new Map(); - private uncommitedWriteCache: Map = new Map(); + private uncommittedWriteCache: Map = new Map(); constructor(private db: MerkleTreeOperations) {} @@ -144,13 +144,17 @@ export class WorldStatePublicDB implements PublicStateDB { */ public async storageRead(contract: AztecAddress, slot: Fr): Promise { const leafSlot = computePublicDataTreeLeafSlot(contract, slot).value; - const uncommited = this.uncommitedWriteCache.get(leafSlot); - if (uncommited !== undefined) { - return uncommited; + const uncommitted = this.uncommittedWriteCache.get(leafSlot); + if (uncommitted !== undefined) { + return uncommitted; } - const commited = this.commitedWriteCache.get(leafSlot); - if (commited !== undefined) { - return commited; + const checkpointed = this.checkpointedWriteCache.get(leafSlot); + if (checkpointed !== undefined) { + return checkpointed; + } + const committed = this.committedWriteCache.get(leafSlot); + if (committed !== undefined) { + return committed; } const lowLeafResult = await this.db.getPreviousValueIndex(MerkleTreeId.PUBLIC_DATA_TREE, leafSlot); @@ -174,7 +178,7 @@ export class WorldStatePublicDB implements PublicStateDB { */ public storageWrite(contract: AztecAddress, slot: Fr, newValue: Fr): Promise { const index = computePublicDataTreeLeafSlot(contract, slot).value; - this.uncommitedWriteCache.set(index, newValue); + this.uncommittedWriteCache.set(index, newValue); return Promise.resolve(); } @@ -183,8 +187,13 @@ export class WorldStatePublicDB implements PublicStateDB { * @returns Nothing. */ commit(): Promise { - for (const [k, v] of this.uncommitedWriteCache) { - this.commitedWriteCache.set(k, v); + for (const [k, v] of this.checkpointedWriteCache) { + this.committedWriteCache.set(k, v); + } + // uncommitted writes take precedence over checkpointed writes + // since they are the most recent + for (const [k, v] of this.uncommittedWriteCache) { + this.committedWriteCache.set(k, v); } return this.rollbackToCommit(); } @@ -193,16 +202,21 @@ export class WorldStatePublicDB implements PublicStateDB { * Rollback the pending changes. * @returns Nothing. */ - rollbackToCommit(): Promise { - this.uncommitedWriteCache = new Map(); + async rollbackToCommit(): Promise { + await this.rollbackToCheckpoint(); + this.checkpointedWriteCache = new Map(); return Promise.resolve(); } checkpoint(): Promise { - return Promise.resolve(); + for (const [k, v] of this.uncommittedWriteCache) { + this.checkpointedWriteCache.set(k, v); + } + return this.rollbackToCheckpoint(); } rollbackToCheckpoint(): Promise { + this.uncommittedWriteCache = new Map(); return Promise.resolve(); } } diff --git a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts index 3f7ff6977d2e..b8992cdef703 100644 --- a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts +++ b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts @@ -210,4 +210,48 @@ describe('world_state_public_db', () => { // should read back the previously committed value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); }); + + it('can use checkpoints', async function () { + const publicStateDb = new WorldStatePublicDB(db); + const read = () => publicStateDb.storageRead(addresses[0], slots[0]); + const write = (value: Fr) => publicStateDb.storageWrite(addresses[0], slots[0], value); + + const newValue = new Fr(dbValues[0].toBigInt() + 1n); + const newValue2 = new Fr(dbValues[0].toBigInt() + 2n); + const newValue3 = new Fr(dbValues[0].toBigInt() + 3n); + const newValue4 = new Fr(dbValues[0].toBigInt() + 4n); + const newValue5 = new Fr(dbValues[0].toBigInt() + 5n); + const newValue6 = new Fr(dbValues[0].toBigInt() + 6n); + + // basic + expect(await read()).toEqual(dbValues[0]); + await write(newValue); + await publicStateDb.checkpoint(); + await write(newValue2); + await publicStateDb.rollbackToCheckpoint(); + expect(await read()).toEqual(newValue); + await publicStateDb.rollbackToCommit(); + expect(await read()).toEqual(dbValues[0]); + + // write, checkpoint, commit, rollback to checkpoint, rollback to commit + await write(newValue3); + await publicStateDb.checkpoint(); + await publicStateDb.rollbackToCheckpoint(); + expect(await read()).toEqual(newValue3); + await publicStateDb.commit(); + await publicStateDb.rollbackToCommit(); + expect(await read()).toEqual(newValue3); + + // writes after checkpoint take precedence + await write(newValue4); + await publicStateDb.checkpoint(); + await write(newValue5); + await publicStateDb.commit(); + expect(await read()).toEqual(newValue5); + + // rollback to checkpoint does not cross commit boundaries + await write(newValue6); + await publicStateDb.rollbackToCheckpoint(); + expect(await read()).toEqual(newValue5); + }); }); From 2bc833ebc6015ab97709264f84f79bab2531d168 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 11 Mar 2024 10:59:53 -0400 Subject: [PATCH 5/9] add tests to public kernels --- .../crates/public-kernel-lib/src/common.nr | 4 ++++ .../public-kernel-lib/src/public_kernel_setup.nr | 10 ++++++++++ .../public-kernel-lib/src/public_kernel_teardown.nr | 10 ++++++++++ 3 files changed, 24 insertions(+) diff --git a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr index 550649723f83..e0ad9309a82c 100644 --- a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr +++ b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr @@ -35,6 +35,10 @@ pub fn validate_inputs(public_call: PublicCallData) { assert(public_call.bytecode_hash != 0, "Bytecode hash cannot be zero"); } +pub fn validate_public_call_non_revert(public_call: PublicCallData) { + assert(public_call.call_stack_item.public_inputs.reverted == false, "Public call cannot be reverted"); +} + pub fn initialize_reverted_flag( previous_kernel: PublicKernelData, public_call: PublicCallData, diff --git a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_setup.nr b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_setup.nr index 560b2b848e66..b103e823945e 100644 --- a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_setup.nr +++ b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_setup.nr @@ -38,6 +38,8 @@ impl PublicKernelSetupCircuitPrivateInputs { fn public_kernel_setup(self) -> PublicKernelCircuitPublicInputs { // construct the circuit outputs let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed(); + // since this phase is non-revertible, we must assert the public call did not revert + common::validate_public_call_non_revert(self.public_call); common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs); // initialise the end state with our provided previous kernel state @@ -523,4 +525,12 @@ mod tests { assert_eq(request_context.counter, request_1.counter); assert_eq(request_context.contract_address, storage_contract_address); } + + #[test(should_fail_with="Public call cannot be reverted")] + fn fails_if_public_call_reverted() { + let mut builder = PublicKernelSetupCircuitPrivateInputsBuilder::new(); + builder.public_call.public_inputs.reverted = true; + + builder.failed(); + } } diff --git a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_teardown.nr b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_teardown.nr index efa4111dcdf9..ea90885b6302 100644 --- a/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_teardown.nr +++ b/noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_teardown.nr @@ -26,6 +26,8 @@ impl PublicKernelTeardownCircuitPrivateInputs { fn public_kernel_teardown(self) -> PublicKernelCircuitPublicInputs { // construct the circuit outputs let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed(); + // since this phase is non-revertible, we must assert the public call did not revert + common::validate_public_call_non_revert(self.public_call); common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs); // initialise the end state with our provided previous kernel state @@ -390,4 +392,12 @@ mod tests { let expected_unencrypted_logs_hash = compute_logs_hash(prev_unencrypted_logs_hash, unencrypted_logs_hash); assert_eq(public_inputs.end.unencrypted_logs_hash, expected_unencrypted_logs_hash); } + + #[test(should_fail_with="Public call cannot be reverted")] + fn fails_if_public_call_reverted() { + let mut builder = PublicKernelTeardownCircuitPrivateInputsBuilder::new(); + builder.public_call.public_inputs.reverted = true; + + builder.failed(); + } } From 022c8e55644963aa5fb88f97fd64cf0fd21ec6fb Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 11 Mar 2024 11:35:22 -0400 Subject: [PATCH 6/9] setup e2e case --- yarn-project/aztec.js/src/contract/sent_tx.ts | 4 +- .../src/fee/public_fee_payment_method.ts | 6 +- yarn-project/end-to-end/src/e2e_fees.test.ts | 85 +++++++++++++++++++ 3 files changed, 91 insertions(+), 4 deletions(-) diff --git a/yarn-project/aztec.js/src/contract/sent_tx.ts b/yarn-project/aztec.js/src/contract/sent_tx.ts index c0d49fa61c29..e6e8b14588bf 100644 --- a/yarn-project/aztec.js/src/contract/sent_tx.ts +++ b/yarn-project/aztec.js/src/contract/sent_tx.ts @@ -64,7 +64,9 @@ export class SentTx { } const receipt = await this.waitForReceipt(opts); if (receipt.status !== TxStatus.MINED) { - throw new Error(`Transaction ${await this.getTxHash()} was ${receipt.status}`); + throw new Error( + `Transaction ${await this.getTxHash()} was ${receipt.status}. Reason: ${receipt.error ?? 'unknown'}`, + ); } if (opts?.debug) { const txHash = await this.getTxHash(); diff --git a/yarn-project/aztec.js/src/fee/public_fee_payment_method.ts b/yarn-project/aztec.js/src/fee/public_fee_payment_method.ts index 9a367f565d44..92b1f4539fcf 100644 --- a/yarn-project/aztec.js/src/fee/public_fee_payment_method.ts +++ b/yarn-project/aztec.js/src/fee/public_fee_payment_method.ts @@ -16,16 +16,16 @@ export class PublicFeePaymentMethod implements FeePaymentMethod { /** * The asset used to pay the fee. */ - private asset: AztecAddress, + protected asset: AztecAddress, /** * Address which will hold the fee payment. */ - private paymentContract: AztecAddress, + protected paymentContract: AztecAddress, /** * An auth witness provider to authorize fee payments */ - private wallet: AccountWallet, + protected wallet: AccountWallet, ) {} /** diff --git a/yarn-project/end-to-end/src/e2e_fees.test.ts b/yarn-project/end-to-end/src/e2e_fees.test.ts index e3f666300b48..9bcddd3b961a 100644 --- a/yarn-project/end-to-end/src/e2e_fees.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees.test.ts @@ -4,14 +4,17 @@ import { DebugLogger, ExtendedNote, Fr, + FunctionCall, FunctionSelector, Note, PrivateFeePaymentMethod, PublicFeePaymentMethod, TxHash, Wallet, + computeAuthWitMessageHash, computeMessageSecretHash, } from '@aztec/aztec.js'; +import { FunctionData } from '@aztec/circuits.js'; import { ContractArtifact, decodeFunctionSignature } from '@aztec/foundation/abi'; import { TokenContract as BananaCoin, @@ -506,6 +509,56 @@ describe('e2e_fees', () => { }); }); + it('fails transaction that error in setup', async () => { + const OutrageousPublicAmountAliceDoesNotHave = 10000n; + // const PublicMintedAlicePublicBananas = 1000n; + const FeeAmount = 1n; + const RefundAmount = 2n; + const MaxFee = FeeAmount + RefundAmount; + const { wallets } = e2eContext; + + // const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await bananaPrivateBalances( + // aliceAddress, + // bananaFPC.address, + // ); + // const [initialAlicePublicBananas, initialFPCPublicBananas] = await bananaPublicBalances( + // aliceAddress, + // bananaFPC.address, + // ); + // const [initialAliceGas, initialFPCGas, initialSequencerGas] = await gasBalances( + // aliceAddress, + // bananaFPC.address, + // sequencerAddress, + // ); + + // simulation throws an error when setup fails + await expect( + bananaCoin.methods + .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) + .send({ + fee: { + maxFee: MaxFee, + paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]), + }, + }) + .wait(), + ).rejects.toThrow(/Message not authorized by account 'is_valid == true'/); + + // so does the sequencer + await expect( + bananaCoin.methods + .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) + .send({ + skipPublicSimulation: true, + fee: { + maxFee: MaxFee, + paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]), + }, + }) + .wait(), + ).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./); + }); + function logFunctionSignatures(artifact: ContractArtifact, logger: DebugLogger) { artifact.functions.forEach(fn => { const sig = decodeFunctionSignature(fn.name, fn.parameters); @@ -543,3 +596,35 @@ describe('e2e_fees', () => { await e2eContext.wallets[accountIndex].addNote(extendedNote); }; }); + +class BuggedSetupFeePaymentMethod extends PublicFeePaymentMethod { + getFunctionCalls(maxFee: Fr): Promise { + const nonce = Fr.random(); + const messageHash = computeAuthWitMessageHash(this.paymentContract, { + args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce], + functionData: new FunctionData( + FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'), + false, + false, + false, + ), + to: this.asset, + }); + + const tooMuchFee = new Fr(maxFee.toBigInt() * 2n); + + return Promise.resolve([ + this.wallet.setPublicAuth(messageHash, true).request(), + { + to: this.getPaymentContract(), + functionData: new FunctionData( + FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'), + false, + true, + false, + ), + args: [tooMuchFee, this.asset, nonce], + }, + ]); + } +} From 866e3010b41c268cc8675f5f1f2ecdf5e2c43191 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Mon, 11 Mar 2024 14:28:59 -0400 Subject: [PATCH 7/9] e2e teardown test working --- yarn-project/end-to-end/src/e2e_fees.test.ts | 132 ++++++++++++++++-- .../simulator/world_state_public_db.test.ts | 8 +- 2 files changed, 122 insertions(+), 18 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_fees.test.ts b/yarn-project/end-to-end/src/e2e_fees.test.ts index 9bcddd3b961a..fb283c8b6de7 100644 --- a/yarn-project/end-to-end/src/e2e_fees.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees.test.ts @@ -517,20 +517,6 @@ describe('e2e_fees', () => { const MaxFee = FeeAmount + RefundAmount; const { wallets } = e2eContext; - // const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await bananaPrivateBalances( - // aliceAddress, - // bananaFPC.address, - // ); - // const [initialAlicePublicBananas, initialFPCPublicBananas] = await bananaPublicBalances( - // aliceAddress, - // bananaFPC.address, - // ); - // const [initialAliceGas, initialFPCGas, initialSequencerGas] = await gasBalances( - // aliceAddress, - // bananaFPC.address, - // sequencerAddress, - // ); - // simulation throws an error when setup fails await expect( bananaCoin.methods @@ -559,6 +545,78 @@ describe('e2e_fees', () => { ).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./); }); + it('fails transaction that error in teardown', async () => { + /** + * We trigger an error in teardown by having the FPC authorize a transfer of its entire balance to Alice + * as part of app logic. This will cause the FPC to not have enough funds to pay the refund back to Alice. + */ + + const PublicMintedAlicePublicBananas = 1000n; + const FeeAmount = 1n; + const RefundAmount = 2n; + const MaxFee = FeeAmount + RefundAmount; + const { wallets } = e2eContext; + + const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await bananaPrivateBalances( + aliceAddress, + bananaFPC.address, + ); + const [initialAlicePublicBananas, initialFPCPublicBananas] = await bananaPublicBalances( + aliceAddress, + bananaFPC.address, + ); + const [initialAliceGas, initialFPCGas, initialSequencerGas] = await gasBalances( + aliceAddress, + bananaFPC.address, + sequencerAddress, + ); + + await bananaCoin.methods.mint_public(aliceAddress, PublicMintedAlicePublicBananas).send().wait(); + + await expect( + bananaCoin.methods + .mint_public(aliceAddress, 1n) // random operation + .send({ + fee: { + maxFee: MaxFee, + paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]), + }, + }) + .wait(), + ).rejects.toThrow(/invalid nonce/); + + // node also drops + await expect( + bananaCoin.methods + .mint_public(aliceAddress, 1n) // random operation + .send({ + skipPublicSimulation: true, + fee: { + maxFee: MaxFee, + paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]), + }, + }) + .wait(), + ).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./); + + // nothing happened + await expectMapping( + bananaPrivateBalances, + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n], + ); + await expectMapping( + bananaPublicBalances, + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePublicBananas + PublicMintedAlicePublicBananas, initialFPCPublicBananas, 0n], + ); + await expectMapping( + gasBalances, + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAliceGas, initialFPCGas, initialSequencerGas], + ); + }); + function logFunctionSignatures(artifact: ContractArtifact, logger: DebugLogger) { artifact.functions.forEach(fn => { const sig = decodeFunctionSignature(fn.name, fn.parameters); @@ -628,3 +686,49 @@ class BuggedSetupFeePaymentMethod extends PublicFeePaymentMethod { ]); } } + +class BuggedTeardownFeePaymentMethod extends PublicFeePaymentMethod { + async getFunctionCalls(maxFee: Fr): Promise { + // authorize the FPC to take the max fee from Alice + const nonce = Fr.random(); + const messageHash1 = computeAuthWitMessageHash(this.paymentContract, { + args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce], + functionData: new FunctionData( + FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'), + false, + false, + false, + ), + to: this.asset, + }); + + // authorize the FPC to take the maxFee + // do this first because we only get 2 feepayload calls + await this.wallet.setPublicAuth(messageHash1, true).send().wait(); + + return Promise.resolve([ + // in this, we're actually paying the fee in setup + { + to: this.getPaymentContract(), + functionData: new FunctionData( + FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'), + false, + true, + false, + ), + args: [maxFee, this.asset, nonce], + }, + // and trying to take a little extra in teardown, but specify a bad nonce + { + to: this.asset, + functionData: new FunctionData( + FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'), + false, + false, + false, + ), + args: [this.wallet.getAddress(), this.paymentContract, new Fr(1), Fr.random()], + }, + ]); + } +} diff --git a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts index b8992cdef703..d33e8a06d564 100644 --- a/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts +++ b/yarn-project/sequencer-client/src/simulator/world_state_public_db.test.ts @@ -85,7 +85,7 @@ describe('world_state_public_db', () => { // write a new value to our first value await publicStateDb.storageWrite(addresses[0], slots[0], newValue); - // should read back the uncommited value + // should read back the uncommitted value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // other slots should be unchanged @@ -132,7 +132,7 @@ describe('world_state_public_db', () => { expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); }); - it('reads original value if rolled back uncommited value', async function () { + it('reads original value if rolled back uncommitted value', async function () { const publicStateDb = new WorldStatePublicDB(db); expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(dbValues[0]); @@ -141,7 +141,7 @@ describe('world_state_public_db', () => { // write a new value to our first value await publicStateDb.storageWrite(addresses[0], slots[0], newValue); - // should read back the uncommited value + // should read back the uncommitted value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue); // now rollback @@ -174,7 +174,7 @@ describe('world_state_public_db', () => { // write a new value to our first value await publicStateDb.storageWrite(addresses[0], slots[0], newValue2); - // should read back the uncommited value + // should read back the uncommitted value expect(await publicStateDb.storageRead(addresses[0], slots[0])).toEqual(newValue2); }); From d8ada695c0296c74f3257df2a936579caacb7f87 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Tue, 12 Mar 2024 13:26:05 -0400 Subject: [PATCH 8/9] fix rebase conflicts --- .../sequencer-client/src/sequencer/public_processor.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts index f64d0585d357..73512de05cca 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts @@ -427,7 +427,6 @@ describe('public_processor', () => { TxL2Logs.empty(), // reverse because `enqueuedPublicFunctions` expects the last element to be the front of the queue callRequests.slice().reverse(), - [ExtendedContractData.random()], ); const contractSlotA = fr(0x100); @@ -532,7 +531,6 @@ describe('public_processor', () => { TxL2Logs.empty(), // reverse because `enqueuedPublicFunctions` expects the last element to be the front of the queue callRequests.slice().reverse(), - [ExtendedContractData.random()], ); const contractSlotA = fr(0x100); From 034eb77e3768fe85e91c0c14fac135fb82d2ede0 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Wed, 13 Mar 2024 16:32:31 -0400 Subject: [PATCH 9/9] fix conflicts --- .../src/sequencer/public_processor.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts index 73512de05cca..acae13c621dd 100644 --- a/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/public_processor.test.ts @@ -407,9 +407,9 @@ describe('public_processor', () => { baseContractAddressSeed, baseContractAddressSeed, ].map(makePublicCallRequest); - callRequests[0].callContext.startSideEffectCounter = 2; - callRequests[1].callContext.startSideEffectCounter = 3; - callRequests[2].callContext.startSideEffectCounter = 4; + callRequests[0].callContext.sideEffectCounter = 2; + callRequests[1].callContext.sideEffectCounter = 3; + callRequests[2].callContext.sideEffectCounter = 4; const kernelOutput = makePrivateKernelTailCircuitPublicInputs(0x10); kernelOutput.end.unencryptedLogsHash = [Fr.ZERO, Fr.ZERO]; @@ -511,9 +511,9 @@ describe('public_processor', () => { baseContractAddressSeed, baseContractAddressSeed, ].map(makePublicCallRequest); - callRequests[0].callContext.startSideEffectCounter = 2; - callRequests[1].callContext.startSideEffectCounter = 3; - callRequests[2].callContext.startSideEffectCounter = 4; + callRequests[0].callContext.sideEffectCounter = 2; + callRequests[1].callContext.sideEffectCounter = 3; + callRequests[2].callContext.sideEffectCounter = 4; const kernelOutput = makePrivateKernelTailCircuitPublicInputs(0x10); kernelOutput.end.unencryptedLogsHash = [Fr.ZERO, Fr.ZERO];