fix: validate private double spends in txs with public funcs

yarn-project/end-to-end/src/e2e_double_spend.test.ts

@@ -0,0 +1,42 @@
+import { type AccountWallet, Fr, type Logger, TxStatus } from '@aztec/aztec.js';
+import { TestContract } from '@aztec/noir-contracts.js/Test';
+
+import { setup } from './fixtures/utils.js';
+
+describe('e2e_double_spend', () => {
+  let wallet: AccountWallet;
+
+  let logger: Logger;
+  let teardown: () => Promise<void>;
+
+  let contract: TestContract;
+
+  beforeAll(async () => {
+    // Setup environment
+    ({ teardown, wallet, logger } = await setup(1));
+
+    contract = await TestContract.deploy(wallet).send().deployed();
+
+    logger.info(`Test contract deployed at ${contract.address}`);
+  });
+
+  afterAll(() => teardown());
+
+  describe('double spends', () => {
+    it('emits a public nullifier and then tries to emit the same nullifier', async () => {
+      const nullifier = new Fr(1);
+      await contract.methods.emit_nullifier_public(nullifier).send().wait();
+
+      // We try emitting again, but our TX is dropped due to trying to emit a duplicate nullifier
+      // first confirm that it fails simulation
+      await expect(contract.methods.emit_nullifier_public(nullifier).send().wait()).rejects.toThrow(
+        /Attempted to emit duplicate nullifier/,
+      );
+      // if we skip simulation before submitting the tx,
+      // tx will be included in a block but with app logic reverted
+      await expect(
+        contract.methods.emit_nullifier_public(nullifier).send({ skipPublicSimulation: true }).wait(),
+      ).rejects.toThrow(TxStatus.APP_LOGIC_REVERTED);
+    });
+  });
+});

yarn-project/end-to-end/src/e2e_private_voting_contract.test.ts

@@ -1,4 +1,4 @@
-import { type AccountWallet, type AztecAddress, Fr, type Logger, TxStatus } from '@aztec/aztec.js';
+import { type AccountWallet, type AztecAddress, Fr, type Logger } from '@aztec/aztec.js';
 import { EasyPrivateVotingContract } from '@aztec/noir-contracts.js/EasyPrivateVoting';
 
 import { setup } from './fixtures/utils.js';
@@ -33,11 +33,10 @@ describe('e2e_voting_contract', () => {
       // We try voting again, but our TX is dropped due to trying to emit duplicate nullifiers
       // first confirm that it fails simulation
       await expect(votingContract.methods.cast_vote(candidate).send().wait()).rejects.toThrow(/Nullifier collision/);
-      // if we skip simulation before submitting the tx,
-      // tx will be included in a block but with app logic reverted
+      // if we skip simulation, tx is dropped
       await expect(
         votingContract.methods.cast_vote(candidate).send({ skipPublicSimulation: true }).wait(),
-      ).rejects.toThrow(TxStatus.APP_LOGIC_REVERTED);
+      ).rejects.toThrow('Reason: Tx dropped by P2P node.');
     });
   });
 });

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

@@ -23,14 +23,23 @@ describe('DoubleSpendTxValidator', () => {
   });
 
   it('rejects duplicates in non revertible data', async () => {
-    const badTx = await mockTxForRollup();
-    badTx.data.forRollup!.end.nullifiers[1] = badTx.data.forRollup!.end.nullifiers[0];
+    const badTx = await mockTx(1, {
+      numberOfNonRevertiblePublicCallRequests: 1,
+      numberOfRevertiblePublicCallRequests: 0,
+    });
+    badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers[1] =
+      badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers[0];
     await expectInvalid(badTx, 'Duplicate nullifier in tx');
   });
 
   it('rejects duplicates in revertible data', async () => {
-    const badTx = await mockTxForRollup();
-    badTx.data.forRollup!.end.nullifiers[1] = badTx.data.forRollup!.end.nullifiers[0];
+    const badTx = await mockTx(1, {
+      numberOfNonRevertiblePublicCallRequests: 0,
+      numberOfRevertiblePublicCallRequests: 1,
+      numberOfRevertibleNullifiers: 1,
+    });
+    badTx.data.forPublic!.revertibleAccumulatedData.nullifiers[1] =
+      badTx.data.forPublic!.revertibleAccumulatedData.nullifiers[0];
     await expectInvalid(badTx, 'Duplicate nullifier in tx');
   });
 
@@ -43,15 +52,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.

yarn-project/stdlib/src/tests/mocks.ts

@@ -73,13 +73,15 @@ export const mockTx = async (
   {
     numberOfNonRevertiblePublicCallRequests = MAX_ENQUEUED_CALLS_PER_TX / 2,
     numberOfRevertiblePublicCallRequests = MAX_ENQUEUED_CALLS_PER_TX / 2,
+    numberOfRevertibleNullifiers = 0,
     hasPublicTeardownCallRequest = false,
     publicCalldataSize = 2,
     feePayer,
     clientIvcProof = ClientIvcProof.empty(),
   }: {
     numberOfNonRevertiblePublicCallRequests?: number;
     numberOfRevertiblePublicCallRequests?: number;
+    numberOfRevertibleNullifiers?: number;
     hasPublicTeardownCallRequest?: boolean;
     publicCalldataSize?: number;
     feePayer?: AztecAddress;
@@ -123,6 +125,11 @@ export const mockTx = async (
       .withPublicCallRequests(publicCallRequests.slice(numberOfRevertiblePublicCallRequests))
       .build();
 
+    for (let i = 0; i < numberOfRevertibleNullifiers; i++) {
+      const revertibleNullifier = new Nullifier(new Fr(seed + 2 + i), 0, Fr.ZERO);
+      revertibleBuilder.pushNullifier(revertibleNullifier.value);
+    }
+
     data.forPublic.revertibleAccumulatedData = revertibleBuilder
       .withPublicCallRequests(publicCallRequests.slice(0, numberOfRevertiblePublicCallRequests))
       .build();