fix: Rejecting txs with duplicate nullifiers (#17176)

Fixes a bug in the `DoubleSpendValidator` where txs that contained
duplicate nullifiers were not being caught (`Set` of non-primitive types
strikes again).

Also updates the public-processor so it always throws when it stumbles
upon a tx with duplicate nullifiers from private-land, and fixes the
`e2e_event_logs` test that caused this due to an incorrect setup (it was
calling `ensureAccountContractsPublished` with the same address twice,
which resulted in duplicate requests in a `BatchCall`).

Author: spalladino

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

@@ -1,4 +1,4 @@
-import { AztecAddress, type AztecNode, Fr, type Wallet, getDecodedPublicEvents } from '@aztec/aztec.js';
+import { AztecAddress, type AztecNode, Fr, type Logger, type Wallet, getDecodedPublicEvents } from '@aztec/aztec.js';
 import { makeTuple } from '@aztec/foundation/array';
 import { timesParallel } from '@aztec/foundation/collection';
 import type { Tuple } from '@aztec/foundation/serialize';
@@ -20,6 +20,7 @@ describe('Logs', () => {
   let account1Address: AztecAddress;
   let account2Address: AztecAddress;
 
+  let log: Logger;
   let teardown: () => Promise<void>;
 
   beforeAll(async () => {
@@ -28,10 +29,13 @@ describe('Logs', () => {
       wallet,
       accounts: [account1Address, account2Address],
       aztecNode,
+      logger: log,
     } = await setup(2));
 
-    await ensureAccountContractsPublished(wallet, [account1Address, account1Address]);
+    log.warn(`Setup complete, checking account contracts published`);
+    await ensureAccountContractsPublished(wallet, [account1Address, account2Address]);
 
+    log.warn(`Deploying test contract`);
     testLogContract = await TestLogContract.deploy(wallet).send({ from: account1Address }).deployed();
   });
 

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

@@ -1,3 +1,4 @@
+import { Fr } from '@aztec/foundation/fields';
 import { mockTx, mockTxForRollup } from '@aztec/stdlib/testing';
 import { type AnyTx, TX_ERROR_DUPLICATE_NULLIFIER_IN_TX, TX_ERROR_EXISTING_NULLIFIER } from '@aztec/stdlib/tx';
 
@@ -27,8 +28,8 @@ describe('DoubleSpendTxValidator', () => {
       numberOfNonRevertiblePublicCallRequests: 1,
       numberOfRevertiblePublicCallRequests: 0,
     });
-    badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers[1] =
-      badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers[0];
+    const nullifiers = badTx.data.forPublic!.nonRevertibleAccumulatedData.nullifiers;
+    nullifiers[1] = new Fr(nullifiers[0].toBigInt());
     await expectInvalid(badTx, TX_ERROR_DUPLICATE_NULLIFIER_IN_TX);
   });
 
@@ -38,8 +39,8 @@ describe('DoubleSpendTxValidator', () => {
       numberOfRevertiblePublicCallRequests: 1,
       numberOfRevertibleNullifiers: 1,
     });
-    badTx.data.forPublic!.revertibleAccumulatedData.nullifiers[1] =
-      badTx.data.forPublic!.revertibleAccumulatedData.nullifiers[0];
+    const nullifiers = badTx.data.forPublic!.revertibleAccumulatedData.nullifiers;
+    nullifiers[1] = new Fr(nullifiers[0].toBigInt());
     await expectInvalid(badTx, TX_ERROR_DUPLICATE_NULLIFIER_IN_TX);
   });
 

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

@@ -24,7 +24,7 @@ export class DoubleSpendTxValidator<T extends AnyTx> implements TxValidator<T> {
     const nullifiers = tx instanceof Tx ? tx.data.getNonEmptyNullifiers() : tx.txEffect.nullifiers;
 
     // Ditch this tx if it has repeated nullifiers
-    const uniqueNullifiers = new Set(nullifiers);
+    const uniqueNullifiers = new Set(nullifiers.map(n => n.toBigInt()));
     if (uniqueNullifiers.size !== nullifiers.length) {
       this.#log.verbose(`Rejecting tx ${'txHash' in tx ? tx.txHash : tx.hash} for emitting duplicate nullifiers`);
       return { result: 'invalid', reason: [TX_ERROR_DUPLICATE_NULLIFIER_IN_TX] };

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

@@ -27,7 +27,6 @@ import {
   StateReference,
   Tx,
   TxExecutionPhase,
-  type TxValidator,
   makeProcessedTxFromPrivateOnlyTx,
   makeProcessedTxFromTxWithPublicCalls,
 } from '@aztec/stdlib/tx';
@@ -384,10 +383,7 @@ export class PublicProcessor implements Traceable {
     return [processedTx, returnValues ?? []];
   }
 
-  private async doTreeInsertionsForPrivateOnlyTx(
-    processedTx: ProcessedTx,
-    txValidator?: TxValidator<ProcessedTx>,
-  ): Promise<void> {
+  private async doTreeInsertionsForPrivateOnlyTx(processedTx: ProcessedTx): Promise<void> {
     const treeInsertionStart = process.hrtime.bigint();
 
     // Update the state so that the next tx in the loop has the correct .startState
@@ -406,14 +402,8 @@ export class PublicProcessor implements Traceable {
         padArrayEnd(processedTx.txEffect.nullifiers, Fr.ZERO, MAX_NULLIFIERS_PER_TX).map(n => n.toBuffer()),
         NULLIFIER_SUBTREE_HEIGHT,
       );
-    } catch {
-      if (txValidator) {
-        // Ideally the validator has already caught this above, but just in case:
-        throw new Error(`Transaction ${processedTx.hash} invalid after processing public functions`);
-      } else {
-        // We have no validator and assume this call should blindly process txs with duplicates being caught later
-        this.log.warn(`Detected duplicate nullifier after public processing for: ${processedTx.hash}.`);
-      }
+    } catch (cause) {
+      throw new Error(`Transaction ${processedTx.hash} failed with duplicate nullifiers`, { cause });
     }
 
     const treeInsertionEnd = process.hrtime.bigint();