From 6adbd27c85f6fbb3a2a17ef4552274e985319c02 Mon Sep 17 00:00:00 2001 From: g11tech Date: Sat, 27 Jan 2024 22:37:49 +0530 Subject: [PATCH] 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 --- .../blocks/verifyBlocksExecutionPayloads.ts | 4 +- .../fork-choice/src/protoArray/interface.ts | 2 +- .../fork-choice/src/protoArray/protoArray.ts | 45 ++++++++++--------- .../protoArray/executionStatusUpdates.test.ts | 10 ++--- 4 files changed, 31 insertions(+), 30 deletions(-) diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts index 5dbe104c9541..91242d879f85 100644 --- 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 index 1910ad2b4206..003a3c8f9f1e 100644 --- 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 index 62f50b03771c..eaa86b2f0ee1 100644 --- 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 index 94e5cd3ac9a0..e6916f24800f 100644 --- 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 );