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 e9c89e3d1880..ae84e24204d7 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 21b4d8d6f80e..e6690396f531 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, });