fix: ptc gossip to dedup by slot not by epoch - #9396
Closed
ensi321 wants to merge 2 commits into
Closed
Conversation
PayloadAttestationMessage gossip dedup was keyed per (epoch, validatorIndex), which is correct for phase0 attesters (one committee assignment per epoch) but wrong for PTC. PTC seats are sampled per slot via `compute_ptc`, so a single validator can be selected into PTC at multiple slots within the same epoch and must emit one valid message per such slot. The epoch-keyed cache silently dropped legitimate later-slot messages from the same validator. Replace `SeenPayloadAttesters extends SeenAttesters` with a stand-alone class holding two parallel maps: - per-slot dedup (`isKnown` / `add(slot, ...)`, `prune(slot)`) — mirrors `SeenSyncCommitteeMessages` with a 3-slot lookback to cover `MAXIMUM_GOSSIP_CLOCK_DISPARITY` and prev-slot stragglers - per-epoch liveness summary (`isKnownAtEpoch` / `pruneEpoch(epoch)`) — keeps the contribution to `validatorSeenAtEpoch` (validator liveness API) that the old epoch-keyed cache provided `add(slot, validatorIndex)` writes both maps atomically. Slot tick prunes the slot map; epoch tick prunes the epoch map. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the SeenPayloadAttesters cache to implement per-slot deduplication for payload attestations, replacing the previous per-epoch approach. This change allows validators to participate in the PTC multiple times per epoch across different slots while maintaining deduplication within a single slot. The implementation introduces dual tracking via validatorIndexesBySlot and validatorIndexesByEpoch to support both gossip validation and liveness APIs, along with updated pruning logic and unit tests. A review comment suggests adding a defensive check in the add method to prevent inserting data for epochs that have already been pruned, which would improve consistency with other cache implementations in the codebase.
Mirror SeenAttesters.add by throwing EpochTooLow when the message's epoch is below lowestPermissibleEpoch. Defensive — the gossip validator's isCurrentSlotGivenGossipDisparity check already filters stale slots — but catches programming bugs early and keeps the lowestPermissibleEpoch field load-bearing instead of write-only. Addresses PR #9396 review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
ensi321
marked this pull request as ready for review
May 22, 2026 05:03
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9396 +/- ##
=========================================
Coverage 52.55% 52.55%
=========================================
Files 848 848
Lines 60947 60947
Branches 4486 4486
=========================================
Hits 32032 32032
Misses 28853 28853
Partials 62 62 🚀 New features to boost your workflow:
|
twoeths
reviewed
May 22, 2026
twoeths
left a comment
twoeths
left a comment
Member
There was a problem hiding this comment.
according to ethereum/consensus-specs#5222 a validator can appear multiple times per slot, it did not mention epoch
for each slot, we shuffle committees of that slot
so if a validator is part of a ptc committee of a slot, it's also part of committee of that slot so it cannot be in committee of another slot of same epoch, hence it cannot be in ptc committees of any other slots of same epoch
Member
Author
Yea you are right. PTC committee is a subset of beacon committee. PTC selection allows duplicates in a slot, but a validator cannot be an attester for more than one slot per epoch. So validator can be in PTC multiple times per slot, but not per epoch but different slot. Closing this PR as the assessment is wrong. |
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.
Follow up on #9369 . Unlike attester, a validator can participate in PTC more than one time per slot and per epoch. Currently our
SeenPayloadAttestersassumes only one payload attestation message from a validator per epoch.This PR updates it so it is per slot