From c7eaf925c26ae9199faaf21ed1b1a220db26cfc7 Mon Sep 17 00:00:00 2001 From: esau <152162806+sklppy88@users.noreply.github.com> Date: Fri, 30 Aug 2024 13:09:49 +0200 Subject: [PATCH] fix: enforce parity of sequencer tx validation and node tx validation (#7951) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of #4781 by having parity between sequencer tx validation and node tx validation. Note that we are using the validators from the sequencer, and they should match. We are omitting `phases` and `gas` tx validator which is in the sequencer and not here is because those tx validators are customizable by the sequencer and not uniform between all sequencers. --------- Co-authored-by: Nicolás Venturo --- yarn-project/aztec-node/package.json | 17 +- .../aztec-node/src/aztec-node/config.ts | 2 +- .../aztec-node/src/aztec-node/server.test.ts | 216 ++++++++++++++++++ .../aztec-node/src/aztec-node/server.ts | 51 +++-- .../tx_metadata_validator.test.ts | 29 --- .../tx_validator/tx_metadata_validator.ts | 40 ---- yarn-project/sequencer-client/src/index.ts | 2 + yarn-project/yarn.lock | 1 + 8 files changed, 262 insertions(+), 96 deletions(-) create mode 100644 yarn-project/aztec-node/src/aztec-node/server.test.ts delete mode 100644 yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.test.ts delete mode 100644 yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.ts diff --git a/yarn-project/aztec-node/package.json b/yarn-project/aztec-node/package.json index e6227b6f048..9e9d0c00048 100644 --- a/yarn-project/aztec-node/package.json +++ b/yarn-project/aztec-node/package.json @@ -25,11 +25,9 @@ "../package.common.json" ], "jest": { - "moduleNameMapper": { - "^(\\.{1,2}/.*)\\.[cm]?js$": "$1" - }, - "testRegex": "./src/.*\\.test\\.(js|mjs|ts)$", - "rootDir": "./src", + "extensionsToTreatAsEsm": [ + ".ts" + ], "transform": { "^.+\\.tsx?$": [ "@swc/jest", @@ -43,9 +41,11 @@ } ] }, - "extensionsToTreatAsEsm": [ - ".ts" - ], + "moduleNameMapper": { + "^(\\.{1,2}/.*)\\.[cm]?js$": "$1" + }, + "testRegex": "./src/.*\\.test\\.(js|mjs|ts)$", + "rootDir": "./src", "reporters": [ [ "default", @@ -83,6 +83,7 @@ "@types/jest": "^29.5.0", "@types/node": "^18.7.23", "jest": "^29.5.0", + "jest-mock-extended": "^3.0.3", "ts-node": "^10.9.1", "typescript": "^5.0.4" }, diff --git a/yarn-project/aztec-node/src/aztec-node/config.ts b/yarn-project/aztec-node/src/aztec-node/config.ts index df9e0bd7309..1280f96c0d7 100644 --- a/yarn-project/aztec-node/src/aztec-node/config.ts +++ b/yarn-project/aztec-node/src/aztec-node/config.ts @@ -10,7 +10,7 @@ import { readFileSync } from 'fs'; import { dirname, resolve } from 'path'; import { fileURLToPath } from 'url'; -export { sequencerClientConfigMappings, SequencerClientConfig } from '@aztec/sequencer-client'; +export { sequencerClientConfigMappings, SequencerClientConfig }; /** * The configuration the aztec node. diff --git a/yarn-project/aztec-node/src/aztec-node/server.test.ts b/yarn-project/aztec-node/src/aztec-node/server.test.ts new file mode 100644 index 00000000000..4a62d116f51 --- /dev/null +++ b/yarn-project/aztec-node/src/aztec-node/server.test.ts @@ -0,0 +1,216 @@ +import { TestCircuitVerifier } from '@aztec/bb-prover'; +import { + type AztecNode, + type L1ToL2MessageSource, + type L2BlockSource, + type L2LogsSource, + MerkleTreeId, + type MerkleTreeOperations, + mockTxForRollup, +} from '@aztec/circuit-types'; +import { AztecAddress, EthAddress, Fr, GasFees, GlobalVariables, MaxBlockNumber } from '@aztec/circuits.js'; +import { type AztecLmdbStore } from '@aztec/kv-store/lmdb'; +import { type P2P } from '@aztec/p2p'; +import { type GlobalVariableBuilder } from '@aztec/sequencer-client'; +import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; +import { type ContractDataSource } from '@aztec/types/contracts'; +import { type WorldStateSynchronizer } from '@aztec/world-state'; + +import { type MockProxy, mock, mockFn } from 'jest-mock-extended'; + +import { type AztecNodeConfig, getConfigEnvVars } from './config.js'; +import { AztecNodeService } from './server.js'; + +describe('aztec node', () => { + let p2p: MockProxy; + let globalVariablesBuilder: MockProxy; + let merkleTreeOps: MockProxy; + + let lastBlockNumber: number; + + let node: AztecNode; + + const chainId = new Fr(12345); + const version = Fr.ZERO; + const coinbase = EthAddress.random(); + const feeRecipient = AztecAddress.random(); + const gasFees = GasFees.empty(); + + beforeEach(() => { + lastBlockNumber = 0; + + p2p = mock(); + + globalVariablesBuilder = mock(); + merkleTreeOps = mock(); + + const worldState = mock({ + getLatest: () => merkleTreeOps, + }); + + const l2BlockSource = mock({ + getBlockNumber: mockFn().mockResolvedValue(lastBlockNumber), + }); + + const l2LogsSource = mock(); + + const l1ToL2MessageSource = mock(); + + // all txs use the same allowed FPC class + const contractSource = mock(); + + const store = mock(); + + const aztecNodeConfig: AztecNodeConfig = getConfigEnvVars(); + + node = new AztecNodeService( + { + ...aztecNodeConfig, + l1Contracts: { + ...aztecNodeConfig.l1Contracts, + rollupAddress: EthAddress.ZERO, + registryAddress: EthAddress.ZERO, + inboxAddress: EthAddress.ZERO, + outboxAddress: EthAddress.ZERO, + availabilityOracleAddress: EthAddress.ZERO, + }, + }, + p2p, + l2BlockSource, + l2LogsSource, + l2LogsSource, + contractSource, + l1ToL2MessageSource, + worldState, + undefined, + 31337, + 1, + globalVariablesBuilder, + store, + new TestCircuitVerifier(), + new NoopTelemetryClient(), + ); + }); + + describe('tx validation', () => { + it('tests that the node correctly validates double spends', async () => { + const txs = [mockTxForRollup(0x10000), mockTxForRollup(0x20000)]; + txs.forEach(tx => { + tx.data.constants.txContext.chainId = chainId; + }); + const doubleSpendTx = txs[0]; + const doubleSpendWithExistingTx = txs[1]; + + const mockedGlobalVariables = new GlobalVariables( + chainId, + version, + new Fr(lastBlockNumber + 1), + new Fr(1), + Fr.ZERO, + coinbase, + feeRecipient, + gasFees, + ); + + globalVariablesBuilder.buildGlobalVariables + .mockResolvedValueOnce(mockedGlobalVariables) + .mockResolvedValueOnce(mockedGlobalVariables); + + expect(await node.isValidTx(doubleSpendTx)).toBe(true); + + // We push a duplicate nullifier that was created in the same transaction + doubleSpendTx.data.forRollup!.end.nullifiers.push(doubleSpendTx.data.forRollup!.end.nullifiers[0]); + + expect(await node.isValidTx(doubleSpendTx)).toBe(false); + + globalVariablesBuilder.buildGlobalVariables + .mockResolvedValueOnce(mockedGlobalVariables) + .mockResolvedValueOnce(mockedGlobalVariables); + + expect(await node.isValidTx(doubleSpendWithExistingTx)).toBe(true); + + // We make a nullifier from `doubleSpendWithExistingTx` a part of the nullifier tree, so it gets rejected as double spend + const doubleSpendNullifier = doubleSpendWithExistingTx.data.forRollup!.end.nullifiers[0].toBuffer(); + merkleTreeOps.findLeafIndex.mockImplementation((treeId: MerkleTreeId, value: any) => { + return Promise.resolve( + treeId === MerkleTreeId.NULLIFIER_TREE && value.equals(doubleSpendNullifier) ? 1n : undefined, + ); + }); + + expect(await node.isValidTx(doubleSpendWithExistingTx)).toBe(false); + }); + + it('tests that the node correctly validates chain id', async () => { + const tx = mockTxForRollup(0x10000); + tx.data.constants.txContext.chainId = chainId; + + const mockedGlobalVariables = new GlobalVariables( + chainId, + version, + new Fr(lastBlockNumber + 1), + new Fr(1), + Fr.ZERO, + coinbase, + feeRecipient, + gasFees, + ); + + globalVariablesBuilder.buildGlobalVariables + .mockResolvedValueOnce(mockedGlobalVariables) + .mockResolvedValueOnce(mockedGlobalVariables); + + expect(await node.isValidTx(tx)).toBe(true); + + // We make the chain id on the tx not equal to the configured chain id + tx.data.constants.txContext.chainId = new Fr(1n + chainId.value); + + expect(await node.isValidTx(tx)).toBe(false); + }); + + it('tests that the node correctly validates max block numbers', async () => { + const txs = [mockTxForRollup(0x10000), mockTxForRollup(0x20000), mockTxForRollup(0x30000)]; + txs.forEach(tx => { + tx.data.constants.txContext.chainId = chainId; + }); + + const noMaxBlockNumberMetadata = txs[0]; + const invalidMaxBlockNumberMetadata = txs[1]; + const validMaxBlockNumberMetadata = txs[2]; + + invalidMaxBlockNumberMetadata.data.forRollup!.rollupValidationRequests = { + maxBlockNumber: new MaxBlockNumber(true, new Fr(1)), + getSize: () => 1, + toBuffer: () => Fr.ZERO.toBuffer(), + }; + + validMaxBlockNumberMetadata.data.forRollup!.rollupValidationRequests = { + maxBlockNumber: new MaxBlockNumber(true, new Fr(5)), + getSize: () => 1, + toBuffer: () => Fr.ZERO.toBuffer(), + }; + + const mockedGlobalVariables = new GlobalVariables( + chainId, + version, + new Fr(lastBlockNumber + 5), + new Fr(1), + Fr.ZERO, + coinbase, + feeRecipient, + gasFees, + ); + + globalVariablesBuilder.buildGlobalVariables + .mockResolvedValueOnce(mockedGlobalVariables) + .mockResolvedValueOnce(mockedGlobalVariables) + .mockResolvedValueOnce(mockedGlobalVariables); + + // Default tx with no max block number should be valid + expect(await node.isValidTx(noMaxBlockNumberMetadata)).toBe(true); + // Tx with max block number < current block number should be invalid + expect(await node.isValidTx(invalidMaxBlockNumberMetadata)).toBe(false); + // Tx with max block number >= current block number should be valid + expect(await node.isValidTx(validMaxBlockNumberMetadata)).toBe(true); + }); + }); +}); diff --git a/yarn-project/aztec-node/src/aztec-node/server.ts b/yarn-project/aztec-node/src/aztec-node/server.ts index fd53a10894e..27d0cbc032f 100644 --- a/yarn-project/aztec-node/src/aztec-node/server.ts +++ b/yarn-project/aztec-node/src/aztec-node/server.ts @@ -2,6 +2,7 @@ import { createArchiver } from '@aztec/archiver'; import { BBCircuitVerifier, TestCircuitVerifier } from '@aztec/bb-prover'; import { type AztecNode, + type ClientProtocolCircuitVerifier, type FromLogType, type GetUnencryptedLogsResponse, type L1ToL2MessageSource, @@ -24,7 +25,6 @@ import { type TxHash, TxReceipt, TxStatus, - type TxValidator, partitionReverts, } from '@aztec/circuit-types'; import { @@ -57,8 +57,15 @@ import { getCanonicalFeeJuice } from '@aztec/protocol-contracts/fee-juice'; import { getCanonicalInstanceDeployer } from '@aztec/protocol-contracts/instance-deployer'; import { getCanonicalKeyRegistryAddress } from '@aztec/protocol-contracts/key-registry'; import { getCanonicalMultiCallEntrypointAddress } from '@aztec/protocol-contracts/multi-call-entrypoint'; -import { AggregateTxValidator, DataTxValidator, GlobalVariableBuilder, SequencerClient } from '@aztec/sequencer-client'; -import { PublicProcessorFactory, WASMSimulator, createSimulationProvider } from '@aztec/simulator'; +import { + AggregateTxValidator, + DataTxValidator, + DoubleSpendTxValidator, + GlobalVariableBuilder, + MetadataTxValidator, + SequencerClient, +} from '@aztec/sequencer-client'; +import { PublicProcessorFactory, WASMSimulator, WorldStateDB, createSimulationProvider } from '@aztec/simulator'; import { type TelemetryClient } from '@aztec/telemetry-client'; import { NoopTelemetryClient } from '@aztec/telemetry-client/noop'; import { @@ -72,7 +79,6 @@ import { MerkleTrees, type WorldStateSynchronizer, createWorldStateSynchronizer import { type AztecNodeConfig, getPackageInfo } from './config.js'; import { NodeMetrics } from './node_metrics.js'; -import { MetadataTxValidator } from './tx_validator/tx_metadata_validator.js'; import { TxProofValidator } from './tx_validator/tx_proof_validator.js'; /** @@ -97,7 +103,7 @@ export class AztecNodeService implements AztecNode { protected readonly version: number, protected readonly globalVariableBuilder: GlobalVariableBuilder, protected readonly merkleTreesDb: AztecKVStore, - private txValidator: TxValidator, + private proofVerifier: ClientProtocolCircuitVerifier, private telemetry: TelemetryClient, private log = createDebugLogger('aztec:node'), ) { @@ -158,11 +164,6 @@ export class AztecNodeService implements AztecNode { await Promise.all([p2pClient.start(), worldStateSynchronizer.start()]); const proofVerifier = config.realProofs ? await BBCircuitVerifier.new(config) : new TestCircuitVerifier(); - const txValidator = new AggregateTxValidator( - new DataTxValidator(), - new MetadataTxValidator(config.l1ChainId), - new TxProofValidator(proofVerifier), - ); const simulationProvider = await createSimulationProvider(config, log); @@ -197,7 +198,7 @@ export class AztecNodeService implements AztecNode { config.version, new GlobalVariableBuilder(config), store, - txValidator, + proofVerifier, telemetry, log, ); @@ -762,7 +763,26 @@ export class AztecNodeService implements AztecNode { } public async isValidTx(tx: Tx): Promise { - const [_, invalidTxs] = await this.txValidator.validateTxs([tx]); + const blockNumber = (await this.blockSource.getBlockNumber()) + 1; + + const newGlobalVariables = await this.globalVariableBuilder.buildGlobalVariables( + new Fr(blockNumber), + // We only need chainId and block number, thus coinbase and fee recipient can be set to 0. + EthAddress.ZERO, + AztecAddress.ZERO, + ); + + // These validators are taken from the sequencer, and should match. + // The reason why `phases` and `gas` tx validator is in the sequencer and not here is because + // those tx validators are customizable by the sequencer. + const txValidator = new AggregateTxValidator( + new DataTxValidator(), + new MetadataTxValidator(newGlobalVariables), + new DoubleSpendTxValidator(new WorldStateDB(this.worldStateSynchronizer.getLatest())), + new TxProofValidator(this.proofVerifier), + ); + + const [_, invalidTxs] = await txValidator.validateTxs([tx]); if (invalidTxs.length > 0) { this.log.warn(`Rejecting tx ${tx.getTxHash()} because of validation errors`); @@ -777,12 +797,7 @@ export class AztecNodeService implements AztecNode { this.sequencer?.updateSequencerConfig(config); if (newConfig.realProofs !== this.config.realProofs) { - const proofVerifier = config.realProofs ? await BBCircuitVerifier.new(newConfig) : new TestCircuitVerifier(); - - this.txValidator = new AggregateTxValidator( - new MetadataTxValidator(this.l1ChainId), - new TxProofValidator(proofVerifier), - ); + this.proofVerifier = config.realProofs ? await BBCircuitVerifier.new(newConfig) : new TestCircuitVerifier(); } this.config = newConfig; diff --git a/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.test.ts b/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.test.ts deleted file mode 100644 index 58f40185eff..00000000000 --- a/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.test.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { mockTx, mockTxForRollup } from '@aztec/circuit-types'; -import { Fr } from '@aztec/circuits.js'; - -import { MetadataTxValidator } from './tx_metadata_validator.js'; - -describe('MetadataTxValidator', () => { - let l1ChainId: Fr; - let validator: MetadataTxValidator; - - beforeEach(() => { - l1ChainId = new Fr(123); - validator = new MetadataTxValidator(l1ChainId); - }); - - it('allows only transactions for the right chain', async () => { - const goodTxs = [mockTx(1), mockTxForRollup(2)]; - const badTxs = [mockTx(3), mockTxForRollup(4)]; - - goodTxs.forEach(tx => { - tx.data.constants.txContext.chainId = l1ChainId; - }); - - badTxs.forEach(tx => { - tx.data.constants.txContext.chainId = l1ChainId.add(new Fr(1)); - }); - - await expect(validator.validateTxs([...goodTxs, ...badTxs])).resolves.toEqual([goodTxs, badTxs]); - }); -}); diff --git a/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.ts b/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.ts deleted file mode 100644 index d6b5795722e..00000000000 --- a/yarn-project/aztec-node/src/aztec-node/tx_validator/tx_metadata_validator.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { Tx, type TxValidator } from '@aztec/circuit-types'; -import { Fr } from '@aztec/circuits.js'; -import { createDebugLogger } from '@aztec/foundation/log'; - -export class MetadataTxValidator implements TxValidator { - #log = createDebugLogger('aztec:sequencer:tx_validator:tx_metadata'); - #l1ChainId: Fr; - - constructor(l1ChainId: number | Fr) { - this.#l1ChainId = new Fr(l1ChainId); - } - - validateTxs(txs: Tx[]): Promise<[validTxs: Tx[], invalidTxs: Tx[]]> { - const validTxs: Tx[] = []; - const invalidTxs: Tx[] = []; - for (const tx of txs) { - if (!this.#hasCorrectChainId(tx)) { - invalidTxs.push(tx); - continue; - } - - validTxs.push(tx); - } - - return Promise.resolve([validTxs, invalidTxs]); - } - - #hasCorrectChainId(tx: Tx): boolean { - if (!tx.data.constants.txContext.chainId.equals(this.#l1ChainId)) { - this.#log.warn( - `Rejecting tx ${Tx.getHash( - tx, - )} because of incorrect chain ${tx.data.constants.txContext.chainId.toNumber()} != ${this.#l1ChainId.toNumber()}`, - ); - return false; - } else { - return true; - } - } -} diff --git a/yarn-project/sequencer-client/src/index.ts b/yarn-project/sequencer-client/src/index.ts index 8dd0cf23889..155263344d6 100644 --- a/yarn-project/sequencer-client/src/index.ts +++ b/yarn-project/sequencer-client/src/index.ts @@ -4,6 +4,8 @@ export * from './publisher/index.js'; export * from './sequencer/index.js'; export * from './tx_validator/aggregate_tx_validator.js'; export * from './tx_validator/data_validator.js'; +export * from './tx_validator/double_spend_validator.js'; +export * from './tx_validator/metadata_validator.js'; // Used by the node to simulate public parts of transactions. Should these be moved to a shared library? export * from './global_variable_builder/index.js'; diff --git a/yarn-project/yarn.lock b/yarn-project/yarn.lock index b27a3416c1e..56cea10eb58 100644 --- a/yarn-project/yarn.lock +++ b/yarn-project/yarn.lock @@ -167,6 +167,7 @@ __metadata: "@types/jest": ^29.5.0 "@types/node": ^18.7.23 jest: ^29.5.0 + jest-mock-extended: ^3.0.3 koa: ^2.14.2 koa-router: ^12.0.0 ts-node: ^10.9.1