From 6155fbeeaae097886b7ead8f8222592baf0752fb Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Tue, 3 Mar 2026 17:31:22 -0300 Subject: [PATCH] refactor(p2p): decouple proposal validators from base class via composition Remove pass-through getters (blockNumber, txHashes, txs) from CheckpointProposal that just delegated to lastBlock. Split ProposalValidator into header validation (validate) and tx validation (validateTxs). BlockProposalValidator and CheckpointProposalValidator now use ProposalValidator by composition instead of inheritance, calling validate() for header checks and validateTxs() for tx checks (on the block proposal directly or via getBlockProposal()). Co-Authored-By: Claude Opus 4.6 --- .../block_proposal_validator.test.ts | 26 -- .../block_proposal_validator.ts | 16 +- .../checkpoint_proposal_validator.test.ts | 130 ------ .../checkpoint_proposal_validator.ts | 25 +- .../proposal_validator.test.ts | 237 +++++++++++ .../proposal_validator/proposal_validator.ts | 105 ++--- .../proposal_validator_test_suite.ts | 379 ------------------ .../stdlib/src/p2p/checkpoint_proposal.ts | 17 - .../src/duties/validation_service.ts | 10 +- .../validator-client/src/validator.ts | 2 - 10 files changed, 328 insertions(+), 619 deletions(-) delete mode 100644 yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.test.ts delete mode 100644 yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.test.ts create mode 100644 yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.test.ts delete mode 100644 yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator_test_suite.ts diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.test.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.test.ts deleted file mode 100644 index 873d387afd70..000000000000 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.test.ts +++ /dev/null @@ -1,26 +0,0 @@ -import type { EpochCacheInterface } from '@aztec/epoch-cache'; -import { BlockNumber, SlotNumber } from '@aztec/foundation/branded-types'; -import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; -import { EthAddress } from '@aztec/foundation/eth-address'; -import { makeBlockHeader, makeBlockProposal } from '@aztec/stdlib/testing'; -import { TxHash } from '@aztec/stdlib/tx'; - -import { mock } from 'jest-mock-extended'; - -import { BlockProposalValidator } from './block_proposal_validator.js'; -import { sharedProposalValidatorTests } from './proposal_validator_test_suite.js'; - -describe('BlockProposalValidator', () => { - sharedProposalValidatorTests({ - validatorFactory: (epochCache, opts) => new BlockProposalValidator(epochCache, opts), - makeProposal: makeBlockProposal, - makeHeader: (epochNumber: number | bigint, slotNumber: number | bigint, blockNumber: number | bigint) => - makeBlockHeader(0, { blockNumber: BlockNumber(Number(blockNumber)), slotNumber: SlotNumber(Number(slotNumber)) }), - getSigner: () => Secp256k1Signer.random(), - getAddress: (signer?: Secp256k1Signer) => (signer ? signer.address : EthAddress.random()), - getSlot: (slot: number | bigint) => SlotNumber(Number(slot)), - getTxHashes: (n: number) => Array.from({ length: n }, () => TxHash.random()), - getTxs: () => [], - epochCacheMock: () => mock(), - }); -}); diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.ts index a481256e9f37..bac274f46475 100644 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.ts +++ b/yarn-project/p2p/src/msg_validators/proposal_validator/block_proposal_validator.ts @@ -1,10 +1,20 @@ import type { EpochCacheInterface } from '@aztec/epoch-cache'; -import type { BlockProposal, P2PValidator } from '@aztec/stdlib/p2p'; +import type { BlockProposal, P2PValidator, ValidationResult } from '@aztec/stdlib/p2p'; import { ProposalValidator } from '../proposal_validator/proposal_validator.js'; -export class BlockProposalValidator extends ProposalValidator implements P2PValidator { +export class BlockProposalValidator implements P2PValidator { + private proposalValidator: ProposalValidator; + constructor(epochCache: EpochCacheInterface, opts: { txsPermitted: boolean; maxTxsPerBlock?: number }) { - super(epochCache, opts, 'p2p:block_proposal_validator'); + this.proposalValidator = new ProposalValidator(epochCache, opts, 'p2p:block_proposal_validator'); + } + + async validate(proposal: BlockProposal): Promise { + const headerResult = await this.proposalValidator.validate(proposal); + if (headerResult.result !== 'accept') { + return headerResult; + } + return this.proposalValidator.validateTxs(proposal); } } diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.test.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.test.ts deleted file mode 100644 index ea093cd2ab2e..000000000000 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.test.ts +++ /dev/null @@ -1,130 +0,0 @@ -import type { EpochCacheInterface } from '@aztec/epoch-cache'; -import { SlotNumber } from '@aztec/foundation/branded-types'; -import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; -import { EthAddress } from '@aztec/foundation/eth-address'; -import { CheckpointHeader } from '@aztec/stdlib/rollup'; -import type { MakeCheckpointProposalOptions } from '@aztec/stdlib/testing'; -import { makeBlockHeader, makeCheckpointHeader, makeCheckpointProposal } from '@aztec/stdlib/testing'; -import { TxHash } from '@aztec/stdlib/tx'; - -import { mock } from 'jest-mock-extended'; - -import { CheckpointProposalValidator } from './checkpoint_proposal_validator.js'; -import { sharedProposalValidatorTests } from './proposal_validator_test_suite.js'; - -describe('CheckpointProposalValidator', () => { - /** - * Adapter function to convert shared test options to CheckpointProposal options. - * The shared test uses blockHeader/lastBlockHeader, but CheckpointProposal uses - * checkpointHeader and lastBlock.blockHeader. - */ - const makeCheckpointProposalAdapter = (options?: { - blockHeader?: CheckpointHeader; - lastBlockHeader?: CheckpointHeader; - signer?: Secp256k1Signer; - txHashes?: TxHash[]; - txs?: any[]; - }) => { - // Use the blockHeader directly as the checkpointHeader - const checkpointHeader = options?.blockHeader ?? makeCheckpointHeader(1); - - // Create a BlockHeader for the lastBlock using the slot from the checkpointHeader - const lastBlockBlockHeader = options?.lastBlockHeader - ? makeBlockHeader(0, { slotNumber: checkpointHeader.slotNumber }) - : undefined; - - const adaptedOptions: MakeCheckpointProposalOptions = { - signer: options?.signer, - checkpointHeader, - // Create lastBlock with a proper BlockHeader - lastBlock: lastBlockBlockHeader - ? { - blockHeader: lastBlockBlockHeader, - txHashes: options?.txHashes, - txs: options?.txs, - } - : undefined, - }; - - return makeCheckpointProposal(adaptedOptions); - }; - - sharedProposalValidatorTests({ - validatorFactory: (epochCache, opts) => new CheckpointProposalValidator(epochCache, opts), - makeProposal: makeCheckpointProposalAdapter, - makeHeader: (_epochNumber: number | bigint, slotNumber: number | bigint, _blockNumber: number | bigint) => - makeCheckpointHeader(0, { slotNumber: SlotNumber(Number(slotNumber)) }), - getSigner: () => Secp256k1Signer.random(), - getAddress: (signer?: Secp256k1Signer) => (signer ? signer.address : EthAddress.random()), - getSlot: (slot: number | bigint) => SlotNumber(Number(slot)), - getTxHashes: (n: number) => Array.from({ length: n }, () => TxHash.random()), - getTxs: () => [], - epochCacheMock: () => mock(), - }); - - describe('maxTxsPerBlock validation', () => { - const currentSlot = SlotNumber(100); - const nextSlot = SlotNumber(101); - let epochCache: ReturnType>; - - function setupEpochCache(proposerAddress: EthAddress) { - epochCache = mock(); - epochCache.getCurrentAndNextSlot.mockReturnValue({ currentSlot, nextSlot }); - epochCache.getProposerAttesterAddressInSlot.mockResolvedValue(proposerAddress); - } - - it('rejects checkpoint proposal when last block txHashes exceed maxTxsPerBlock', async () => { - const signer = Secp256k1Signer.random(); - setupEpochCache(signer.address); - const validator = new CheckpointProposalValidator(epochCache, { txsPermitted: true, maxTxsPerBlock: 2 }); - - const header = makeCheckpointHeader(0, { slotNumber: currentSlot }); - const proposal = await makeCheckpointProposalAdapter({ - blockHeader: header, - lastBlockHeader: header, - signer, - txHashes: Array.from({ length: 3 }, () => TxHash.random()), - }); - - const result = await validator.validate(proposal); - expect(result).toEqual({ result: 'reject', severity: expect.anything() }); - }); - - it('accepts checkpoint proposal when last block txHashes are within maxTxsPerBlock', async () => { - const signer = Secp256k1Signer.random(); - setupEpochCache(signer.address); - const validator = new CheckpointProposalValidator(epochCache, { txsPermitted: true, maxTxsPerBlock: 5 }); - - const header = makeCheckpointHeader(0, { slotNumber: currentSlot }); - const proposal = await makeCheckpointProposalAdapter({ - blockHeader: header, - lastBlockHeader: header, - signer, - txHashes: Array.from({ length: 3 }, () => TxHash.random()), - }); - - const result = await validator.validate(proposal); - expect(result).toEqual({ result: 'accept' }); - }); - - it('skips maxTxsPerBlock check when not configured', async () => { - const signer = Secp256k1Signer.random(); - setupEpochCache(signer.address); - const validator = new CheckpointProposalValidator(epochCache, { - txsPermitted: true, - maxTxsPerBlock: undefined, - }); - - const header = makeCheckpointHeader(0, { slotNumber: currentSlot }); - const proposal = await makeCheckpointProposalAdapter({ - blockHeader: header, - lastBlockHeader: header, - signer, - txHashes: Array.from({ length: 100 }, () => TxHash.random()), - }); - - const result = await validator.validate(proposal); - expect(result).toEqual({ result: 'accept' }); - }); - }); -}); diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.ts index 74804fe45d21..11d94fe6a9d5 100644 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.ts +++ b/yarn-project/p2p/src/msg_validators/proposal_validator/checkpoint_proposal_validator.ts @@ -1,13 +1,26 @@ import type { EpochCacheInterface } from '@aztec/epoch-cache'; -import type { CheckpointProposal, P2PValidator } from '@aztec/stdlib/p2p'; +import type { CheckpointProposal, P2PValidator, ValidationResult } from '@aztec/stdlib/p2p'; import { ProposalValidator } from '../proposal_validator/proposal_validator.js'; -export class CheckpointProposalValidator - extends ProposalValidator - implements P2PValidator -{ +export class CheckpointProposalValidator implements P2PValidator { + private proposalValidator: ProposalValidator; + constructor(epochCache: EpochCacheInterface, opts: { txsPermitted: boolean; maxTxsPerBlock?: number }) { - super(epochCache, opts, 'p2p:checkpoint_proposal_validator'); + this.proposalValidator = new ProposalValidator(epochCache, opts, 'p2p:checkpoint_proposal_validator'); + } + + async validate(proposal: CheckpointProposal): Promise { + const headerResult = await this.proposalValidator.validate(proposal); + if (headerResult.result !== 'accept') { + return headerResult; + } + + const blockProposal = proposal.getBlockProposal(); + if (blockProposal) { + return this.proposalValidator.validateTxs(blockProposal); + } + + return { result: 'accept' }; } } diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.test.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.test.ts new file mode 100644 index 000000000000..8df14cd951f0 --- /dev/null +++ b/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.test.ts @@ -0,0 +1,237 @@ +import type { EpochCacheInterface } from '@aztec/epoch-cache'; +import { NoCommitteeError } from '@aztec/ethereum/contracts'; +import { SlotNumber } from '@aztec/foundation/branded-types'; +import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; +import { EthAddress } from '@aztec/foundation/eth-address'; +import { PeerErrorSeverity } from '@aztec/stdlib/p2p'; +import { + makeBlockHeader, + makeBlockProposal, + makeCheckpointHeader, + makeCheckpointProposal, +} from '@aztec/stdlib/testing'; +import { TxHash } from '@aztec/stdlib/tx'; + +import { jest } from '@jest/globals'; +import { type MockProxy, mock } from 'jest-mock-extended'; + +import { ProposalValidator } from './proposal_validator.js'; + +describe('ProposalValidator', () => { + const currentSlot = SlotNumber(100); + const nextSlot = SlotNumber(101); + const previousSlot = SlotNumber(99); + let epochCache: MockProxy; + let validator: ProposalValidator; + + function mockGetProposer(currentProposer: EthAddress, nextProposer: EthAddress, previousProposer?: EthAddress) { + epochCache.getProposerAttesterAddressInSlot.mockImplementation(slot => { + if (slot === currentSlot) { + return Promise.resolve(currentProposer); + } + if (slot === nextSlot) { + return Promise.resolve(nextProposer); + } + if (slot === previousSlot && previousProposer) { + return Promise.resolve(previousProposer); + } + throw new Error('Unexpected argument'); + }); + } + + beforeEach(() => { + epochCache = mock(); + validator = new ProposalValidator(epochCache, { txsPermitted: true, maxTxsPerBlock: undefined }, 'test'); + epochCache.getCurrentAndNextSlot.mockReturnValue({ currentSlot, nextSlot }); + }); + + describe.each([ + { + name: 'block proposal', + factory: (slotNumber: SlotNumber, signer: Secp256k1Signer) => + makeBlockProposal({ blockHeader: makeBlockHeader(0, { slotNumber }), signer }), + }, + { + name: 'checkpoint proposal', + factory: (slotNumber: SlotNumber, signer: Secp256k1Signer) => + makeCheckpointProposal({ checkpointHeader: makeCheckpointHeader(0, { slotNumber }), signer }), + }, + ])('validate with $name', ({ factory }) => { + it('rejects with high tolerance error if slot is outside clock tolerance', async () => { + const proposal = await factory(previousSlot, Secp256k1Signer.random()); + + epochCache.getEpochAndSlotNow.mockReturnValue({ + epoch: 1 as any, + slot: currentSlot, + ts: 1000n, + nowMs: 1001000n, // 1000ms elapsed, outside 500ms tolerance + }); + + epochCache.getProposerAttesterAddressInSlot.mockResolvedValue(EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.HighToleranceError }); + expect(epochCache.getProposerAttesterAddressInSlot).not.toHaveBeenCalled(); + }); + + it('ignores if previous slot proposal is within clock tolerance', async () => { + const signer = Secp256k1Signer.random(); + const proposal = await factory(previousSlot, signer); + + epochCache.getEpochAndSlotNow.mockReturnValue({ + epoch: 1 as any, + slot: currentSlot, + ts: 1000n, + nowMs: 1000100n, // 100ms elapsed, within 500ms tolerance + }); + + mockGetProposer(EthAddress.random(), EthAddress.random(), signer.address); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'ignore' }); + }); + + it('rejects with mid tolerance error if signature is invalid', async () => { + const signer = Secp256k1Signer.random(); + const proposal = await factory(currentSlot, signer); + + jest.spyOn(proposal as any, 'getSender').mockReturnValue(undefined); + + mockGetProposer(signer.address, EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + expect(epochCache.getProposerAttesterAddressInSlot).not.toHaveBeenCalled(); + }); + + it('rejects with mid tolerance error if proposer is wrong for current slot', async () => { + const proposal = await factory(currentSlot, Secp256k1Signer.random()); + + mockGetProposer(EthAddress.random(), EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('rejects with mid tolerance error if proposer is wrong for next slot', async () => { + const proposal = await factory(nextSlot, Secp256k1Signer.random()); + + mockGetProposer(EthAddress.random(), EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('rejects with mid tolerance error if current proposer sends for next slot', async () => { + const currentProposer = Secp256k1Signer.random(); + const proposal = await factory(nextSlot, currentProposer); + + mockGetProposer(currentProposer.address, EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('accepts when proposer is undefined (open committee)', async () => { + const proposal = await factory(currentSlot, Secp256k1Signer.random()); + + epochCache.getProposerAttesterAddressInSlot.mockResolvedValue(undefined); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + + it('rejects with low tolerance error on NoCommitteeError', async () => { + const proposal = await factory(currentSlot, Secp256k1Signer.random()); + + epochCache.getProposerAttesterAddressInSlot.mockRejectedValue(new NoCommitteeError()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.LowToleranceError }); + }); + + it('accepts valid proposal for current slot', async () => { + const signer = Secp256k1Signer.random(); + const proposal = await factory(currentSlot, signer); + + mockGetProposer(signer.address, EthAddress.random()); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + + it('accepts valid proposal for next slot', async () => { + const signer = Secp256k1Signer.random(); + const proposal = await factory(nextSlot, signer); + + mockGetProposer(EthAddress.random(), signer.address); + const result = await validator.validate(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + }); + + describe('validateTxs', () => { + describe('txsPermitted', () => { + it('rejects proposal with txHashes when txs not permitted', async () => { + validator = new ProposalValidator(epochCache, { txsPermitted: false, maxTxsPerBlock: undefined }, 'test'); + + const proposal = await makeBlockProposal({ txHashes: [TxHash.random(), TxHash.random()] }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('accepts proposal with no txHashes when txs not permitted', async () => { + validator = new ProposalValidator(epochCache, { txsPermitted: false, maxTxsPerBlock: undefined }, 'test'); + + const proposal = await makeBlockProposal({ txHashes: [] }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + + it('accepts proposal with txHashes when txs permitted', async () => { + const proposal = await makeBlockProposal({ txHashes: [TxHash.random(), TxHash.random()] }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + }); + + describe('embedded tx validation', () => { + it('rejects if embedded txs are not listed in txHashes', async () => { + const txHashes = [TxHash.random(), TxHash.random()]; + const proposal = await makeBlockProposal({ txHashes }); + + const fakeTx = { getTxHash: () => TxHash.random(), validateTxHash: () => Promise.resolve(true) }; + Object.defineProperty(proposal, 'txs', { get: () => [fakeTx], configurable: true }); + + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('rejects if embedded tx has invalid tx hash', async () => { + const txHashes = [TxHash.random(), TxHash.random()]; + const proposal = await makeBlockProposal({ txHashes }); + + const fakeTx = { getTxHash: () => txHashes[0], validateTxHash: () => Promise.resolve(false) }; + Object.defineProperty(proposal, 'txs', { get: () => [fakeTx], configurable: true }); + + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.LowToleranceError }); + }); + }); + + describe('maxTxsPerBlock', () => { + it('rejects when txHashes exceed maxTxsPerBlock', async () => { + validator = new ProposalValidator(epochCache, { txsPermitted: true, maxTxsPerBlock: 2 }, 'test'); + + const proposal = await makeBlockProposal({ txHashes: Array.from({ length: 3 }, () => TxHash.random()) }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); + }); + + it('accepts when txHashes count equals maxTxsPerBlock', async () => { + validator = new ProposalValidator(epochCache, { txsPermitted: true, maxTxsPerBlock: 2 }, 'test'); + + const proposal = await makeBlockProposal({ txHashes: Array.from({ length: 2 }, () => TxHash.random()) }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + + it('accepts when maxTxsPerBlock is not set (unlimited)', async () => { + const proposal = await makeBlockProposal({ txHashes: Array.from({ length: 10 }, () => TxHash.random()) }); + const result = await validator.validateTxs(proposal); + expect(result).toEqual({ result: 'accept' }); + }); + }); + }); +}); diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.ts index a926d1f3c144..45c38dd61529 100644 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.ts +++ b/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator.ts @@ -1,15 +1,21 @@ import type { EpochCacheInterface } from '@aztec/epoch-cache'; import { NoCommitteeError } from '@aztec/ethereum/contracts'; import { type Logger, createLogger } from '@aztec/foundation/log'; -import { BlockProposal, CheckpointProposal, PeerErrorSeverity, type ValidationResult } from '@aztec/stdlib/p2p'; +import { + type BlockProposal, + type CheckpointProposalCore, + PeerErrorSeverity, + type ValidationResult, +} from '@aztec/stdlib/p2p'; import { isWithinClockTolerance } from '../clock_tolerance.js'; -export abstract class ProposalValidator { - protected epochCache: EpochCacheInterface; - protected logger: Logger; - protected txsPermitted: boolean; - protected maxTxsPerBlock?: number; +/** Validates header-level and tx-level fields of block and checkpoint proposals. */ +export class ProposalValidator { + private epochCache: EpochCacheInterface; + private logger: Logger; + private txsPermitted: boolean; + private maxTxsPerBlock?: number; constructor( epochCache: EpochCacheInterface, @@ -22,7 +28,8 @@ export abstract class ProposalValidator { + /** Validates header-level fields: slot, signature, and proposer. */ + public async validate(proposal: BlockProposal | CheckpointProposalCore): Promise { try { // Slot check const { currentSlot, nextSlot } = this.epochCache.getCurrentAndNextSlot(); @@ -44,38 +51,6 @@ export abstract class ProposalValidator 0 || embeddedTxCount > 0)) { - this.logger.warn( - `Penalizing peer for proposal with ${proposal.txHashes.length} transaction(s) when transactions are not permitted`, - ); - return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; - } - - // Max txs per block check - if (this.maxTxsPerBlock !== undefined && proposal.txHashes.length > this.maxTxsPerBlock) { - this.logger.warn( - `Penalizing peer for proposal with ${proposal.txHashes.length} transaction(s) when max is ${this.maxTxsPerBlock}`, - ); - return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; - } - - // Embedded txs must be listed in txHashes - const hashSet = new Set(proposal.txHashes.map(h => h.toString())); - const missingTxHashes = - embeddedTxCount > 0 - ? proposal.txs!.filter(tx => !hashSet.has(tx.getTxHash().toString())).map(tx => tx.getTxHash().toString()) - : []; - if (embeddedTxCount > 0 && missingTxHashes.length > 0) { - this.logger.warn('Penalizing peer for embedded transaction(s) not included in txHashes', { - embeddedTxCount, - txHashesLength: proposal.txHashes.length, - missingTxHashes, - }); - return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; - } - // Proposer check const expectedProposer = await this.epochCache.getProposerAttesterAddressInSlot(slotNumber); if (expectedProposer !== undefined && !proposer.equals(expectedProposer)) { @@ -86,15 +61,6 @@ export abstract class ProposalValidator tx.validateTxHash()) ?? [])).every(v => v)) { - this.logger.warn(`Penalizing peer for invalid tx hashes in proposal`, { - proposer, - slotNumber, - }); - return { result: 'reject', severity: PeerErrorSeverity.LowToleranceError }; - } - return { result: 'accept' }; } catch (e) { if (e instanceof NoCommitteeError) { @@ -103,4 +69,47 @@ export abstract class ProposalValidator { + // Transactions permitted check + const embeddedTxCount = proposal.txs?.length ?? 0; + if (!this.txsPermitted && (proposal.txHashes.length > 0 || embeddedTxCount > 0)) { + this.logger.warn( + `Penalizing peer for proposal with ${proposal.txHashes.length} transaction(s) when transactions are not permitted`, + ); + return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; + } + + // Max txs per block check + if (this.maxTxsPerBlock !== undefined && proposal.txHashes.length > this.maxTxsPerBlock) { + this.logger.warn( + `Penalizing peer for proposal with ${proposal.txHashes.length} transaction(s) when max is ${this.maxTxsPerBlock}`, + ); + return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; + } + + // Embedded txs must be listed in txHashes + const hashSet = new Set(proposal.txHashes.map(h => h.toString())); + const missingTxHashes = + embeddedTxCount > 0 + ? proposal.txs!.filter(tx => !hashSet.has(tx.getTxHash().toString())).map(tx => tx.getTxHash().toString()) + : []; + if (embeddedTxCount > 0 && missingTxHashes.length > 0) { + this.logger.warn('Penalizing peer for embedded transaction(s) not included in txHashes', { + embeddedTxCount, + txHashesLength: proposal.txHashes.length, + missingTxHashes, + }); + return { result: 'reject', severity: PeerErrorSeverity.MidToleranceError }; + } + + // Validate tx hashes for all txs embedded in the proposal + if (!(await Promise.all(proposal.txs?.map(tx => tx.validateTxHash()) ?? [])).every(v => v)) { + this.logger.warn(`Penalizing peer for invalid tx hashes in proposal`); + return { result: 'reject', severity: PeerErrorSeverity.LowToleranceError }; + } + + return { result: 'accept' }; + } } diff --git a/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator_test_suite.ts b/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator_test_suite.ts deleted file mode 100644 index ec12ec3442f6..000000000000 --- a/yarn-project/p2p/src/msg_validators/proposal_validator/proposal_validator_test_suite.ts +++ /dev/null @@ -1,379 +0,0 @@ -import type { EpochCacheInterface } from '@aztec/epoch-cache'; -import { NoCommitteeError } from '@aztec/ethereum/contracts'; -import type { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; -import type { EthAddress } from '@aztec/foundation/eth-address'; -import { - type BlockProposal, - type CheckpointProposal, - PeerErrorSeverity, - type ValidationResult, -} from '@aztec/stdlib/p2p'; -import type { TxHash } from '@aztec/stdlib/tx'; - -import { jest } from '@jest/globals'; -import type { MockProxy } from 'jest-mock-extended'; - -export interface ProposalValidatorTestParams { - validatorFactory: ( - epochCache: EpochCacheInterface, - opts: { txsPermitted: boolean; maxTxsPerBlock?: number }, - ) => { validate: (proposal: TProposal) => Promise }; - makeProposal: (options?: any) => Promise; - makeHeader: (epochNumber: number | bigint, slotNumber: number | bigint, blockNumber: number | bigint) => any; - getSigner: () => Secp256k1Signer; - getAddress: (signer?: Secp256k1Signer) => EthAddress; - getSlot: (slot: number | bigint) => any; - getTxHashes: (n: number) => TxHash[]; - getTxs: () => any[]; - epochCacheMock: () => MockProxy; -} - -export function sharedProposalValidatorTests( - params: ProposalValidatorTestParams, -) { - const { validatorFactory, makeProposal, makeHeader, getSigner, getAddress, getSlot, getTxHashes, epochCacheMock } = - params; - - describe('shared proposal validation logic', () => { - let epochCache: MockProxy; - let validator: { validate: (proposal: TProposal) => Promise }; - const previousSlot = getSlot(99); - const currentSlot = getSlot(100); - const nextSlot = getSlot(101); - - function mockGetProposer(currentProposer: EthAddress, nextProposer: EthAddress, previousProposer?: EthAddress) { - epochCache.getProposerAttesterAddressInSlot.mockImplementation(slot => { - if (slot === currentSlot) { - return Promise.resolve(currentProposer); - } - if (slot === nextSlot) { - return Promise.resolve(nextProposer); - } - if (slot === previousSlot && previousProposer) { - return Promise.resolve(previousProposer); - } - throw new Error('Unexpected argument'); - }); - } - - beforeEach(() => { - epochCache = epochCacheMock(); - validator = validatorFactory(epochCache, { txsPermitted: true, maxTxsPerBlock: undefined }); - epochCache.getCurrentAndNextSlot.mockReturnValue({ - currentSlot: currentSlot, - nextSlot: nextSlot, - }); - }); - - it('returns high tolerance error if slot number is not current or next slot (outside clock tolerance)', async () => { - const header = makeHeader(1, 99, 99); - const mockProposal = await makeProposal({ blockHeader: header, lastBlockHeader: header }); - - // Mock getEpochAndSlotNow to return time OUTSIDE clock tolerance (1000ms elapsed) - epochCache.getEpochAndSlotNow.mockReturnValue({ - epoch: 1 as any, - slot: currentSlot, - ts: 1000n, // slot started at 1000 seconds - nowMs: 1001000n, // 1000ms elapsed, outside 500ms tolerance - }); - - epochCache.getProposerAttesterAddressInSlot.mockResolvedValue(getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.HighToleranceError }); - - // Should not try to resolve proposers if base validation fails - expect(epochCache.getProposerAttesterAddressInSlot).not.toHaveBeenCalled(); - }); - - it('returns ignore if previous slot proposal is within clock tolerance', async () => { - const previousProposer = getSigner(); - const header = makeHeader(1, 99, 99); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: previousProposer, - }); - - // Mock getEpochAndSlotNow to return time WITHIN clock tolerance (100ms elapsed) - epochCache.getEpochAndSlotNow.mockReturnValue({ - epoch: 1 as any, - slot: currentSlot, - ts: 1000n, // slot started at 1000 seconds - nowMs: 1000100n, // 100ms elapsed, within 500ms tolerance - }); - - mockGetProposer(getAddress(), getAddress(), getAddress(previousProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'ignore' }); - }); - - it('returns mid tolerance error if proposal has invalid signature', async () => { - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - }); - - // Override getSender to return undefined (invalid signature) - jest.spyOn(mockProposal as any, 'getSender').mockReturnValue(undefined); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - - // Should not try to resolve proposer if signature is invalid - expect(epochCache.getProposerAttesterAddressInSlot).not.toHaveBeenCalled(); - }); - - it('returns mid tolerance error if proposer is not current proposer for current slot', async () => { - const currentProposer = getSigner(); - const nextProposer = getSigner(); - const invalidProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: invalidProposer, - }); - - mockGetProposer(getAddress(currentProposer), getAddress(nextProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('returns mid tolerance error if proposer is not next proposer for next slot', async () => { - const currentProposer = getSigner(); - const nextProposer = getSigner(); - const invalidProposer = getSigner(); - const header = makeHeader(1, 101, 101); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: invalidProposer, - }); - - mockGetProposer(getAddress(currentProposer), getAddress(nextProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('returns mid tolerance error if proposer is current proposer but proposal is for next slot', async () => { - const currentProposer = getSigner(); - const nextProposer = getSigner(); - const header = makeHeader(1, 101, 101); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - }); - - mockGetProposer(getAddress(currentProposer), getAddress(nextProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('accepts proposal when proposer is undefined (open committee)', async () => { - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - }); - - epochCache.getProposerAttesterAddressInSlot.mockResolvedValue(undefined); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - - it('returns low tolerance error when getProposerAttesterAddressInSlot throws NoCommitteeError', async () => { - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - }); - - epochCache.getProposerAttesterAddressInSlot.mockRejectedValue(new NoCommitteeError()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.LowToleranceError }); - }); - - it('returns undefined if proposal is valid for current slot and proposer', async () => { - const currentProposer = getSigner(); - const nextProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - }); - - mockGetProposer(getAddress(currentProposer), getAddress(nextProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - - it('returns undefined if proposal is valid for next slot and proposer', async () => { - const currentProposer = getSigner(); - const nextProposer = getSigner(); - const header = makeHeader(1, 101, 101); - const mockProposal = await makeProposal({ blockHeader: header, lastBlockHeader: header, signer: nextProposer }); - - mockGetProposer(getAddress(currentProposer), getAddress(nextProposer)); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - - describe('transaction permission validation', () => { - it('returns mid tolerance error if txs not permitted and proposal contains txHashes', async () => { - const currentProposer = getSigner(); - const validatorWithTxsDisabled = validatorFactory(epochCache, { - txsPermitted: false, - maxTxsPerBlock: undefined, - }); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(2), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validatorWithTxsDisabled.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('returns undefined if txs not permitted but proposal has no txHashes', async () => { - const currentProposer = getSigner(); - const validatorWithTxsDisabled = validatorFactory(epochCache, { - txsPermitted: false, - maxTxsPerBlock: undefined, - }); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(0), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validatorWithTxsDisabled.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - - it('returns undefined if txs permitted and proposal contains txHashes', async () => { - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(2), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - }); - - describe('embedded tx validation', () => { - it('returns mid tolerance error if embedded txs are not listed in txHashes', async () => { - const currentProposer = getSigner(); - const txHashes = getTxHashes(2); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes, - }); - - // Create a fake tx whose hash is NOT in txHashes - const fakeTxHash = getTxHashes(1)[0]; - const fakeTx = { getTxHash: () => fakeTxHash, validateTxHash: () => Promise.resolve(true) }; - Object.defineProperty(mockProposal, 'txs', { get: () => [fakeTx], configurable: true }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('returns low tolerance error if embedded tx has invalid tx hash', async () => { - const currentProposer = getSigner(); - const txHashes = getTxHashes(2); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes, - }); - - // Create a fake tx whose hash IS in txHashes but validateTxHash returns false - const fakeTx = { getTxHash: () => txHashes[0], validateTxHash: () => Promise.resolve(false) }; - Object.defineProperty(mockProposal, 'txs', { get: () => [fakeTx], configurable: true }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.LowToleranceError }); - }); - }); - - describe('maxTxsPerBlock validation', () => { - it('rejects proposal when txHashes exceed maxTxsPerBlock', async () => { - const validatorWithMaxTxs = validatorFactory(epochCache, { txsPermitted: true, maxTxsPerBlock: 2 }); - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(3), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validatorWithMaxTxs.validate(mockProposal); - expect(result).toEqual({ result: 'reject', severity: PeerErrorSeverity.MidToleranceError }); - }); - - it('accepts proposal when txHashes count equals maxTxsPerBlock', async () => { - const validatorWithMaxTxs = validatorFactory(epochCache, { txsPermitted: true, maxTxsPerBlock: 2 }); - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(2), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validatorWithMaxTxs.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - - it('accepts proposal when maxTxsPerBlock is not set (unlimited)', async () => { - // Default validator has no maxTxsPerBlock - const currentProposer = getSigner(); - const header = makeHeader(1, 100, 100); - const mockProposal = await makeProposal({ - blockHeader: header, - lastBlockHeader: header, - signer: currentProposer, - txHashes: getTxHashes(10), - }); - - mockGetProposer(getAddress(currentProposer), getAddress()); - const result = await validator.validate(mockProposal); - expect(result).toEqual({ result: 'accept' }); - }); - }); - }); -} diff --git a/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts b/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts index fff36a91b996..fdf4d679510f 100644 --- a/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts +++ b/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts @@ -101,23 +101,6 @@ export class CheckpointProposal extends Gossipable { return this.checkpointHeader.slotNumber; } - get blockNumber(): BlockNumber { - if (!this.lastBlock) { - throw new Error('Cannot get blockNumber without lastBlock'); - } - return this.lastBlock.blockHeader.getBlockNumber(); - } - - /** Convenience getter for txHashes from lastBlock */ - get txHashes(): TxHash[] { - return this.lastBlock?.txHashes ?? []; - } - - /** Convenience getter for txs from lastBlock */ - get txs(): Tx[] | undefined { - return this.lastBlock?.signedTxs?.txs; - } - /** * Extract a BlockProposal from the last block info. * Uses inHash from checkpointHeader.contentCommitment.inHash diff --git a/yarn-project/validator-client/src/duties/validation_service.ts b/yarn-project/validator-client/src/duties/validation_service.ts index 99d4740b78c7..314969b4ce88 100644 --- a/yarn-project/validator-client/src/duties/validation_service.ts +++ b/yarn-project/validator-client/src/duties/validation_service.ts @@ -150,16 +150,10 @@ export class ValidationService { ); // TODO(spy/ha): Use checkpointNumber instead of blockNumber once CheckpointHeader includes it. - // Currently using lastBlock.blockNumber as a proxy for checkpoint identification in HA signing. + // CheckpointProposalCore doesn't have lastBlock info, so use 0 as a proxy. // blockNumber is NOT used for the primary key so it's safe to use here. // See CheckpointHeader TODO and SigningContext types documentation. - let blockNumber: BlockNumber; - try { - blockNumber = proposal.blockNumber; - } catch { - // Checkpoint proposal may not have lastBlock, use 0 as fallback - blockNumber = BlockNumber(0); - } + const blockNumber = BlockNumber(0); const context: SigningContext = { slot: proposal.slotNumber, blockNumber, diff --git a/yarn-project/validator-client/src/validator.ts b/yarn-project/validator-client/src/validator.ts index 76f97ed0d86b..f60f2277eac7 100644 --- a/yarn-project/validator-client/src/validator.ts +++ b/yarn-project/validator-client/src/validator.ts @@ -520,11 +520,9 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) slotNumber, archive: proposal.archive.toString(), proposer: proposer.toString(), - txCount: proposal.txHashes.length, }; this.log.info(`Received checkpoint proposal for slot ${slotNumber}`, { ...proposalInfo, - txHashes: proposal.txHashes.map(t => t.toString()), fishermanMode: this.config.fishermanMode || false, });