Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve fork choice performance by preventing re-aggregation of historical gossip signatures (pruning them once successfully aggregated), and to update a devnet fork-choice reorg test to match the newer 5-interval-per-slot model.
Changes:
- Prune per-validator
gossip_signaturesentries once they’ve been included in an aggregated proof. - Update
test_reorg_with_slot_gapstick timings intended to align with a 5-interval slot schedule.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/consensus/devnet/fc/test_fork_choice_reorgs.py | Adjusts TickStep timing intended to match 5-interval-per-slot behavior. |
| src/lean_spec/subspecs/forkchoice/store.py | Removes successfully aggregated signatures from gossip_signatures to avoid repeated work across slots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| time=(7 * 5 + 4), # Slot 7, interval 4 (acceptance) | ||
| checks=StoreChecks( |
There was a problem hiding this comment.
TickStep.time is passed directly to Store.on_tick() as a Unix timestamp in seconds, but these new values treat time as if it were an (slot * intervals + interval) counter. With SECONDS_PER_SLOT=4 and INTERVALS_PER_SLOT=5, interval boundaries are 0.8s, so 7 * 5 + 4 seconds will advance the store well past slot 7 and may make the check flaky/incorrect. Consider computing the tick time from SECONDS_PER_SLOT (e.g., advance to the next slot boundary (Slot(7) + 1) * SECONDS_PER_SLOT, which will include interval-4 processing for slot 7) or otherwise deriving an actual second-based timestamp consistent with MILLISECONDS_PER_INTERVAL.
| time=(9 * 5 + 4), # Slot 9, interval 4 (end of slot) | ||
| checks=StoreChecks( |
There was a problem hiding this comment.
Same issue as above: time=(9 * 5 + 4) is being treated like an interval counter, but Store.on_tick() expects seconds. With 4s slots and 5 intervals/slot, this will advance time significantly beyond slot 9. Prefer deriving the tick timestamp from SECONDS_PER_SLOT (e.g., tick to (Slot(9) + 1) * SECONDS_PER_SLOT to ensure interval-4 acceptance for slot 9 occurs during ticking).
| @@ -988,7 +989,17 @@ def aggregate_committee_signatures(self) -> "Store": | |||
| if sig_key not in new_aggregated_payloads: | |||
| new_aggregated_payloads[sig_key] = [] | |||
| new_aggregated_payloads[sig_key].append(aggregated_signature) | |||
| return self.model_copy(update={"latest_new_aggregated_payloads": new_aggregated_payloads}) | |||
|
|
|||
| # Prune successfully aggregated signature from gossip map | |||
| if sig_key in new_gossip_sigs: | |||
| del new_gossip_sigs[sig_key] | |||
|
|
|||
| return self.model_copy( | |||
| update={ | |||
| "latest_new_aggregated_payloads": new_aggregated_payloads, | |||
| "gossip_signatures": new_gossip_sigs, | |||
| } | |||
There was a problem hiding this comment.
This change alters aggregate_committee_signatures() semantics by pruning entries from gossip_signatures after aggregation. There are existing tests calling aggregate_committee_signatures() (e.g., in tests/lean_spec/subspecs/forkchoice/test_attestation_target.py) but none appear to assert the post-condition that successfully aggregated SignatureKeys are removed. Adding a focused assertion-based test would help prevent regressions (e.g., accidental re-aggregation / O(N^2) behavior returning).
| @@ -988,7 +989,17 @@ def aggregate_committee_signatures(self) -> "Store": | |||
| if sig_key not in new_aggregated_payloads: | |||
| new_aggregated_payloads[sig_key] = [] | |||
| new_aggregated_payloads[sig_key].append(aggregated_signature) | |||
| return self.model_copy(update={"latest_new_aggregated_payloads": new_aggregated_payloads}) | |||
|
|
|||
| # Prune successfully aggregated signature from gossip map | |||
| if sig_key in new_gossip_sigs: | |||
| del new_gossip_sigs[sig_key] | |||
There was a problem hiding this comment.
new_gossip_sigs = dict(self.gossip_signatures) copies the entire gossip signature map even when aggregated_results is empty. Given the PR goal is performance, it would be cheaper to only copy/prune when there is at least one aggregated result (or compute a keys_to_prune set first and then build a filtered dict).
2 participants
🗒️ Description
This PR optimizes the performance of the fork choice store by pruning successfully aggregated gossip signatures and fixes failing test vectors caused by the transition to a 5-interval-per-slot configuration.
Previously, signatures were kept in the map indefinitely, causing the aggregator to re-aggregate historical signatures from all previous slots every time a new slot was processed. This led to$O(N^2)$ computational complexity relative to chain length. By pruning signatures once they are successfully aggregated into a proof, we ensure each signature is processed exactly once, reducing the fill command execution time.
Updates the test_reorg_with_slot_gaps test in test_fork_choice_reorgs.py. The test had hardcoded TickStep times based on the old 4-interval-per-slot configuration. These have been updated to the current 5-interval-per-slot timing (using interval 4 for end-of-slot acceptance) to restore test correctness.
🔗 Related Issues or PRs
N/A
✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox