From 39c1ca9b0ec9db45b0f3553377bdda69606b3e28 Mon Sep 17 00:00:00 2001 From: harkamal Date: Sat, 27 Jan 2024 15:55:16 +0530 Subject: [PATCH 1/4] fix: ignore forkchoice invalidations if latestValidHash not found --- packages/fork-choice/src/protoArray/protoArray.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/fork-choice/src/protoArray/protoArray.ts b/packages/fork-choice/src/protoArray/protoArray.ts index 62f50b03771c..e372c590646c 100644 --- a/packages/fork-choice/src/protoArray/protoArray.ts +++ b/packages/fork-choice/src/protoArray/protoArray.ts @@ -299,10 +299,12 @@ export class ProtoArray { * ii) lazy: that invalidation was result of simple check and the EL just * responded with a bogus LVH * - * So we will just invalidate the current payload and let future responses take care - * to be as robust as possible. + * In such case for robustness, lets not process this invalidation into forkchoice + * as it might poision it since the invalidations can't be processed unless latestValidHashIndex + * is known as invalidateFromIndex is the parent of the payload being verified which has not been + * imported yet into forkchoice. */ - this.invalidateNodeByIndex(invalidateFromIndex); + throw Error(`Unable to find latestValidExecHash=${latestValidExecHash} in the forkchoice`); } else { this.propagateInValidExecutionStatusByIndex(invalidateFromIndex, latestValidHashIndex, currentSlot); } From c3f812005d11d4c9dcfae223d3ef4bbed819c041 Mon Sep 17 00:00:00 2001 From: harkamal Date: Sat, 27 Jan 2024 16:12:46 +0530 Subject: [PATCH 2/4] rename for better understanding --- .../blocks/verifyBlocksExecutionPayloads.ts | 4 ++-- .../fork-choice/src/protoArray/interface.ts | 2 +- .../fork-choice/src/protoArray/protoArray.ts | 22 +++++++++---------- .../protoArray/executionStatusUpdates.test.ts | 10 ++++----- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts index 5dbe104c9541..41e4c89dce7d 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), + invalidateFromParentBlockHash: 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, + invalidateFromParentBlockHash: parentBlock.blockRoot, }; } } diff --git a/packages/fork-choice/src/protoArray/interface.ts b/packages/fork-choice/src/protoArray/interface.ts index 1910ad2b4206..4ebaa58a8c18 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; + invalidateFromParentBlockHash: 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 e372c590646c..7b504b3b6bb2 100644 --- a/packages/fork-choice/src/protoArray/protoArray.ts +++ b/packages/fork-choice/src/protoArray/protoArray.ts @@ -279,13 +279,13 @@ 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 {invalidateFromParentBlockHash, latestValidExecHash} = execResponse; + const invalidateFromParentIndex = this.indices.get(invalidateFromParentBlockHash); + if (invalidateFromParentIndex === undefined) { + throw Error(`Unable to find invalidateFromParentBlockHash=${invalidateFromParentBlockHash} 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, @@ -301,12 +301,12 @@ export class ProtoArray { * * In such case for robustness, lets not process this invalidation into forkchoice * as it might poision it since the invalidations can't be processed unless latestValidHashIndex - * is known as invalidateFromIndex is the parent of the payload being verified which has not been - * imported yet into forkchoice. + * is known as invalidateFromParentIndex is the parent of the payload being verified which has not + * been imported yet into forkchoice. */ throw Error(`Unable to find latestValidExecHash=${latestValidExecHash} in the forkchoice`); } else { - this.propagateInValidExecutionStatusByIndex(invalidateFromIndex, latestValidHashIndex, currentSlot); + this.propagateInValidExecutionStatusByIndex(invalidateFromParentIndex, latestValidHashIndex, currentSlot); } } } @@ -335,12 +335,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; diff --git a/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts b/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts index 94e5cd3ac9a0..ad8475d6d331 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", + invalidateFromParentBlockHash: "3C", }, 3 ); @@ -212,7 +212,7 @@ describe("executionStatus / normal updates", () => { { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "1A", - invalidateFromBlockHash: "3A", + invalidateFromParentBlockHash: "3A", }, 3 ); @@ -259,7 +259,7 @@ describe("executionStatus / invalidate all postmerge chain", () => { { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromBlockHash: "3B", + invalidateFromParentBlockHash: "3B", }, 3 ); @@ -336,7 +336,7 @@ describe("executionStatus / poision forkchoice if we invalidate previous valid", { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromBlockHash: "3A", + invalidateFromParentBlockHash: "3A", }, 3 ) @@ -373,7 +373,7 @@ describe("executionStatus / poision forkchoice if we validate previous invalid", { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromBlockHash: "3B", + invalidateFromParentBlockHash: "3B", }, 3 ); From 2532f6d9ef06d1843a48a36b126f091ea39f2fad Mon Sep 17 00:00:00 2001 From: harkamal Date: Sat, 27 Jan 2024 16:54:22 +0530 Subject: [PATCH 3/4] update the lvh search start index --- packages/fork-choice/src/protoArray/protoArray.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/fork-choice/src/protoArray/protoArray.ts b/packages/fork-choice/src/protoArray/protoArray.ts index 7b504b3b6bb2..f655d3d1b15b 100644 --- a/packages/fork-choice/src/protoArray/protoArray.ts +++ b/packages/fork-choice/src/protoArray/protoArray.ts @@ -370,8 +370,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 ( From a30838de880e31e995e33877c8cfe951f0eba112 Mon Sep 17 00:00:00 2001 From: harkamal Date: Sat, 27 Jan 2024 22:27:49 +0530 Subject: [PATCH 4/4] apply feedback --- .../blocks/verifyBlocksExecutionPayloads.ts | 4 +-- .../fork-choice/src/protoArray/interface.ts | 2 +- .../fork-choice/src/protoArray/protoArray.ts | 29 +++++++++---------- .../protoArray/executionStatusUpdates.test.ts | 10 +++---- 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts b/packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts index 41e4c89dce7d..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, - invalidateFromParentBlockHash: 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, - invalidateFromParentBlockHash: parentBlock.blockRoot, + invalidateFromParentBlockRoot: parentBlock.blockRoot, }; } } diff --git a/packages/fork-choice/src/protoArray/interface.ts b/packages/fork-choice/src/protoArray/interface.ts index 4ebaa58a8c18..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; - invalidateFromParentBlockHash: 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 f655d3d1b15b..eaa86b2f0ee1 100644 --- a/packages/fork-choice/src/protoArray/protoArray.ts +++ b/packages/fork-choice/src/protoArray/protoArray.ts @@ -279,30 +279,29 @@ 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 {invalidateFromParentBlockHash, latestValidExecHash} = execResponse; - const invalidateFromParentIndex = this.indices.get(invalidateFromParentBlockHash); + const {invalidateFromParentBlockRoot, latestValidExecHash} = execResponse; + const invalidateFromParentIndex = this.indices.get(invalidateFromParentBlockRoot); if (invalidateFromParentIndex === undefined) { - throw Error(`Unable to find invalidateFromParentBlockHash=${invalidateFromParentBlockHash} in forkChoice`); + throw Error(`Unable to find invalidateFromParentBlockRoot=${invalidateFromParentBlockRoot} in forkChoice`); } const latestValidHashIndex = 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. * - * In such case for robustness, lets not process this invalidation into forkchoice - * as it might poision it since the invalidations can't be processed unless latestValidHashIndex - * is known as invalidateFromParentIndex is the parent of the payload being verified which has not - * been imported yet into forkchoice. + * - "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. */ throw Error(`Unable to find latestValidExecHash=${latestValidExecHash} in the forkchoice`); } else { diff --git a/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts b/packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts index ad8475d6d331..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", - invalidateFromParentBlockHash: "3C", + invalidateFromParentBlockRoot: "3C", }, 3 ); @@ -212,7 +212,7 @@ describe("executionStatus / normal updates", () => { { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "1A", - invalidateFromParentBlockHash: "3A", + invalidateFromParentBlockRoot: "3A", }, 3 ); @@ -259,7 +259,7 @@ describe("executionStatus / invalidate all postmerge chain", () => { { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromParentBlockHash: "3B", + invalidateFromParentBlockRoot: "3B", }, 3 ); @@ -336,7 +336,7 @@ describe("executionStatus / poision forkchoice if we invalidate previous valid", { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromParentBlockHash: "3A", + invalidateFromParentBlockRoot: "3A", }, 3 ) @@ -373,7 +373,7 @@ describe("executionStatus / poision forkchoice if we validate previous invalid", { executionStatus: ExecutionStatus.Invalid, latestValidExecHash: "0x0000000000000000000000000000000000000000000000000000000000000000", - invalidateFromParentBlockHash: "3B", + invalidateFromParentBlockRoot: "3B", }, 3 );