fix(storage): store fork-choice votes independently - #552
Conversation
Greptile SummaryThis PR separates latest fork-choice votes from bounded proof retention.
Confidence Score: 4/5This PR should not merge until vote pruning preserves latest messages whose heads remain relevant above the finalized checkpoint. The new pruning predicate uses the attestation target, while LMD-GHOST consumes its head; after finalization this can remove live-branch voting weight from subsequent head calculations. Files Needing Attention: crates/storage/src/store.rs
|
| Filename | Overview |
|---|---|
| crates/storage/src/store.rs | Adds the independent latest-vote store and ingestion paths, but pruning by target slot can remove votes whose heads remain relevant to fork choice. |
| crates/blockchain/src/store.rs | Records validated block-included attestation votes after persisting the imported block and state. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Known aggregate] --> V[Independent latest-vote store]
B[Block-included attestation] --> V
A --> P[Bounded proof buffer]
V --> F[LMD-GHOST fork choice]
C[Finalization advances] --> R[Prune votes by target slot]
R --> V
Prompt To Fix All With AI
### Issue 1
crates/storage/src/store.rs:1087-1090
**Target-based pruning drops live votes**
When a validator's latest attestation targets a finalized checkpoint but its head remains on a live post-finalization branch, `prune_known_votes` removes the vote based solely on `target.slot`. LMD-GHOST weights `head.root`, so subsequent head calculations lose that validator's weight and can select the wrong branch until a replacement vote arrives.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(storage): store fork-choice votes in..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
Looks good
Left some comments about pruning. The issue didn't mention it, but the main issue with how we stored votes is that proof are pruned regularly, which shouldn't affect votes: LMD-GHOST uses the latest message from each validator, so otherwise no pruning should happen.
Also, an additional PR is needed to finish addressing the issue, since we still need to replace the payloads and proofs in fork-choice with the new votes.
| fn new_vote_store() -> Arc<Mutex<VoteStore>> { | ||
| Arc::new(Mutex::new(Vec::new())) | ||
| } |
There was a problem hiding this comment.
This is just Default::default(), right?
There was a problem hiding this comment.
Yep, this is Default::default() now. I removed the helper and wrapped the vote maps in ForkChoiceState as requested.
| fn record_votes<I>(votes: &mut VoteStore, data: &AttestationData, validator_ids: I) | ||
| where | ||
| I: IntoIterator<Item = u64>, | ||
| { | ||
| for validator_id in validator_ids { | ||
| Self::record_vote(votes, validator_id, data); | ||
| } | ||
| } | ||
|
|
||
| fn record_known_votes<I>(&self, data: &AttestationData, validator_ids: I) | ||
| where | ||
| I: IntoIterator<Item = u64>, | ||
| { | ||
| let mut fork_choice = self.fork_choice.lock().unwrap(); | ||
| Self::record_votes(&mut fork_choice.known_votes, data, validator_ids); | ||
| } | ||
|
|
||
| fn record_new_votes<I>(&self, data: &AttestationData, validator_ids: I) | ||
| where | ||
| I: IntoIterator<Item = u64>, | ||
| { | ||
| let mut fork_choice = self.fork_choice.lock().unwrap(); | ||
| Self::record_votes(&mut fork_choice.new_votes, data, validator_ids); | ||
| } |
There was a problem hiding this comment.
We should inline these. No reason to use generics for 2 lines of code
| /// broken toward the larger canonical attestation-data root — the same rule the | ||
| /// block-level fork-choice tiebreak applies to block roots (leanSpec #1181). The | ||
| /// pool key is already `hash_tree_root(data)`, so the tie needs no extra hashing. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Why only tests? Can we remove this instead?
|
There are some conflicts |
2 participants
🗒️ Description / Motivation
This PR fixes fork-choice vote tracking so validator votes are stored independently from aggregated proofs and gossip signatures.
Previously, fork-choice votes were derived from bounded in-memory proof/signature buffers. When those buffers evicted old entries through FIFO cleanup, a validator’s latest vote could disappear even though the vote should still count for fork choice. This PR adds a separate proof-less vote store so fork-choice weights are not affected by proof/signature retention.
What Changed
Updated
crates/storage/src/store.rsUpdated
crates/blockchain/src/store.rsCorrectness / Behavior Guarantees
Tests Added / Run
Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(cargo test --workspace --profile release-fast) — all passing