diff --git a/yarn-project/end-to-end/src/e2e_double_spend.test.ts b/yarn-project/end-to-end/src/e2e_double_spend.test.ts new file mode 100644 index 000000000000..462983475ac3 --- /dev/null +++ b/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; + + 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); + }); + }); +}); diff --git a/yarn-project/end-to-end/src/e2e_private_voting_contract.test.ts b/yarn-project/end-to-end/src/e2e_private_voting_contract.test.ts index a6f43f78012c..353bd81b11ed 100644 --- a/yarn-project/end-to-end/src/e2e_private_voting_contract.test.ts +++ b/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.'); }); }); }); diff --git a/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.test.ts b/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.test.ts index 04febc003c7f..4ad6b44e3af6 100644 --- a/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.test.ts +++ b/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); }); }); diff --git a/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.ts b/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.ts index 931e26c2ebf4..8416d92f8191 100644 --- a/yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.ts +++ b/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; @@ -14,25 +14,20 @@ export class DoubleSpendTxValidator implements TxValidator { } async validateTx(tx: T): Promise { - // 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' }; } } diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.ts index 5a8c100d7028..322de447d142 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.ts +++ b/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) { diff --git a/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts b/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts index ce986df46b8e..1faa6fa54f75 100644 --- a/yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts +++ b/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; - postprocessValidator: TxValidator; 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 { - return new DoubleSpendTxValidator(nullifierCache); -} diff --git a/yarn-project/simulator/src/public/public_processor/public_processor.test.ts b/yarn-project/simulator/src/public/public_processor/public_processor.test.ts index f5f434a730be..3ede06b72da0 100644 --- a/yarn-project/simulator/src/public/public_processor/public_processor.test.ts +++ b/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> = 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 })); diff --git a/yarn-project/simulator/src/public/public_processor/public_processor.ts b/yarn-project/simulator/src/public/public_processor/public_processor.ts index 3167901f0b3e..b2bf25da4b43 100644 --- a/yarn-project/simulator/src/public/public_processor/public_processor.ts +++ b/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; - postprocessValidator?: TxValidator; 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. diff --git a/yarn-project/stdlib/src/tests/mocks.ts b/yarn-project/stdlib/src/tests/mocks.ts index b048d2a15384..7f98d4c710a7 100644 --- a/yarn-project/stdlib/src/tests/mocks.ts +++ b/yarn-project/stdlib/src/tests/mocks.ts @@ -73,6 +73,7 @@ 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, @@ -80,6 +81,7 @@ export const mockTx = async ( }: { 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();