Repository navigation
fix(p2p): penalize peers that fail our requests, and prefer reserved peers for block fetching - #3326
Conversation
A single misbehaving external peer is responsible for ~100% of the block header fetch failures on the public mainnet sentries, which run 10-45 blocks behind the block producer. The failures are transport level - `P2P(Timeout)`, `P2P(ConnectionClosed)` and `P2P(Io(..))` - and each one of them aborts the whole import range for a full `--request-timeout`. Nothing ever pushed back on that peer: - The failure was swallowed. `get_sealed_block_headers` drops the error with `.ok()`, so `get_headers_batch` returns a batch with no peer, and the `report_peer` call that follows is a no-op on `None`. The peer id is only known in the p2p service, in the `OutboundFailure` handler, which just logged it. - The peer kept being selected. `get_peer_id_with_height` chained the reserved and the non-reserved peers together and picked uniformly at random among all of them, so the bad peer - which advertises the highest height and therefore always passes the filter - was redrawn indefinitely, with no preference for the operator's own nodes. Report the peer from the `OutboundFailure` handler, where its id is still known, and prefer the reserved peers when selecting the peer to fetch a block height from. Only the failures the remote peer is responsible for are penalized: `UnsupportedProtocols` is a protocol mismatch we already handle by disconnecting, and `DialFailure` may be caused by our own connectivity. Reserved peers are never penalized, since `update_app_score` only touches non-reserved peers; that is now documented and covered by a test. The penalty is `-48`, and its magnitude is forced by the reputation decay: the score decays by 0.9 every second and the ban threshold is -50, so a peer failing once per 20s request timeout converges to `1.14 * penalty` and anything weaker than -43.9 could never ban it. -48 bans a peer failing twice in less than 30 seconds while leaving a peer with a single transient failure alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Request/response reputation: On Block-fetch peer choice: Reviewed by Cursor Bugbot for commit ae425bc. Bugbot is set up for automated code reviews on this repo. Configure here. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The penalty could never be applied. `peer_report` is declared - and so polled - before `request_response` on `FuelBehaviour`, so when a peer's last connection closes the swarm emits `PeerDisconnected` first, `PeerManager::handle_peer_disconnect` removes the peer from `non_reserved_connected_peers`, and the `OutboundFailure::ConnectionClosed` that follows reaches `update_app_score` with nothing to look up. It hit the `log_missing_peer` arm and silently did nothing. `remaining_established == 0` gates that `PeerDisconnected`, so the penalty can technically still land when one of several connections to the same peer drops. That is not the case worth keeping the code for: the case that matters is a peer hanging up on us, and such a peer is out of the block-fetch selection pool until it reconnects, with its `PeerInfo` - and therefore its score - dropped along with it. Rather than leave a penalty that reads as working and doesn't, drop `ConnectionClosed` from `is_peer_at_fault` and document why. `Timeout` and `Io` are unaffected: the peer stays connected for both, which is exactly why they are the failures that stall block import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ae425bc. Configure here.
## Version 0.48.3 ### Added - [3320](#3320): Add `/v1/ready` HTTP endpoint reporting whether the node has finished startup and is p2p-synced, for use as a Kubernetes readiness/startup probe distinct from the process-only `/v1/health` liveness check. ### Changed - [3280](#3280): Move the PoA redis-publish path out of the importer and into the PoA service itself, running natively on tokio with parallel per-node fan-out, cached connections, and split quorum-latency/lock-held metrics. - [3320](#3320): PoA readiness now combines a live reserved-peer height-gap check (network height vs. local DB height, default max diff 1) with the existing time-until-synced debounce; `/v1/health` is unchanged. ### Fixed - [3323](#3323): Fix the `cargo-audit` and `version-compatibility` CI checks. - [3324](#3324): Fix the `cargo-test-kms` CI job by mocking AWS KMS with LocalStack instead of a live AWS account. - [3326](#3326): Penalize peers that fail request/response requests and prefer reserved peers for block fetching, so a single misbehaving peer can no longer stall block synchronization. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Same change as #3325, targeted at
masterinstead ofrelease/v0.48.2. #3325 exists so the fix can ship on the deployed v0.48.2 line; this PR keepsmastercorrect. Cherry-picked cleanly — the three touched files are byte-identical betweenv0.48.2andmaster.Problem
Fuel mainnet's public p2p sentries run 10-45 blocks behind the block producer, while every internal-only sentry sits at 0-8. Measured against a healthy sentry over 579 common blocks: p50 16.6-21.5s behind, p95 up to 42s, max 54s, with 72-75% of wall clock spent inside a stalled range.
The cycle, from production logs:
100% of header-fetch failures paired within 1ms with a
RequestResponse outbound errornaming one external peer. Two root causes:1. Transport failures never penalize the peer.
crates/services/sync/src/import.rsdoes.trace_err("Failed to get headers").ok(), discarding the error, then returnsBatch::new(None, range, vec![]), soreport_peeris called withpeer_id: Noneand silently no-ops. The peer id is available in theOutboundFailurehandler inp2p_service.rs, which only logged it. Net effect:appScoreis ~0.0 for every peer, including one responsible for ~100% of failures, one 21,599 blocks behind, and one 1,831,510 blocks behind — all still connected.The contrast is stark: a peer that answers "here are 3 of your 10 headers" is banned forever (
MissingBlockHeaders, -100), while a peer that times out 248 times is untouched.2. Peer selection ignores reputation and gives reserved peers no preference.
get_peer_id_with_heightchainednon_reserved_connected_peerswithreserved_connected_peersand did a uniform.choose(), with an explicit// TODO: Optimize the selection of the peer.The bad peer advertised the highest height, so it always passed the height filter and was re-drawn indefinitely.Changes
(A)
handle_request_response_eventreports the peer from theOutboundFailurearm via a newis_peer_at_faulthelper. Penalized:Timeout,ConnectionClosed,Io(_). Not penalized:UnsupportedProtocols(already handled by disconnect — avoids double-punishing) andDialFailure(may be our own connectivity; the peer never received the request). The match is exhaustive so a libp2p bump forces a decision. Reserved peers remain exempt —update_app_scoreonly touchesnon_reserved_connected_peers; that was previously incidental and is now documented and test-pinned.(B)
get_peer_id_with_heightpicks uniformly among qualifying reserved peers, falling back to uniformly among qualifying non-reserved peers. Height-filter semantics unchanged (extracted to apeers_with_heighthelper).Penalty magnitude: -48
Decay is
0.9applied every second against aMIN_APP_SCOREof-50. A peer failing once per 20s--request-timeoutconverges top/(1 - 0.9²⁰) = 1.14·p, so anything weaker than -43.9 can never ban — reusing the heartbeat's -5 would settle at -5.7 forever and ship a no-op. Reusing -100 bans on a single transient blip. The feasible band is43.9 < |p| ≤ 50.-48gives: one failure → no ban, back above -0.5 in 44s; ban requires two failures within 30.2s, ~50% margin over the observed 20s cadence. (-44/-45 also nominally work but leave <1s of margin, which jitter would erase.)Tests
cargo test -p fuel-core-p2p --features test-helpers→ 63 passed, 0 failed, 2 ignored on this base.Added: reserved preference over 100 draws; fallback +
Nonewhen nothing qualifies; reserved score untouched while non-reserved is scored and banned; a single failure doesn't ban and decays away; two failures at the 20s cadence do ban; a per-variant fault table foris_peer_at_fault. Also extendedreq_res_outbound_timeout_worksto assert the timed-out peer's score goes negative — it stayed at 0.0 before, so the existing test was quietly encoding the bug.Risk
With
block_stream_buffer_size: 10, a single 20s blip can produce two concurrent failures against the same peer and ban it permanently for the process lifetime —libp2p::allow_block_listhas no TTL and there is nounblock_peeranywhere in the codebase. A follow-up giving banned peers a route back (time-based unban, or score-driven removal from the block list) is worth doing separately.Note also that the culprit peer rotated within a day in production, which is why (B) matters more than (A): it makes the class of problem irrelevant regardless of which peer misbehaves.
🤖 Generated with Claude Code