fix: reject beacon state requests while syncing to avoid regen wedge#9641
fix: reject beacon state requests while syncing to avoid regen wedge#9641lodekeeper wants to merge 2 commits into
Conversation
A far-behind node polled for a state via REST can trigger a regen that walks back past the block-root window (`SLOTS_PER_HISTORICAL_ROOT`) and wedges (blocks the main thread, 0 blocks imported). Guard the shared `getStateResponseWithRegen` entry point with the existing `notWhileSyncing` check so such requests return 503 (`NodeIsSyncing`) while the node is behind, matching the validator duties endpoints. - Extract `notWhileSyncing` + `SYNC_TOLERANCE_EPOCHS` from the validator API into the shared `api/impl/utils.ts` and reuse them (no behavior change for validator endpoints). - Thread `sync` into `getStateResponseWithRegen` and its callers (beacon state, proof, debug, lodestar, validator) so the single choke point covers all REST state-serving endpoints. - Test: `getStateResponseWithRegen` throws `NodeIsSyncing` while syncing. Alternative to ChainSafe#9634 (per @nflaig review) — simpler, and covers all REST state endpoints rather than only the far-slot regen case. 🤖 Generated with AI assistance Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request extracts the notWhileSyncing helper function and SYNC_TOLERANCE_EPOCHS constant to a shared utility file (utils.ts) and integrates the notWhileSyncing check into getStateResponseWithRegen to prevent far-behind nodes from wedging during state lookups. This requires passing the sync module to several API implementations (beacon state, debug, lodestar, and proof APIs). The review feedback suggests that rejecting all state requests with a 503 error while syncing is too aggressive, as queries for static or already available states (like "head", "finalized", "justified", and "genesis") do not trigger historical state regeneration. Consequently, the reviewer recommends bypassing the sync check for these specific state IDs and updating the corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // 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.
Rejecting all state requests with a 503 NodeIsSyncing while 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 the notWhileSyncing check for these specific state IDs.
| // 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); | |
| // 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, | |
| // except for "head", "finalized", "justified", and "genesis" which are already available and do not trigger regen. | |
| if (inStateId !== "head" && inStateId !== "finalized" && inStateId !== "justified" && inStateId !== "genesis") { | |
| notWhileSyncing(chain, sync); | |
| } |
There was a problem hiding this comment.
Fixed in d777f859 — applied the bypass exactly as suggested. head/finalized/justified/genesis now 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.
| 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); | ||
| }); |
There was a problem hiding this comment.
Since we bypassed the notWhileSyncing check for "head", "finalized", "justified", and "genesis", the existing test querying "head" will no longer throw NodeIsSyncing. We should update the test to query an arbitrary slot number (which should throw) and add a new test verifying that "head", "finalized", "justified", and "genesis" are successfully served without throwing.
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();
});There was a problem hiding this comment.
Done in d777f859 — the syncing-rejection test now uses an arbitrary slot (5 * SLOTS_PER_EPOCH), and I added a test asserting head/finalized/justified/genesis are still served while syncing. Both pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995c51df87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ): 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.
Preserve state reads when the node is only peer-stalled
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: BeaconSync.state can report SyncState.Stalled solely because the peer-count check fails, and notWhileSyncing treats Stalled as unconditional NodeIsSyncing. That is not the far-behind regen path this change is protecting, but it blocks /eth/v1/beacon/states/head/* and the debug/proof/lodestar callers from serving already-available state during a peer outage; consider applying the same head-slot distance check for Stalled or using a state-specific guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the peer-stalled case. With the bypass in d777f859, head/finalized/justified/genesis are now served even during a peer outage, which covers the main observability/dashboard/VC paths.
For arbitrary slot / state-root lookups we keep the conservative 503 while Stalled: this reuses the shared notWhileSyncing so the state endpoints stay consistent with the duties endpoints, and a node with no peers can't confirm it's actually at the network head. If you'd prefer Stalled gated on head-slot distance for the state path specifically, I can add that — but it would diverge the state guard from the shared helper, so I left it out for now.
Address review (Gemini): `head`/`finalized`/`justified` resolve to already-available cached states (and `genesis` to a historical DB read) and never trigger the forward regen that can wedge a far-behind node. Returning 503 for them while syncing needlessly breaks node observability, dashboards, and validator-client checks, so only guard the regen-capable lookups (arbitrary slots and state roots). Tests: the syncing-rejection case now uses an arbitrary slot; add a test that `head`/`finalized`/`justified`/`genesis` are still served while syncing. 🤖 Generated with AI assistance Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
twoeths
left a comment
There was a problem hiding this comment.
need to take a step back a little bit on why do we have to take a state and dial to a slot and call regen.getBlockSlotState() api
the beacon api did not specify that. If we have an unfinalized missed slot, we can just return 404 I think.
lodekeeper
left a comment
There was a problem hiding this comment.
I think this is a fair point. The current Lodestar behavior is broader than what the beacon API requires: for a numeric, unfinalized slot we find the closest canonical block <= that slot and regen forward, so a missed slot can still be served as a synthetic "state at slot". That is convenient for some state-at-slot consumers, but it is not mandated by the API, and this incident is a good example of why letting REST requests trigger arbitrary forward regen is risky.
For the immediate wedge fix, this PR keeps the behavior unchanged and only returns 503 while the node is syncing/stalled, so a far-behind node can recover instead of burning the main thread. But I agree the cleaner follow-up may be to tighten numeric slot lookups: if the requested unfinalized slot has no canonical block/state we can identify without forward regen, return 404 rather than dialing a previous block state forward. That would avoid this class of missed-slot regen work by contract.
I can either keep #9641 as the minimal sync guard and open that behavior change separately, or pivot this PR if you prefer the 404-on-unfinalized-missed-slot semantics here.
twoeths
left a comment
There was a problem hiding this comment.
on another thought, it would be better for consumers to return validator data for a skipped slot, for example, also I don't want to change behavior just because of this bug
need to dedup notWhileSyncing() through
|
@twoeths thanks for the look — two things: Dedup — already done in this PR: On not changing behavior for serveable slots — fair point, and worth settling before this goes further. This actually started as #9634, which did the narrower thing you're describing: a regen-level fast-fail that only rejected when So the tradeoff is:
Happy to switch back to bounding regen, or keep this shape but only trip the guard once we are far enough behind that regen genuinely can not succeed (rather than at 1 epoch), which preserves skipped-slot serving. @nflaig — given @twoeths' point, do you want to keep the |
| * 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 { |
There was a problem hiding this comment.
maybe this should take a sync state instead of the whole sync module?
There was a problem hiding this comment.
Agreed — SyncState is cleaner: narrower dep, and it drops the IBeaconSync mock in tests. Will narrow notWhileSyncing to take it.
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 SLOTS_PER_HISTORICAL_ROOT, i.e. the genuinely unserveable case). If we land there, the state endpoints drop notWhileSyncing and it goes back to a validator/index.ts closure, which makes the shared-helper signature moot. So I will fold this in once the approach is settled (pending @nflaig) — good call if the shared helper stays.
Problem
A far-behind node (e.g. resumed from a stale DB, hundreds of epochs behind) that is polled for a beacon state via REST can trigger a
regenthat needs to walk back further thanSLOTS_PER_HISTORICAL_ROOT(8192) slots. Regen can't get a block root more than 8192 slots in the past, so the node wedges: the request grinds on the main thread, 0 blocks get imported, and the only recovery is detaching the poller or checkpoint-syncing fresh.Fix
Guard the shared
getStateResponseWithRegenentry point with the existingnotWhileSyncingcheck, so REST state requests return 503NodeIsSyncingwhile the node is behind — before regen is ever reached. This matches how the validator duties endpoints already behave (and other clients).notWhileSyncing+SYNC_TOLERANCE_EPOCHSfrom the validator API into sharedapi/impl/utils.tsand reuse them (no behavior change for validator endpoints).syncintogetStateResponseWithRegenand its callers (beaconstate,proof,debug,lodestar, validator duties). It's the single choke point for all REST state-serving endpoints, so guarding it there covers all of them.head/finalized/justified/genesisare exempt from the guard — they resolve to already-available states (no regen), so they keep serving during sync (observability, dashboards, VC checks).getStateResponseWithRegenthrowsNodeIsSyncingfor a regen-capable slot while syncing, and still serves the four always-available state ids.Relation to #9634
This is the
notWhileSyncingalternative to #9634, per @nflaig's review. It's simpler (drops the customRegenError/SLOT_TOO_FAR_FROM_BLOCKmachinery) and covers all REST state-serving endpoints, not just the far-slot regen case. If this is preferred, #9634 can be closed.Behavior note
While the node is more than
SYNC_TOLERANCE_EPOCHSbehind, this returns 503 for regen-capable state lookups (arbitrary slots / state roots).head/finalized/justified/genesiskeep serving (they never trigger regen). This is broader than the far-slot-only guard in #9634 but consistent with the duties endpoints and other clients.🤖 Generated with AI assistance