Skip to content

Commit

Permalink
refactor: extract tx validation to separate class
Browse files Browse the repository at this point in the history
  • Loading branch information
@alexghr
alexghr committed Mar 19, 2024
1 parent 3216d40 commit 80390a1
Show file tree
Hide file tree
Showing 6 changed files with 292 additions and 138 deletions.
7 changes: 5 additions & 2 deletions yarn-project/circuit-types/src/tx/tx.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ export class Tx {
* @returns - The hash.
*/
static getHash(tx: Tx | HasHash): TxHash {
const hasHash = (tx: Tx | HasHash): tx is HasHash => (tx as HasHash).hash !== undefined;
return hasHash(tx) ? tx.hash : tx.getTxHash();
}

Expand All @@ -186,7 +185,7 @@ export class Tx {
* @returns The corresponding array of hashes.
*/
static getHashes(txs: (Tx | HasHash)[]): TxHash[] {
return txs.map(tx => Tx.getHash(tx));
return txs.map(Tx.getHash);
}

/**
Expand All @@ -208,3 +207,7 @@ export class Tx {

/** Utility type for an entity that has a hash property for a txhash */
type HasHash = { /** The tx hash */ hash: TxHash };

function hasHash(tx: Tx | HasHash): tx is HasHash {
return (tx as HasHash).hash !== undefined;
}
106 changes: 62 additions & 44 deletions yarn-project/end-to-end/src/e2e_block_building.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
PXE,
SentTx,
TxReceipt,
TxStatus,
Wallet,
} from '@aztec/aztec.js';
import { times } from '@aztec/foundation/collection';
Expand Down Expand Up @@ -110,8 +111,7 @@ describe('e2e_block_building', () => {
}, 60_000);
});

// Regressions for https://github.com/AztecProtocol/aztec-packages/issues/2502
describe('double-spends on the same block', () => {
describe('double-spends', () => {
let contract: TestContract;
let teardown: () => Promise<void>;

Expand All @@ -123,48 +123,66 @@ describe('e2e_block_building', () => {

afterAll(() => teardown());

it('drops tx with private nullifier already emitted on the same block', async () => {
const nullifier = Fr.random();
const calls = times(2, () => contract.methods.emit_nullifier(nullifier));
for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);

it('drops tx with public nullifier already emitted on the same block', async () => {
const secret = Fr.random();
const calls = times(2, () => contract.methods.create_nullifier_public(140n, secret));
for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);

it('drops tx with two equal nullifiers', async () => {
const nullifier = Fr.random();
const calls = times(2, () => contract.methods.emit_nullifier(nullifier).request());
await expect(new BatchCall(owner, calls).send().wait()).rejects.toThrow(/dropped/);
}, 30_000);

it('drops tx with private nullifier already emitted from public on the same block', async () => {
const secret = Fr.random();
// See yarn-project/simulator/src/public/index.test.ts 'Should be able to create a nullifier from the public context'
const emittedPublicNullifier = pedersenHash([new Fr(140), secret].map(a => a.toBuffer()));

const calls = [
contract.methods.create_nullifier_public(140n, secret),
contract.methods.emit_nullifier(emittedPublicNullifier),
];

for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);
// Regressions for https://github.com/AztecProtocol/aztec-packages/issues/2502
describe('in the same block', () => {
it('drops tx with private nullifier already emitted on the same block', async () => {
const nullifier = Fr.random();
const calls = times(2, () => contract.methods.emit_nullifier(nullifier));
for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);

it('drops tx with public nullifier already emitted on the same block', async () => {
const secret = Fr.random();
const calls = times(2, () => contract.methods.create_nullifier_public(140n, secret));
for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);

it('drops tx with two equal nullifiers', async () => {
const nullifier = Fr.random();
const calls = times(2, () => contract.methods.emit_nullifier(nullifier).request());
await expect(new BatchCall(owner, calls).send().wait()).rejects.toThrow(/dropped/);
}, 30_000);

it('drops tx with private nullifier already emitted from public on the same block', async () => {
const secret = Fr.random();
// See yarn-project/simulator/src/public/index.test.ts 'Should be able to create a nullifier from the public context'
const emittedPublicNullifier = pedersenHash([new Fr(140), secret].map(a => a.toBuffer()));

const calls = [
contract.methods.create_nullifier_public(140n, secret),
contract.methods.emit_nullifier(emittedPublicNullifier),
];

for (const call of calls) {
await call.simulate();
}
const [tx1, tx2] = calls.map(call => call.send());
await expectXorTx(tx1, tx2);
}, 30_000);
});

describe('across blocks', () => {
it('drops a tx that tries to spend a nullifier already emitted on a previous block', async () => {
const secret = Fr.random();
const emittedPublicNullifier = pedersenHash([new Fr(140), secret].map(a => a.toBuffer()));

await expect(contract.methods.create_nullifier_public(140n, secret).send().wait()).resolves.toEqual(
expect.objectContaining({
status: TxStatus.MINED,
}),
);

await expect(contract.methods.emit_nullifier(emittedPublicNullifier).send().wait()).rejects.toThrow(/dropped/);
});
});
});
});

Expand Down
2 changes: 1 addition & 1 deletion yarn-project/sequencer-client/src/sequencer/sequencer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ describe('sequencer', () => {
);

// We make a nullifier from tx1 a part of the nullifier tree, so it gets rejected as double spend
const doubleSpendNullifier = doubleSpendTx.data.end.newNullifiers[0].toBuffer();
const doubleSpendNullifier = doubleSpendTx.data.end.newNullifiers[0].value.toBuffer();
merkleTreeOps.findLeafIndex.mockImplementation((treeId: MerkleTreeId, value: Buffer) => {
return Promise.resolve(
treeId === MerkleTreeId.NULLIFIER_TREE && value.equals(doubleSpendNullifier) ? 1n : undefined,
Expand Down
110 changes: 19 additions & 91 deletions yarn-project/sequencer-client/src/sequencer/sequencer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ceilPowerOfTwo } from '../utils.js';
import { SequencerConfig } from './config.js';
import { ProcessedTx } from './processed_tx.js';
import { PublicProcessorFactory } from './public_processor.js';
import { TxValidator } from './tx_validator.js';

/**
* Sequencer client
Expand Down Expand Up @@ -171,8 +172,18 @@ export class Sequencer {
);

// Filter out invalid txs
const trees = this.worldState.getLatest();
const txValidator = new TxValidator(
{
getNullifierIndex(nullifier: Fr): Promise<bigint | undefined> {
return trees.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, nullifier.toBuffer());
},
},
newGlobalVariables,
);

// TODO: It should be responsibility of the P2P layer to validate txs before passing them on here
const validTxs = await this.takeValidTxs(pendingTxs, newGlobalVariables);
const validTxs = await this.takeValidTxs(pendingTxs, txValidator);
if (validTxs.length < this.minTxsPerBLock) {
return;
}
Expand All @@ -193,7 +204,7 @@ export class Sequencer {
// 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.
const processedValidTxs = await this.takeValidTxs(processedTxs, newGlobalVariables);
const processedValidTxs = await this.takeValidTxs(processedTxs, txValidator);

if (processedValidTxs.length === 0) {
this.log('No txs processed correctly to build block. Exiting');
Expand Down Expand Up @@ -251,47 +262,14 @@ export class Sequencer {
}
}

protected async takeValidTxs<T extends Tx | ProcessedTx>(txs: T[], globalVariables: GlobalVariables): Promise<T[]> {
const validTxs: T[] = [];
const txsToDelete = [];
const thisBlockNullifiers: Set<bigint> = new Set();

// Process txs until we get to maxTxsPerBlock, rejecting double spends in the process
for (const tx of txs) {
if (tx.data.constants.txContext.chainId.value !== globalVariables.chainId.value) {
this.log(
`Deleting tx for incorrect chain ${tx.data.constants.txContext.chainId.toString()}, tx hash ${Tx.getHash(
tx,
)}`,
);
txsToDelete.push(tx);
continue;
}
if (await this.isTxDoubleSpend(tx)) {
this.log(`Deleting double spend tx ${Tx.getHash(tx)}`);
txsToDelete.push(tx);
continue;
} else if (this.isTxDoubleSpendSameBlock(tx, thisBlockNullifiers)) {
// We don't drop these txs from the p2p pool immediately since they become valid
// again if the current block fails to be published for some reason.
this.log(`Skipping tx with double-spend for this same block ${Tx.getHash(tx)}`);
continue;
}

tx.data.end.newNullifiers.forEach(n => thisBlockNullifiers.add(n.value.toBigInt()));
tx.data.endNonRevertibleData.newNullifiers.forEach(n => thisBlockNullifiers.add(n.value.toBigInt()));
validTxs.push(tx);
if (validTxs.length >= this.maxTxsPerBlock) {
break;
}
protected async takeValidTxs<T extends Tx | ProcessedTx>(txs: T[], validator: TxValidator): Promise<T[]> {
const [valid, invalid] = await validator.validateTxs(txs);
if (invalid.length > 0) {
this.log(`Dropping invalid txs from the p2p pool ${Tx.getHashes(invalid).join(', ')}`);
await this.p2pClient.deleteTxs(Tx.getHashes(invalid));
}

// Make sure we remove these from the tx pool so we do not consider it again
if (txsToDelete.length > 0) {
await this.p2pClient.deleteTxs(Tx.getHashes([...txsToDelete]));
}

return validTxs;
return valid.slice(0, this.maxTxsPerBlock);
}

/**
Expand Down Expand Up @@ -334,56 +312,6 @@ export class Sequencer {
return block;
}

/**
* Returns true if one of the tx nullifiers exist on the block being built.
* @param tx - The tx to test.
* @param thisBlockNullifiers - The nullifiers added so far.
*/
protected isTxDoubleSpendSameBlock(tx: Tx | ProcessedTx, thisBlockNullifiers: Set<bigint>): boolean {
// We only consider non-empty nullifiers
const newNullifiers = [
...tx.data.endNonRevertibleData.newNullifiers.filter(n => !n.isEmpty()),
...tx.data.end.newNullifiers.filter(n => !n.isEmpty()),
];

for (const nullifier of newNullifiers) {
if (thisBlockNullifiers.has(nullifier.value.toBigInt())) {
return true;
}
}
return false;
}

/**
* Returns true if one of the transaction nullifiers exist.
* Nullifiers prevent double spends in a private context.
* @param tx - The transaction.
* @returns Whether this is a problematic double spend that the L1 contract would reject.
*/
protected async isTxDoubleSpend(tx: Tx | ProcessedTx): Promise<boolean> {
// We only consider non-empty nullifiers
const newNullifiers = [
...tx.data.endNonRevertibleData.newNullifiers.filter(n => !n.isEmpty()),
...tx.data.end.newNullifiers.filter(n => !n.isEmpty()),
];

// Ditch this tx if it has a repeated nullifiers
const uniqNullifiers = new Set(newNullifiers.map(n => n.value.toBigInt()));
if (uniqNullifiers.size !== newNullifiers.length) {
return true;
}

for (const nullifier of newNullifiers) {
// TODO(AD): this is an exhaustive search currently
const db = this.worldState.getLatest();
const indexInDb = await db.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, nullifier.toBuffer());
if (indexInDb !== undefined) {
return true;
}
}
return false;
}

get coinbase(): EthAddress {
return this._coinbase;
}
Expand Down
77 changes: 77 additions & 0 deletions yarn-project/sequencer-client/src/sequencer/tx_validator.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { mockTx as baseMockTx } from '@aztec/circuit-types';
import { Fr, GlobalVariables } from '@aztec/circuits.js';
import { makeGlobalVariables } from '@aztec/circuits.js/testing';

import { MockProxy, mock, mockFn } from 'jest-mock-extended';

import { NullifierSource, TxValidator } from './tx_validator.js';

describe('TxValidator', () => {
let validator: TxValidator;
let globalVariables: GlobalVariables;
let nullifierSource: MockProxy<NullifierSource>;

beforeEach(() => {
nullifierSource = mock<NullifierSource>({
getNullifierIndex: mockFn().mockImplementation(() => {
return Promise.resolve(undefined);
}),
});
globalVariables = makeGlobalVariables();
validator = new TxValidator(nullifierSource, globalVariables);
});

describe('inspects tx metadata', () => {
it('allows only transactions for the right chain', async () => {
const goodTx = mockTx();
const badTx = mockTx();
badTx.data.constants.txContext.chainId = Fr.random();

await expect(validator.validateTxs([goodTx, badTx])).resolves.toEqual([[goodTx], [badTx]]);
});
});

describe('inspects tx nullifiers', () => {
it('rejects duplicates in non revertible data', async () => {
const badTx = mockTx();
badTx.data.endNonRevertibleData.newNullifiers[1] = badTx.data.endNonRevertibleData.newNullifiers[0];
await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]);
});

it('rejects duplicates in revertible data', async () => {
const badTx = mockTx();
badTx.data.end.newNullifiers[1] = badTx.data.end.newNullifiers[0];
await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]);
});

it('rejects duplicates across phases', async () => {
const badTx = mockTx();
badTx.data.end.newNullifiers[0] = badTx.data.endNonRevertibleData.newNullifiers[0];
await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]);
});

it('rejects duplicates across txs', async () => {
const firstTx = mockTx();
const secondTx = mockTx();
secondTx.data.end.newNullifiers[0] = firstTx.data.end.newNullifiers[0];
await expect(validator.validateTxs([firstTx, secondTx])).resolves.toEqual([[firstTx], [secondTx]]);
});

it('rejects duplicates against history', async () => {
const badTx = mockTx();
nullifierSource.getNullifierIndex.mockReturnValueOnce(Promise.resolve(1n));
await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]);
});
});

// get unique txs that are also stable across test runs
let txSeed = 1;
/** Creates a mock tx for the current chain */
function mockTx() {
const tx = baseMockTx(txSeed++, false);
tx.data.constants.txContext.chainId = globalVariables.chainId;
tx.data.constants.txContext.version = globalVariables.version;

return tx;
}
});
Loading

0 comments on commit 80390a1

Please sign in to comment.