fix: enforce parity of sequencer tx validation and node tx validation

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"
   },

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.

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<P2P>;
+  let globalVariablesBuilder: MockProxy<GlobalVariableBuilder>;
+  let merkleTreeOps: MockProxy<MerkleTreeOperations>;
+
+  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<P2P>();
+
+    globalVariablesBuilder = mock<GlobalVariableBuilder>();
+    merkleTreeOps = mock<MerkleTreeOperations>();
+
+    const worldState = mock<WorldStateSynchronizer>({
+      getLatest: () => merkleTreeOps,
+    });
+
+    const l2BlockSource = mock<L2BlockSource>({
+      getBlockNumber: mockFn().mockResolvedValue(lastBlockNumber),
+    });
+
+    const l2LogsSource = mock<L2LogsSource>();
+
+    const l1ToL2MessageSource = mock<L1ToL2MessageSource>();
+
+    // all txs use the same allowed FPC class
+    const contractSource = mock<ContractDataSource>();
+
+    const store = mock<AztecLmdbStore>();
+
+    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);
+    });
+  });
+});

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<boolean> {
-    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;