Skip to content

Commit

Permalink
feat: Validate block proposal txs iteratively (#10921)
Browse files Browse the repository at this point in the history
Instead of loading all txs from the p2p pool and validating them all, we
now get an iterator to the p2p pool and iteratively run through them and
validate them as we go. This ensures we only load and validate strictly
the txs we need. This also makes it easy to enforce new block
constraints such as gas limits, which we add as two new env vars.

As part of this PR, we also change the interface of validators. Since
there is no point anymore in validating txs in bulk, we drop the
`validateTxs` method in favor of just `validateTx`. And since we're at
it, we enrich `validateTx` to return `valid/invalid/skip` and to include
the failure reason.

Fixes #10869
  • Loading branch information
spalladino authored Jan 7, 2025
1 parent 1cb7cd7 commit c92129e
Show file tree
Hide file tree
Showing 44 changed files with 911 additions and 931 deletions.
31 changes: 19 additions & 12 deletions yarn-project/aztec-node/src/aztec-node/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
type WorldStateSynchronizer,
mockTxForRollup,
} from '@aztec/circuit-types';
import { type ContractDataSource, EthAddress, Fr, MaxBlockNumber } from '@aztec/circuits.js';
import { type ContractDataSource, EthAddress, Fr, GasFees, MaxBlockNumber } from '@aztec/circuits.js';
import { type P2P } from '@aztec/p2p';
import { type GlobalVariableBuilder } from '@aztec/sequencer-client';
import { NoopTelemetryClient } from '@aztec/telemetry-client/noop';
Expand All @@ -37,8 +37,9 @@ describe('aztec node', () => {
p2p = mock<P2P>();

globalVariablesBuilder = mock<GlobalVariableBuilder>();
merkleTreeOps = mock<MerkleTreeReadOperations>();
globalVariablesBuilder.getCurrentBaseFees.mockResolvedValue(new GasFees(0, 0));

merkleTreeOps = mock<MerkleTreeReadOperations>();
merkleTreeOps.findLeafIndices.mockImplementation((_treeId: MerkleTreeId, _value: any[]) => {
return Promise.resolve([undefined]);
});
Expand Down Expand Up @@ -99,14 +100,14 @@ describe('aztec node', () => {
const doubleSpendWithExistingTx = txs[1];
lastBlockNumber += 1;

expect(await node.isValidTx(doubleSpendTx)).toBe(true);
expect(await node.isValidTx(doubleSpendTx)).toEqual({ result: 'valid' });

// We push a duplicate nullifier that was created in the same transaction
doubleSpendTx.data.forRollup!.end.nullifiers.push(doubleSpendTx.data.forRollup!.end.nullifiers[0]);

expect(await node.isValidTx(doubleSpendTx)).toBe(false);
expect(await node.isValidTx(doubleSpendTx)).toEqual({ result: 'invalid', reason: ['Duplicate nullifier in tx'] });

expect(await node.isValidTx(doubleSpendWithExistingTx)).toBe(true);
expect(await node.isValidTx(doubleSpendWithExistingTx)).toEqual({ result: 'valid' });

// We make a nullifier from `doubleSpendWithExistingTx` a part of the nullifier tree, so it gets rejected as double spend
const doubleSpendNullifier = doubleSpendWithExistingTx.data.forRollup!.end.nullifiers[0].toBuffer();
Expand All @@ -116,20 +117,23 @@ describe('aztec node', () => {
);
});

expect(await node.isValidTx(doubleSpendWithExistingTx)).toBe(false);
expect(await node.isValidTx(doubleSpendWithExistingTx)).toEqual({
result: 'invalid',
reason: ['Existing nullifier'],
});
lastBlockNumber = 0;
});

it('tests that the node correctly validates chain id', async () => {
const tx = mockTxForRollup(0x10000);
tx.data.constants.txContext.chainId = chainId;

expect(await node.isValidTx(tx)).toBe(true);
expect(await node.isValidTx(tx)).toEqual({ result: 'valid' });

// We make the chain id on the tx not equal to the configured chain id
tx.data.constants.txContext.chainId = new Fr(1n + chainId.value);
tx.data.constants.txContext.chainId = new Fr(1n + chainId.toBigInt());

expect(await node.isValidTx(tx)).toBe(false);
expect(await node.isValidTx(tx)).toEqual({ result: 'invalid', reason: ['Incorrect chain id'] });
});

it('tests that the node correctly validates max block numbers', async () => {
Expand Down Expand Up @@ -159,11 +163,14 @@ describe('aztec node', () => {
lastBlockNumber = 3;

// Default tx with no max block number should be valid
expect(await node.isValidTx(noMaxBlockNumberMetadata)).toBe(true);
expect(await node.isValidTx(noMaxBlockNumberMetadata)).toEqual({ result: 'valid' });
// Tx with max block number < current block number should be invalid
expect(await node.isValidTx(invalidMaxBlockNumberMetadata)).toBe(false);
expect(await node.isValidTx(invalidMaxBlockNumberMetadata)).toEqual({
result: 'invalid',
reason: ['Invalid block number'],
});
// Tx with max block number >= current block number should be valid
expect(await node.isValidTx(validMaxBlockNumberMetadata)).toBe(true);
expect(await node.isValidTx(validMaxBlockNumberMetadata)).toEqual({ result: 'valid' });
});
});
});
67 changes: 28 additions & 39 deletions yarn-project/aztec-node/src/aztec-node/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
NullifierMembershipWitness,
type NullifierWithBlockSource,
P2PClientType,
type ProcessedTx,
type ProverConfig,
PublicDataWitness,
PublicSimulationOutput,
Expand All @@ -29,7 +28,7 @@ import {
TxReceipt,
type TxScopedL2Log,
TxStatus,
type TxValidator,
type TxValidationResult,
type WorldStateSynchronizer,
tryStop,
} from '@aztec/circuit-types';
Expand Down Expand Up @@ -64,17 +63,16 @@ import { DateProvider, Timer } from '@aztec/foundation/timer';
import { type AztecKVStore } from '@aztec/kv-store';
import { openTmpStore } from '@aztec/kv-store/lmdb';
import { SHA256Trunc, StandardTree, UnbalancedTree } from '@aztec/merkle-tree';
import {
AggregateTxValidator,
DataTxValidator,
DoubleSpendTxValidator,
MetadataTxValidator,
type P2P,
TxProofValidator,
createP2PClient,
} from '@aztec/p2p';
import { type P2P, createP2PClient } from '@aztec/p2p';
import { ProtocolContractAddress } from '@aztec/protocol-contracts';
import { GlobalVariableBuilder, type L1Publisher, SequencerClient, createSlasherClient } from '@aztec/sequencer-client';
import {
GlobalVariableBuilder,
type L1Publisher,
SequencerClient,
createSlasherClient,
createValidatorForAcceptingTxs,
getDefaultAllowedSetupFunctions,
} from '@aztec/sequencer-client';
import { PublicProcessorFactory } from '@aztec/simulator';
import { Attributes, type TelemetryClient, type Traceable, type Tracer, trackSpan } from '@aztec/telemetry-client';
import { NoopTelemetryClient } from '@aztec/telemetry-client/noop';
Expand Down Expand Up @@ -418,15 +416,21 @@ export class AztecNodeService implements AztecNode, Traceable {
*/
public async sendTx(tx: Tx) {
const timer = new Timer();
this.log.info(`Received tx ${tx.getTxHash()}`);
const txHash = tx.getTxHash().toString();

if (!(await this.isValidTx(tx))) {
const valid = await this.isValidTx(tx);
if (valid.result !== 'valid') {
const reason = valid.reason.join(', ');
this.metrics.receivedTx(timer.ms(), false);
this.log.warn(`Invalid tx ${txHash}: ${reason}`, { txHash });
// TODO(#10967): Throw when receiving an invalid tx instead of just returning
// throw new Error(`Invalid tx: ${reason}`);
return;
}

await this.p2pClient!.sendTx(tx);
this.metrics.receivedTx(timer.ms(), true);
this.log.info(`Received tx ${tx.getTxHash()}`, { txHash });
}

public async getTxReceipt(txHash: TxHash): Promise<TxReceipt> {
Expand Down Expand Up @@ -878,34 +882,19 @@ export class AztecNodeService implements AztecNode, Traceable {
}
}

public async isValidTx(tx: Tx, isSimulation: boolean = false): Promise<boolean> {
public async isValidTx(tx: Tx, isSimulation: boolean = false): Promise<TxValidationResult> {
const blockNumber = (await this.blockSource.getBlockNumber()) + 1;
const db = this.worldStateSynchronizer.getCommitted();
// These validators are taken from the sequencer, and should match.
// The reason why `phases` and `gas` tx validator is in the sequencer and not here is because
// those tx validators are customizable by the sequencer.
const txValidators: TxValidator<Tx | ProcessedTx>[] = [
new DataTxValidator(),
new MetadataTxValidator(new Fr(this.l1ChainId), new Fr(blockNumber)),
new DoubleSpendTxValidator({
getNullifierIndices: nullifiers => db.findLeafIndices(MerkleTreeId.NULLIFIER_TREE, nullifiers),
}),
];

if (!isSimulation) {
txValidators.push(new TxProofValidator(this.proofVerifier));
}

const txValidator = new AggregateTxValidator(...txValidators);

const [_, invalidTxs] = await txValidator.validateTxs([tx]);
if (invalidTxs.length > 0) {
this.log.warn(`Rejecting tx ${tx.getTxHash()} because of validation errors`);

return false;
}
const verifier = isSimulation ? undefined : this.proofVerifier;
const validator = createValidatorForAcceptingTxs(db, this.contractDataSource, verifier, {
blockNumber,
l1ChainId: this.l1ChainId,
enforceFees: !!this.config.enforceFees,
setupAllowList: this.config.allowedInSetup ?? getDefaultAllowedSetupFunctions(),
gasFees: await this.getCurrentBaseFees(),
});

return true;
return await validator.validateTx(tx);
}

public async setConfig(config: Partial<SequencerConfig & ProverConfig>): Promise<void> {
Expand Down
14 changes: 10 additions & 4 deletions yarn-project/circuit-types/src/interfaces/aztec-node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { MerkleTreeId } from '../merkle_tree_id.js';
import { EpochProofQuote } from '../prover_coordination/epoch_proof_quote.js';
import { PublicDataWitness } from '../public_data_witness.js';
import { SiblingPath } from '../sibling_path/sibling_path.js';
import { type TxValidationResult } from '../tx/index.js';
import { PublicSimulationOutput } from '../tx/public_simulation_output.js';
import { Tx } from '../tx/tx.js';
import { TxHash } from '../tx/tx_hash.js';
Expand Down Expand Up @@ -293,9 +294,14 @@ describe('AztecNodeApiSchema', () => {
expect(response).toBeInstanceOf(PublicSimulationOutput);
});

it('isValidTx', async () => {
it('isValidTx(valid)', async () => {
const response = await context.client.isValidTx(Tx.random(), true);
expect(response).toEqual({ result: 'valid' });
});

it('isValidTx(invalid)', async () => {
const response = await context.client.isValidTx(Tx.random());
expect(response).toBe(true);
expect(response).toEqual({ result: 'invalid', reason: ['Invalid'] });
});

it('setConfig', async () => {
Expand Down Expand Up @@ -559,9 +565,9 @@ class MockAztecNode implements AztecNode {
expect(tx).toBeInstanceOf(Tx);
return Promise.resolve(PublicSimulationOutput.random());
}
isValidTx(tx: Tx, _isSimulation?: boolean | undefined): Promise<boolean> {
isValidTx(tx: Tx, isSimulation?: boolean | undefined): Promise<TxValidationResult> {
expect(tx).toBeInstanceOf(Tx);
return Promise.resolve(true);
return Promise.resolve(isSimulation ? { result: 'valid' } : { result: 'invalid', reason: ['Invalid'] });
}
setConfig(config: Partial<SequencerConfig & ProverConfig>): Promise<void> {
expect(config.coinbase).toBeInstanceOf(EthAddress);
Expand Down
13 changes: 10 additions & 3 deletions yarn-project/circuit-types/src/interfaces/aztec-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,14 @@ import { MerkleTreeId } from '../merkle_tree_id.js';
import { EpochProofQuote } from '../prover_coordination/epoch_proof_quote.js';
import { PublicDataWitness } from '../public_data_witness.js';
import { SiblingPath } from '../sibling_path/index.js';
import { PublicSimulationOutput, Tx, TxHash, TxReceipt } from '../tx/index.js';
import {
PublicSimulationOutput,
Tx,
TxHash,
TxReceipt,
type TxValidationResult,
TxValidationResultSchema,
} from '../tx/index.js';
import { TxEffect } from '../tx_effect.js';
import { type SequencerConfig, SequencerConfigSchema } from './configs.js';
import { type L2BlockNumber, L2BlockNumberSchema } from './l2_block_number.js';
Expand Down Expand Up @@ -395,7 +402,7 @@ export interface AztecNode
* @param tx - The transaction to validate for correctness.
* @param isSimulation - True if the transaction is a simulated one without generated proofs. (Optional)
*/
isValidTx(tx: Tx, isSimulation?: boolean): Promise<boolean>;
isValidTx(tx: Tx, isSimulation?: boolean): Promise<TxValidationResult>;

/**
* Updates the configuration of this node.
Expand Down Expand Up @@ -567,7 +574,7 @@ export const AztecNodeApiSchema: ApiSchemaFor<AztecNode> = {

simulatePublicCalls: z.function().args(Tx.schema, optional(z.boolean())).returns(PublicSimulationOutput.schema),

isValidTx: z.function().args(Tx.schema, optional(z.boolean())).returns(z.boolean()),
isValidTx: z.function().args(Tx.schema, optional(z.boolean())).returns(TxValidationResultSchema),

setConfig: z.function().args(SequencerConfigSchema.merge(ProverConfigSchema).partial()).returns(z.void()),

Expand Down
6 changes: 6 additions & 0 deletions yarn-project/circuit-types/src/interfaces/configs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ export interface SequencerConfig {
maxTxsPerBlock?: number;
/** The minimum number of txs to include in a block. */
minTxsPerBlock?: number;
/** The maximum L2 block gas. */
maxL2BlockGas?: number;
/** The maximum DA block gas. */
maxDABlockGas?: number;
/** Recipient of block reward. */
coinbase?: EthAddress;
/** Address to receive fees. */
Expand Down Expand Up @@ -53,6 +57,8 @@ export const SequencerConfigSchema = z.object({
transactionPollingIntervalMS: z.number().optional(),
maxTxsPerBlock: z.number().optional(),
minTxsPerBlock: z.number().optional(),
maxL2BlockGas: z.number().optional(),
maxDABlockGas: z.number().optional(),
coinbase: schemas.EthAddress.optional(),
feeRecipient: schemas.AztecAddress.optional(),
acvmWorkingDirectory: z.string().optional(),
Expand Down
16 changes: 16 additions & 0 deletions yarn-project/circuit-types/src/tx/tx.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {
ClientIvcProof,
Fr,
PrivateKernelTailCircuitPublicInputs,
PrivateLog,
type PrivateToPublicAccumulatedData,
type ScopedLogHash,
} from '@aztec/circuits.js';
Expand Down Expand Up @@ -230,6 +232,20 @@ export class Tx extends Gossipable {
);
}

/**
* Estimates the tx size based on its private effects. Note that the actual size of the tx
* after processing will probably be larger, as public execution would generate more data.
*/
getEstimatedPrivateTxEffectsSize() {
return (
this.unencryptedLogs.getSerializedLength() +
this.contractClassLogs.getSerializedLength() +
this.data.getNonEmptyNoteHashes().length * Fr.SIZE_IN_BYTES +
this.data.getNonEmptyNullifiers().length * Fr.SIZE_IN_BYTES +
this.data.getNonEmptyPrivateLogs().length * PrivateLog.SIZE_IN_BYTES
);
}

/**
* Convenience function to get a hash out of a tx or a tx-like.
* @param tx - Tx-like object.
Expand Down
10 changes: 3 additions & 7 deletions yarn-project/circuit-types/src/tx/validator/empty_validator.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import { type AnyTx, type TxValidator } from './tx_validator.js';
import { type AnyTx, type TxValidationResult, type TxValidator } from './tx_validator.js';

export class EmptyTxValidator<T extends AnyTx = AnyTx> implements TxValidator<T> {
public validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[], skippedTxs: T[]]> {
return Promise.resolve([txs, [], []]);
}

public validateTx(_tx: T): Promise<boolean> {
return Promise.resolve(true);
public validateTx(_tx: T): Promise<TxValidationResult> {
return Promise.resolve({ result: 'valid' });
}
}
18 changes: 16 additions & 2 deletions yarn-project/circuit-types/src/tx/validator/tx_validator.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,23 @@
import { type ZodFor } from '@aztec/foundation/schemas';

import { z } from 'zod';

import { type ProcessedTx } from '../processed_tx.js';
import { type Tx } from '../tx.js';

export type AnyTx = Tx | ProcessedTx;

export type TxValidationResult =
| { result: 'valid' }
| { result: 'invalid'; reason: string[] }
| { result: 'skipped'; reason: string[] };

export interface TxValidator<T extends AnyTx = AnyTx> {
validateTx(tx: T): Promise<boolean>;
validateTxs(txs: T[]): Promise<[validTxs: T[], invalidTxs: T[], skippedTxs?: T[]]>;
validateTx(tx: T): Promise<TxValidationResult>;
}

export const TxValidationResultSchema = z.discriminatedUnion('result', [
z.object({ result: z.literal('valid'), reason: z.array(z.string()).optional() }),
z.object({ result: z.literal('invalid'), reason: z.array(z.string()) }),
z.object({ result: z.literal('skipped'), reason: z.array(z.string()) }),
]) satisfies ZodFor<TxValidationResult>;
5 changes: 5 additions & 0 deletions yarn-project/circuit-types/src/tx_effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ export class TxEffect {
]);
}

/** Returns the size of this tx effect in bytes as serialized onto DA. */
getDASize() {
return this.toBlobFields().length * Fr.SIZE_IN_BYTES;
}

/**
* Deserializes the TxEffect object from a Buffer.
* @param buffer - Buffer or BufferReader object to deserialize.
Expand Down
5 changes: 5 additions & 0 deletions yarn-project/circuits.js/src/structs/gas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ export class Gas {
return new Gas(Math.ceil(this.daGas * scalar), Math.ceil(this.l2Gas * scalar));
}

/** Returns true if any of this instance's dimensions is greater than the corresponding on the other. */
gtAny(other: Gas) {
return this.daGas > other.daGas || this.l2Gas > other.l2Gas;
}

computeFee(gasFees: GasFees) {
return GasDimensions.reduce(
(acc, dimension) => acc.add(gasFees.get(dimension).mul(new Fr(this.get(dimension)))),
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/e2e_block_building.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ describe('e2e_block_building', () => {
// to pick up and validate the txs, so we may need to bump it to work on CI. Note that we need
// at least 3s here so the archiver has time to loop once and sync, and the sequencer has at
// least 1s to loop.
sequencer.sequencer.timeTable[SequencerState.WAITING_FOR_TXS] = 4;
sequencer.sequencer.timeTable[SequencerState.INITIALIZING_PROPOSAL] = 4;
sequencer.sequencer.timeTable[SequencerState.CREATING_BLOCK] = 4;
sequencer.sequencer.processTxTime = 1;

Expand Down
Loading

0 comments on commit c92129e

Please sign in to comment.