cl/phase1/forkchoice: use the incremental indexed weight store for the GLOAS head#21694
Open
yperbasis wants to merge 11 commits into
Open
cl/phase1/forkchoice: use the incremental indexed weight store for the GLOAS head#21694yperbasis wants to merge 11 commits into
yperbasis wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Caplin’s fork-choice head selection to compute head weights incrementally using the existing indexedWeightStore (instead of per-GetHead full validator scans / sampled approximations), and wires this approach into both pre-GLOAS and GLOAS head paths. It also removes the probabilistic/sampled head-weight path and adds a differential test validating indexed vs full-scan weight calculations.
Changes:
- Switch pre-GLOAS
getHeadand GLOASgetHeadGloasto score candidates via an incrementalindexedWeightStoreseeded with the per-query justified checkpoint state. - Make
indexedWeightStorestructural (root → vote entries) and read balances/active/slashed status fresh from the checkpoint state during scoring; prune indexed votes on finalization. - Remove probabilistic head getter plumbing and add a new diff test + benchmark comparing indexed vs full-scan scoring.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/caplin/caplin1/run.go | Removes probabilistic head-weight sampling flag/wiring when constructing forkchoice store. |
| cl/spectest/consensus_tests/fork_choice.go | Updates spectest forkchoice store construction for new constructor signature. |
| cl/phase1/forkchoice/weight_store.go | Adds headWeightStore(cs) to reuse the incremental indexed store during head walks. |
| cl/phase1/forkchoice/weight_store_indexed.go | Removes stored balances from indexed votes, adds per-pass checkpointState, adds finalized pruning, and updates scoring to read balances fresh. |
| cl/phase1/forkchoice/weight_store_diff_test.go | Adds differential test ensuring indexed store matches full-scan store + a benchmark comparison. |
| cl/phase1/forkchoice/utils.go | Prunes indexed votes when new finalized checkpoint is observed. |
| cl/phase1/forkchoice/get_head.go | Replaces legacy vote accounting/sampling with incremental weight scoring for both GLOAS and pre-GLOAS head selection. |
| cl/phase1/forkchoice/forkchoice.go | Removes probabilistic head getter field/param; constructor signature updated accordingly. |
| cl/phase1/forkchoice/fork_choice_test.go | Updates tests for the constructor signature change and adds an end-to-end ex-ante proposer boost vs attestations test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d3df030 to
c360d5c
Compare
getHeadGloas scored the head with the full-scan weightStore while the incremental indexedWeightStore — already maintained on every attestation — went unused. Score the GLOAS head via the index instead. Make the index structural: store only (validator, slot, payloadPresent) per target root and read balance + active/slashed status fresh from a per-query justified checkpoint state in GetAttestationScore, instead of caching balances that go stale when the checkpoint advances. Drop indexed votes for finalized-away roots in onNewFinalized. Pre-GLOAS fork choice is unchanged. Validated by a differential test asserting the index equals the full-scan weightStore for GetWeight/GetAttestationScore across the filtered tree.
c360d5c to
b8aac17
Compare
Resolves the on_attestation.go conflict by taking #21698's GLOAS-gated index maintenance (the stale IndexVote comment this branch fixed was removed there). Since #21698 gates indexed-vote maintenance to the GLOAS vote path, the differential test's pre-GLOAS fixtures no longer populate the index, so buildExAnteStore now mirrors latestMessages into the index directly.
…ial test headWeightStore documents that f.mu must be held for the scoring pass; keep that contract in the test (getCheckpointState stays outside the lock, mirroring production).
RemoveVote allocated a fresh slice per call to filter out a validator's entry. On the GLOAS vote path it runs per attestation, so on high-validator-count networks it is a top CPU consumer and GC source under the fork-choice lock (the cost #21698 removed pre-GLOAS but which returns under GLOAS now that getHeadGloas reads the index). Compact the entry out in place instead.
… the benchmark GetAttestationScore converted entry.ValidatorIndex to int before the bounds check, so an index past math.MaxInt (e.g. on a 32-bit build) could wrap negative and bypass it. Bounds-check the uint64 first, convert only once it is in range. Also hold f.mu across the differential benchmark's scoring loops, matching the headWeightStore contract the test already follows.
…mark cs GetIndexedWeightStore has no callers and now exposes a store that returns 0 unless primed via the unexported setCheckpointState, so remove it. The differential benchmark ignored getCheckpointState's error/nil result and could silently measure the degenerate cs==nil path; assert it like the correctness test does.
getHeadGloas reads the indexedWeightStore, but index maintenance is GLOAS-gated (#21698): pre-GLOAS attestations fill latestMessages without touching the index. At the pre-GLOAS->GLOAS boundary GetHead flips from the full-scan store (which reads all of latestMessages) to the index, still empty — so attestation weight is understated until validators re-attest over ~1 epoch, risking a wrong head if a fork straddles the transition. The spec keeps store.latest_messages as a single dict never reset across forks, and Prysm/Lighthouse keep one continuous per-validator vote store; Erigon already maintains latestMessages continuously, so make the derived index match it at first use: headWeightStore imports the full latestMessages snapshot into the index once (clear-then-rebuild, so early GLOAS votes are not double-counted). The differential test now starts from a cold index and asserts headWeightStore seeds it to equal the full-scan store; a new test covers seed idempotency. Interim fix; superseded by the delta-propagation rework tracked in #21704.
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
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.
Summary
Compute the GLOAS fork-choice head (
getHeadGloas) from the incrementalindexedWeightStoreinstead of the full-scanweightStore. The index ismaintained on every GLOAS attestation (see #21698) but was not previously read
for head selection. Because that maintenance is GLOAS-gated, the index is cold
at the pre-GLOAS→GLOAS transition;
getHeadGloasseeds it fromlatestMessageson first use so it is not missing votes cast before activation.
Scoped to GLOAS only — pre-GLOAS fork choice is unchanged.
Changes
getHeadGloasscores viaheadWeightStore(cs)(the index), fetching thejustified checkpoint state before acquiring
f.mu(a nil state degrades tozero attestation weight, as before — same pattern as pre-GLOAS
getHead).indexedWeightStoreis now structural: it stores only(validator, slot, payloadPresent)per target root and reads balance +active/slashed status fresh from the per-query justified checkpoint state
in
GetAttestationScore, instead of caching balances that go stale when thecheckpoint advances.
IndexVoteno longer does a per-votegetCheckpointStatelookup.
RemoveVoteis now allocation-free (in-place compaction). It runs perattestation on the GLOAS vote path, so the previous fresh-slice-per-call was a
CPU/GC hotspot under the fork-choice lock on high-validator-count networks.
onNewFinalizeddrops indexed votes for finalized-away roots.getHeadGloas,headWeightStoreseeds the index once from thefull
latestMessagessnapshot (seedFromLatestMessages, clear-then-rebuild).Index maintenance only begins at GLOAS activation, so without the seed the head
would undercount attestation weight at the transition until validators
re-attest.
GetIndexedWeightStoreaccessor,GetWeightStore(orphaned oncegetHeadGloasswitched toheadWeightStore;NewWeightStorestays forisHeadWeak),Invalidate(cleared votes but leftseededset, so any future caller would silently undercount weights), and thewrite-only
versioncounter.Testing
TestIndexedWeightStoreMatchesFullScan: starts from a cold index, hasheadWeightStoreseed it, then asserts it returns the sameGetWeight/GetAttestationScoreas the trusted full-scanweightStorefor every node inthe filtered tree (non-vacuous) — exercising the real seed path.
TestSeedFromLatestMessagesIsIdempotent: the seed mirrorslatestMessagesonce and is a no-op thereafter (no double-counting).
TestRemoveVoteCompactsInPlace: removal keeps the other voters and drops theroot once empty.
TestPruneFinalizedDropsFinalizedAndUnknownRoots: votes targeting roots at orbelow the finalized slot, or missing from the fork graph, are dropped; votes
for live roots above it survive.
TestOnNewFinalizedPrunesIndexedVotes: theonNewFinalizedwiring empties theindex once finalization passes every indexed root. Both pruning tests were
mutation-checked: they fail against a no-op
pruneFinalized.go test ./cl/phase1/forkchoice/...and scopedgolangci-lintclean.Follow-up
The indexed store and its transition seed are superseded by the delta-propagation
rework tracked in #21704 (the canonical proto-array model both Prysm and
Lighthouse ship).