core/txpool, eth, p2p: control rebroadcast and peer traffic - #2418
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
Rebroadcast candidates can be repeatedly re-emitted and starve relayable transactions, while pooled-transaction replies bypass the new traffic budgets.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request adds sync-aware transaction rebroadcast control, bounded peer traffic handling, and temporary peer jailing.
Changes:
- Gates rebroadcasts and tracks queue acknowledgements.
- Adds per-peer request, gossip, announcement, and response limits.
- Bounds jail and rebroadcast-claim registries with trusted-peer exemptions.
File summaries
| File | Reviewed changes |
|---|---|
p2p/server.go |
Adds bounded, expiring peer-jail entries. |
p2p/peer_jail_test.go |
Tests jail expiry, capacity, and custom durations. |
eth/sync.go |
Buffers peer events for coalesced sync updates. |
eth/sync_rebroadcast_test.go |
Tests sync and rebroadcast interactions. |
eth/rebroadcast.go |
Implements rebroadcast gating and claim tracking. |
eth/rebroadcast_test.go |
Tests rebroadcast claim behavior. |
eth/rebroadcast_lifecycle_test.go |
Tests claim lifecycle and reconnect behavior. |
eth/rebroadcast_delivery_test.go |
Tests rebroadcast assignment and acknowledgements. |
eth/protocols/eth/peer.go |
Adds queue acceptance and reply-rate checks. |
eth/protocols/eth/peer_limits.go |
Defines per-peer traffic limiters. |
eth/protocols/eth/peer_limits_test.go |
Tests traffic limits and trusted-peer handling. |
eth/protocols/eth/handlers.go |
Limits block announcements. |
eth/protocols/eth/handler.go |
Applies inbound message limits. |
eth/peer_backoff_test.go |
Tests traffic-triggered peer jailing. |
eth/handler.go |
Integrates rebroadcast gating and acknowledgements. |
eth/handler_eth.go |
Applies backoff after rate-limit violations. |
core/txpool/txpool.go |
Aggregates rebroadcast acknowledgement callbacks. |
core/txpool/legacypool/rebroadcast.go |
Tracks successfully queued rebroadcasts. |
core/txpool/legacypool/rebroadcast_test.go |
Tests rebroadcast accounting and metrics. |
core/txpool/legacypool/legacypool.go |
Defers rebroadcast tracking until delivery or fallback. |
core/txpool/legacypool/legacypool_test.go |
Updates acknowledgement integration coverage. |
Review details
Suppressed comments (1)
eth/protocols/eth/peer_limits.go:55
- The new limits do not cover transaction-sync replies:
GetPooledTransactionsMsgis absent from the request limiter andReplyPooledTransactionsRLPnever callscheckReplyRate. A peer can repeatedly request known transactions and receive up to the per-responsesoftResponseLimitwithout consuming either budget, bypassing the advertised bounded peer-traffic control. Apply the same reply limiter and an appropriate request limit to this path, or explicitly bound transaction sync separately.
func (p *Peer) checkMessageRate(code uint64, size uint32, now time.Time) error {
if p.Trusted() {
return nil
}
switch code {
case GetBlockHeadersMsg, GetBlockBodiesMsg, GetReceiptsMsg:
if !p.limits.requests.AllowN(now, 1) {
return fmt.Errorf("%w: block requests", ErrPeerRateLimit)
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
It includes an unresolved rebroadcast loop issue across broad peer and transaction-pool changes.
Review details
Suppressed comments (2)
eth/handler.go:1089
- When
broadcastTransactionsfilters the whole batch (for example, private or PIP-15 conditional transactions), no queue operation occurs andonBroadcastis never called. The legacy pool therefore never recordslastRebroadcast, whileidentifyStuckTransactionscontinues to emit the same intentionally non-gossipable transaction on every interval, creating an endless selection/feed loop. Exclude these transaction classes from rebroadcast selection or record an explicit suppression outcome for them.
return h.broadcastTransactions(txs, onBroadcast)
eth/protocols/eth/peer_limits.go:15
- This sentinel is also returned by
ReplyReceiptsRLP, so the text "block response" is inaccurate for receipt responses and makes the resulting error misleading. Use a generic sync/peer response message instead.
errPeerResponseScheduling = errors.New("block response cannot be scheduled")
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved critical and moderate findings require code changes and final human review.
Review details
Suppressed comments (1)
p2p/server.go:160
- When an existing entry is extended,
nextExpiryis left pointing at the old deadline. Once that deadline passes, the capacity path believes an expiry is due and scans the entire 4,096-entry map on every subsequent jail attempt;IsJailedcan create the same stale minimum when it removes the earliest entry. Recompute the cached minimum whenever its entry is extended or removed (or use an expiry heap) so repeated abusive connections cannot turn jailing into O(capacity) work.
if current, exists := pj.jailed[id]; exists {
pj.jailed[id] = max(current, unbanTime)
return
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Outgoing sync response paths do not yet consume the byte limiter, leaving a critical traffic-control gap.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Two unresolved moderate rebroadcast-accounting issues require fixes before approval.
Review details
Suppressed comments (2)
core/txpool/legacypool/legacypool.go:495
rebroadcastTxFeed.Sendreturns nonzero for any existing subscriber, so the fallback acknowledgement is skipped whenever a consumer subscribes. The existingSubscribeRebroadcastTransactionscontract did not require consumers to call this new side-channel acknowledgement; such a subscriber now leaves every candidate out oflastRebroadcastand causes it to be emitted on every interval. Preserve accounting for legacy subscribers or make the acknowledgement contract explicit before changing this behavior.
if pool.rebroadcastTxFeed.Send(core.StuckTxsEvent{Txs: stuckTxs}) == 0 {
hashes := make([]common.Hash, len(stuckTxs))
for i, tx := range stuckTxs {
hashes[i] = tx.Hash()
}
eth/rebroadcast.go:61
onBroadcast(hashes)acknowledges the entire batch as soon as the handoff channel accepts it, but the per-peer broadcaster can still discard those hashes: it drops the batch when its writer has already failed, and truncates batches larger thanmaxQueuedTxs/maxQueuedTxAnns. In either case the affected transactions are recorded as rebroadcast even though they were never retained for sending, suppressing their later retries. Return/acknowledge only the hashes actually retained (or cap/split the submitted batches and reject a failed writer).
if send(hashes) {
count += len(hashes)
if onBroadcast != nil {
onBroadcast(hashes)
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2.10.2-candidate #2418 +/- ##
=====================================================
+ Coverage 55.61% 55.80% +0.19%
=====================================================
Files 918 921 +3
Lines 167138 167491 +353
=====================================================
+ Hits 92947 93470 +523
+ Misses 68722 68560 -162
+ Partials 5469 5461 -8
... and 21 files with indirect coverage changes
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🔵 Needs a closer look
Asynchronous propagation behavior requires correction, and the broad networking changes warrant final human review.
Review details
Suppressed comments (2)
eth/protocols/eth/peer.go:224
- This changes the existing
AsyncSendTransactionscontract into a synchronous operation:queueTxPropagationwaits forbroadcastTransactionsto process the batch before returning, and that broadcaster may itself be waiting onSendTransactions/the network writer. In particular,syncTransactionscalls this fromrunEthPeer, so a slow or stalled peer can block the protocol handler instead of merely delaying propagation. Keep theAsyncSend*wrappers fire-and-forget (and provide a separate acknowledgement-capable path for callers that need retained hashes), rather than making them wait on the broadcaster.
func (p *Peer) AsyncSendTransactions(hashes []common.Hash) {
p.QueueTransactions(hashes)
eth/protocols/eth/peer.go:248
- The announcement API has the same regression as
AsyncSendTransactions: this wrapper now waits for the announcement worker to retain the batch. A slowsendPooledTransactionHasheswrite can therefore blocksyncTransactions/other callers, despite the public method being explicitly asynchronous. Preserve fire-and-forget behavior forAsyncSendPooledTransactionHashesand expose retention acknowledgement through a separate path.
func (p *Peer) AsyncSendPooledTransactionHashes(hashes []common.Hash) {
p.QueuePooledTransactionHashes(hashes)
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Reconcile retained broadcast queues when static membership is removed.
Review details
Suppressed comments (1)
eth/protocols/eth/broadcast.go:213
- When a peer is static, this loop can retain up to
maxQueuedTxAnnsTrustedhashes. IfRemovePeerthen clearsstaticConn, the next batch uses the ordinary limit, but the already-retained queue is never reduced; those excess hashes continue to be announced during teardown even though the peer is no longer exempt. Reconcile the existing queue when membership changes (and handle known-hash bookkeeping for any entries removed), rather than only applying the lower limit to future batches.
case batch := <-p.txAnnounce:
queueLimit := maxQueuedTxAnns
if p.Trusted() || p.Static() {
queueLimit = maxQueuedTxAnnsTrusted
}
queue = retainTxPropagation(queue, batch, queueLimit, failed.Load())
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@claude review |
| if s.claims == nil { | ||
| s.claims = make(map[string]*rebroadcastPeerClaim) | ||
| } | ||
| state = &rebroadcastPeerClaim{head: head, td: new(big.Int).Set(td), until: now.Add(rebroadcastPeerGrace)} |
There was a problem hiding this comment.
blocksRebroadcast lets an unknown head with only a self-reported TD above our TD create the one-minute veto at line 188. Status handshake values are not proof that the peer can serve that head. A single identity is bounded, but an attacker can rotate identities about once per minute; with the default 30-second ticker and 10-minute rebroadcast age, it can suppress every recovery rebroadcast opportunity for a stuck transaction. Please require a locally verified head, or make the grace claim contingent on real sync progress. A regression should cover fresh peer IDs carrying unverified high-TD claims across the eligibility window.
| for _, hash := range query { | ||
| if bytes >= softResponseLimit { | ||
| for lookups, hash := range query { | ||
| if bytes >= softResponseLimit || lookups >= maxPooledTxsServe { |
There was a problem hiding this comment.
The 256-lookup cap is helpful, but GetPooledTransactionsMsg is intentionally outside the jail-triggering request limiter and the reply queue only applies backpressure after these lookups and full reply RLP encoding. A peer can flood valid 256-hash requests while the pooled-reply writer is stalled and repeatedly force work for replies rejected by the full queue; this removes the natural write backpressure from the former synchronous response path. Please reserve reply capacity before lookup/serialization, or use a separate pooled-request limit that disconnects without jailing. Add a stalled-writer flood test that bounds lookup/encoding work.
There was a problem hiding this comment.
🔵 Needs a closer look
Broad networking and transaction-pool changes include an unresolved rebroadcast double-accounting issue.
Review details
Suppressed comments (1)
core/txpool/legacypool/rebroadcast.go:52
- When both a legacy subscriber and the new acknowledgement subscriber are present,
legacy > 0causespublishRebroadcastTransactionsto record every hash immediately, but the explicit subscriber later invokes its ownRebroadcastAcknowledgementcallback. Each callback has an independentremainingmap, so the same batch is tracked twice andrebroadcastTxMeteris incremented twice, contrary to the exactly-once acknowledgement behavior documented above. Coordinate the legacy fallback with the explicit callback (or otherwise share batch accounting) so mixed subscribers cannot double-account.
explicit := pool.rebroadcastAckFeed.Send(core.StuckTxsEvent{Txs: txs})
legacy := pool.rebroadcastTxFeed.Send(core.StuckTxsEvent{Txs: txs})
if explicit == 0 || legacy > 0 {
hashes := make([]common.Hash, len(txs))
for i, tx := range txs {
hashes[i] = tx.Hash()
}
pool.rebroadcastAcknowledgement(txs, false)(hashes)
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Transaction announcement writer failures can strand acknowledged hashes and suppress future broadcasts.
Review details
Suppressed comments (2)
eth/protocols/eth/broadcast.go:134
- If
SendTransactionsfails, this goroutine returns without closingdoneor clearing the queue. A batch accepted while the failed send was in flight has already been returned as retained (and marked known), but remains queued forever becausedonestays non-nil and no subsequent send can start; future broadcasts are then suppressed for those hashes on this peer. On failure, terminate the writer state and discard/unmark any pending accepted hashes (or disconnect the peer) so they cannot be stranded.
failed.Store(true)
p.Log().Debug("Broadcast: failed to send transactions, discarding future txs", "err", err)
eth/protocols/eth/broadcast.go:198
- The announcement writer has the same failure window: a batch accepted while
sendPooledTransactionHashesis in flight is acknowledged and appended, but an error leavesdoneopen and the queued hashes permanently unsent/known. Clear the pending queue and its known-hash state or otherwise terminate this broadcaster when the writer fails.
failed.Store(true)
p.Log().Debug("Broadcast: failed to announce transactions, discarding future txs", "err", err)
- Files reviewed: 29/29 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Combines the transaction rebroadcast gating from #2397 with bounded peer traffic handling. Bor suppresses stuck-transaction rebroadcast during initial sync or while a connected peer has a locally verified higher-TD head, and applies a two-minute backoff through the existing jail path for excessive block requests or block gossip. Pooled-transaction requests are excluded from the jail-triggering request cap, and transaction replies use a separate bandwidth allowance and response worker from block data. Trusted peers and configured static peers in either connection direction are exempt. Peer info and broadcast prioritization use the same static membership, and announcement queue capacity follows runtime membership changes. Removing a static peer clears its live membership before disconnect begins, ending static traffic exemptions during teardown. Pooled-transaction replies mark hashes known only after the reply is accepted. Private and conditional transactions are excluded before filling rebroadcast batches and before queueing initial peer-sync announcements. Rebroadcast accounting acknowledges only hashes retained by a peer's broadcaster, rejects new excess hashes without evicting previously accepted ones, and preserves retry accounting for legacy subscribers through an explicit opt-in acknowledgement subscription.
Pooled-transaction responses inspect at most 256 requested hashes and retain the existing response-size bound. Incoming pooled-transaction requests are checked against the maximum encoded size of a 256-hash request (8,463 bytes, including the largest request ID) before RLP decoding or pool lookups. Oversized requests disconnect without triggering jail. Ordinary peers reserve one response slot and the maximum response size (10 MiB) before request decoding, pool lookups, or reply encoding. The reservation becomes the actual encoded size when queued and is released on errors. Requests without sufficient reply capacity disconnect without jailing; pooled-transaction request counts remain uncapped.
Ordinary peers also have an 11,011-byte pre-decode limit on block-hash announcements: the maximum encoding of a 256-entry batch with uint64 block numbers. Larger announcements enter the existing two-minute backoff without payload decoding or backend delivery. The separate hash-count/rate check still rejects compact batches exceeding the allowance, and trusted/configured-static exemptions remain.
Executed tests
662fb0376: Quality metrics / Diffguard and lint.662fb0376, production fixfbc8f067b): the rotating-peer and stalled-writer regressions failed before the fix and pass afterward. Race tests cover fresh unverified peers across all recovery opportunities in the ten-minute eligibility window, locally verified ahead heads and catch-up, pre-decode pooled-request admission under both queue limits in eth/68 and eth/69, reservation cleanup, known-hash accounting, and shutdown. The 128 pooled requests/s plus 64 block-body requests/s test now uses the actual pooled-request handler and passes. Full ETH protocol race tests, focused ETH/ETH-protocol race tests, andmake lintpassed. Additional regressions verify actual body/hash packet selection, transaction-category logging, and immediate allowance refunds after shutdown.c328c39ab): fullgo test -race ./p2p -count=1, ETH traffic-backoff and jail-handler race tests, andmake lintpassed. New capacity regressions fail before the fix and pass afterward. They verify inbound/outbound reconnect rejection for a replacement ban, the full two-minute expiry boundary, earliest-expiry selection after extending an existing ban, the 4,096-entry cap, and reclamation of partially/fully expired registries without evicting active entries unnecessarily.a1f7edf41): each worker captures its receive channel under the queue mutex and uses that local channel throughout its loop. Fullgo test -race ./eth/protocols/eth -count=1andmake lintpassed. The existing shutdown and write-failure tests also passed 50 repetitions under the race detector before this cleanup; no baseline race was reproduced.1f3b2f64b):go test -race ./p2p ./eth/protocols/eth -count=1andmake lintpassed. The regression holds protocol teardown open and checks thatPeer.Static()and peer info lose static membership before removal completes, while dial history remains intact. It fails before the fix and passes afterward.efb530648): fullgo test -race ./eth/protocols/eth -count=1,go test -race ./eth -run TestPeerTraffic -count=1, andmake lintpassed. New eth/68 and eth/69 regressions fail before the fix and pass afterward: oversized payloads are rejected unread; the largest valid 256-entry batch is accepted; trusted, outbound/inbound static, and runtime-added static peers retain their exemptions. The existing compact overfull-batch regression and two-minute jail tests also pass.6ca9ec824): fullgo test -race ./eth/protocols/eth -count=1andmake lintpassed. New eth/68 and eth/69 regressions fail before the fix and pass afterward: valid empty/single/full requests and maximum request IDs are accepted with the block-request allowance exhausted; oversized requests consume zero payload bytes and perform zero pool lookups../p2p,./eth/protocols/eth,./eth,./core/txpool, and./core/txpool/legacypoolfor peer limits, static membership, rebroadcast, initial sync, and queue retention. The full networking package race suites also passed on the production revision.TestPeerRepliesUnderConcurrentLoadnow exercises 128 pooled-transaction requests/s with approximately 128 KiB replies plus 64 block-body requests/s with approximately 256 KiB replies for 30 simulated seconds: approximately 16 MiB/s per class, 32 MiB/s combined, without rejection or backlog. The strengthened test passed under the race detector.make lint: 0 issues.git diff --check: passed.origin/v2.10.2-candidate, with mutation testing enabled, 20% sampling, focused regression tests, and a stricter 90% T1 gate: exit 0; 50/58 mutations caught overall (86.2%), 26/28 T1 logic (92.9%), 5/7 T2 semantic (71.4%), and 19/23 T3 observability (82.6%). Eight sampled mutants survive. Complexity, size, dependency, and dead-code checks pass; the existingeth/backend.go:Newchurn warning remains. The local mutation timeout was one minute; the workflow retains its five-minute timeout and unchanged thresholds.Two-node devnet load verification
Ran an isolated Kurtosis v1.4.2 devnet with one Bor RPC node and one validator, Heimdall v0.11.0, Anvil L1, one-second blocks, and a configured 140M gas ceiling. Both sides used discovery with empty static/trusted lists and
txannouncementonly=true, forcing the pooled-transaction fetch path. Tests ran after block 128 on ARM64 Docker Desktop with 15 CPUs and 8 GB Docker memory. Bor production revision:c4a651769; the subsequent7856cb2fbchanges only the regression test. The decode-bound follow-ups6ca9ec824andefb530648were verified with the race/regression checks above; these devnet measurements remain fromc4a651769. Load generator: polygon-cliv0.1.113-4-g4f254b4.Each load transaction contained 4,096 zero calldata bytes and used 61,960 gas. Both runs sent for 30 seconds, then observed a 15-second drain period.
Counts exclude the one funding transaction in each run. All accepted transactions were mined and both pools emptied after draining. Both nodes remained connected: every one-second sample showed one non-static, untrusted peer, and node logs contained zero traffic backoffs, disconnects, or response-queue-full errors. Invalid-transaction and pool-overflow counters stayed unchanged. Sender/receiver protocol counters confirm transaction fetching was exercised. A sampled transaction/receipt verified the zero-filled payload, successful execution, and expected gas use.
The generator reported 11 requests canceled at its deadline in the first run and 2 in the second; these are not counted as accepted transactions. Acceptance rate is not a claim of sustained mining throughput: mining continued during the drain period. Independent block-body and pooled-reply budgets are verified by the simulated concurrent-load test. No Amoy/mainnet or upgrade scenario was run.
Rollout notes
At the 4,096-entry jail limit, expired entries are removed first. If the registry remains full, the earliest-expiring ban is replaced so the new ban is recorded before disconnection. This keeps storage bounded; an evicted peer can reconnect before its previous deadline under sustained capacity pressure.
No consensus change or coordinated upgrade is required. Wire protocol, database format, operator configuration, and RPC APIs are unchanged. Trusted and configured static peers are exempt in both directions without granting static peers trusted connection-capacity privileges. Ordinary peers can receive replies more slowly when their allowance is exhausted. Block-data and transaction response queues each hold at most 32 MiB including the active response, preserving a combined 64 MiB pending-data cap; each queue allows 128 pending messages. Queue exhaustion disconnects without a jail period. Allowance waits run outside the protocol reader, and pooled-transaction request counts do not trigger jailing. Unknown heads and self-reported TD do not suppress recovery rebroadcast; the unverified grace-claim cache has been removed. A locally verified ahead head suppresses rebroadcast until catch-up; acknowledgement records queue retention rather than remote delivery.