cl/beacon: publish fork-choice head before state copy - #23155
Conversation
… state The memoized head state's root and slot are stored only after two full CachingBeaconState copies complete, while the head SSE event is emitted several steps earlier. Resolving head from it served the parent root for the duration of those copies, which cost sync committee duties whose messages are signed against the head root partway into the slot. Fixes #23154
There was a problem hiding this comment.
Pull request overview
This PR fixes a Beacon API head-lag issue by resolving the head identity (root/slot) directly from fork choice (forkchoiceStore.GetHead()), instead of from the memoized head state metadata in syncedData, which only updates after expensive state copies complete. This aligns REST “head” responses with the earlier “head” SSE event timing and reduces the window where clients can observe a stale head.
Changes:
- Update
ApiHandler.getHead()to always read head root/slot from fork choice, while keeping the existing syncing guard behavior. - Add unit tests that reproduce the “fork choice ahead of memoized head state” scenario and assert the syncing guard still returns 503.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cl/beacon/handler/handler.go | Switch getHead() root/slot resolution to fork choice cached head. |
| cl/beacon/handler/head_test.go | Add regression tests for stale memoized-head vs fork-choice-head and syncing guard. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // getHead resolves the head from fork choice, not from the memoized head state: the latter | ||
| // only advances once the head state has been copied, so reading it hands out the parent root | ||
| // for the whole duration of that copy. | ||
| func (a *ApiHandler) getHead() (common.Hash, uint64, int, error) { | ||
| if a.enableMemoizedHeadState { | ||
| if a.syncedData.Syncing() { | ||
| return common.Hash{}, 0, http.StatusServiceUnavailable, errors.New("beacon node is syncing") | ||
| } | ||
| return a.syncedData.HeadRoot(), a.syncedData.HeadSlot(), 0, nil | ||
| if a.enableMemoizedHeadState && a.syncedData.Syncing() { | ||
| return common.Hash{}, 0, http.StatusServiceUnavailable, errors.New("beacon node is syncing") |
There was a problem hiding this comment.
Good catch on the naming — after this change the flag only gates the syncing check, which the name no longer describes.
Both suggested remedies break the test suite, though. setupTestingHandler passes enableMemoizedHeadState: false and, on that path, builds syncedData as a bare MockSyncedData with no Syncing() expectation. Removing the flag, or switching to a.syncedData != nil && a.syncedData.Syncing(), makes every head-resolving test call Syncing() on that mock and fail with an unexpected-call error — the mock is non-nil, so the nil guard does not help.
That leaves a rename. I would rather not fold it into this PR: it changes the NewApiHandler signature across three call sites, and this fix will need a clean cherry-pick to the release branches. Happy to do it as a follow-up, or add a clarifying comment on the field here if you would prefer something in-tree now.
domiwei
left a comment
domiwei
left a comment
There was a problem hiding this comment.
Two blocking findings before broadening getHead to all callers:
-
[High] handler.go:480 calls ForkChoiceStore.GetHead(nil), which is not a guaranteed cached RLock read. OnBlock, OnAttestation, and OnTick invalidate headHash. A public request that arrives after invalidation can therefore load checkpoint state, scan every validator vote instead of using the production auxiliary-state sampling path, take the fork-choice write lock, and populate the shared head cache. Concurrent requests that observed the miss also recompute serially because the cache is not rechecked after acquiring the write lock. API traffic can consequently change which computation path supplies the cached production head and amplify CPU and lock contention.
-
[High] the shared helper is also used by state_id=head callers. During the intended lag window it returns the new fork-choice root while ViewHeadState still exposes the previous memoized state. builder.go:61-78 can therefore classify the new root as head and return expected withdrawals computed from the parent state; validator and committee endpoints have the same root/state generation mismatch.
I recommend publishing a separate immutable selected-head identity immediately after the normal fork-choice stage computes it. Root/header-only endpoints can read that value without invoking GetHead, while state-backed endpoints continue to use the atomically consistent memoized head root/state pair. Tests should cover cold/warm publication, reorg to a lower slot, failed computation retaining the last published identity, concurrent root/slot reads, and the selected-head-ahead-of-state-head window.
domiwei
changed the title
|
Pushed The implementation now publishes selected head identity from fork choice itself, before EL notification and memoized-state copying, rather than recomputing fork choice in public handlers. Selected identity and state identity are intentionally separate atomic The adversarial follow-up also covered the reverse cases: lower-slot replacement, checkpoint-sync anchor fallback, Gloas, accepted side-branch local blocks, explicit current-head state roots, cold-state 503 behavior, and canonical header visibility before DB promotion. Local verification is recorded in the updated PR description. Two independent final review passes converged with no remaining actionable findings. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- cl/beacon/synced_data/mock_services/synced_data_mock.go: Generated file
Suppressed comments (3)
cl/beacon/synced_data/synced_data.go:71
OnSelectedHeadstores a freshly allocatedheadIdentityon every call. SinceForkChoiceStore.GetHead()calls this even on cached reads, hot read-only endpoints can create steady GC pressure for an identity that often does not change.
Consider avoiding the allocation/store when (root,slot) is unchanged (load current pointer and compare) so only actual head transitions allocate.
func (s *SyncedDataManager) OnSelectedHead(blockRoot common.Hash, blockSlot uint64) {
if !s.enabled {
return
}
s.selectedHead.Store(&headIdentity{root: blockRoot, slot: blockSlot})
cl/beacon/handler/block_production.go:2142
- The error message
failed to get head stateis misleading here: the nil check is forauxiliaryStatereturned fromGetStateAtBlockRoot(auxiliaryRoot, ...), not for the selected head state. This makes debugging harder when the auxiliary root is missing from the DB/cache.
if auxiliaryState == nil {
return common.Hash{}, 0, nil, errors.New("failed to get head state")
}
cl/beacon/handler/block_production.go:2156
- Similar to the earlier nil-state check,
failed to get selected head statedoesn’t include which root was missing. Including the root in the message (or naming it) would make logs actionable when this occurs in production.
if headState == nil {
return common.Hash{}, 0, nil, errors.New("failed to get selected head state")
}
|
@copilot review |
Reviewed the full change set. No actionable findings. The fixes are consistent with the earlier review findings:
|
|
@copilot review |
No new changes since the previous review pass. The findings from that review stand: no actionable issues remain in the current commit set. |
…under it (erigontech#24165) Fixes erigontech#24164. ## Problem `OnHeadStateWithBlockRoot`/`OnHeadState` copied the entire incoming `CachingBeaconState` while holding `s.mu`/`s.accessLock` for writing. `ViewHeadState`, `ViewPreviousHeadState`, and the single-field accessors (`ValidatorPublicKeyByIndex`, `ValidatorIndexByPublicKey`, `CommitteeCount`, `HistoricalRootElementAtIndex`, `HistoricalSummaryElementAtIndex`) all take the matching read lock, so every one of them was blocked for however long that copy took. This is the state-backed remainder of erigontech#23154 that erigontech#23155 explicitly scoped out: erigontech#23155 publishes the selected head identity (root/slot) before the state copy runs, so identity-only reads no longer wait on it, but state-backed reads still do. Any duty computation needing actual state content - not just a root - goes through `ViewHeadState` and is still exposed. Reproduced on a mainnet validator node running a release/3.7 dev build: sync-committee efficiency dropped to 94.60% (199/3686 missed) in a 24h window while attestation efficiency stayed at 99.59%, matching erigontech#23154's own description of this failure mode (attestation data is produced before the head-state update; sync-committee messages need the state after it). Full field evidence is in erigontech#24164. ## Change Both `OnHeadState` and `OnHeadStateWithBlockRoot` now acquire a new `writeLock sync.Mutex`, independent of `mu`/`accessLock`, before doing any work - `OnHeadState`'s `BlockRoot()` resolution included, not just the copy - then call a shared `publishHeadStateLocked` (documented as requiring the lock already held), which: 1. Copies the incoming state under `writeLock`. 2. Takes `mu`/`accessLock` only to swap the copy in as the new head state and demote the current head state to previous (`previousHeadState = headState; headState = copied` - a pointer reassignment, no data copy) - an O(1) critical section regardless of state size. `writeLock` matters because it isn't just about latency: without it, a writer that started earlier but is slower to resolve its root or copy its state could be overtaken and then overwritten by a writer that started later but finishes faster - a silent regression to a stale head, not just a slow reader. This applies equally to `OnHeadState` (root not yet known, one caller - `cmd/capcli`, called sequentially today, but still part of the exported `SyncedData` contract) and `OnHeadStateWithBlockRoot` (root given, used by every concurrent caller: forkchoice, forward sync, block production). `UnsetHeadState` also takes `writeLock` (before `mu`), so it can't land in the middle of an in-flight publish and then get resurrected by it. Net effect: lock hold time for readers is now O(1); writers stay fully serialized in arrival order, same guarantee the old single-mutex design gave for free. `previousHeadState` is now the actual prior `headState` object (aliased, not copied into a reused buffer) - same observable content, one fewer full-state copy per update. No behavior change to any exported signature. ## Testing Per this repo's TDD convention, each fix has a test that reproduces it against the commit before the fix and passes after: - `TestViewHeadStateDoesNotWaitForHeadStateCopy` - a `ViewHeadState` reader is not blocked for the incoming state's copy duration (only the swap). Uses `copyHookForTest`, a package-level hook bound to the copy itself via a small `copyState` helper (not just called before it - so the hook's pause point can't be separated from the copy by a future reorder), to pause a writer at an exact point and prove a concurrent read completes while it's genuinely paused there. - `TestOnHeadStateWithBlockRootSerializesConcurrentWriters` - a writer cannot be overtaken and overwritten by a writer that starts later. Deterministic: holds `writeLock` directly to represent a writer already inside its critical section. - `TestOnHeadStateSerializesAgainstOnHeadStateWithBlockRoot` - `OnHeadState`'s root resolution is covered by the same serialization as `OnHeadStateWithBlockRoot`'s copy, not just the copy step. Drives both writers through the real public API with a calibrated head start (documented in the test). - `TestUnsetHeadStateSerializesWithPublish` - `UnsetHeadState` cannot be resurrected by a publish that was already in flight when it was called. Same hold-`writeLock` technique. Plus: - `TestOnHeadStateWithBlockRootColdStart` - first update populates head state, no previous state yet. - `TestOnHeadStateWithBlockRootDemotesPriorHeadToPrevious` - `ViewPreviousHeadState` observes the state that was head right before the latest update, proving the alias-based swap is behaviorally equivalent to the old copy-based one. - `TestOnHeadStateWithBlockRootConcurrentReadWrite` - races real writers against real readers through the public API. Passes under `-race`. Full existing suite (`cl/beacon/synced_data`, `cl/beacon/handler`, `cl/phase1/stages`) passes unchanged, including the generation-consistency and TOCTTOU-regression tests added for erigontech#23155. All new tests pass under `-race`, repeated. ## Trade-offs - `OnHeadState`/`OnHeadStateWithBlockRoot` now always allocate a fresh state copy (`newState.Copy()`), rather than reusing a preallocated buffer via `CopyInto` on the steady-state path. This trades a modest increase in per-block allocation for removing the reader-blocking lock hold - the copy's own cost is unchanged either way, only whether readers wait for it. Flagging in case reviewers want a buffer-pooling follow-up instead; I kept this PR to the minimal change that fixes the blocking behavior. - Not backporting in this PR. release/3.6 already carries erigontech#23155's prerequisite (backported as erigontech#23172, included in v3.6.1) so this fix would cherry-pick cleanly there. release/3.5 never received erigontech#23155/erigontech#23172, so it still has the original (larger) erigontech#23154 bug, not just this remainder - a release/3.5 backport would need that fix first. Happy to open both as follow-ups if maintainers want them. - erigontech#24197 tracks two remaining test-hardening items from review (real-path writer-ordering coverage, `accessLock` accessor coverage) - explicitly scoped as non-blocking follow-ups, not production bugs.
Fixes #23154.
Problem
Beacon API
headidentity came from the memoized head state. That identity advances only after the new state has been copied, so block/root endpoints could return the parent for roughly 750 ms after fork choice had already selected the new head.Calling
forkchoiceStore.GetHead(nil)from public handlers avoids that lag, but it can perform expensive fork-choice/checkpoint-state work on a cache miss and can expose a selected root with the previous memoized state.Change
(root, slot)fromForkChoiceStore.GetHeadat the successful selection boundary, while the fork-choice lock still orders concurrent selections.headendpoints to selected head; state-backed endpoints continue using memoized state identity.Public read-only endpoints no longer invoke fork-choice computation when memoized head state is enabled.
Trade-offs
Testing
go test ./cl/beacon/handler -count=1go test ./cl/beacon/synced_data -count=1go test ./cl/phase1/forkchoice -count=1go test ./cl/phase1/stages -count=1go test -race ./cl/beacon/synced_data -run '^(TestSelectedHeadRootAndSlotStayInOneGeneration|TestStateHeadRootAndSlotStayInOneGeneration|TestViewHeadStateWithIdentityStaysInOneGeneration|TestViewHeadStateRejectsMissingStateAfterReadinessPublished)$' -count=1make lintmake erigon integrationFocused coverage includes lower-slot replacement, cache hit, checkpoint-sync anchor fallback, Gloas selection, local side-branch selection, root/slot generation coherence, explicit current-head state roots, cold-state 503 responses, optimistic debug-head metadata, and pre-commit canonical header responses.