net: bound who stays connected - #877
Conversation
rearden-grok
Bot
added
the
core-functional
b60f86f to
787b4d9
Compare
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Inbound eviction and the in-memory refusal set match the decisions that were already made: longest-connected peers stay, the newest peer in the largest fixed netgroup goes, and asmap is not read. The IBD reader does keep the requested-body channel unbounded, does not wait on a decode permit, and does not put an unsolicited hash on that channel or on the stall clock. Two holes remain. A hash stays solicited until the IBD thread applies the first copy, so replays of that hash skip the 16 MB/s window, refresh progress, and pile onto the shared body channel. The one-day address refusal runs only from punish_disconnect, not from the read-loop exits that already disconnect for the misbehavior threshold.
Issue counts by severity
- bugs: 2
- suggestions: 2
- nits: 0
| &sol_ms_r, | ||
| &hash, | ||
| n, | ||
| ) { |
There was a problem hiding this comment.
[bug] note_solicited_block returns true for every frame whose header hash is still in the per-peer set, and it leaves the hash there. The set is cleared only when the IBD thread later applies BlockFramed and calls track_remove. Until that happens, a peer that was asked for a block can resend the same hash as fast as it can write. Each copy skips the 16 MB/s window, adds solicited_bytes, and stores a new solicited_ms, so the stall clock and the relative-slow EWMA treat the replay as progress. Each copy is also send_body'd onto the shared unbounded body channel. The first copy must stay unmetered; the copies behind it are not new requests. The door stays open for every other in-flight hash while the thread drains those duplicates.
Suggestion: Under the same requested lock, remove the hash when the check hits, and only then add the bytes and return true. track_remove is already idempotent. Leave the channel unbounded and do not rate-limit that first copy.
| if let Some(s) = session { | ||
| s.request_disconnect(); | ||
| if let Some(hub) = s.peer_hub() { | ||
| hub.note_misbehavior_addr(s.addr.ip()); |
There was a problem hiding this comment.
[bug] The one-day refusal is recorded only here. The session read loop still disconnects without calling it. Rate-limit and oversize frames return NetError::Protocol("peer misbehavior threshold") directly, and after handle_peer_frame the misbehavior_disconnects exit does the same for a score that climbed there by tx or compact-block penalties (including getblocktxn out-of-range, which request_disconnects on its own). Those are the disconnects a flood hits, and that address is accepted again on the next inbound. The new IBD reader path that sends Dead for the same threshold also never records the address. apply_peer_dead cools the dial only while first_data_ms is still 0, and one applied body clears that.
Suggestion: Record the address in one place used by every threshold exit: punish_disconnect, the read-loop rate-limit and oversize returns, and the misbehavior_disconnects return. For an IBD Dead with this reason, cool the dial address even when a body was already counted.
| .discouraged_addrs | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); | ||
| if !g.contains_key(&ip) && g.len() >= Self::DISCOURAGE_CAP { |
There was a problem hiding this comment.
[suggestion] note_misbehavior_addr and note_slot_evict apply the cap to map length and do not drop expired rows. The only sweep is inbound_discouraged, which the accept loop runs after a note that already returned. When the map is at the cap and at least one row is expired, the new address is ignored. The following accept sweeps, does not find that address, and allows it.
Suggestion: Drop expired keys before the cap check in both note functions. Refreshing a key that is already present can stay as it is.
|
|
||
| pub(crate) fn track_retain(&mut self, mut keep: impl FnMut(&BlockHash) -> bool) { | ||
| self.in_flight.retain(|h| keep(h)); | ||
| self.requested_set().retain(|h| keep(h)); |
There was a problem hiding this comment.
[suggestion] track_retain runs keep twice. The caller passes has_block, which takes the confirmed-set read lock. The second pass holds the per-peer requested mutex that the IBD reader blocks on inside note_solicited_block. Assign runs this for every live peer on the work loop. A confirm or reorg between the two passes can also drop the hash from only one set, so the reader and in_flight disagree about whether the next body is solicited.
Suggestion: Choose the retained hashes once, without the mutex, then replace in_flight and the requested set under the lock. The reader then waits only for that update, not for chain lookups.
787b4d9 to
0c2526c
Compare
a07991d to
d8600b3
Compare
Full inbound slots disconnected the longest-connected unprotected peer. Keep a share of those long-lived peers, and disconnect the newest peer in the largest netgroup instead. The group id is fixed when the peer is accepted.
punish_disconnect dropped the peer and forgot the address, so the same host reconnected at once. Refuse that address for one day, and make a netgroup that just lost an inbound slot wait ten minutes. Both sets are capped, in memory, and swept when the next inbound is accepted.
Unsolicited blocks and decoys were moving the stall clock, and a block body was queued before the node checked that it had asked for it. The reader drops a block that is not in the requested set, and the stall sample uses that set's bytes. Light decodes take a permit without waiting.
The wire counter is no longer the stall clock. The disconnect line is where that total is still read, so a decoy flood stays visible without counting as block progress.
The index links 060-062 for eviction, in-memory discouragement, and IBD progress. Each note names the severity, the behavior, and the regression.
Rate-limit, oversize, and score-threshold exits dropped the peer without the one-day refusal that punish_disconnect records. Those exits now share that record. An IBD death for the same reason cools the dial even after a block body was counted.
The 21-peer set is fully protected once the longest-connected peers stay. The pin now expects no victim there, and expects the newest slow peer once ten slow peers leave one unprotected.
read_ibd_peer and apply_decoded_message were each one function, so the coverage CRAP gate scored them above 30. Frame handling now lives in small functions, and the tests drive those shipped paths.
Sealing each mainnet scripthash shard walked all 2^25 ingest slots even when occupancy was known zero. That walk holds the ingest lock through tip entry, so an empty --sh-index genesis missed the RPC cookie window.
A one-day refusal of 127.0.0.1 blocks every later local connection after a protocol disconnect. Core disconnects that peer and leaves the address usable.
The Core script fills 21 inbound peers and expects the next connection to evict one slow peer. Those peers are all protected, including a share of the longest-connected, so the extra inbound is rejected. The unmodified script is not a run.
2ef5f2e to
8e7670b
Compare
The second pass held the reader mutex across another has_block lookup and could keep a different set than the assign loop.
1 participant
Summary
Stacked on
net/peer-resources.Test plan
cargo test -p rbitcoin-net --lib -- eviction_drops_the_newest_in_the_largest_netgroup misbehavior_disconnect_refuses_the_same_address evicted_netgroup_waits_less_than_a_day_and_the_set_is_capped unsolicited_block_does_not_refresh_progress light_decode_permit_does_not_wait -- --quiet