eth: protect high-value peers from random dropping based on tx inclusion stats - #34702
Merged
Conversation
healthykim
reviewed
Apr 14, 2026
healthykim
reviewed
Apr 14, 2026
cskiraly
force-pushed
the
peerdrop-protected-pools
branch
from
June 23, 2026 07:50
aaecdbd to
e9ecf5d
Compare
… stats The dropper periodically disconnects random peers to create churn. This was blind to peer quality. Add inclusion-based peer protection using two categories: 1. Total inclusions: protects peers with the highest cumulative count of delivered txs that were included on chain 2. Recent inclusions (EMA): protects peers with the best recent inclusion rate, giving newly productive peers faster protection Each category independently protects the top 10% of inbound and top 10% of dialed peers. The union of both sets is protected. Only peers with positive scores qualify. The dropper defines its own PeerInclusionStats struct and callback type (getPeerInclusionStatsFunc) so any stats provider (e.g. a transaction tracker) can plug in without a package dependency. The callback is nil by default (protection disabled until wired). The protectionCategories slice is designed for easy extension — adding a new category requires only appending a struct with a name, scoring function, and protection fraction.
Minimal txtracker that records which peer delivered each transaction and credits peers when their transactions appear on chain. Provides the PeerInclusionStats needed by the dropper's protection logic. Design: - NotifyReceived(peer, txs): records deliverer per tx hash (called from handler_eth.go when tx bodies arrive via P2P) - Subscribes to ChainHeadEvent, fetches block txs, credits the delivering peer for each included tx - Per-peer EMA of recent inclusions (alpha=0.05), updated every block - LRU eviction at 262K entries to bound memory - Mutex-based (not channel-based) for simplicity — the hot path (NotifyReceived) is a fast map insert Wired into the dropper via an adapter callback in backend.go that converts txtracker.PeerStats to the dropper's PeerInclusionStats.
Change the long-term protection category from total inclusions to total finalized inclusions. Finalized txs are harder to game (require actual block finality, not just inclusion) and represent confirmed on-chain value. The recent-inclusion EMA stays on chain head inclusions for responsiveness — a peer delivering txs that appear in the latest blocks gets quick protection without waiting for finalization. The tracker now checks CurrentFinalBlock() on each chain head event and credits delivering peers for all newly finalized blocks since the last check.
Expand the txtracker package doc to describe the tracking flow (NotifyReceived → chain head → finalization → peer credit) and its role as stats provider for the dropper. Rewrite the dropper struct comment to document the full behavior including the inclusion-based peer protection: two scoring categories (total finalized + recent EMA), top 10% per pool, union of protected sets.
txtracker tests (7 tests): - NotifyReceived: stats empty before chain events - InclusionEMA: EMA increases on inclusion, decays on empty blocks - Finalization: Finalized counter credited after finalization - MultiplePeers: each peer credited for own txs only - FirstDelivererWins: duplicate delivery ignored - NoFinalizationCredit: no credit without finalization - EMADecay: EMA approaches zero after 30 empty blocks dropper tests (6 tests): - FilterProtectedNoStats: nil stats → all droppable - FilterProtectedEmptyStats: empty map → all droppable - FilterProtectedTopPeer: top-scored peers removed from droppable - FilterProtectedZeroScore: zero scores → no protection - FilterProtectedOverlap: peer top in both categories → counted once - FilterProtectedAllProtected: all droppable protected → empty list Also fix: create peer entries during EMA update for peers with inclusions in the current block (previously only created during finalization, so EMA was not tracked before first finalization).
lastFinalNum started at 0, so the first checkFinalization after startup iterated from block 1 to the current finalized head (~20M blocks on mainnet) under the mutex, stalling the tracker and potentially awarding bogus credit for ancient txs whose hashes happened to match recently-received ones. Seed lastFinalNum from chain.CurrentFinalBlock() in Start() so only blocks finalized after startup are processed.
NotifyReceived was called before pool validation, allowing a peer to claim deliverer credit by replaying already-included txs or sending invalid packets. Rename to NotifyAccepted (takes hashes, not full txs). Call it from a new enqueueAndTrack helper in handler_eth.go that runs after Enqueue and checks pool.Has to identify accepted txs. Only accepted txs are credited to the delivering peer.
handleChainHead fetched the block by number only. If the tracker goroutine lagged and that height was reorged before processing, the EMA was computed from the wrong canonical block. Use GetBlock(hash, number) with the header hash from the event to fetch the exact block the event refers to, not whatever is currently canonical at that height.
Peer stats were never pruned, so the peers map grew with every peer ever seen. The EMA decay loop and stats copy iterated all historical peers on every block/query. Add NotifyPeerDrop(peer) that deletes the peer's stats entry. Called from handler.unregisterPeer alongside txFetcher.Drop.
protectTopN used maxPeers (configured capacity) to compute the number of peers to protect. With small droppable sets this could protect everyone, permanently disabling churn. Use len(entries) (current droppable count in each category) instead. With 20 droppable dialed peers and 10% fraction, 2 are protected. With 3 droppable peers, 0 are protected — churn is never blocked.
Add the standard go-ethereum LGPL header to tracker.go, tracker_test.go, and dropper_test.go.
enqueueAndTrack used pool.Has() after Enqueue to determine accepted txs. Under concurrent delivery of the same tx from two peers, both could see Has()==true, making attribution non-deterministic. Add an onAccepted callback to the fetcher, called from Enqueue with (peer, acceptedHashes) immediately after pool.Add returns for each batch. Attribution happens atomically inside Enqueue using the per-tx error from addTxs (nil = accepted), before another goroutine can race. Remove the enqueueAndTrack helper from handler_eth.go — the fetcher now handles notification directly.
Compute the protected peer set once in dropRandomPeer via protectedPeers(), then include protection as a condition in selectDoNotDrop alongside trusted/static/recent checks. This eliminates the separate filterProtectedPeers post-pass and the awkward "all protected → skip" branch. Rename filterProtectedPeers to protectedPeers, returning map[*p2p.Peer]bool instead of filtering a slice. The map is checked directly in selectDoNotDrop via protected[p].
Replace peerWithStats wrapper, manual slice copying, and protectTopN closure with a generic topN[T] function that sorts by score and returns top elements. protectedPeers now works directly with []*p2p.Peer slices, building per-category score functions that close over the stats map.
Remove the custom topN generic function. Use slices.SortedFunc (creates a sorted copy from an iterator) + slices.DeleteFunc (filters score <= 0) from the standard library. No custom generics needed.
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
order = order[1:] reslices without releasing the backing array. After N total insertions the array retains N hashes (32 bytes each) but only the last maxTracked are live. On a long-running node processing ~100 txs/s this leaks ~275 MB/day. Compact by copying to a fresh array when capacity exceeds 2×maxTracked.
NotifyPeerDrop deleted t.peers[peer] but left t.txs entries pointing to that peer. When those txs later finalized, checkFinalization recreated the peer entry, and the EMA loop decayed it forever. Fix: create peer entries in NotifyAccepted (when txs are first accepted), not in handleChainHead or checkFinalization. Both chain event handlers now skip peers with no entry — disconnected peers whose entries were deleted by NotifyPeerDrop stay deleted.
Tests used time.Sleep(50ms) to wait for async chain head processing, making the suite slow (~2s) and flaky under CI load. Add a step channel (buffered 1) to the Tracker, sent after each event in the loop. Tests wait on the channel with a 1s timeout. Suite now completes in <10ms.
Fix goimports violations: align callback field comments in tx_fetcher.go, sort import of eth/txtracker after eth/protocols in handler.go. Add missing onAccepted parameter (nil) to the txfetcher fuzzer.
When neither the dialed nor inbound peer pool is close to capacity, every non-trusted/non-static peer is already marked do-not-drop by the pool-threshold rules in selectDoNotDrop, so the droppable set is guaranteed empty regardless of inclusion protection. Return early in that case to avoid the wasted peerStatsFunc call, per-direction split, and per-category sort in protectedPeers.
Rename eth/dropper/protected to eth/dropper/skipped and mark it on every skip (fast-path headroom, all candidates un-droppable, or protection emptied the list). Add eth/dropper/skipped_protected to count the subset of skips where at least one otherwise-droppable peer was kept only because of inclusion protection. The pair lets operators see both the total churn-miss rate and how often peer protection specifically is the cause.
The original assertions read stats["peerA"].Finalized and .RecentIncluded, both of which return zero for a missing key — so the test would pass even if NotifyAccepted were a complete no-op, contradicting its stated purpose. Assert that exactly one peer entry exists, peerA is present, and the internal txs map and FIFO order slice are populated as NotifyAccepted is meant to do.
handleChainHead resolves the head block via GetBlock(hash, number) so that a stale head event after a reorg cannot credit transactions from the wrong block. The existing mockChain ignored the hash argument, so a regression to GetBlockByNumber would have gone undetected. Make mockChain hash-aware: store blocks keyed by hash with a separate canonical-by-number index for the finalization path, and have sendHead emit the real block's hash. Add TestReorgSafety with two blocks at the same height to exercise the hash selector directly.
The protection feature promises top-N per inbound/dialed pool, but
every existing test constructed peers via p2p.NewPeer (which produces
no-flag peers), so all test peers landed in the dialed pool and the
per-pool split was never validated.
Extract the selection logic from protectedPeers into a pure helper
protectedPeersByPool(inbound, dialed, stats) that accepts pre-split
pools. This sidesteps the unexported p2p.connFlag types and makes the
interesting behavior directly testable. Add three tests covering:
- exact top-N selected independently in each pool
- cross-category union with overlap deduplication
- per-pool independence: top dialed peers stay protected even when
every inbound peer scores higher globally
The skipped_protected metric (added earlier on this branch) counted the subset of drop skips where inclusion protection was the cause. The signal can be inferred from rising dropSkipped rate plus the existing "Protecting high-value peers" debug log, which wasn't worth the second metric, the causality-check loop over the protected set, and the baseNotDrop closure extracted solely to share the predicate. Collapse baseNotDrop back into selectDoNotDrop and remove the metric. dropSkipped still fires on every skip (fast-path headroom + all-filtered).
…ctly PeerInclusionStats was declared identically to txtracker.PeerStats as a decoupling abstraction: any stats provider could implement the dropper's callback by returning this shape. In practice there's one provider and the two types were kept in sync by a rote copy adapter in backend.go. Delete PeerInclusionStats, have the dropper consume txtracker.PeerStats directly via getPeerStatsFunc. backend.go now passes txTracker.GetAllPeerStats as the callback with no adapter. If a second stats provider ever appears, the abstraction can come back; until then, one fewer type and 8 fewer lines of ceremony.
… EMA The total-finalized protection category ranked peers by a monotonic cumulative count, so a peer that had been productive in the past kept a high score forever — even if they had since gone silent — and held a protected slot without contributing. Replace txtracker.PeerStats.Finalized (int64 cumulative) with RecentFinalized (float64 EMA). On each chain head, finalization credits accumulated over the newly-finalized range are folded into a slow EMA (alpha=0.0001, half-life ~6930 blocks ≈ 23 hours on 12s mainnet blocks). Peers that continue contributing keep a high score; peers that stop decay toward zero over roughly a day. The dropper category renames to "recent-finalized" accordingly. The type's docstring is rewritten to describe both categories as EMAs with different time horizons (slow finalized, fast included). Refactors checkFinalization to return a per-peer credits map rather than mutating state directly, so both EMAs update in the same loop over tracked peers.
Add a per-entry arrival timestamp at NotifyAccepted time and skip inclusion and finalization credit when the delivery was at or after the slot of the inclusion block. This prevents a peer that learned a tx from the just-propagated block (or any post-slot source) from harvesting credit it didn't earn through genuine pre-slot relay work. Does not defeat builder-feed peers that deliver within the feed's lead time before slot start; but that's also not fundamentally different from sending to the mempool in the first place. Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
…r model
No functional change. Prepares the minimal model to mirror the field
naming of the richer txtracker variants on adjacent branches so that
upcoming cherry-picks (lastFinalNum seed, IncludedDeliverer freeze,
collectFinalization pivot to iterate t.txs) can apply with smaller
deltas.
Changes:
- txEntry -> TxInfo (exported, pointer in t.txs map)
- txEntry.peer -> TxInfo.Deliverer
- txEntry.addedAt -> TxInfo.AddedAt
- add TxStatus enum (StatusUnknown / StatusIncluded / StatusFinalized);
not yet assigned on any code path here
- add TxInfo.BlockNum and TxInfo.BlockHash; not yet assigned
Instead of walking every newly-finalized block from disk, iterate the tracker's existing per-tx state (which already records BlockNum and BlockHash at inclusion time) and confirm canonicality with one cheap GetCanonicalHash lookup per unique BlockNum. The walk-by-block design was the dominant cost during catch-up after a restart: the chain's recent-block LRU does not cover blocks that finalized 32+ blocks ago, so each iteration paid a full RLP decode plus fresh tx.Hash() and types.Sender() against cache-cold *Transaction instances. With ~150 txs/block × ~100µs ECDSA × N blocks of finality to catch up, that scales to seconds-to-minutes of CPU on long restarts — and the per-tx work was waste, since most decoded txs are not tracked. The pivot keeps the same correctness guarantee. Each TxInfo's BlockHash is what we recorded at inclusion; comparing it to the canonical hash at that height confirms the tx really is on the canonical chain (an orphan-block reference would not match). When the recorded hash and canonical hash agree, transition the entry to StatusFinalized; when they disagree, skip — the tx's recorded inclusion was reorged out and the entry will see a fresh inclusion or eviction signal through the normal head/reorg path. Adds GetCanonicalHash to the Chain interface; *core.BlockChain already satisfies it. The mock in the unit tests gains a tiny stub. (cherry picked from commit 7a44f66118467c3a30d89e29685235078dfe1e4c)
The finalization-credit pivot replaced the per-block walk with an iterate-t.txs scan, eliminating the only caller of GetBlockByNumber in this package. Drop it from the Chain interface and from mockChain. *core.BlockChain still satisfies the slimmed interface (it has all the remaining methods); production wiring is unchanged. Updates two stale test comments that referenced the removed call.
cskiraly
force-pushed
the
peerdrop-protected-pools
branch
from
June 23, 2026 08:39
e9ecf5d to
ffb92fe
Compare
Contributor
Author
|
@fjl @healthykim I've rebased this and extended it with 2 things:
Ready for review |
healthykim
approved these changes
Jul 2, 2026
healthykim
left a comment
healthykim
left a comment
Contributor
There was a problem hiding this comment.
@cskiraly I pushed some changes, I think it is good to merge
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
3 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.
The peer dropper periodically disconnects random peers to create churn. This was previously blind to peer quality.
This PR adds peer-score based peer protection, handling the multi-dimensionality problem of peer scoring through the concept of protected peer pools.
Assisted-by: Claude:claude-3-opus