From c75828e127d95b194a254f7754ef0ab22c4387df Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Wed, 4 Mar 2026 10:23:03 -0300 Subject: [PATCH] fix(p2p): remove default block handler in favor of block handler The default block handler was limited in that it failed to handle block proposals out of order, missed validations, missed proper timeouts, etc. It's not removed in favor of the full block proposal handler, which is installed if the validator client is not set. --- .../aztec-node/src/aztec-node/server.ts | 32 ++++---- yarn-project/p2p/src/client/p2p_client.ts | 22 ------ .../src/block_proposal_handler.ts | 78 +++++++++++-------- .../validator-client/src/validator.test.ts | 2 + 4 files changed, 64 insertions(+), 70 deletions(-) diff --git a/yarn-project/aztec-node/src/aztec-node/server.ts b/yarn-project/aztec-node/src/aztec-node/server.ts index 5d8bc7bd69d8..92d557749def 100644 --- a/yarn-project/aztec-node/src/aztec-node/server.ts +++ b/yarn-project/aztec-node/src/aztec-node/server.ts @@ -380,22 +380,24 @@ export class AztecNodeService implements AztecNode, AztecNodeAdmin, Traceable { await validatorClient.registerHandlers(); } } + } - // If there's no validator client but alwaysReexecuteBlockProposals is enabled, - // create a BlockProposalHandler to reexecute block proposals for monitoring - if (!validatorClient && config.alwaysReexecuteBlockProposals) { - log.info('Setting up block proposal reexecution for monitoring'); - createBlockProposalHandler(config, { - checkpointsBuilder: validatorCheckpointsBuilder, - worldState: worldStateSynchronizer, - epochCache, - blockSource: archiver, - l1ToL2MessageSource: archiver, - p2pClient, - dateProvider, - telemetry, - }).registerForReexecution(p2pClient); - } + // If there's no validator client, create a BlockProposalHandler to handle block proposals + // for monitoring or reexecution. Reexecution (default) allows us to follow the pending chain, + // while non-reexecution is used for validating the proposals and collecting their txs. + if (!validatorClient) { + const reexecute = !!config.alwaysReexecuteBlockProposals; + log.info(`Setting up block proposal handler` + (reexecute ? ' with reexecution of proposals' : '')); + createBlockProposalHandler(config, { + checkpointsBuilder: validatorCheckpointsBuilder, + worldState: worldStateSynchronizer, + epochCache, + blockSource: archiver, + l1ToL2MessageSource: archiver, + p2pClient, + dateProvider, + telemetry, + }).register(p2pClient, reexecute); } // Start world state and wait for it to sync to the archiver. diff --git a/yarn-project/p2p/src/client/p2p_client.ts b/yarn-project/p2p/src/client/p2p_client.ts index 7996594ff9cb..a6be4b34799a 100644 --- a/yarn-project/p2p/src/client/p2p_client.ts +++ b/yarn-project/p2p/src/client/p2p_client.ts @@ -18,7 +18,6 @@ import { type L2TipsStore, } from '@aztec/stdlib/block'; import type { ContractDataSource } from '@aztec/stdlib/contract'; -import { getTimestampForSlot } from '@aztec/stdlib/epoch-helpers'; import { type PeerInfo, tryStop } from '@aztec/stdlib/interfaces/server'; import { type BlockProposal, CheckpointAttestation, type CheckpointProposal, type TopicType } from '@aztec/stdlib/p2p'; import type { BlockHeader, Tx, TxHash } from '@aztec/stdlib/tx'; @@ -111,27 +110,6 @@ export class P2PClient extends WithTracer implements P2P { this.telemetry, ); - // Default to collecting all txs when we see a valid proposal - // This can be overridden by the validator client to validate, and it will call getTxsForBlockProposal on its own - // Note: Validators do NOT attest to individual blocks - attestations are only for checkpoint proposals. - // TODO(palla/txs): We should not trigger a request for txs on a proposal before fully validating it. We need to bring - // validator-client code into here so we can validate a proposal is reasonable. - this.registerBlockProposalHandler(async (block, sender) => { - this.log.debug(`Received block proposal from ${sender.toString()}`); - // TODO(palla/txs): Need to subtract validatorReexecuteDeadlineMs from this deadline (see ValidatorClient.getReexecutionDeadline) - const constants = this.txCollection.getConstants(); - const nextSlotTimestampSeconds = Number(getTimestampForSlot(SlotNumber(block.slotNumber + 1), constants)); - const deadline = new Date(nextSlotTimestampSeconds * 1000); - const parentBlock = await this.l2BlockSource.getBlockHeaderByArchive(block.blockHeader.lastArchive.root); - if (!parentBlock) { - this.log.debug(`Cannot collect txs for proposal as parent block not found`); - return false; - } - const blockNumber = BlockNumber(parentBlock.getBlockNumber() + 1); - await this.txProvider.getTxsForBlockProposal(block, blockNumber, { pinnedPeer: sender, deadline }); - return true; - }); - this.l2Tips = new L2TipsKVStore(store, 'p2p_client'); this.synchedLatestSlot = store.openSingleton('p2p_pool_last_l2_slot'); } diff --git a/yarn-project/validator-client/src/block_proposal_handler.ts b/yarn-project/validator-client/src/block_proposal_handler.ts index 0c8812aee9a8..3aac5ec77b25 100644 --- a/yarn-project/validator-client/src/block_proposal_handler.ts +++ b/yarn-project/validator-client/src/block_proposal_handler.ts @@ -1,6 +1,7 @@ import { INITIAL_L2_BLOCK_NUM } from '@aztec/constants'; import type { EpochCache } from '@aztec/epoch-cache'; import { BlockNumber, CheckpointNumber, SlotNumber } from '@aztec/foundation/branded-types'; +import { pick } from '@aztec/foundation/collection'; import { Fr } from '@aztec/foundation/curves/bn254'; import { TimeoutError } from '@aztec/foundation/error'; import { createLogger } from '@aztec/foundation/log'; @@ -87,25 +88,28 @@ export class BlockProposalHandler { this.tracer = telemetry.getTracer('BlockProposalHandler'); } - registerForReexecution(p2pClient: P2P): BlockProposalHandler { - // Non-validator handler that re-executes for monitoring but does not attest. + register(p2pClient: P2P, shouldReexecute: boolean): BlockProposalHandler { + // Non-validator handler that processes or re-executes for monitoring but does not attest. // Returns boolean indicating whether the proposal was valid. const handler = async (proposal: BlockProposal, proposalSender: PeerId): Promise => { try { - const result = await this.handleBlockProposal(proposal, proposalSender, true); + const { slotNumber, blockNumber } = proposal; + const result = await this.handleBlockProposal(proposal, proposalSender, shouldReexecute); if (result.isValid) { - this.log.info(`Non-validator reexecution completed for slot ${proposal.slotNumber}`, { + this.log.info(`Non-validator block proposal ${blockNumber} at slot ${slotNumber} handled`, { blockNumber: result.blockNumber, + slotNumber, reexecutionTimeMs: result.reexecutionResult?.reexecutionTimeMs, totalManaUsed: result.reexecutionResult?.totalManaUsed, numTxs: result.reexecutionResult?.block?.body?.txEffects?.length ?? 0, + reexecuted: shouldReexecute, }); return true; } else { - this.log.warn(`Non-validator reexecution failed for slot ${proposal.slotNumber}`, { - blockNumber: result.blockNumber, - reason: result.reason, - }); + this.log.warn( + `Non-validator block proposal ${blockNumber} at slot ${slotNumber} failed processing with ${result.reason}`, + { blockNumber: result.blockNumber, slotNumber, reason: result.reason }, + ); return false; } } catch (error) { @@ -184,6 +188,15 @@ export class BlockProposalHandler { deadline: this.getReexecutionDeadline(slotNumber, config), }); + // If reexecution is disabled, bail. We are just interested in triggering tx collection. + if (!shouldReexecute) { + this.log.info( + `Received valid block ${blockNumber} proposal at index ${proposal.indexWithinCheckpoint} on slot ${slotNumber}`, + proposalInfo, + ); + return { isValid: true, blockNumber }; + } + // Compute the checkpoint number for this block and validate checkpoint consistency const checkpointResult = this.computeCheckpointNumber(proposal, parentBlock, proposalInfo); if (checkpointResult.reason) { @@ -210,30 +223,28 @@ export class BlockProposalHandler { return { isValid: false, blockNumber, reason: 'txs_not_available' }; } + // Collect the out hashes of all the checkpoints before this one in the same epoch + const epoch = getEpochAtSlot(slotNumber, this.epochCache.getL1Constants()); + const previousCheckpointOutHashes = (await this.blockSource.getCheckpointsDataForEpoch(epoch)) + .filter(c => c.checkpointNumber < checkpointNumber) + .map(c => c.checkpointOutHash); + // Try re-executing the transactions in the proposal if needed let reexecutionResult; - if (shouldReexecute) { - // Collect the out hashes of all the checkpoints before this one in the same epoch - const epoch = getEpochAtSlot(slotNumber, this.epochCache.getL1Constants()); - const previousCheckpointOutHashes = (await this.blockSource.getCheckpointsDataForEpoch(epoch)) - .filter(c => c.checkpointNumber < checkpointNumber) - .map(c => c.checkpointOutHash); - - try { - this.log.verbose(`Re-executing transactions in the proposal`, proposalInfo); - reexecutionResult = await this.reexecuteTransactions( - proposal, - blockNumber, - checkpointNumber, - txs, - l1ToL2Messages, - previousCheckpointOutHashes, - ); - } catch (error) { - this.log.error(`Error reexecuting txs while processing block proposal`, error, proposalInfo); - const reason = this.getReexecuteFailureReason(error); - return { isValid: false, blockNumber, reason, reexecutionResult }; - } + try { + this.log.verbose(`Re-executing transactions in the proposal`, proposalInfo); + reexecutionResult = await this.reexecuteTransactions( + proposal, + blockNumber, + checkpointNumber, + txs, + l1ToL2Messages, + previousCheckpointOutHashes, + ); + } catch (error) { + this.log.error(`Error reexecuting txs while processing block proposal`, error, proposalInfo); + const reason = this.getReexecuteFailureReason(error); + return { isValid: false, blockNumber, reason, reexecutionResult }; } // If we succeeded, push this block into the archiver (unless disabled) @@ -242,8 +253,8 @@ export class BlockProposalHandler { } this.log.info( - `Successfully processed block ${blockNumber} proposal at index ${proposal.indexWithinCheckpoint} on slot ${slotNumber}`, - proposalInfo, + `Successfully re-executed block ${blockNumber} proposal at index ${proposal.indexWithinCheckpoint} on slot ${slotNumber}`, + { ...proposalInfo, ...pick(reexecutionResult, 'reexecutionTimeMs', 'totalManaUsed') }, ); return { isValid: true, blockNumber, reexecutionResult }; @@ -488,10 +499,11 @@ export class BlockProposalHandler { const { block, failedTxs } = result; const numFailedTxs = failedTxs.length; - this.log.verbose(`Transaction re-execution complete for slot ${slot}`, { + this.log.verbose(`Block proposal ${blockNumber} at slot ${slot} transaction re-execution complete`, { numFailedTxs, numProposalTxs: txHashes.length, numProcessedTxs: block.body.txEffects.length, + blockNumber, slot, }); diff --git a/yarn-project/validator-client/src/validator.test.ts b/yarn-project/validator-client/src/validator.test.ts index 7d9c4b975288..da7c38bf6d70 100644 --- a/yarn-project/validator-client/src/validator.test.ts +++ b/yarn-project/validator-client/src/validator.test.ts @@ -618,6 +618,7 @@ describe('ValidatorClient', () => { }); it('should return false if the transactions are not available', async () => { + enableReexecution(); txProvider.getTxsForBlockProposal.mockImplementation(proposal => Promise.resolve({ txs: [], @@ -694,6 +695,7 @@ describe('ValidatorClient', () => { // L1 messages for the checkpoint) will catch it. it('should return false if global variables do not match parent for non-first block in checkpoint', async () => { + enableReexecution(); // Create a proposal with indexWithinCheckpoint > 0 (non-first block in checkpoint) const parentSlotNumber = 100; const parentBlockNumber = 10;