[r3.6] cl/beacon: publish fork-choice head before state copy - #23172
Merged
Merged
Conversation
Cherry-pick of #23155 to release/3.6. r3.6-specific adaptations: - getHeadGloas keeps this branch's structure and only gains the publishSelectedHead call; main's closure restructure of that loop is an unrelated refactor that was never backported. - getFinalizedExecutionHashForkGraph in forkchoice_test.go gains a headers map and a GetHeader that consults it, matching main. This branch's fixture panicked instead, which the new selected-head test needs.
domiwei
approved these changes
Aug 11, 2026
domiwei
left a comment
domiwei
left a comment
Member
There was a problem hiding this comment.
Reviewed as a release/3.6 port of #23155. The selected-head publication points remain ordered by the fork-choice lock across cached, anchor-fallback, pre-Gloas, and Gloas paths; the release-specific Gloas loop preserves checkpoint revalidation and error behavior. State-backed endpoints retain the memoized state identity. No release-specific blockers found.
Sahil-4555
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Sep 21, 2026
…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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Cherry-pick of #23155 to release/3.6.
r3.6-specific adaptations
getHeadGloaskeeps this branch's structure and only gains thepublishSelectedHeadcall. Main restructured that loop into a closure in an unrelated refactor that was never backported, so importing it here would be scope creep.getFinalizedExecutionHashForkGraphinforkchoice_test.gogains aheadersmap and aGetHeaderthat consults it, matching main. This branch's fixture panicked instead, which the new selected-head test needs.Neither is a semantic conflict with the fix — both are pre-existing main-only changes this branch never received.
Testing
go test ./cl/...green, including the new selected-head and head-identity tests.make lintclean.