Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Validate block proposal txs iteratively #10921

Merged
merged 6 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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' }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use constants or enums here so we don't end up with string literals around the codebase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen most discriminated unions use just string literals (see the typescript handbook for an example). But after some googling it seems that enums vs string literals is actually a hot topic. I personally prefer string literals for their simplicity, and because tsc guards you against any typos, but this is not a hill I'm willing to die on.

| { 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
Loading