Update forkChoice with valid/invalid updates from execution

 * extend forkchoice execution type

 * handle the invalid status in protoarray

 * add the pre execution status validation testcase

 * relocate validateLatestHash to protoarray for better testing

 * add case for invalidating a branch

 * add invalidations and validations

 * add a bit more desc to test case title

 * fix the invalidate index bug

 * refac the reverse lookup and throw if forkchoice becomes invalidated

 * relax the LVH conditions in invalid responses

 * only invalidate post merge

 * modify the invalidations to allow for various LVH scenarios

 * add invalidation case

 * simplyfy lvh search and be more forgiving when not found

 * add a fail hard case

 * correct the lvh passed in valid response

 * refactor how to extract lvh in valid scenario

 * replace invalidate all with proper error handling to be more verbose and swallow less

 * add test for invalid to valid

 * add type safety to executionStatus

packages/beacon-node/src/chain/blocks/index.ts

@@ -73,14 +73,15 @@ export async function processBlocks(
 
     // Fully verify a block to be imported immediately after. Does not produce any side-effects besides adding intermediate
     // states in the state cache through regen.
-    const {postStates, executionStatuses, proposerBalanceDeltas} = await verifyBlocksInEpoch(
+    const {postStates, executionStatuses, proposerBalanceDeltas, segmentExecStatus} = await verifyBlocksInEpoch(
       chain,
       parentBlock,
       relevantBlocks,
       opts
     );
 
-    const fullyVerifiedBlocks = relevantBlocks.map(
+    const relevantBlocksPostExec = relevantBlocks.slice(0, segmentExecStatus.mayBeValidTillIndex + 1);
+    const fullyVerifiedBlocks = relevantBlocksPostExec.map(
       (block, i): FullyVerifiedBlock => ({
         block,
         postState: postStates[i],
@@ -97,6 +98,16 @@ export async function processBlocks(
       // TODO: Consider batching importBlock too if it takes significant time
       await importBlock(chain, fullyVerifiedBlock, opts);
     }
+
+    // last Valid LVH response should be processed first
+    if (segmentExecStatus.lastValidLHVResponse !== null) {
+      chain.forkChoice.validateLatestHash(segmentExecStatus.lastValidLHVResponse);
+    }
+    // If the exec eval was aborted because of invalid execution response, process
+    // the same
+    if (segmentExecStatus.execAborted !== null && segmentExecStatus.execAborted.lvhResponse !== undefined) {
+      chain.forkChoice.validateLatestHash(segmentExecStatus.execAborted.lvhResponse);
+    }
   } catch (e) {
     // above functions should only throw BlockError
     const err = getBlockError(e, blocks[0]);

packages/beacon-node/src/chain/blocks/types.ts

@@ -1,5 +1,5 @@
 import {CachedBeaconStateAllForks} from "@lodestar/state-transition";
-import {ExecutionStatus} from "@lodestar/fork-choice";
+import {MaybeValidExecutionStatus} from "@lodestar/fork-choice";
 import {allForks, Slot} from "@lodestar/types";
 
 export type ImportBlockOpts = {
@@ -48,9 +48,10 @@ export type FullyVerifiedBlock = {
   parentBlockSlot: Slot;
   proposerBalanceDelta: number;
   /**
-   * If the execution payload couldnt be verified because of EL syncing status, used in optimistic sync or for merge block
+   * If the execution payload couldnt be verified because of EL syncing status,
+   * used in optimistic sync or for merge block
    */
-  executionStatus: ExecutionStatus;
+  executionStatus: MaybeValidExecutionStatus;
   /** Seen timestamp seconds */
   seenTimestampSec: number;
 };

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, ProtoBlock} from "@lodestar/fork-choice";
+import {IForkChoice, MaybeValidExecutionStatus, ProtoBlock} from "@lodestar/fork-choice";
 import {IChainForkConfig} from "@lodestar/config";
 import {ILogger} from "@lodestar/utils";
 import {IMetrics} from "../../metrics/index.js";
@@ -16,7 +16,7 @@ import {ImportBlockOpts} from "./types.js";
 import {POS_PANDA_MERGE_TRANSITION_BANNER} from "./utils/pandaMergeTransitionBanner.js";
 import {verifyBlocksStateTransitionOnly} from "./verifyBlocksStateTransitionOnly.js";
 import {verifyBlocksSignatures} from "./verifyBlocksSignatures.js";
-import {verifyBlocksExecutionPayload} from "./verifyBlocksExecutionPayloads.js";
+import {verifyBlocksExecutionPayload, SegmentExecStatus} from "./verifyBlocksExecutionPayloads.js";
 
 export type VerifyBlockModules = {
   bls: IBlsVerifier;
@@ -48,8 +48,9 @@ export async function verifyBlocksInEpoch(
   opts: BlockProcessOpts & ImportBlockOpts
 ): Promise<{
   postStates: CachedBeaconStateAllForks[];
-  executionStatuses: ExecutionStatus[];
+  executionStatuses: MaybeValidExecutionStatus[];
   proposerBalanceDeltas: number[];
+  segmentExecStatus: SegmentExecStatus;
 }> {
   if (blocks.length === 0) {
     throw Error("Empty partiallyVerifiedBlocks");
@@ -80,7 +81,11 @@ export async function verifyBlocksInEpoch(
   const abortController = new AbortController();
 
   try {
-    const [{postStates, proposerBalanceDeltas}, , {executionStatuses, mergeBlockFound}] = await Promise.all([
+    const [
+      {postStates, proposerBalanceDeltas},
+      ,
+      {executionStatuses, mergeBlockFound, segmentExecStatus},
+    ] = await Promise.all([
       // Run state transition only
       // TODO: Ensure it yields to allow flushing to workers and engine API
       verifyBlocksStateTransitionOnly(chain, preState0, blocks, abortController.signal, opts),
@@ -92,13 +97,21 @@ export async function verifyBlocksInEpoch(
       verifyBlocksExecutionPayload(chain, parentBlock, blocks, preState0, abortController.signal, opts),
     ]);
 
+    if (segmentExecStatus.execAborted !== null) {
+      if (segmentExecStatus.mayBeValidTillIndex < 0) {
+        // If exec was aborted on the first block itself, throw an error as none of the blocks
+        // can be processed and added for forkchoice
+        throw segmentExecStatus.execAborted.execError;
+      }
+    }
+
     if (mergeBlockFound !== null) {
       // merge block found and is fully valid = state transition + signatures + execution payload.
       // TODO: Will this banner be logged during syncing?
       logOnPowBlock(chain, mergeBlockFound);
     }
 
-    return {postStates, executionStatuses, proposerBalanceDeltas};
+    return {postStates, executionStatuses, proposerBalanceDeltas, segmentExecStatus};
   } finally {
     abortController.abort();
   }

packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts

@@ -7,7 +7,15 @@ import {
 } from "@lodestar/state-transition";
 import {bellatrix, allForks} from "@lodestar/types";
 import {toHexString} from "@chainsafe/ssz";
-import {IForkChoice, ExecutionStatus, assertValidTerminalPowBlock, ProtoBlock} from "@lodestar/fork-choice";
+import {
+  IForkChoice,
+  assertValidTerminalPowBlock,
+  ProtoBlock,
+  ExecutionStatus,
+  MaybeValidExecutionStatus,
+  LVHValidResponse,
+  LVHInvalidResponse,
+} from "@lodestar/fork-choice";
 import {IChainForkConfig} from "@lodestar/config";
 import {ErrorAborted, ILogger} from "@lodestar/utils";
 import {IExecutionEngine} from "../../execution/engine/index.js";
@@ -26,6 +34,25 @@ type VerifyBlockModules = {
   config: IChainForkConfig;
 };
 
+type ExecAbortType = {
+  blockIndex: number;
+  execError: BlockError;
+  lvhResponse?: LVHInvalidResponse;
+};
+
+type VerifyExecutionResponse =
+  | {executionStatus: ExecutionStatus.Valid; lvhResponse: LVHValidResponse}
+  | {executionStatus: ExecutionStatus.Invalid; lvhResponse: LVHInvalidResponse; execError: BlockError}
+  | {executionStatus: ExecutionStatus.Syncing; lvhResponse?: LVHValidResponse}
+  | {executionStatus: ExecutionStatus.PreMerge}
+  | {executionStatus: null; execError: BlockError};
+
+export type SegmentExecStatus = {
+  mayBeValidTillIndex: number;
+  lastValidLHVResponse: LVHValidResponse | null;
+  execAborted: ExecAbortType | null;
+};
+
 /**
  * Verifies 1 or more execution payloads from a linear sequence of blocks.
  *
@@ -38,9 +65,16 @@ export async function verifyBlocksExecutionPayload(
   preState0: CachedBeaconStateAllForks,
   signal: AbortSignal,
   opts: BlockProcessOpts
-): Promise<{executionStatuses: ExecutionStatus[]; mergeBlockFound: bellatrix.BeaconBlock | null}> {
-  const executionStatuses: ExecutionStatus[] = [];
+): Promise<{
+  executionStatuses: MaybeValidExecutionStatus[];
+  mergeBlockFound: bellatrix.BeaconBlock | null;
+  segmentExecStatus: SegmentExecStatus;
+}> {
+  const executionStatuses: MaybeValidExecutionStatus[] = [];
   let mergeBlockFound: bellatrix.BeaconBlock | null = null;
+  let execAborted: ExecAbortType | null = null;
+  let lastValidLHVResponse: LVHValidResponse | null = null;
+  let mayBeValidTillIndex = -1;
 
   // Error in the same way as verifyBlocksSanityChecks if empty blocks
   if (blocks.length === 0) {
@@ -112,14 +146,30 @@ export async function verifyBlocksExecutionPayload(
     parentBlock.executionStatus !== ExecutionStatus.PreMerge ||
     lastBlock.message.slot + opts.safeSlotsToImportOptimistically < chain.clock.currentSlot;
 
-  for (const block of blocks) {
+  for (let blockIndex = 0; blockIndex < blocks.length; blockIndex++) {
+    const block = blocks[blockIndex];
     // If blocks are invalid in consensus the main promise could resolve before this loop ends.
     // In that case stop sending blocks to execution engine
     if (signal.aborted) {
       throw new ErrorAborted("verifyBlockExecutionPayloads");
     }
+    const verifyResponse = await verifyBlockExecutionPayload(chain, block, preState0, opts, isOptimisticallySafe);
 
-    const {executionStatus} = await verifyBlockExecutionPayload(chain, block, preState0, opts, isOptimisticallySafe);
+    // If we had an invalid status or we had a block error even before evaluating status
+    //   then we need to set execAborted and break out of the loop as the child blocks
+    //   can't be evaluated further and need to be rejected
+    if (verifyResponse.executionStatus === ExecutionStatus.Invalid || verifyResponse.executionStatus === null) {
+      const {execError} = verifyResponse;
+      const lvhResponse =
+        verifyResponse.executionStatus === ExecutionStatus.Invalid ? verifyResponse.lvhResponse : undefined;
+      execAborted = {blockIndex, execError, lvhResponse};
+      break;
+    } else if (verifyResponse.executionStatus === ExecutionStatus.Valid) {
+      lastValidLHVResponse = verifyResponse.lvhResponse;
+    }
+
+    // If we are here then its because executionStatus is one of MaybeValidExecutionStatus
+    const {executionStatus} = verifyResponse;
     // 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.
@@ -187,9 +237,14 @@ export async function verifyBlocksExecutionPayload(
       // to the end of the verify block routine, which confirms that this block is fully valid.
       mergeBlockFound = mergeBlock;
     }
+    mayBeValidTillIndex++;
   }
 
-  return {executionStatuses, mergeBlockFound};
+  return {
+    executionStatuses,
+    mergeBlockFound,
+    segmentExecStatus: {mayBeValidTillIndex, lastValidLHVResponse, execAborted},
+  };
 }
 
 /**
@@ -201,7 +256,7 @@ export async function verifyBlockExecutionPayload(
   preState0: CachedBeaconStateAllForks,
   opts: BlockProcessOpts,
   isOptimisticallySafe: boolean
-): Promise<{executionStatus: ExecutionStatus}> {
+): Promise<VerifyExecutionResponse> {
   /** Not null if execution is enabled */
   const executionPayloadEnabled =
     isBellatrixStateType(preState0) &&
@@ -224,23 +279,25 @@ export async function verifyBlockExecutionPayload(
   const execResult = await chain.executionEngine.notifyNewPayload(executionPayloadEnabled);
 
   switch (execResult.status) {
-    case ExecutePayloadStatus.VALID:
-      chain.forkChoice.validateLatestHash(execResult.latestValidHash, null);
-      return {executionStatus: ExecutionStatus.Valid};
+    case ExecutePayloadStatus.VALID: {
+      const executionStatus: ExecutionStatus.Valid = ExecutionStatus.Valid;
+      const lvhResponse = {executionStatus, latestValidExecHash: execResult.latestValidHash};
+      return {executionStatus, lvhResponse};
+    }
 
     case ExecutePayloadStatus.INVALID: {
-      // If the parentRoot is not same as latestValidHash, then the branch from latestValidHash
-      // to parentRoot needs to be invalidated
-      const parentHashHex = toHexString(block.message.parentRoot);
-      chain.forkChoice.validateLatestHash(
-        execResult.latestValidHash,
-        parentHashHex !== execResult.latestValidHash ? parentHashHex : null
-      );
-      throw new BlockError(block, {
+      const executionStatus: ExecutionStatus.Invalid = ExecutionStatus.Invalid;
+      const lvhResponse = {
+        executionStatus,
+        latestValidExecHash: execResult.latestValidHash,
+        invalidateTillBlockHash: toHexString(block.message.parentRoot),
+      };
+      const execError = new BlockError(block, {
         code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
         execStatus: execResult.status,
         errorMessage: execResult.validationError ?? "",
       });
+      return {executionStatus, lvhResponse, execError};
     }
 
     // Accepted and Syncing have the same treatment, as final validation of block is pending
@@ -253,11 +310,12 @@ export async function verifyBlockExecutionPayload(
         !isOptimisticallySafe &&
         block.message.slot + opts.safeSlotsToImportOptimistically >= chain.clock.currentSlot
       ) {
-        throw new BlockError(block, {
+        const execError = 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`,
         });
+        return {executionStatus: null, execError};
       }
 
       return {executionStatus: ExecutionStatus.Syncing};
@@ -282,11 +340,13 @@ export async function verifyBlockExecutionPayload(
 
     case ExecutePayloadStatus.INVALID_BLOCK_HASH:
     case ExecutePayloadStatus.ELERROR:
-    case ExecutePayloadStatus.UNAVAILABLE:
-      throw new BlockError(block, {
+    case ExecutePayloadStatus.UNAVAILABLE: {
+      const execError = new BlockError(block, {
         code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
         execStatus: execResult.status,
         errorMessage: execResult.validationError,
       });
+      return {executionStatus: null, execError};
+    }
   }
 }

packages/beacon-node/src/execution/engine/interface.ts

@@ -27,7 +27,7 @@ export enum ExecutePayloadStatus {
 export type ExecutePayloadResponse =
   | {status: ExecutePayloadStatus.SYNCING | ExecutePayloadStatus.ACCEPTED; latestValidHash: null; validationError: null}
   | {status: ExecutePayloadStatus.VALID; latestValidHash: RootHex; validationError: null}
-  | {status: ExecutePayloadStatus.INVALID; latestValidHash: RootHex; validationError: string | null}
+  | {status: ExecutePayloadStatus.INVALID; latestValidHash: RootHex | null; validationError: string | null}
   | {
       status: ExecutePayloadStatus.INVALID_BLOCK_HASH | ExecutePayloadStatus.ELERROR | ExecutePayloadStatus.UNAVAILABLE;
       latestValidHash: null;

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

@@ -22,7 +22,14 @@ import {computeUnrealizedCheckpoints} from "@lodestar/state-transition/epoch";
 import {IChainConfig, IChainForkConfig} from "@lodestar/config";
 
 import {computeDeltas} from "../protoArray/computeDeltas.js";
-import {HEX_ZERO_HASH, VoteTracker, ProtoBlock, ExecutionStatus} from "../protoArray/interface.js";
+import {
+  HEX_ZERO_HASH,
+  VoteTracker,
+  ProtoBlock,
+  ExecutionStatus,
+  MaybeValidExecutionStatus,
+  LVHExecResponse,
+} from "../protoArray/interface.js";
 import {ProtoArray} from "../protoArray/protoArray.js";
 
 import {IForkChoiceMetrics} from "../metrics.js";
@@ -277,7 +284,7 @@ export class ForkChoice implements IForkChoice {
     state: CachedBeaconStateAllForks,
     blockDelaySec: number,
     currentSlot: Slot,
-    executionStatus: ExecutionStatus
+    executionStatus: MaybeValidExecutionStatus
   ): void {
     const {parentRoot, slot} = block;
     const parentRootHex = toHexString(parentRoot);
@@ -743,14 +750,20 @@ export class ForkChoice implements IForkChoice {
   }
 
   /**
-   * Optimistic sync validate till validated latest hash, invalidate any decendant branch if invalidate till hash provided
-   * TODO: implementation:
-   * 1. verify is_merge_block if the mergeblock has not yet been validated
-   * 2. Throw critical error and exit if a block in finalized chain gets invalidated
+   * Optimistic sync validate till validated latest hash, invalidate any decendant
+   * branch if invalidate till hash provided
+   *
+   * Proxies to protoArray's validateLatestHash and could run extra validations for the
+   * justified's status as well as validate the terminal conditions if terminal block
+   * becomes valid
    */
-  validateLatestHash(_latestValidHash: RootHex, _invalidateTillHash: RootHex | null): void {
-    // Silently ignore for now if all calls were valid
-    return;
+  validateLatestHash(execResponse: LVHExecResponse): void {
+    this.protoArray.validateLatestHash(execResponse);
+
+    // Call findHead to validate that the forkChoice consensus has not broken down
+    // as it is possible for invalidation to invalidate the entire forkChoice if
+    // the consensus breaks down, which will cause findHead to throw
+    this.protoArray.findHead(this.fcStore.justified.checkpoint.rootHex, this.fcStore.currentSlot);
   }
 
   /**
@@ -790,13 +803,15 @@ export class ForkChoice implements IForkChoice {
     );
   }
 
-  private getPreMergeExecStatus(executionStatus: ExecutionStatus): ExecutionStatus.PreMerge {
+  private getPreMergeExecStatus(executionStatus: MaybeValidExecutionStatus): ExecutionStatus.PreMerge {
     if (executionStatus !== ExecutionStatus.PreMerge)
       throw Error(`Invalid pre-merge execution status: expected: ${ExecutionStatus.PreMerge}, got ${executionStatus}`);
     return executionStatus;
   }
 
-  private getPostMergeExecStatus(executionStatus: ExecutionStatus): ExecutionStatus.Valid | ExecutionStatus.Syncing {
+  private getPostMergeExecStatus(
+    executionStatus: MaybeValidExecutionStatus
+  ): ExecutionStatus.Valid | ExecutionStatus.Syncing {
     if (executionStatus === ExecutionStatus.PreMerge)
       throw Error(
         `Invalid post-merge execution status: expected: ${ExecutionStatus.Syncing} or ${ExecutionStatus.Valid} , got ${executionStatus}`