diff --git a/yarn-project/.claude/skills/unit-test-implementation/SKILL.md b/yarn-project/.claude/skills/unit-test-implementation/SKILL.md index 0bc59961fb87..da1c1eab66f7 100644 --- a/yarn-project/.claude/skills/unit-test-implementation/SKILL.md +++ b/yarn-project/.claude/skills/unit-test-implementation/SKILL.md @@ -23,6 +23,28 @@ beforeEach(() => { }); ``` +### NEVER Pass Complex Objects as mock() Props + +`jest-mock-extended`'s `mock(props)` deep-processes any objects passed as initial properties. When those objects contain class instances with internal state (like `Fr`, `EthAddress`, `AztecAddress`, `GasFees`, `Buffer`, etc.), this causes **O(2^n) exponential slowdown** across tests — each test doubles the time of the previous one. + +```typescript +// ❌ NEVER: Passing complex domain objects as mock props +// This causes exponential test slowdown (1s → 2s → 4s → 8s → ...) +const constants = { chainId: new Fr(1), coinbase: EthAddress.random(), gasFees: GasFees.empty() }; +beforeEach(() => { + builder = mock({ checkpointNumber, constants }); +}); + +// ✅ GOOD: Create mock without props, then set properties directly +beforeEach(() => { + builder = mock(); + Object.defineProperty(builder, 'checkpointNumber', { value: checkpointNumber }); + Object.defineProperty(builder, 'constants', { value: constants }); +}); +``` + +Simple primitives (strings, numbers, booleans) and arrow functions are safe to pass as props. The issue is specifically with class instances that have complex prototypes. + ### When to Use Real Instances vs Mocks **Mock external dependencies** that are: diff --git a/yarn-project/end-to-end/src/e2e_epochs/epochs_mbps.parallel.test.ts b/yarn-project/end-to-end/src/e2e_epochs/epochs_mbps.parallel.test.ts index 4b729728b706..69aeef5670e0 100644 --- a/yarn-project/end-to-end/src/e2e_epochs/epochs_mbps.parallel.test.ts +++ b/yarn-project/end-to-end/src/e2e_epochs/epochs_mbps.parallel.test.ts @@ -9,6 +9,7 @@ import { isL1ToL2MessageReady } from '@aztec/aztec.js/messaging'; import { waitForTx } from '@aztec/aztec.js/node'; import { RollupContract } from '@aztec/ethereum/contracts'; import type { Operator } from '@aztec/ethereum/deploy-aztec-l1-contracts'; +import { waitUntilL1Timestamp } from '@aztec/ethereum/l1-tx-utils'; import { asyncMap } from '@aztec/foundation/async-map'; import { CheckpointNumber, SlotNumber } from '@aztec/foundation/branded-types'; import { times, timesAsync } from '@aztec/foundation/collection'; @@ -17,6 +18,8 @@ import { retryUntil } from '@aztec/foundation/retry'; import { bufferToHex } from '@aztec/foundation/string'; import { executeTimeout } from '@aztec/foundation/timer'; import { TestContract } from '@aztec/noir-test-contracts.js/Test'; +import { getSlotAtTimestamp, getTimestampForSlot } from '@aztec/stdlib/epoch-helpers'; +import { GasFees } from '@aztec/stdlib/gas'; import { TxStatus } from '@aztec/stdlib/tx'; import { jest } from '@jest/globals'; @@ -67,6 +70,7 @@ describe('e2e_epochs/epochs_mbps', () => { maxTxsPerBlock?: number; buildCheckpointIfEmpty?: boolean; deployCrossChainContract?: boolean; + skipPushProposedBlocksToArchiver?: boolean; }) { const { syncChainTip = 'checkpointed', deployCrossChainContract = false, ...setupOpts } = opts; @@ -493,4 +497,91 @@ describe('e2e_epochs/epochs_mbps', () => { const multiBlockCheckpoint = await assertMultipleBlocksPerSlot(2, logger); await waitForProvenCheckpoint(multiBlockCheckpoint); }); + + it('deploys a contract and calls it in separate blocks within a slot', async () => { + await setupTest({ + syncChainTip: 'checkpointed', + minTxsPerBlock: 1, + maxTxsPerBlock: 1, + }); + + // Prepare deploy tx for a new TestContract. Get the instance address so we can construct the call tx. + const highPriority = new GasFees(100, 100); + const lowPriority = new GasFees(1, 1); + + const deployMethod = TestContract.deploy(wallet); + const deployInstance = await deployMethod.getInstance(); + logger.warn(`Will deploy TestContract at ${deployInstance.address}`); + + // Register the contract on the PXE so we can prove the call interaction against it. + await wallet.registerContract(deployInstance, TestContract.artifact); + const deployedContract = TestContract.at(deployInstance.address, wallet); + + // Pre-prove both txs before starting sequencers. This ensures both arrive in the pool + // at the same time, so the sequencer can sort by priority fee for correct ordering. + logger.warn(`Pre-proving deploy tx (high priority) and call tx (low priority)`); + const deployTx = await proveInteraction(wallet, deployMethod, { + from, + fee: { gasSettings: { maxPriorityFeesPerGas: highPriority } }, + }); + const callTx = await proveInteraction(wallet, deployedContract.methods.emit_nullifier_public(new Fr(42)), { + from, + fee: { gasSettings: { maxPriorityFeesPerGas: lowPriority } }, + }); + logger.warn(`Pre-proved both txs`); + + // Start the sequencers + await Promise.all(nodes.map(n => n.getSequencer()!.start())); + logger.warn(`Started all sequencers`); + + // Wait until one L1 slot before the start of the next L2 slot. + // This ensures both txs land in the pending pool right before the proposer starts building. + // REFACTOR: This should go into a shared "waitUntilNextSlotStartsBuilding" utility + const currentL1Block = await test.l1Client.getBlock({ blockTag: 'latest' }); + const currentTimestamp = currentL1Block.timestamp; + const currentSlot = getSlotAtTimestamp(currentTimestamp, test.constants); + const nextSlot = SlotNumber(currentSlot + 1); + const nextSlotTimestamp = getTimestampForSlot(nextSlot, test.constants); + const targetTimestamp = nextSlotTimestamp - BigInt(test.L1_BLOCK_TIME_IN_S); + logger.warn(`Waiting until L1 timestamp ${targetTimestamp} (one L1 slot before L2 slot ${nextSlot})`, { + currentTimestamp, + currentSlot, + nextSlot, + nextSlotTimestamp, + targetTimestamp, + }); + await waitUntilL1Timestamp(test.l1Client, targetTimestamp, undefined, test.L2_SLOT_DURATION_IN_S * 3); + + // Send both pre-proved txs simultaneously, waiting for them to be checkpointed. + const timeout = test.L2_SLOT_DURATION_IN_S * 5; + logger.warn(`Sending both txs and waiting for checkpointed receipts`); + const [deployReceipt, callReceipt] = await executeTimeout( + () => Promise.all([deployTx.send({ wait: { timeout } }), callTx.send({ wait: { timeout } })]), + timeout * 1000, + ); + logger.warn(`Both txs checkpointed`, { + deployBlock: deployReceipt.blockNumber, + callBlock: callReceipt.blockNumber, + }); + + // Both txs should succeed (send throws on revert). Deploy should be in an earlier block. + expect(deployReceipt.blockNumber).toBeLessThan(callReceipt.blockNumber!); + + // Verify both blocks belong to the same checkpoint. + const deployCheckpointedBlock = await retryUntil( + async () => (await context.aztecNode.getCheckpointedBlocks(deployReceipt.blockNumber!, 1))[0], + 'deploy checkpointed block', + timeout, + ); + const callCheckpointedBlock = await retryUntil( + async () => (await context.aztecNode.getCheckpointedBlocks(callReceipt.blockNumber!, 1))[0], + 'call checkpointed block', + timeout, + ); + expect(deployCheckpointedBlock.checkpointNumber).toBe(callCheckpointedBlock.checkpointNumber); + logger.warn(`Both blocks in checkpoint ${deployCheckpointedBlock.checkpointNumber}`); + + // Wait for the checkpoint to be proven. + await waitForProvenCheckpoint(deployCheckpointedBlock.checkpointNumber); + }); }); diff --git a/yarn-project/simulator/src/public/public_processor/public_processor.ts b/yarn-project/simulator/src/public/public_processor/public_processor.ts index 20ce6fbaa3e4..f8b708d0a568 100644 --- a/yarn-project/simulator/src/public/public_processor/public_processor.ts +++ b/yarn-project/simulator/src/public/public_processor/public_processor.ts @@ -306,6 +306,9 @@ export class PublicProcessor implements Traceable { totalBlockGas = totalBlockGas.add(processedTx.gasUsed.totalGas); totalSizeInBytes += txSize; totalBlobFields += txBlobFields; + + // Commit the tx-level contracts checkpoint on success + this.contractsDB.commitCheckpoint(); } catch (err: any) { if (err?.name === 'PublicProcessorTimeoutError') { this.log.warn(`Stopping tx processing due to timeout.`); @@ -354,7 +357,6 @@ export class PublicProcessor implements Traceable { } finally { // Base case is we always commit the checkpoint. Using the ForkCheckpoint means this has no effect if the tx was previously reverted await checkpoint.commit(); - this.contractsDB.commitCheckpointOkIfNone(); } } diff --git a/yarn-project/validator-client/src/checkpoint_builder.test.ts b/yarn-project/validator-client/src/checkpoint_builder.test.ts index 9dab4a4778c9..f8b3ce509dc3 100644 --- a/yarn-project/validator-client/src/checkpoint_builder.test.ts +++ b/yarn-project/validator-client/src/checkpoint_builder.test.ts @@ -10,7 +10,7 @@ import { Fr } from '@aztec/foundation/curves/bn254'; import { EthAddress } from '@aztec/foundation/eth-address'; import { TestDateProvider } from '@aztec/foundation/timer'; import type { LightweightCheckpointBuilder } from '@aztec/prover-client/light'; -import type { PublicProcessor } from '@aztec/simulator/server'; +import type { PublicContractsDB, PublicProcessor } from '@aztec/simulator/server'; import { AztecAddress } from '@aztec/stdlib/aztec-address'; import { L2Block } from '@aztec/stdlib/block'; import type { ContractDataSource } from '@aztec/stdlib/contract'; @@ -22,17 +22,22 @@ import { type PublicProcessorLimits, type PublicProcessorValidator, } from '@aztec/stdlib/interfaces/server'; -import { TxHash } from '@aztec/stdlib/tx'; -import type { CheckpointGlobalVariables, GlobalVariables, ProcessedTx, Tx } from '@aztec/stdlib/tx'; +import { + type CheckpointGlobalVariables, + type GlobalVariables, + type ProcessedTx, + type Tx, + TxHash, +} from '@aztec/stdlib/tx'; import type { TelemetryClient } from '@aztec/telemetry-client'; -import { describe, expect, it } from '@jest/globals'; +import { describe, expect, it, jest } from '@jest/globals'; import { type MockProxy, mock } from 'jest-mock-extended'; import { CheckpointBuilder } from './checkpoint_builder.js'; describe('CheckpointBuilder', () => { - let checkpointBuilder: CheckpointBuilder; + let checkpointBuilder: TestCheckpointBuilder; let lightweightCheckpointBuilder: MockProxy; let fork: MockProxy; let config: FullNodeBlockBuilderConfig; @@ -57,6 +62,8 @@ describe('CheckpointBuilder', () => { }; class TestCheckpointBuilder extends CheckpointBuilder { + declare public contractsDB: PublicContractsDB; + public override makeBlockBuilderDeps(_globalVariables: GlobalVariables, _fork: MerkleTreeWriteOperations) { return Promise.resolve({ processor, validator }); } @@ -101,7 +108,9 @@ describe('CheckpointBuilder', () => { } beforeEach(() => { - lightweightCheckpointBuilder = mock({ checkpointNumber, constants }); + lightweightCheckpointBuilder = mock(); + Object.defineProperty(lightweightCheckpointBuilder, 'checkpointNumber', { value: checkpointNumber }); + Object.defineProperty(lightweightCheckpointBuilder, 'constants', { value: constants }); lightweightCheckpointBuilder.getBlocks.mockReturnValue([]); fork = mock(); @@ -117,6 +126,50 @@ describe('CheckpointBuilder', () => { setupBuilder(); }); + describe('contractsDB checkpointing', () => { + let createCheckpointSpy: jest.SpiedFunction<() => void>; + let commitCheckpointSpy: jest.SpiedFunction<() => void>; + let revertCheckpointSpy: jest.SpiedFunction<() => void>; + + beforeEach(() => { + const db = checkpointBuilder.contractsDB; + createCheckpointSpy = jest.spyOn(db, 'createCheckpoint'); + commitCheckpointSpy = jest.spyOn(db, 'commitCheckpoint'); + revertCheckpointSpy = jest.spyOn(db, 'revertCheckpoint'); + + lightweightCheckpointBuilder.getBlockCount.mockReturnValue(0); + }); + + async function mockSuccessfulBlock() { + const block = await L2Block.random(blockNumber); + lightweightCheckpointBuilder.addBlock.mockResolvedValue({ block, timings: {} }); + processor.process.mockResolvedValue([[{ hash: TxHash.random() } as ProcessedTx], [], [], [], []]); + return block; + } + + it('uses the same contractsDB across multiple block builds', async () => { + await mockSuccessfulBlock(); + await checkpointBuilder.buildBlock([], blockNumber, 1000n); + + await mockSuccessfulBlock(); + await checkpointBuilder.buildBlock([], BlockNumber(blockNumber + 1), 1001n); + + expect(createCheckpointSpy).toHaveBeenCalledTimes(2); + expect(commitCheckpointSpy).toHaveBeenCalledTimes(2); + expect(revertCheckpointSpy).not.toHaveBeenCalled(); + }); + + it('calls revertCheckpoint when public processor fails', async () => { + processor.process.mockRejectedValue(new Error('processor failure')); + + await expect(checkpointBuilder.buildBlock([], blockNumber, 1000n)).rejects.toThrow('processor failure'); + + expect(createCheckpointSpy).toHaveBeenCalledTimes(1); + expect(commitCheckpointSpy).not.toHaveBeenCalled(); + expect(revertCheckpointSpy).toHaveBeenCalledTimes(1); + }); + }); + describe('buildBlock', () => { it('builds a block successfully when transactions are processed', async () => { lightweightCheckpointBuilder.getBlockCount.mockReturnValue(0); diff --git a/yarn-project/validator-client/src/checkpoint_builder.ts b/yarn-project/validator-client/src/checkpoint_builder.ts index 906bcfe7da98..20a9625c8e71 100644 --- a/yarn-project/validator-client/src/checkpoint_builder.ts +++ b/yarn-project/validator-client/src/checkpoint_builder.ts @@ -46,6 +46,9 @@ export type { BuildBlockInCheckpointResult } from '@aztec/stdlib/interfaces/serv export class CheckpointBuilder implements ICheckpointBlockBuilder { private log: Logger; + /** Persistent contracts DB shared across all blocks in this checkpoint. */ + protected contractsDB: PublicContractsDB; + constructor( private checkpointBuilder: LightweightCheckpointBuilder, private fork: MerkleTreeWriteOperations, @@ -60,6 +63,7 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { ...bindings, instanceId: `checkpoint-${checkpointBuilder.checkpointNumber}`, }); + this.contractsDB = new PublicContractsDB(this.contractDataSource, this.log.getBindings()); } getConstantData(): CheckpointGlobalVariables { @@ -104,6 +108,8 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { ...this.capLimitsByCheckpointBudgets(opts), }; + // Create a block-level checkpoint on the contracts DB so we can roll back on failure + this.contractsDB.createCheckpoint(); // We execute all merkle tree operations on a world state fork checkpoint // This enables us to discard all modifications in the event that we fail to successfully process sufficient transactions const forkCheckpoint = await ForkCheckpoint.new(this.fork); @@ -112,6 +118,7 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { const [publicProcessorDuration, [processedTxs, failedTxs, usedTxs]] = await elapsed(() => processor.process(pendingTxs, cappedOpts, validator), ); + // Throw before updating state if we don't have enough valid txs const minValidTxs = opts.minValidTxs ?? 0; if (processedTxs.length < minValidTxs) { @@ -126,6 +133,8 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { expectedEndState: opts.expectedEndState, }); + this.contractsDB.commitCheckpoint(); + this.log.debug('Built block within checkpoint', { header: block.header.toInspect(), processedTxs: processedTxs.map(tx => tx.hash.toString()), @@ -140,6 +149,8 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { usedTxs, }; } catch (err) { + // Revert all changes to contracts db + this.contractsDB.revertCheckpoint(); // If we reached the point of committing the checkpoint, this does nothing // Otherwise it reverts any changes made to the fork for this failed block await forkCheckpoint.revert(); @@ -233,7 +244,7 @@ export class CheckpointBuilder implements ICheckpointBlockBuilder { ...(await getDefaultAllowedSetupFunctions()), ...(this.config.txPublicSetupAllowListExtend ?? []), ]; - const contractsDB = new PublicContractsDB(this.contractDataSource, this.log.getBindings()); + const contractsDB = this.contractsDB; const guardedFork = new GuardedMerkleTreeOperations(fork); const collectDebugLogs = this.debugLogStore.isEnabled;