fix: validate private double spends in txs with public funcs

yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.test.ts

@@ -43,15 +43,8 @@ describe('DoubleSpendTxValidator', () => {
     await expectInvalid(badTx, 'Existing nullifier');
   });
 
-  // If the tx has public calls, all merkle insertions will be performed by the AVM,
-  // and the AVM will catch any duplicates. So we don't need to check during tx validation.
-  it('accepts duplicates if the tx has public calls', async () => {
-    const badTx = await mockTx(1, {
-      numberOfNonRevertiblePublicCallRequests: 1,
-      numberOfRevertiblePublicCallRequests: 1,
-    });
-    badTx.data.forPublic!.revertibleAccumulatedData.nullifiers[0] =
-      badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers[0];
-    await expectValid(badTx);
+  it('accepts txs with no duplicates', async () => {
+    const tx = await mockTxForRollup();
+    await expectValid(tx);
   });
 });

yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.ts

@@ -1,5 +1,5 @@
 import { createLogger } from '@aztec/foundation/log';
-import { type AnyTx, Tx, type TxValidationResult, type TxValidator, hasPublicCalls } from '@aztec/stdlib/tx';
+import { type AnyTx, Tx, type TxValidationResult, type TxValidator } from '@aztec/stdlib/tx';
 
 export interface NullifierSource {
   nullifiersExist: (nullifiers: Buffer[]) => Promise<boolean[]>;
@@ -14,25 +14,20 @@ export class DoubleSpendTxValidator<T extends AnyTx> implements TxValidator<T> {
   }
 
   async validateTx(tx: T): Promise<TxValidationResult> {
-    // Don't need to check for duplicate nullifiers if the tx has public calls
-    // because the AVM will perform merkle insertions as it goes and will fail on
-    // duplicate nullifier. In fact we CANNOT check here because the nullifiers
-    // have already been inserted, and so they will exist in nullifierSource.
-    if (!hasPublicCalls(tx)) {
-      const nullifiers = tx instanceof Tx ? tx.data.getNonEmptyNullifiers() : tx.txEffect.nullifiers;
+    const nullifiers = tx instanceof Tx ? tx.data.getNonEmptyNullifiers() : tx.txEffect.nullifiers;
 
-      // Ditch this tx if it has repeated nullifiers
-      const uniqueNullifiers = new Set(nullifiers);
-      if (uniqueNullifiers.size !== nullifiers.length) {
-        this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for emitting duplicate nullifiers`);
-        return { result: 'invalid', reason: ['Duplicate nullifier in tx'] };
-      }
+    // Ditch this tx if it has repeated nullifiers
+    const uniqueNullifiers = new Set(nullifiers);
+    if (uniqueNullifiers.size !== nullifiers.length) {
+      this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for emitting duplicate nullifiers`);
+      return { result: 'invalid', reason: ['Duplicate nullifier in tx'] };
+    }
 
-      if ((await this.#nullifierSource.nullifiersExist(nullifiers.map(n => n.toBuffer()))).some(Boolean)) {
-        this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for repeating a nullifier`);
-        return { result: 'invalid', reason: ['Existing nullifier'] };
-      }
+    if ((await this.#nullifierSource.nullifiersExist(nullifiers.map(n => n.toBuffer()))).some(Boolean)) {
+      this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for repeating a nullifier`);
+      return { result: 'invalid', reason: ['Existing nullifier'] };
     }
+
     return { result: 'valid' };
   }
 }

yarn-project/sequencer-client/src/sequencer/sequencer.ts

@@ -46,7 +46,7 @@ import type { ValidatorClient } from '@aztec/validator-client';
 import type { GlobalVariableBuilder } from '../global_variable_builder/global_builder.js';
 import { type SequencerPublisher, VoteType } from '../publisher/sequencer-publisher.js';
 import type { SlasherClient } from '../slasher/slasher_client.js';
-import { createValidatorsForBlockBuilding } from '../tx_validator/tx_validator_factory.js';
+import { createValidatorForBlockBuilding } from '../tx_validator/tx_validator_factory.js';
 import { getDefaultAllowedSetupFunctions } from './allowed.js';
 import type { SequencerConfig } from './config.js';
 import { SequencerMetrics } from './metrics.js';
@@ -465,7 +465,7 @@ export class Sequencer {
         deadline,
       });
 
-      const validators = createValidatorsForBlockBuilding(
+      const validator = createValidatorForBlockBuilding(
         publicProcessorDBFork,
         this.contractDataSource,
         newGlobalVariables,
@@ -482,7 +482,7 @@ export class Sequencer {
       };
       const limits = opts.validateOnly ? { deadline } : { deadline, ...proposerLimits };
       const [publicProcessorDuration, [processedTxs, failedTxs]] = await elapsed(() =>
-        processor.process(pendingTxs, limits, validators),
+        processor.process(pendingTxs, limits, validator),
       );
 
       if (!opts.validateOnly && failedTxs.length > 0) {

yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts

@@ -19,7 +19,7 @@ import type {
   MerkleTreeReadOperations,
 } from '@aztec/stdlib/interfaces/server';
 import type { PublicDataTreeLeafPreimage } from '@aztec/stdlib/trees';
-import { GlobalVariables, type ProcessedTx, type Tx, type TxValidator } from '@aztec/stdlib/tx';
+import { GlobalVariables, type Tx, type TxValidator } from '@aztec/stdlib/tx';
 
 import { ArchiveCache } from './archive_cache.js';
 import { GasTxValidator, type PublicStateSource } from './gas_validator.js';
@@ -63,14 +63,13 @@ export function createValidatorForAcceptingTxs(
   return new AggregateTxValidator(...validators);
 }
 
-export function createValidatorsForBlockBuilding(
+export function createValidatorForBlockBuilding(
   db: MerkleTreeReadOperations,
   contractDataSource: ContractDataSource,
   globalVariables: GlobalVariables,
   setupAllowList: AllowedElement[],
 ): {
   preprocessValidator: TxValidator<Tx>;
-  postprocessValidator: TxValidator<ProcessedTx>;
   nullifierCache: NullifierCache;
 } {
   const nullifierCache = new NullifierCache(db);
@@ -86,7 +85,6 @@ export function createValidatorsForBlockBuilding(
       globalVariables,
       setupAllowList,
     ),
-    postprocessValidator: postprocessValidator(nullifierCache),
     nullifierCache,
   };
 }
@@ -128,7 +126,3 @@ function preprocessValidator(
     new BlockHeaderTxValidator(archiveCache),
   );
 }
-
-function postprocessValidator(nullifierCache: NullifierCache): TxValidator<ProcessedTx> {
-  return new DoubleSpendTxValidator(nullifierCache);
-}

yarn-project/simulator/src/public/public_processor/public_processor.test.ts

@@ -10,7 +10,7 @@ import { Gas, GasFees } from '@aztec/stdlib/gas';
 import type { TreeInfo } from '@aztec/stdlib/interfaces/server';
 import { ProvingRequestType } from '@aztec/stdlib/proofs';
 import { mockTx } from '@aztec/stdlib/testing';
-import { GlobalVariables, type ProcessedTx, Tx, type TxValidator } from '@aztec/stdlib/tx';
+import { GlobalVariables, Tx, type TxValidator } from '@aztec/stdlib/tx';
 import { getTelemetryClient } from '@aztec/telemetry-client';
 
 import { type MockProxy, mock } from 'jest-mock-extended';
@@ -157,19 +157,6 @@ describe('public_processor', () => {
       expect(failed.length).toBe(1);
     });
 
-    it('does not send a transaction to the prover if post validation fails', async function () {
-      const tx = await mockPrivateOnlyTx();
-
-      const txValidator: MockProxy<TxValidator<ProcessedTx>> = mock();
-      txValidator.validateTx.mockResolvedValue({ result: 'invalid', reason: ['Invalid'] });
-
-      const [processed, failed] = await processor.process([tx], {}, { postprocessValidator: txValidator });
-
-      expect(processed).toEqual([]);
-      expect(failed.length).toBe(1);
-      expect(failed[0].tx).toEqual(tx);
-    });
-
     // Flakey timing test that's totally dependent on system load/architecture etc.
     it.skip('does not go past the deadline', async function () {
       const txs = await timesParallel(3, seed => mockTxWithPublicCalls({ seed }));

yarn-project/simulator/src/public/public_processor/public_processor.ts

@@ -131,7 +131,8 @@ export class PublicProcessor implements Traceable {
   /**
    * Run each tx through the public circuit and the public kernel circuit if needed.
    * @param txs - Txs to process.
-   * @param processedTxHandler - Handler for processed txs in the context of block building or proving.
+   * @param limits - Limits for processing the txs.
+   * @param validator - Pre-process validator and nullifier cache to use for processing the txs.
    * @returns The list of processed txs with their circuit simulation outputs.
    */
   public async process(
@@ -142,14 +143,13 @@ export class PublicProcessor implements Traceable {
       maxBlockGas?: Gas;
       deadline?: Date;
     } = {},
-    validators: {
+    validator: {
       preprocessValidator?: TxValidator<Tx>;
-      postprocessValidator?: TxValidator<ProcessedTx>;
       nullifierCache?: { addNullifiers: (nullifiers: Buffer[]) => void };
     } = {},
   ): Promise<[ProcessedTx[], FailedTx[], NestedProcessReturnValues[]]> {
     const { maxTransactions, maxBlockSize, deadline, maxBlockGas } = limits;
-    const { preprocessValidator, postprocessValidator, nullifierCache } = validators;
+    const { preprocessValidator, nullifierCache } = validator;
     const result: ProcessedTx[] = [];
     const failed: FailedTx[] = [];
     const timer = new Timer();
@@ -242,26 +242,6 @@ export class PublicProcessor implements Traceable {
           continue;
         }
 
-        // Re-validate the transaction
-        if (postprocessValidator) {
-          // Only accept processed transactions that are not double-spends,
-          // public functions emitting nullifiers would pass earlier check but fail here.
-          // Note that we're checking all nullifiers generated in the private execution twice,
-          // we could store the ones already checked and skip them here as an optimization.
-          // TODO(palla/txs): Can we get into this case? AVM validates this. We should be able to remove it.
-          const result = await postprocessValidator.validateTx(processedTx);
-          if (result.result !== 'valid') {
-            const reason = result.reason.join(', ');
-            this.log.error(`Rejecting tx ${processedTx.hash} after processing: ${reason}.`);
-            failed.push({ tx, error: new Error(`Tx failed post-process validation: ${reason}`) });
-            // Need to revert the checkpoint here and don't go any further
-            await checkpoint.revert();
-            continue;
-          } else {
-            this.log.trace(`Tx ${txHash.toString()} is valid post processing.`);
-          }
-        }
-
         if (!tx.hasPublicCalls()) {
           // If there are no public calls, perform all tree insertions for side effects from private
           // When there are public calls, the PublicTxSimulator & AVM handle tree insertions.