-
-
Notifications
You must be signed in to change notification settings - Fork 460
fix: invalidate fork-choice head on FCU INVALID with latestValidHash #9332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
lodekeeper
wants to merge
2
commits into
ChainSafe:unstable
Choose a base branch
from
lodekeeper:fix/fcu-invalid-lvh-handling
base: unstable
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+328
−10
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
packages/beacon-node/src/chain/blocks/utils/forkchoiceUpdateInvalid.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import {ExecutionStatus} from "@lodestar/fork-choice"; | ||
| import {RootHex} from "@lodestar/types"; | ||
| import type {ForkchoiceUpdateError} from "../../../execution/engine/interface.js"; | ||
| import type {BeaconChain} from "../../chain.js"; | ||
| import {ForkchoiceCaller} from "../../forkChoice/index.js"; | ||
|
|
||
| /** | ||
| * Engine API spec: when EL responds INVALID to `engine_forkchoiceUpdated`, the CL must | ||
| * mark the offending head and its ancestors back to (but not including) `latestValidHash` | ||
| * as INVALID in fork choice and recompute the head. Without this, the CL's tree never | ||
| * abandons the bad branch — every slot it re-fires the same FCU, EL keeps responding | ||
| * INVALID, and the node stays wedged at the same head while the network advances. | ||
| * | ||
| * `validateLatestHash` walks up from `invalidateFromParentBlockRoot` (the invalid block | ||
| * itself, despite the name) marking ancestors invalid until reaching the LVH, then | ||
| * marks all descendants invalid in a second pass and recomputes scores. After that we | ||
| * trigger an explicit head recompute so subsequent attestations, proposals, and APIs | ||
| * see the corrected head immediately rather than waiting for the next `prepareNextSlot` | ||
| * or block import. | ||
| */ | ||
| export function invalidateForkchoiceHeadFromFcuInvalid( | ||
| chain: BeaconChain, | ||
| headBlockRoot: RootHex, | ||
| headBlockHash: RootHex, | ||
| e: ForkchoiceUpdateError | ||
| ): void { | ||
| const latestValidHash = e.type.latestValidHash; | ||
| chain.logger.warn( | ||
| "Invalidating head after FCU INVALID response", | ||
| { | ||
| headBlockRoot, | ||
| headBlockHash, | ||
| latestValidHash: latestValidHash ?? "null", | ||
| validationError: e.type.validationError ?? "", | ||
| }, | ||
| e | ||
| ); | ||
|
|
||
| try { | ||
| chain.forkChoice.validateLatestHash({ | ||
| executionStatus: ExecutionStatus.Invalid, | ||
| latestValidExecHash: latestValidHash, | ||
| invalidateFromParentBlockRoot: headBlockRoot, | ||
| invalidateFromParentBlockHash: headBlockHash, | ||
| }); | ||
| } catch (err) { | ||
| chain.logger.error( | ||
| "Failed to invalidate head after FCU INVALID response", | ||
| {headBlockRoot, headBlockHash, latestValidHash: latestValidHash ?? "null"}, | ||
| err as Error | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const newHead = chain.recomputeForkChoiceHead(ForkchoiceCaller.forkchoiceUpdateInvalid); | ||
| if (newHead.blockRoot !== headBlockRoot) { | ||
| chain.logger.info("Switched head after FCU INVALID invalidation", { | ||
| oldHeadBlockRoot: headBlockRoot, | ||
| newHeadBlockRoot: newHead.blockRoot, | ||
| newHeadSlot: newHead.slot, | ||
| }); | ||
| } | ||
| } catch (err) { | ||
| chain.logger.error( | ||
| "Failed to recompute head after FCU INVALID invalidation", | ||
| {headBlockRoot, headBlockHash}, | ||
| err as Error | ||
| ); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
141 changes: 141 additions & 0 deletions
141
packages/beacon-node/test/unit/chain/blocks/utils/forkchoiceUpdateInvalid.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| import {describe, expect, it, vi} from "vitest"; | ||
| import {ExecutionStatus} from "@lodestar/fork-choice"; | ||
| import {invalidateForkchoiceHeadFromFcuInvalid} from "../../../../../src/chain/blocks/utils/forkchoiceUpdateInvalid.js"; | ||
| import type {BeaconChain} from "../../../../../src/chain/chain.js"; | ||
| import {ForkchoiceCaller} from "../../../../../src/chain/forkChoice/index.js"; | ||
| import {ForkchoiceUpdateError, ForkchoiceUpdateErrorCode} from "../../../../../src/execution/engine/interface.js"; | ||
|
|
||
| describe("chain / blocks / utils / invalidateForkchoiceHeadFromFcuInvalid", () => { | ||
| const headBlockRoot = "0xbeac0a000000000000000000000000000000000000000000000000000000000a"; | ||
| const headBlockHash = "0x19dc0a000000000000000000000000000000000000000000000000000000000a"; | ||
| const lvh = "0x05a771b000000000000000000000000000000000000000000000000000000a05"; | ||
| const newHeadBlockRoot = "0xbeac12e000000000000000000000000000000000000000000000000000000a12"; | ||
|
|
||
| function makeChain(): { | ||
| chain: BeaconChain; | ||
| validateLatestHash: ReturnType<typeof vi.fn>; | ||
| recomputeForkChoiceHead: ReturnType<typeof vi.fn>; | ||
| logger: {warn: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn>; info: ReturnType<typeof vi.fn>}; | ||
| } { | ||
| const validateLatestHash = vi.fn(); | ||
| const recomputeForkChoiceHead = vi.fn().mockReturnValue({blockRoot: newHeadBlockRoot, slot: 5598}); | ||
| const logger = {warn: vi.fn(), error: vi.fn(), info: vi.fn()}; | ||
| const chain = { | ||
| forkChoice: {validateLatestHash}, | ||
| logger, | ||
| recomputeForkChoiceHead, | ||
| } as unknown as BeaconChain; | ||
| return {chain, validateLatestHash, recomputeForkChoiceHead, logger}; | ||
| } | ||
|
|
||
| it("calls forkChoice.validateLatestHash with the head as the invalid block and EL LVH", () => { | ||
| const {chain, validateLatestHash} = makeChain(); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: lvh, | ||
| validationError: "HeaderGasUsedMismatch", | ||
| }); | ||
|
|
||
| invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e); | ||
|
|
||
| expect(validateLatestHash).toHaveBeenCalledTimes(1); | ||
| expect(validateLatestHash).toHaveBeenCalledWith({ | ||
| executionStatus: ExecutionStatus.Invalid, | ||
| latestValidExecHash: lvh, | ||
| invalidateFromParentBlockRoot: headBlockRoot, | ||
| invalidateFromParentBlockHash: headBlockHash, | ||
| }); | ||
| }); | ||
|
|
||
| it("recomputes head after invalidation so attestations and proposals see the corrected head", () => { | ||
| const {chain, recomputeForkChoiceHead, logger} = makeChain(); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: lvh, | ||
| validationError: null, | ||
| }); | ||
|
|
||
| invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e); | ||
|
|
||
| expect(recomputeForkChoiceHead).toHaveBeenCalledTimes(1); | ||
| expect(recomputeForkChoiceHead).toHaveBeenCalledWith(ForkchoiceCaller.forkchoiceUpdateInvalid); | ||
| // head changed → info log noting the switch | ||
| expect(logger.info).toHaveBeenCalledWith( | ||
| "Switched head after FCU INVALID invalidation", | ||
| expect.objectContaining({ | ||
| oldHeadBlockRoot: headBlockRoot, | ||
| newHeadBlockRoot, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it("does not log a switch if head recompute returns the same root (no descendant alternative)", () => { | ||
| const {chain, recomputeForkChoiceHead, logger} = makeChain(); | ||
| recomputeForkChoiceHead.mockReturnValueOnce({blockRoot: headBlockRoot, slot: 5597}); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: lvh, | ||
| validationError: null, | ||
| }); | ||
|
|
||
| invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e); | ||
|
|
||
| expect(recomputeForkChoiceHead).toHaveBeenCalledTimes(1); | ||
| expect(logger.info).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("forwards null LVH unchanged so protoArray can apply its 'unknown LVH' policy", () => { | ||
| const {chain, validateLatestHash} = makeChain(); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: null, | ||
| validationError: null, | ||
| }); | ||
|
|
||
| invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e); | ||
|
|
||
| expect(validateLatestHash).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| executionStatus: ExecutionStatus.Invalid, | ||
| latestValidExecHash: null, | ||
| invalidateFromParentBlockRoot: headBlockRoot, | ||
| invalidateFromParentBlockHash: headBlockHash, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it("swallows validateLatestHash failures and skips head recompute so the FCU catch handler does not crash the import path", () => { | ||
| const {chain, validateLatestHash, recomputeForkChoiceHead} = makeChain(); | ||
| validateLatestHash.mockImplementationOnce(() => { | ||
| throw new Error("invalidateFromParentBlockRoot not in forkchoice"); | ||
| }); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: lvh, | ||
| validationError: null, | ||
| }); | ||
|
|
||
| expect(() => invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e)).not.toThrow(); | ||
| expect(recomputeForkChoiceHead).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("swallows recomputeForkChoiceHead failures so the FCU catch handler does not crash the import path", () => { | ||
| const {chain, recomputeForkChoiceHead} = makeChain(); | ||
| recomputeForkChoiceHead.mockImplementationOnce(() => { | ||
| throw new Error("findHead failed"); | ||
| }); | ||
| const e = new ForkchoiceUpdateError({ | ||
| code: ForkchoiceUpdateErrorCode.INVALID, | ||
| headBlockHash, | ||
| latestValidHash: lvh, | ||
| validationError: null, | ||
| }); | ||
|
|
||
| expect(() => invalidateForkchoiceHeadFromFcuInvalid(chain, headBlockRoot, headBlockHash, e)).not.toThrow(); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After invalidating the head in the fork-choice store via
validateLatestHash, it is highly recommended to trigger a head recompute in theBeaconChaininstance. This ensures that the node immediately switches to the correct branch, updates its internal head state (e.g.,this.regen.head), and emits the necessary head events. Without this, the node might remain on the invalid head until the next slot start or block import, which could lead to invalid attestations or proposals if the node is a validator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — confirmed.
validateLatestHashcallsprotoArray.applyScoreChanges(updatesbestChild/bestDescendantinternally) but does NOT callfindHeadand does NOT updateForkChoice.head. The cached head would stay on the now-invalid block untilprepareNextSlot(~slot+8s) or the nextimportBlock(~slot+12s), so a validator attesting at slot+4s could vote for the stale invalid head.Fixed in ef2a182: after
validateLatestHashsucceeds, the helper now callschain.recomputeForkChoiceHead(ForkchoiceCaller.forkchoiceUpdateInvalid). Added a new caller value for clean metrics, log an info when the head actually switches, and added two unit tests covering the recompute path + the failure-swallowing.Note: I'm not also wiring the full reorg side-effects (regen
updateHeadState,ChainEvent.reorgemission, etc.) thatimportBlockdoes after its ownrecomputeForkChoiceHead. That's a deeper change touching more state and I'd rather keep this PR focused on breaking the wedge — happy to do a follow-up if you'd prefer the full treatment in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This helper already does the proto-array recompute via
chain.recomputeForkChoiceHead(ForkchoiceCaller.forkchoiceUpdateInvalid), so the missing piece is a bit narrower: after that recompute we still do not mirror the usual chain-level head switch path (regen.updateHeadState(...)+ head event / related metrics). So fork choice moves immediately, but the chain-facing head can still stay stale until the next slot or import. I agree that follow-up is needed there.