refactor(forkchoice): extract AttestationPool from Store#685
Closed
tcoratger wants to merge 1 commit intoleanEthereum:mainfrom
Closed
refactor(forkchoice): extract AttestationPool from Store#685tcoratger wants to merge 1 commit intoleanEthereum:mainfrom
tcoratger wants to merge 1 commit intoleanEthereum:mainfrom
Conversation
Three parallel dict fields on Store — attestation_signatures,
latest_new_aggregated_payloads, latest_known_aggregated_payloads — collapse
into a single immutable AttestationPool value object that owns the gossip →
new → known lifecycle.
The pool exposes one method per protocol step that previously lived as
deep-copy boilerplate inline on Store:
- prune_finalized(slot) — atomic three-stage prune at finalization advances
- add_signature(data, entry) — gossip arrival into the aggregator inbox
- add_new_proof(data, proof) — gossip arrival of an aggregated proof
- add_block_proofs(by_data) — block-included proofs land directly in known
- migrate_new_to_known() — slot-rollover stage promotion
- replace_after_aggregation(new_proofs) — wholesale new-stage swap, drops
consumed gossip signatures keyed by data
Five Store methods (prune_stale_attestation_data, on_gossip_attestation,
on_gossip_aggregated_attestation, on_block, accept_new_attestations,
aggregate) shed their {k: set(v) for k, v in d.items()} deep-copy patterns
and call the pool methods instead.
Read paths stay direct: aggregate, compute_block_weights, update_head,
update_safe_target, and produce_block_with_signatures access pool.signatures,
pool.new_proofs, and pool.known_proofs as plain attributes — there's no need
to encapsulate iteration when the call site already knows which stage it cares
about and the docstring explains why.
Field renames on the pool: latest_new_aggregated_payloads → new_proofs,
latest_known_aggregated_payloads → known_proofs, attestation_signatures →
signatures (the AttestationPool prefix already conveys "attestation").
Behavior preserved exactly:
- Pruning predicate is strict-greater (target.slot > finalized_slot)
- Aggregation replaces the new pool wholesale; lone-child early-exit drops
apply, gossip retention predicate is data-keyed
- on_block keeps its order: persist + advance checkpoints → process body →
update_head → prune
- accept_new_attestations migrates new → known and clears new in one shot
- AttestationSignatureEntry stays a NamedTuple (must be hashable for sets)
AttestationSignatureEntry moves from store.py to the new attestation_pool.py
since it has no consumer outside the pool.
Net: store.py shrinks by ~80 lines; one new module (attestation_pool.py)
and one new test file (test_attestation_pool.py, 17 cases covering each
method including the lone-child drop and gossip-retention edges flagged
as test gaps during research). All 3178 unit tests pass; all linter
checks pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Three parallel dict fields lived directly on
Store:Each was deep-copied with
{k: set(v) for k, v in d.items()}in five differentStoremethods. This PR collapses all three into a single immutableAttestationPoolvalue object that owns the gossip → new → known lifecycle, and rewrites those five methods to delegate to pool methods.Net diff:
store.pyshrinks by ~80 lines; one new module (attestation_pool.py); one new test file with 17 cases covering each pool method, including the edge cases flagged as test gaps during research (lone-child early-exit drop, gossip retention by data key).Pool API
prune_finalized(slot)prune_stale_attestation_databodyadd_signature(data, entry)on_gossip_attestationdeep-copy blockadd_new_proof(data, proof)on_gossip_aggregated_attestationdeep-copy blockadd_block_proofs(by_data)on_blockbody-attestation mergemigrate_new_to_known()accept_new_attestationsmerge stepreplace_after_aggregation(new)aggregateend-of-method bookkeepingRead paths stay direct:
aggregate,compute_block_weights,update_head,update_safe_target, andproduce_block_with_signaturesaccesspool.signatures,pool.new_proofs,pool.known_proofsas plain attributes — every read site already knows which stage it cares about and the docstrings explain why (see e.g. the safe-target rationale).Field renames on the pool
attestation_signatures→signatureslatest_new_aggregated_payloads→new_proofslatest_known_aggregated_payloads→known_proofsThe
AttestationPoolprefix already conveys "attestation"; thelatest_prefix was misleading (the maps don't store latest-vote-per-validator). No back-compat aliases — every call site is updated in this PR.Behavior preserved exactly
The design phase ran through both consensus-correctness and Pythonic-API reviews before any code was written. Specifically:
target.slot > finalized_slot); test boundaries verified.on_blockordering preserved: persist + advance checkpoints → process body attestations →update_head→ prune. Pruning runs on the post-update store as before.AttestationSignatureEntrystays aNamedTuple(must be hashable for sets) and moves toattestation_pool.pysince it has no consumer outside the pool.Tests
tests/lean_spec/subspecs/forkchoice/test_attestation_pool.pycovering each pool method, including the two edges flagged as gaps in the existing test suite during research:test_replace_after_aggregation_overwrites_new_pool(lone-child entries drop)test_replace_after_aggregation_keeps_unconsumed_signatures(gossip retention)tests/lean_spec/subspecs/forkchoice/andtests/consensus/devnet/fc/test suites all still pass.Test plan
uvx tox -e all-checks(ruff, format, ty, codespell, mdformat)uv run pytest tests/lean_spec/subspecs/forkchoice/ tests/lean_spec/subspecs/containers/test_state_aggregation.py tests/lean_spec/subspecs/validator/test_service.py— 150 passeduv run pytest tests/lean_spec --no-cov— 3178 passedreplace_after_aggregationsemantics against the existing aggregator behavior🤖 Generated with Claude Code