Fix the terminal validations of the merge block

packages/fork-choice/src/forkChoice/forkChoice.ts

@@ -19,7 +19,7 @@ import {ProtoArray} from "../protoArray/protoArray";
 
 import {IForkChoiceMetrics} from "../metrics";
 import {ForkChoiceError, ForkChoiceErrorCode, InvalidBlockCode, InvalidAttestationCode} from "./errors";
-import {IForkChoice, ILatestMessage, IQueuedAttestation, OnBlockPrecachedData} from "./interface";
+import {IForkChoice, ILatestMessage, IQueuedAttestation, OnBlockPrecachedData, PowBlockHex} from "./interface";
 import {IForkChoiceStore, CheckpointWithHex, toCheckpointWithHex} from "./store";
 
 /* eslint-disable max-len */
@@ -328,13 +328,15 @@ export class ForkChoice implements IForkChoice {
       });
     }
 
-    if (
-      preCachedData?.isMergeTransitionBlock ||
-      (bellatrix.isBellatrixStateType(state) &&
-        bellatrix.isBellatrixBlockBodyType(block.body) &&
-        bellatrix.isMergeTransitionBlock(state, block.body))
-    )
-      assertValidTerminalPowBlock(this.config, (block as unknown) as bellatrix.BeaconBlock, preCachedData);
+    // As per specs, we should be validating here the terminal conditions of
+    // the PoW if this were a merge transition block.
+    // (https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/fork-choice.md#on_block)
+    //
+    // However this check has been moved to the `verifyBlockStateTransition` in
+    // `packages/lodestar/src/chain/blocks/verifyBlock.ts` as:
+    //
+    //  1. Its prudent to fail fast and not try importing a block in forkChoice.
+    //  2. Also the data to run such a validation is readily available there.
 
     let shouldUpdateJustified = false;
     const {finalizedCheckpoint} = state;
@@ -1003,16 +1005,28 @@ export class ForkChoice implements IForkChoice {
   }
 }
 
-function assertValidTerminalPowBlock(
+/**
+ * This function checks the terminal pow conditions on the merge block as
+ * specified in the config either via TTD or TBH. This function is part of
+ * forkChoice because if the merge block was previously imported as syncing
+ * and the EL eventually signals it catching up via validateLatestHash
+ * the specs mandates validating terminal conditions on the previously
+ * imported merge block.
+ */
+export function assertValidTerminalPowBlock(
   config: IChainConfig,
   block: bellatrix.BeaconBlock,
-  preCachedData?: OnBlockPrecachedData
+  preCachedData: {
+    executionStatus: ExecutionStatus.Syncing | ExecutionStatus.Valid;
+    powBlock?: PowBlockHex | null;
+    powBlockParent?: PowBlockHex | null;
+  }
 ): void {
   if (!ssz.Root.equals(config.TERMINAL_BLOCK_HASH, ZERO_HASH)) {
     if (computeEpochAtSlot(block.slot) < config.TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH)
       throw Error(`Terminal block activation epoch ${config.TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH} not reached`);
 
-    // powBock.blockhash is hex, so we just pick the corresponding root
+    // powBock.blockHash is hex, so we just pick the corresponding root
     if (!ssz.Root.equals(block.body.executionPayload.parentHash, config.TERMINAL_BLOCK_HASH))
       throw new Error(
         `Invalid terminal block hash, expected: ${toHexString(config.TERMINAL_BLOCK_HASH)}, actual: ${toHexString(
@@ -1026,9 +1040,9 @@ function assertValidTerminalPowBlock(
     // syncing response in notifyNewPayload call while verifying
     if (preCachedData?.executionStatus === ExecutionStatus.Syncing) return;
 
-    const {powBlock, powBlockParent} = preCachedData || {};
+    const {powBlock, powBlockParent} = preCachedData;
     if (!powBlock) throw Error("onBlock preCachedData must include powBlock");
-    if (!powBlockParent) throw Error("onBlock preCachedData must include powBlock");
+    if (!powBlockParent) throw Error("onBlock preCachedData must include powBlockParent");
 
     const isTotalDifficultyReached = powBlock.totalDifficulty >= config.TERMINAL_TOTAL_DIFFICULTY;
     const isParentTotalDifficultyValid = powBlockParent.totalDifficulty < config.TERMINAL_TOTAL_DIFFICULTY;

packages/fork-choice/src/forkChoice/interface.ts

@@ -142,7 +142,7 @@ export interface IForkChoice {
 
 /** Same to the PowBlock but we want RootHex to work with forkchoice conveniently */
 export type PowBlockHex = {
-  blockhash: RootHex;
+  blockHash: RootHex;
   parentHash: RootHex;
   totalDifficulty: bigint;
 };
@@ -152,24 +152,6 @@ export type OnBlockPrecachedData = {
   justifiedBalances?: EffectiveBalanceIncrements;
   /** Time in seconds when the block was received */
   blockDelaySec: number;
-  /**
-   * POW chain block parent, from getPowBlock() `eth_getBlockByHash` JSON RPC endpoint
-   * ```ts
-   * powBlock = getPowBlock((block as bellatrix.BeaconBlock).body.executionPayload.parentHash)
-   * ```
-   */
-  powBlock?: PowBlockHex | null;
-  /**
-   * POW chain block's block parent, from getPowBlock() `eth_getBlockByHash` JSON RPC endpoint
-   * ```ts
-   * const powParent = getPowBlock(powBlock.parentHash);
-   * ```
-   */
-  powBlockParent?: PowBlockHex | null;
-  /**
-   * Optimistic sync fields
-   */
-  isMergeTransitionBlock?: boolean;
   executionStatus?: ExecutionStatus;
 };
 

packages/fork-choice/src/index.ts

@@ -1,7 +1,7 @@
 export {ProtoArray} from "./protoArray/protoArray";
 export {IProtoBlock, IProtoNode, ExecutionStatus} from "./protoArray/interface";
 
-export {ForkChoice} from "./forkChoice/forkChoice";
+export {ForkChoice, assertValidTerminalPowBlock} from "./forkChoice/forkChoice";
 export {
   IForkChoice,
   OnBlockPrecachedData,

packages/lodestar/src/chain/blocks/importBlock.ts

@@ -6,21 +6,13 @@ import {
   CachedBeaconStateAltair,
   computeStartSlotAtEpoch,
   getEffectiveBalanceIncrementsZeroInactive,
-  bellatrix,
   altair,
   computeEpochAtSlot,
 } from "@chainsafe/lodestar-beacon-state-transition";
-import {
-  IForkChoice,
-  OnBlockPrecachedData,
-  ExecutionStatus,
-  ForkChoiceError,
-  ForkChoiceErrorCode,
-} from "@chainsafe/lodestar-fork-choice";
+import {IForkChoice, OnBlockPrecachedData, ForkChoiceError, ForkChoiceErrorCode} from "@chainsafe/lodestar-fork-choice";
 import {ILogger} from "@chainsafe/lodestar-utils";
 import {IChainForkConfig} from "@chainsafe/lodestar-config";
 import {IMetrics} from "../../metrics";
-import {IEth1ForBlockProduction} from "../../eth1";
 import {IExecutionEngine} from "../../executionEngine";
 import {IBeaconDb} from "../../db";
 import {ZERO_HASH_HEX} from "../../constants";
@@ -40,7 +32,6 @@ const FORK_CHOICE_ATT_EPOCH_LIMIT = 1;
 
 export type ImportBlockModules = {
   db: IBeaconDb;
-  eth1: IEth1ForBlockProduction;
   forkChoice: IForkChoice;
   stateCache: StateContextCache;
   checkpointStateCache: CheckpointStateCache;
@@ -102,25 +93,6 @@ export async function importBlock(chain: ImportBlockModules, fullyVerifiedBlock:
     onBlockPrecachedData.justifiedBalances = getEffectiveBalanceIncrementsZeroInactive(state);
   }
 
-  if (
-    bellatrix.isBellatrixStateType(postState) &&
-    bellatrix.isBellatrixBlockBodyType(block.message.body) &&
-    bellatrix.isMergeTransitionBlock(postState, block.message.body)
-  ) {
-    // pow_block = get_pow_block(block.body.execution_payload.parent_hash)
-    const powBlockRootHex = toHexString(block.message.body.executionPayload.parentHash);
-    const powBlock = await chain.eth1.getPowBlock(powBlockRootHex);
-    if (!powBlock && executionStatus !== ExecutionStatus.Syncing)
-      throw Error(`merge block parent POW block not found ${powBlockRootHex}`);
-    // pow_parent = get_pow_block(pow_block.parent_hash)
-    const powBlockParent = powBlock && (await chain.eth1.getPowBlock(powBlock.parentHash));
-    if (powBlock && !powBlockParent)
-      throw Error(`merge block parent's parent POW block not found ${powBlock.parentHash}`);
-    onBlockPrecachedData.powBlock = powBlock;
-    onBlockPrecachedData.powBlockParent = powBlockParent;
-    onBlockPrecachedData.isMergeTransitionBlock = true;
-  }
-
   const prevFinalizedEpoch = chain.forkChoice.getFinalizedCheckpoint().epoch;
   chain.forkChoice.onBlock(block.message, postState, onBlockPrecachedData);
 

packages/lodestar/src/chain/blocks/verifyBlock.ts

@@ -5,7 +5,7 @@ import {
   bellatrix,
 } from "@chainsafe/lodestar-beacon-state-transition";
 import {toHexString} from "@chainsafe/ssz";
-import {IForkChoice, IProtoBlock, ExecutionStatus} from "@chainsafe/lodestar-fork-choice";
+import {IForkChoice, IProtoBlock, ExecutionStatus, assertValidTerminalPowBlock} from "@chainsafe/lodestar-fork-choice";
 import {IChainForkConfig} from "@chainsafe/lodestar-config";
 import {ILogger} from "@chainsafe/lodestar-utils";
 import {IMetrics} from "../../metrics";
@@ -18,9 +18,11 @@ import {IBlsVerifier} from "../bls";
 import {FullyVerifiedBlock, PartiallyVerifiedBlock} from "./types";
 import {ExecutePayloadStatus} from "../../executionEngine/interface";
 import {byteArrayEquals} from "../../util/bytes";
+import {IEth1ForBlockProduction} from "../../eth1";
 
 export type VerifyBlockModules = {
   bls: IBlsVerifier;
+  eth1: IEth1ForBlockProduction;
   executionEngine: IExecutionEngine;
   regen: IStateRegenerator;
   clock: IBeaconClock;
@@ -128,6 +130,11 @@ export async function verifyBlockStateTransition(
     throw new BlockError(block, {code: BlockErrorCode.PRESTATE_MISSING, error: e as Error});
   });
 
+  const isMergeTransitionBlock =
+    bellatrix.isBellatrixStateType(preState) &&
+    bellatrix.isBellatrixBlockBodyType(block.message.body) &&
+    bellatrix.isMergeTransitionBlock(preState, block.message.body);
+
   // STFN - per_slot_processing() + per_block_processing()
   // NOTE: `regen.getPreState()` should have dialed forward the state already caching checkpoint states
   const useBlsBatchVerify = !opts?.disableBlsBatchVerify;
@@ -272,6 +279,28 @@ export async function verifyBlockStateTransition(
           errorMessage: execResult.validationError,
         });
     }
+
+    // If this is a merge transition block, check to ensure if it references
+    // a valid terminal PoW block.
+    //
+    // However specs define this check to be run inside forkChoice's onBlock
+    // (https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/fork-choice.md#on_block)
+    // but we perform the check here (as inspired from the lighthouse impl)
+    //
+    // Reasons:
+    //  1. If the block is not valid, we should fail early and not wait till
+    //     forkChoice import.
+    //  2. It makes logical sense to pair it with the block validations and
+    //     deal it with the external services like eth1 tracker here than
+    //     in import block
+    if (isMergeTransitionBlock) {
+      const mergeBlock = block.message as bellatrix.BeaconBlock;
+      const powBlockRootHex = toHexString(mergeBlock.body.executionPayload.parentHash);
+      const powBlock = await chain.eth1.getPowBlock(powBlockRootHex);
+      const powBlockParent = powBlock && (await chain.eth1.getPowBlock(powBlock.parentHash));
+
+      assertValidTerminalPowBlock(chain.config, mergeBlock, {executionStatus, powBlock, powBlockParent});
+    }
   } else {
     // isExecutionEnabled() -> false
     executionStatus = ExecutionStatus.PreMerge;

packages/lodestar/src/eth1/eth1MergeBlockTracker.ts

@@ -122,7 +122,7 @@ export class Eth1MergeBlockTracker {
     const blockRaw = await this.eth1Provider.getBlockByHash(powBlockHash);
     if (blockRaw) {
       const block = toPowBlock(blockRaw);
-      this.blocksByHashCache.set(block.blockhash, block);
+      this.blocksByHashCache.set(block.blockHash, block);
       return block;
     }
 
@@ -135,7 +135,7 @@ export class Eth1MergeBlockTracker {
 
   private setTerminalPowBlock(mergeBlock: PowMergeBlock): void {
     this.logger.info("Terminal POW block found!", {
-      hash: mergeBlock.blockhash,
+      hash: mergeBlock.blockHash,
       number: mergeBlock.number,
       totalDifficulty: mergeBlock.totalDifficulty,
     });
@@ -233,7 +233,7 @@ export class Eth1MergeBlockTracker {
             parentBlock.totalDifficulty < this.config.TERMINAL_TOTAL_DIFFICULTY
           ) {
             // Is terminal total difficulty block
-            if (childBlock.parentHash === parentBlock.blockhash) {
+            if (childBlock.parentHash === parentBlock.blockHash) {
               // AND has verified block -> parent relationship
               return this.setTerminalPowBlock(childBlock);
             } else {
@@ -280,11 +280,11 @@ export class Eth1MergeBlockTracker {
   private async fetchPotentialMergeBlock(block: PowMergeBlock): Promise<void> {
     this.logger.debug("Potential terminal POW block", {
       number: block.number,
-      hash: block.blockhash,
+      hash: block.blockHash,
       totalDifficulty: block.totalDifficulty,
     });
     // Persist block for future searches
-    this.blocksByHashCache.set(block.blockhash, block);
+    this.blocksByHashCache.set(block.blockHash, block);
 
     // Check if this block is already visited
 
@@ -310,8 +310,8 @@ export class Eth1MergeBlockTracker {
       }
 
       // Guard against infinite loops
-      if (parent.blockhash === block.blockhash) {
-        throw Error("Infinite loop: parent.blockhash === block.blockhash");
+      if (parent.blockHash === block.blockHash) {
+        throw Error("Infinite loop: parent.blockHash === block.blockHash");
       }
 
       // Fetch parent's parent
@@ -348,7 +348,7 @@ export function toPowBlock(block: EthJsonRpcBlockRaw): PowMergeBlock {
   // Validate untrusted data from API
   return {
     number: quantityToNum(block.number),
-    blockhash: dataToRootHex(block.hash),
+    blockHash: dataToRootHex(block.hash),
     parentHash: dataToRootHex(block.parentHash),
     totalDifficulty: quantityToBigint(block.totalDifficulty),
   };

packages/lodestar/src/eth1/index.ts

@@ -92,7 +92,7 @@ export class Eth1ForBlockProduction implements IEth1ForBlockProduction {
 
   getTerminalPowBlock(): Root | null {
     const block = this.eth1MergeBlockTracker.getTerminalPowBlock();
-    return block && fromHexString(block.blockhash);
+    return block && fromHexString(block.blockHash);
   }
 
   mergeCompleted(): void {

packages/lodestar/src/eth1/interface.ts

@@ -60,7 +60,7 @@ export type Eth1Block = {
 
 export type PowMergeBlock = {
   number: number;
-  blockhash: RootHex;
+  blockHash: RootHex;
   parentHash: RootHex;
   totalDifficulty: bigint;
 };