diff --git a/packages/beacon-node/src/chain/blocks/index.ts b/packages/beacon-node/src/chain/blocks/index.ts index df47824824de..4d0b777670a3 100644 --- a/packages/beacon-node/src/chain/blocks/index.ts +++ b/packages/beacon-node/src/chain/blocks/index.ts @@ -63,10 +63,11 @@ export async function processBlocks( } try { - const {relevantBlocks, parentSlots} = verifyBlocksSanityChecks(chain, blocks, opts); + const {relevantBlocks, parentSlots, parentBlock} = verifyBlocksSanityChecks(chain, blocks, opts); // No relevant blocks, skip verifyBlocksInEpoch() - if (relevantBlocks.length === 0) { + if (relevantBlocks.length === 0 || parentBlock === null) { + // parentBlock can only be null if relevantBlocks are empty return; } @@ -74,6 +75,7 @@ export async function processBlocks( // states in the state cache through regen. const {postStates, executionStatuses, proposerBalanceDeltas} = await verifyBlocksInEpoch( chain, + parentBlock, relevantBlocks, opts ); diff --git a/packages/beacon-node/src/chain/blocks/verifyBlock.ts b/packages/beacon-node/src/chain/blocks/verifyBlock.ts index 41babad566ea..580e10887e80 100644 --- a/packages/beacon-node/src/chain/blocks/verifyBlock.ts +++ b/packages/beacon-node/src/chain/blocks/verifyBlock.ts @@ -1,7 +1,7 @@ import {CachedBeaconStateAllForks, computeEpochAtSlot} from "@lodestar/state-transition"; import {allForks, bellatrix} from "@lodestar/types"; import {toHexString} from "@chainsafe/ssz"; -import {IForkChoice, ExecutionStatus} from "@lodestar/fork-choice"; +import {IForkChoice, ExecutionStatus, ProtoBlock} from "@lodestar/fork-choice"; import {IChainForkConfig} from "@lodestar/config"; import {ILogger} from "@lodestar/utils"; import {IMetrics} from "../../metrics/index.js"; @@ -43,6 +43,7 @@ export type VerifyBlockModules = { */ export async function verifyBlocksInEpoch( chain: VerifyBlockModules, + parentBlock: ProtoBlock, blocks: allForks.SignedBeaconBlock[], opts: BlockProcessOpts & ImportBlockOpts ): Promise<{ @@ -88,7 +89,7 @@ export async function verifyBlocksInEpoch( verifyBlocksSignatures(chain, preState0, blocks, opts), // Execution payloads - verifyBlocksExecutionPayload(chain, blocks, preState0, abortController.signal, opts), + verifyBlocksExecutionPayload(chain, parentBlock, blocks, preState0, abortController.signal, opts), ]); if (mergeBlockFound !== null) { diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts index ffd1f946183a..4c5d65fa2c7d 100644 --- a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts +++ b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts @@ -7,7 +7,7 @@ import { } from "@lodestar/state-transition"; import {bellatrix, allForks} from "@lodestar/types"; import {toHexString} from "@chainsafe/ssz"; -import {IForkChoice, ExecutionStatus, assertValidTerminalPowBlock} from "@lodestar/fork-choice"; +import {IForkChoice, ExecutionStatus, assertValidTerminalPowBlock, ProtoBlock} from "@lodestar/fork-choice"; import {IChainForkConfig} from "@lodestar/config"; import {ErrorAborted, ILogger} from "@lodestar/utils"; import {IExecutionEngine} from "../../execution/engine/index.js"; @@ -33,6 +33,7 @@ type VerifyBlockModules = { */ export async function verifyBlocksExecutionPayload( chain: VerifyBlockModules, + parentBlock: ProtoBlock, blocks: allForks.SignedBeaconBlock[], preState0: CachedBeaconStateAllForks, signal: AbortSignal, @@ -41,6 +42,76 @@ export async function verifyBlocksExecutionPayload( const executionStatuses: ExecutionStatus[] = []; let mergeBlockFound: bellatrix.BeaconBlock | null = null; + // Error in the same way as verifyBlocksSanityChecks if empty blocks + if (blocks.length === 0) { + throw Error("Empty partiallyVerifiedBlocks"); + } + + // For a block with SYNCING status (called optimistic block), it's okay to import with + // SYNCING status as EL could switch into syncing + // + // 1. On intial startup/restart + // 2. When some reorg might have occured and EL doesn't has a parent root + // (observed on devnets) + // 3. Because of some unavailable (and potentially invalid) root but there is no way + // of knowing if this is invalid/unavailable. For unavailable block, some proposer + // will (sooner or later) build on the available parent head which will + // eventually win in fork-choice as other validators vote on VALID blocks. + // + // Once EL catches up again and respond VALID, the fork choice will be updated which + // will either validate or prune invalid blocks + // + // We need to track and keep updating if its safe to optimistically import these blocks. + // The following is how we determine for a block if its safe: + // + // (but we need to modify this check for this segment of blocks because it checks if the + // parent of any block imported in forkchoice is post-merge and currently we could only + // have blocks[0]'s parent imported in the chain as this is no longer one by one verify + + // import.) + // + // + // When to import such blocks: + // From: https://github.com/ethereum/consensus-specs/pull/2844 + // A block MUST NOT be optimistically imported, unless either of the following + // conditions are met: + // + // 1. Parent of the block has execution + // + // Since with the sync optimizations, the previous block might not have been in the + // forkChoice yet, so the below check could fail for safeSlotsToImportOptimistically + // + // Luckily, we can depend on the preState0 to see if we are already post merge w.r.t + // the blocks we are importing. + // + // Or in other words if + // - block status is syncing + // - and we are not in a post merge world and is parent is not optimistically safe + // - and we are syncing close to the chain head i.e. clock slot + // - and parent is optimistically safe + // + // then throw error + // + // + // - if we haven't yet imported a post merge ancestor in forkchoice i.e. + // - and we are syncing close to the clockSlot, i.e. merge Transition could be underway + // + // + // 2. The current slot (as per the system clock) is at least + // SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the slot of the block being + // imported. + // This means that the merge transition could be underway and we can't afford to import + // a block which is not fully validated as it could affect liveliness of the network. + // + // + // For this segment of blocks: + // We are optimistically safe with respec to this entire block segment if: + // - all the blocks are way behind the current slot + // - or we have already imported a post-merge parent of first block of this chain in forkchoice + const lastBlock = blocks[blocks.length - 1]; + let isOptimisticallySafe = + parentBlock.executionStatus !== ExecutionStatus.PreMerge || + lastBlock.message.slot + opts.safeSlotsToImportOptimistically < chain.clock.currentSlot; + for (const block of blocks) { // If blocks are invalid in consensus the main promise could resolve before this loop ends. // In that case stop sending blocks to execution engine @@ -48,7 +119,13 @@ export async function verifyBlocksExecutionPayload( throw new ErrorAborted("verifyBlockExecutionPayloads"); } - const {executionStatus} = await verifyBlockExecutionPayload(chain, block, preState0, opts); + const {executionStatus} = await verifyBlockExecutionPayload(chain, block, preState0, opts, isOptimisticallySafe); + // It becomes optimistically safe for following blocks if a post-merge block is deemed fit + // for import. If it would not have been safe verifyBlockExecutionPayload would throw error + // and we would not be here. + if (executionStatus !== ExecutionStatus.PreMerge) { + isOptimisticallySafe = true; + } executionStatuses.push(executionStatus); const isMergeTransitionBlock = @@ -122,7 +199,8 @@ export async function verifyBlockExecutionPayload( chain: VerifyBlockModules, block: allForks.SignedBeaconBlock, preState0: CachedBeaconStateAllForks, - opts: BlockProcessOpts + opts: BlockProcessOpts, + isOptimisticallySafe: boolean ): Promise<{executionStatus: ExecutionStatus}> { /** Not null if execution is enabled */ const executionPayloadEnabled = @@ -168,43 +246,17 @@ export async function verifyBlockExecutionPayload( // Accepted and Syncing have the same treatment, as final validation of block is pending case ExecutePayloadStatus.ACCEPTED: case ExecutePayloadStatus.SYNCING: { - // It's okay to ignore SYNCING status as EL could switch into syncing - // 1. On intial startup/restart - // 2. When some reorg might have occured and EL doesn't has a parent root - // (observed on devnets) - // 3. Because of some unavailable (and potentially invalid) root but there is no way - // of knowing if this is invalid/unavailable. For unavailable block, some proposer - // will (sooner or later) build on the available parent head which will - // eventually win in fork-choice as other validators vote on VALID blocks. - // Once EL catches up again and respond VALID, the fork choice will be updated which - // will either validate or prune invalid blocks - // - // When to import such blocks: - // From: https://github.com/ethereum/consensus-specs/pull/2844 - // A block MUST NOT be optimistically imported, unless either of the following - // conditions are met: - // - // 1. Parent of the block has execution - // 2. The justified checkpoint has execution enabled - // 3. The current slot (as per the system clock) is at least - // SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the slot of the block being - // imported. - - const parentRoot = toHexString(block.message.parentRoot); - const parentBlock = chain.forkChoice.getBlockHex(parentRoot); - const justifiedBlock = chain.forkChoice.getJustifiedBlock(); - + // Check if the entire segment was deemed safe or, this block specifically itself if not in + // the safeSlotsToImportOptimistically window of current slot, then we can import else + // we need to throw and not import his block if ( - !parentBlock || - // Following condition is the !(Not) of the safe import condition - (parentBlock.executionStatus === ExecutionStatus.PreMerge && - justifiedBlock.executionStatus === ExecutionStatus.PreMerge && - block.message.slot + opts.safeSlotsToImportOptimistically > chain.clock.currentSlot) + !isOptimisticallySafe && + block.message.slot + opts.safeSlotsToImportOptimistically >= chain.clock.currentSlot ) { throw new BlockError(block, { code: BlockErrorCode.EXECUTION_ENGINE_ERROR, execStatus: ExecutePayloadStatus.UNSAFE_OPTIMISTIC_STATUS, - errorMessage: `not safe to import ${execResult.status} payload within ${opts.safeSlotsToImportOptimistically} of currentSlot, status=${execResult.status}`, + errorMessage: `not safe to import ${execResult.status} payload within ${opts.safeSlotsToImportOptimistically} of currentSlot`, }); } diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts index 4b3e70be8e02..1203845532e3 100644 --- a/packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts +++ b/packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts @@ -1,6 +1,6 @@ import {computeStartSlotAtEpoch} from "@lodestar/state-transition"; import {IChainForkConfig} from "@lodestar/config"; -import {IForkChoice} from "@lodestar/fork-choice"; +import {IForkChoice, ProtoBlock} from "@lodestar/fork-choice"; import {allForks, Slot} from "@lodestar/types"; import {toHexString} from "@lodestar/utils"; import {IBeaconClock} from "../clock/interface.js"; @@ -23,13 +23,14 @@ export function verifyBlocksSanityChecks( chain: {forkChoice: IForkChoice; clock: IBeaconClock; config: IChainForkConfig}, blocks: allForks.SignedBeaconBlock[], opts: ImportBlockOpts -): {relevantBlocks: allForks.SignedBeaconBlock[]; parentSlots: Slot[]} { +): {relevantBlocks: allForks.SignedBeaconBlock[]; parentSlots: Slot[]; parentBlock: ProtoBlock | null} { if (blocks.length === 0) { throw Error("Empty partiallyVerifiedBlocks"); } const relevantBlocks: allForks.SignedBeaconBlock[] = []; const parentSlots: Slot[] = []; + let parentBlock: ProtoBlock | null = null; for (const block of blocks) { const blockSlot = block.message.slot; @@ -57,16 +58,16 @@ export function verifyBlocksSanityChecks( let parentBlockSlot: Slot; - // When importing a block segment, only the first NON-IGNORED block must be known to the fork-choice. if (relevantBlocks.length > 0) { parentBlockSlot = relevantBlocks[relevantBlocks.length - 1].message.slot; } else { - // Parent is known to the fork-choice + // When importing a block segment, only the first NON-IGNORED block must be known to the fork-choice. const parentRoot = toHexString(block.message.parentRoot); - const parentBlock = chain.forkChoice.getBlockHex(parentRoot); + parentBlock = chain.forkChoice.getBlockHex(parentRoot); if (!parentBlock) { throw new BlockError(block, {code: BlockErrorCode.PARENT_UNKNOWN, parentRoot}); } else { + // Parent is known to the fork-choice parentBlockSlot = parentBlock.slot; } } @@ -95,5 +96,11 @@ export function verifyBlocksSanityChecks( parentSlots.push(parentBlockSlot); } - return {relevantBlocks, parentSlots}; + // Just assert to be over cautious and for purposes to be more explicit for someone + // going through the code segment + if (parentBlock === null && relevantBlocks.length > 0) { + throw Error(`Internal error, parentBlock should not be null for relevantBlocks=${relevantBlocks.length}`); + } + + return {relevantBlocks, parentSlots, parentBlock}; }