fix: ignore forkchoice invalidations if latestValidHash not found (#6361

) * fix: ignore forkchoice invalidations if latestValidHash not found * rename for better understanding * update the lvh search start index * apply feedback
ChainSafe · Jan 27, 2024 · 6adbd27 · 6adbd27
1 parent 347c95f
commit 6adbd27
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 30 deletions.
diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts
@@ -319,7 +319,7 @@ export async function verifyBlockExecutionPayload(
       const lvhResponse = {
         executionStatus,
         latestValidExecHash: execResult.latestValidHash,
-        invalidateFromBlockHash: toHexString(block.message.parentRoot),
+        invalidateFromParentBlockRoot: toHexString(block.message.parentRoot),
       };
       const execError = new BlockError(block, {
         code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
@@ -416,7 +416,7 @@ function getSegmentErrorResponse(
       invalidSegmentLVH = {
         executionStatus: ExecutionStatus.Invalid,
         latestValidExecHash: lvhResponse.latestValidExecHash,
-        invalidateFromBlockHash: parentBlock.blockRoot,
+        invalidateFromParentBlockRoot: parentBlock.blockRoot,
       };
     }
   }

diff --git a/packages/fork-choice/src/protoArray/interface.ts b/packages/fork-choice/src/protoArray/interface.ts
@@ -29,7 +29,7 @@ export type LVHValidResponse = {
 export type LVHInvalidResponse = {
   executionStatus: ExecutionStatus.Invalid;
   latestValidExecHash: RootHex | null;
-  invalidateFromBlockHash: RootHex;
+  invalidateFromParentBlockRoot: RootHex;
 };
 export type LVHExecResponse = LVHValidResponse | LVHInvalidResponse;
 

diff --git a/packages/fork-choice/src/protoArray/protoArray.ts b/packages/fork-choice/src/protoArray/protoArray.ts
@@ -279,32 +279,33 @@ export class ProtoArray {
       // Mark chain ii) as Invalid if LVH is found and non null, else only invalidate invalid_payload
       // if its in fcU.
       //
-      const {invalidateFromBlockHash, latestValidExecHash} = execResponse;
-      const invalidateFromIndex = this.indices.get(invalidateFromBlockHash);
-      if (invalidateFromIndex === undefined) {
-        throw Error(`Unable to find invalidateFromBlockHash=${invalidateFromBlockHash} in forkChoice`);
+      const {invalidateFromParentBlockRoot, latestValidExecHash} = execResponse;
+      const invalidateFromParentIndex = this.indices.get(invalidateFromParentBlockRoot);
+      if (invalidateFromParentIndex === undefined) {
+        throw Error(`Unable to find invalidateFromParentBlockRoot=${invalidateFromParentBlockRoot} in forkChoice`);
       }
       const latestValidHashIndex =
-        latestValidExecHash !== null ? this.getNodeIndexFromLVH(latestValidExecHash, invalidateFromIndex) : null;
+        latestValidExecHash !== null ? this.getNodeIndexFromLVH(latestValidExecHash, invalidateFromParentIndex) : null;
       if (latestValidHashIndex === null) {
         /**
-         *  If the LVH is null or not found, represented with latestValidHashIndex=undefined,
-         *   then just invalidate the invalid_payload and bug out.
+         * The LVH (latest valid hash) is null or not found.
          *
-         *   Ideally in not found scenario we should invalidate the entire chain upwards, but
-         *   it is possible (and observed in the testnets) that the EL was
+         * The spec gives an allowance for the EL being able to return a nullish LVH if it could not
+         * "determine" one. There are two interpretations:
          *
-         *     i) buggy: that the LVH was not really the parent of the invalid block, but on
-         *        some side chain
-         *     ii) lazy: that invalidation was result of simple check and the EL just
-         *         responded with a bogus LVH
+         * - "the LVH is unknown" - simply throw and move on. We can't determine which chain to invalidate
+         *   since we don't know which ancestor is valid.
          *
-         *   So we will just invalidate the current payload and let future responses take care
-         *   to be as robust as possible.
+         * - "the LVH doesn't exist" - this means that the entire ancestor chain is invalid, and should
+         *   be marked as such.
+         *
+         * The more robust approach is to treat nullish LVH as "the LVH is unknown" rather than
+         * "the LVH doesn't exist". The alternative means that we will poison a valid chain when the
+         * EL is lazy (or buggy) with its LVH response.
          */
-        this.invalidateNodeByIndex(invalidateFromIndex);
+        throw Error(`Unable to find latestValidExecHash=${latestValidExecHash} in the forkchoice`);
       } else {
-        this.propagateInValidExecutionStatusByIndex(invalidateFromIndex, latestValidHashIndex, currentSlot);
+        this.propagateInValidExecutionStatusByIndex(invalidateFromParentIndex, latestValidHashIndex, currentSlot);
       }
     }
   }
@@ -333,12 +334,12 @@ export class ProtoArray {
    */
 
   private propagateInValidExecutionStatusByIndex(
-    invalidateFromIndex: number,
+    invalidateFromParentIndex: number,
     latestValidHashIndex: number,
     currentSlot: Slot
   ): void {
-    // Pass 1: mark invalidateFromIndex and its parents invalid
-    let invalidateIndex: number | undefined = invalidateFromIndex;
+    // Pass 1: mark invalidateFromParentIndex and its parents invalid
+    let invalidateIndex: number | undefined = invalidateFromParentIndex;
     while (invalidateIndex !== undefined && invalidateIndex > latestValidHashIndex) {
       const invalidNode = this.invalidateNodeByIndex(invalidateIndex);
       invalidateIndex = invalidNode.parent;
@@ -368,8 +369,8 @@ export class ProtoArray {
     });
   }
 
-  private getNodeIndexFromLVH(latestValidExecHash: RootHex, ancestorOfIndex: number): number | null {
-    let nodeIndex = this.nodes[ancestorOfIndex].parent;
+  private getNodeIndexFromLVH(latestValidExecHash: RootHex, ancestorFromIndex: number): number | null {
+    let nodeIndex: number | undefined = ancestorFromIndex;
     while (nodeIndex !== undefined && nodeIndex >= 0) {
       const node = this.getNodeFromIndex(nodeIndex);
       if (

diff --git a/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts b/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts
@@ -149,7 +149,7 @@ describe("executionStatus / normal updates", () => {
     {
       executionStatus: ExecutionStatus.Invalid,
       latestValidExecHash: "2C",
-      invalidateFromBlockHash: "3C",
+      invalidateFromParentBlockRoot: "3C",
     },
     3
   );
@@ -212,7 +212,7 @@ describe("executionStatus / normal updates", () => {
     {
       executionStatus: ExecutionStatus.Invalid,
       latestValidExecHash: "1A",
-      invalidateFromBlockHash: "3A",
+      invalidateFromParentBlockRoot: "3A",
     },
     3
   );
@@ -259,7 +259,7 @@ describe("executionStatus / invalidate all postmerge chain", () => {
     {
       executionStatus: ExecutionStatus.Invalid,
       latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000",
-      invalidateFromBlockHash: "3B",
+      invalidateFromParentBlockRoot: "3B",
     },
     3
   );
@@ -336,7 +336,7 @@ describe("executionStatus / poision forkchoice if we invalidate previous valid",
         {
           executionStatus: ExecutionStatus.Invalid,
           latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000",
-          invalidateFromBlockHash: "3A",
+          invalidateFromParentBlockRoot: "3A",
         },
         3
       )
@@ -373,7 +373,7 @@ describe("executionStatus / poision forkchoice if we validate previous invalid",
     {
       executionStatus: ExecutionStatus.Invalid,
       latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000",
-      invalidateFromBlockHash: "3B",
+      invalidateFromParentBlockRoot: "3B",
     },
     3
   );