-
-
Notifications
You must be signed in to change notification settings - Fork 465
fix: reject beacon state requests while syncing to avoid regen wedge #9641
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
base: unstable
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,9 @@ import { | |
| } from "@lodestar/types"; | ||
| import {byteArrayEquals, fromHex} from "@lodestar/utils"; | ||
| import {IBeaconChain} from "../../../../chain/index.js"; | ||
| import {IBeaconSync} from "../../../../sync/index.js"; | ||
| import {ApiError, ValidationError} from "../../errors.js"; | ||
| import {notWhileSyncing} from "../../utils.js"; | ||
|
|
||
| export function resolveStateId( | ||
| forkChoice: IForkChoice, | ||
|
|
@@ -51,8 +53,13 @@ export function resolveStateId( | |
|
|
||
| export async function getStateResponseWithRegen( | ||
| chain: IBeaconChain, | ||
| sync: IBeaconSync, | ||
| inStateId: routes.beacon.StateId | ||
| ): Promise<{state: IBeaconStateView | Uint8Array; executionOptimistic: boolean; finalized: boolean}> { | ||
| // A far-behind node must not serve state lookups: a REST-triggered regen can walk back past the | ||
| // block-root window (SLOTS_PER_HISTORICAL_ROOT) and wedge the node. Reject with 503 while syncing. | ||
| notWhileSyncing(chain, sync); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because this new guard now runs for every state lookup, a node that is at the current slot but temporarily has no connected peers returns 503 for local state: Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the peer-stalled case. With the bypass in For arbitrary slot / state-root lookups we keep the conservative 503 while |
||
|
|
||
| const stateId = resolveStateId(chain.forkChoice, inStateId); | ||
|
|
||
| const res = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,54 @@ | ||
| import {ApiError} from "./errors.js"; | ||
| import {SLOTS_PER_EPOCH} from "@lodestar/params"; | ||
| import type {IBeaconChain} from "../../chain/index.js"; | ||
| import {type IBeaconSync, SyncState} from "../../sync/index.js"; | ||
| import {ApiError, NodeIsSyncing} from "./errors.js"; | ||
|
|
||
| /** | ||
| * If the node is within this many epochs from the head, we declare it to be synced regardless of | ||
| * the network sync state. | ||
| * | ||
| * This helps prevent attacks where nodes can convince us that we're syncing some non-existent | ||
| * finalized head. | ||
| * | ||
| * TODO: Lighthouse uses 8 for the attack described above. However, 8 kills Lodestar since validators | ||
| * can trigger regen to fast-forward head state 8 epochs to be immediately invalidated as sync sets | ||
| * a new head. Then the checkpoint state cache grows unbounded with very different states (because | ||
| * they are 8 epochs apart) and causes an OOM. Research a proper solution once regen and the state | ||
| * caches are better. | ||
| */ | ||
| export const SYNC_TOLERANCE_EPOCHS = 1; | ||
|
|
||
| /** | ||
| * Reject any request while the node is syncing. Used by endpoints that must not serve while the | ||
| * node is behind — validator duties, and beacon state lookups whose regen could otherwise walk | ||
| * back past the block-root window (`SLOTS_PER_HISTORICAL_ROOT`) and wedge the node. Throws | ||
| * {@link NodeIsSyncing} (503). | ||
| */ | ||
| export function notWhileSyncing(chain: IBeaconChain, sync: IBeaconSync): void { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should take a sync state instead of the whole sync module?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — One sequencing note so this does not get reworked: @twoeths is leaning toward not changing serving behavior at all and instead bounding the regen at the source (closer to the closed #9634 — only fail a lookup that would have to forward-regen past |
||
| // Consider node synced before or close to genesis | ||
| if (chain.clock.currentSlot < SLOTS_PER_EPOCH) { | ||
| return; | ||
| } | ||
|
|
||
| switch (sync.state) { | ||
| case SyncState.SyncingFinalized: | ||
| case SyncState.SyncingHead: { | ||
| const currentSlot = chain.clock.currentSlot; | ||
| const headSlot = chain.forkChoice.getHead().slot; | ||
| if (currentSlot - headSlot > SYNC_TOLERANCE_EPOCHS * SLOTS_PER_EPOCH) { | ||
| throw new NodeIsSyncing(`headSlot ${headSlot} currentSlot ${currentSlot}`); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| case SyncState.Synced: | ||
| return; | ||
|
|
||
| case SyncState.Stalled: | ||
| throw new NodeIsSyncing("waiting for peers"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensures that the array contains unique values, and throws an ApiError | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| import {describe, expect, it} from "vitest"; | ||
| import {toHexString} from "@chainsafe/ssz"; | ||
| import {SLOTS_PER_EPOCH} from "@lodestar/params"; | ||
| import {BeaconStateView} from "@lodestar/state-transition"; | ||
| import {getStateValidatorIndex} from "../../../../../../src/api/impl/beacon/state/utils.js"; | ||
| import {getStateResponseWithRegen, getStateValidatorIndex} from "../../../../../../src/api/impl/beacon/state/utils.js"; | ||
| import {NodeIsSyncing} from "../../../../../../src/api/impl/errors.js"; | ||
| import {IBeaconChain} from "../../../../../../src/chain/index.js"; | ||
| import {IBeaconSync, SyncState} from "../../../../../../src/sync/index.js"; | ||
| import {generateCachedAltairState} from "../../../../../utils/state.js"; | ||
|
|
||
| describe("beacon state api utils", () => { | ||
|
|
@@ -61,4 +65,17 @@ describe("beacon state api utils", () => { | |
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("getStateResponseWithRegen", () => { | ||
| it("throws NodeIsSyncing while the node is behind and syncing", async () => { | ||
| const chain = { | ||
| clock: {currentSlot: 10 * SLOTS_PER_EPOCH}, | ||
| forkChoice: {getHead: () => ({slot: 0})}, | ||
| } as unknown as IBeaconChain; | ||
| const sync = {state: SyncState.SyncingFinalized} as unknown as IBeaconSync; | ||
|
|
||
| // notWhileSyncing runs before any regen, so a minimal clock/forkChoice/sync mock is enough | ||
| await expect(getStateResponseWithRegen(chain, sync, "head")).rejects.toThrow(NodeIsSyncing); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we bypassed the it("throws NodeIsSyncing while the node is behind and syncing for arbitrary slots", async () => {
const chain = {
clock: {currentSlot: 10 * SLOTS_PER_EPOCH},
forkChoice: {getHead: () => ({slot: 0})},
} as unknown as IBeaconChain;
const sync = {state: SyncState.SyncingFinalized} as unknown as IBeaconSync;
// notWhileSyncing runs before any regen, so a minimal clock/forkChoice/sync mock is enough
await expect(getStateResponseWithRegen(chain, sync, 5)).rejects.toThrow(NodeIsSyncing);
});
it("does not throw NodeIsSyncing for head, finalized, justified, or genesis", async () => {
const chain = {
clock: {currentSlot: 10 * SLOTS_PER_EPOCH},
forkChoice: {
getHead: () => ({slot: 0, stateRoot: "0x00"}),
getFinalizedCheckpoint: () => ({epoch: 0, root: "0x00"}),
getJustifiedCheckpoint: () => ({epoch: 0, root: "0x00"}),
getFinalizedBlock: () => ({slot: 0}),
},
getStateByStateRoot: async () => ({state: {}, executionOptimistic: false, finalized: true}),
getStateOrBytesByCheckpoint: async () => ({state: {}, executionOptimistic: false, finalized: true}),
getStateBySlot: async () => ({state: {}, executionOptimistic: false, finalized: true}),
} as unknown as IBeaconChain;
const sync = {state: SyncState.SyncingFinalized} as unknown as IBeaconSync;
await expect(getStateResponseWithRegen(chain, sync, "head")).resolves.toBeDefined();
await expect(getStateResponseWithRegen(chain, sync, "finalized")).resolves.toBeDefined();
await expect(getStateResponseWithRegen(chain, sync, "justified")).resolves.toBeDefined();
await expect(getStateResponseWithRegen(chain, sync, "genesis")).resolves.toBeDefined();
});
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in |
||
| }); | ||
| }); | ||
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.
Rejecting all state requests with a 503
NodeIsSyncingwhile syncing is too aggressive. Standard queries for static or already available states like"head","finalized","justified", and"genesis"do not trigger any historical state regeneration that could walk back blocks and wedge the node. Blocking these endpoints completely breaks basic node observability, dashboards, and validator client checks during sync. We should bypass thenotWhileSyncingcheck for these specific state IDs.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.
Fixed in
d777f859— applied the bypass exactly as suggested.head/finalized/justified/genesisnow skip the guard (they resolve to already-available cached states or a genesis DB read, no regen), so only regen-capable lookups (arbitrary slots / state roots) return 503 while syncing.