Repository navigation
ibd: disconnect quiet peers on their own silence, and store only dialable addresses - #1019
Conversation
A getdata write must not refresh a quiet peer, and a pass where every in-flight peer is quiet must disconnect nobody.
Disconnect at most one peer whose getdata is on the wire and whose block bytes have been quiet for 30s. A later getdata write does not refresh that clock. If every on-wire peer is quiet in the same pass, disconnect nobody. The status thread only logs.
Clearnet must not survive an onion-only merge. Onion, I2P, and CJDNS stay out until this process can dial them, and turning a net off drops rows already stored. An unreachable overlay must not take a clearnet dial slot.
Onion requires --proxy or --onion, I2P requires --i2p-sam, and CJDNS requires --cjdns-reachable. --only-net drops every other row, including ones restored from the peers file. Clearnet stays unless --only-net excludes it.
There was a problem hiding this comment.
Summary
The per-peer stall move (getdata must already be on the socket, a later write does not refresh the clock, at most one disconnect per pass, tip motion is not an input) matches the coordination loop, and the status thread no longer aborts peers. The dialable-book predicate is one check on insert, load still restores the file, and startup sets onion/I2P/CJDNS before --only-net and then sweeps. Hub set_i2p_reachable / set_addrman do not nest the hub addrman mutex with the book mutex; learn_addrv2 takes hub then book, and no path takes the book and then the hub lock. --connect still bypasses the book. What does not hold is recovery once every on-wire peer is quiet, and what counts as "received" progress. Two or more peers that accept getdata and then send nothing are kept forever, so all_peers_dead / GiveUpMidCatchup never runs. Any socket byte, not a solicited block, refreshes that same clock, and relative-slow still ignores bps == 0 and will not decay a peer whose progress_ms is kept fresh. A peers file that the new sweep reduces to nothing, with no dialable seed refill, takes the existing empty-target path and marks catch-up complete instead of failing IBD.
Issue counts by severity
- bugs: 3
- suggestions: 0
- nits: 0
| } | ||
| // Every judged peer quiet at once is our clock (we stopped reading), not | ||
| // eight independent failures. One quiet peer among live senders is a peer. | ||
| if stalled_peers.is_empty() || (stalled_peers.len() >= 2 && stalled_peers.len() == on_wire) { |
There was a problem hiding this comment.
[bug] When every on-wire peer is past the stall clock in one pass (stalled_peers.len() >= 2 && stalled_peers.len() == on_wire), disconnect_stalled_block_peers_at returns without disconnecting anyone. That set is not rotated on later passes: each peer's progress_ms stays at note_connected (or the last byte), getdata stays outstanding, and the same equality holds every peer-slow tick. all_peers_dead runs only when every slot is !alive, and relative-slow refuses a kick when the whole set is slow or has bps == 0 ("absolute stall owns silence"). Redial runs only while alive < target. A dial batch that fills the target, writes getdata, and then sends nothing therefore keeps those slots for the life of the process. Confirm can sit idle (the tip-stall WARN is log-only and is not an input here) and IBD never reaches GiveUpMidCatchup. Readers are separate tasks on an unbounded body channel, so universal silence is not evidence this process stopped reading; the guard also fires when the peers are simply not sending. One quiet peer is still dropped, which is why a single-peer stall can still exit and a full set cannot. Previously a frozen tip disconnected every stalled wire, which freed the slots and let empty redials hit the give-up budget.
Suggestion: Keep "at most one disconnect per pass" so a stuck coordination tick cannot mass-disconnect, but do not treat a fully quiet on-wire set as immortal. After the same set has stayed quiet across a pass in which the readers have run (or after a longer silence budget), disconnect one peer so slots turn over and all_peers_dead remains reachable. If the guard must stay for a single unread pass, clear it once a later pass still shows no new solicited_ms.
There was a problem hiding this comment.
The stall check treated a full set of silent peers as one bad clock and disconnected nobody. The same check runs again every second, and each of those peers was still silent with the same block requests outstanding, so the slots never freed and catch-up could not give up.
A quiet peer now means two things together: we have already put block getdata on the wire for that peer, and that peer has sent no bytes for 30 seconds. A peer with no block requests assigned stays connected even when it is silent. Each pass disconnects one quiet peer, including when every peer we asked is quiet. That frees a slot so a later redial can run, and a set that never answers can still reach give-up.
| note_stream_bytes(bytes_io, delta as u64); | ||
| if delta > 0 { | ||
| note_stream_bytes(bytes_io, delta as u64); | ||
| rx_ms.store(ibd_mono_ms(), Ordering::Relaxed); |
There was a problem hiding this comment.
[bug] note_read_progress stores ibd_mono_ms into solicited_ms on every positive read, including ping, pong, addr, headers, and inv. sample_peer_rates then copies any newer solicited_ms into PeerRate::progress_ms before the 64KiB solicited-byte sample. Absolute stall, eviction_bps age, has_recent_rx, tip-hole owner drop, and hung-densify steal all treat that timestamp as block progress. A peer with getdata on the wire can withhold blocks and reset the 30s clock with any other traffic. Relative-slow does not catch it: mature_relative_slow_samples drops bps == 0, and relative_slow_pick ignores zero, on the comment that absolute stall owns silence. A peer that once transferred blocks and then only chats keeps progress_ms fresh, so eviction_bps does not decay and the peer still ranks as fast; slower honest peers are the ones relative-slow can disconnect. Tip-hole steal also bails out while has_recent_rx is true, so the chatty peer keeps the hash. Honest Bitcoin Core pings are 120s and do not by themselves hold a 30s window; an attacker who fills outbound slots and sends a message every few seconds does. The field comment on solicited_ms, getdata_on_wire_stalled, and PeerRate still say solicited block bytes, and note_solicited_block was the only writer that matched that contract.
Suggestion: Stamp a separate last-rx time if "any bytes" is only a hint that the socket is alive, and keep solicited_ms / progress_ms on note_solicited_block, block note_rx, and the 64KiB solicited sample. Absolute stall and has_recent_rx must move only on those. Do not let non-block traffic freeze eviction_bps.
There was a problem hiding this comment.
Every socket read, including pings, address messages, headers, and inv, was stored as if it were a requested block. The block-progress clock, the eviction rate, and the recent-receive checks all moved on that timestamp. The relative-slow check also skipped any peer whose block rate was zero. A peer could keep the slot by chatting and never sending a block. If every outbound slot did that, nothing disconnected them.
Quiet is still "no bytes at all", and only for a peer that already has getdata assigned. Those bytes refresh the quiet clock and nothing else. Block progress moves when a requested block arrives. A peer that is still sending, but has not delivered a block and is not in the middle of a body, shows up as a zero block rate and can be the one slow outlier. When every peer we asked is in that state, one of them is disconnected so the set cannot sit there.
| addrman.set_onion_reachable(config.listen.proxy.is_some() || config.listen.onion.is_some()); | ||
| addrman.set_i2p_reachable(config.listen.i2p_sam.is_some()); | ||
| addrman.set_cjdns_reachable(config.listen.cjdns_reachable); | ||
| addrman.set_only_net(config.listen.only_net.clone()); |
There was a problem hiding this comment.
[bug] Startup now drops every loaded row the process cannot dial (set_onion_reachable / set_i2p_reachable / set_cjdns_reachable, then set_only_net → retain_dialable). If nothing dialable remains, follow_dial_targets is empty and run_ibd_or_skip takes the pre-existing ibd_targets.is_empty() arm, which returns CatchUp::complete() and enters tip mode. That is not the give-up path: give-up returns Err and, unless tip indexes are already on, stays CatchUp::Incomplete ("no tip mode"). Two easy ways to wipe the book with no refill: --only-net=onion without --proxy/--onion (loaded onions are not dialable, and DNS seeds are clearnet so add drops them), and --proxy with a peers file of only I2P/CJDNS when --i2p-sam / --cjdns-reachable is off (should_resolve_default_seeds is false because a proxy is set; addrfetch is only queued, and target selection does not wait). The tip-follow loop then sees the same empty startup AddrMan and skips stale redial. A behind node stops seeking the most-work chain and, once the stored tip meets --min-chain-work, can relay that tip. Previously those rows were dial targets; dial failure could give up without declaring catch-up finished.
Suggestion: If the sweep leaves no dialable address, do not return CatchUp::complete(). Keep catch-up incomplete (or wait for proxy addrfetch / seed inject to land in the book that IBD and tip-follow actually dial). Default DNS seeds with no proxy still refill clearnet; this is the case where that refill cannot happen.
There was a problem hiding this comment.
Startup drops every saved address this process cannot dial. If that removes the whole book, there is nobody to connect to. The old empty-target path treated that as catch-up finished and entered tip mode on the tip already on disk. The same thing happens when a proxy is set and seed addresses are only queued, not written into the book yet. Asking for onion-only without a proxy is already rejected before this code runs.
If the book is empty because the sweep removed rows, or because those proxy seeds have not arrived, catch-up stays incomplete and tip mode does not start. A book that was already empty, with no seed fetch waiting, still takes the serve-only path.
A full set of peers with getdata on the wire and no bytes must drop one peer. A silent peer with nothing assigned must stay. A zero block rate beside live senders is the relative-slow outlier.
Quiet means getdata is on the wire and the peer has sent no bytes. A peer with nothing assigned stays. One quiet peer is dropped even when the whole set is quiet. Pings do not move block progress, so a chatting peer that delivers no blocks is the relative-slow outlier, and a set of them drops one peer.
An empty target list still reports catch-up complete after the reachability sweep or while proxy seeds are only queued.
A reachability sweep that deletes every row, or a proxy seed fetch that has not landed in the book, must not mark the stored tip caught up. An already empty book with no seed fetch still serves.
rustfmt and the argument-count lint on the empty-target call.
What
IBD no longer disconnects peers because the confirm tip stopped moving. A peer is stalled only when its getdata is already on the wire and that peer's received bytes have been still for 30 seconds. A later getdata write does not refresh the clock. If every on-wire peer is quiet in the same pass, none are disconnected. The status thread only logs.
The address book stores only nets this process can dial. IPv4 and IPv6 stay unless
--only-netexcludes them. Onion requires--proxyor--onion, I2P requires--i2p-sam, and CJDNS requires--cjdns-reachable. Turning a net off, including after a peers-file load, drops those rows.--connectis still operator-pinned and is not filtered by the book.Tests
Local:
cargo test -p rbitcoin-net --lib,cargo test -p rbitcoin-node --lib, fmt, clippy--workspace --all-targets -D warnings, deny, ast-grep, andci-os-smoke. The workspace suite runs on GitHub.