net: bound connected-peer resources - #874
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Send-budget charging, 1000-inv batching, and the weighted rate window match the stated decisions. The IBD block reader does not enter the limiter; decoys are the only new note on that path, and the window does not allocate. Three behavior bugs are new.
Issue 1 -- bug
crates/rbitcoin-net/src/peer.rs:3220
note_inv returns false for both the per-peer cap and the process-wide cap. on_inv treats that as misbehavior, disconnects, and returns before getheaders and before getdata already collected in want. The global cap is 200_000 rows (40 peers at the 5_000 per-peer cap), which fits in the inbound limit, and those rows live until the holding peer's heartbeat expires the 60s in-flight entry. An honest peer is then disconnected for a table someone else filled. schedule drops orphan parents on the same cap with no signal.
Suggestion: Disconnect only at MAX_PARENT_ANN_PER_PEER. On the process-wide cap, skip the new row, do not set parent_capped, and still send getheaders plus any getdata already accepted.
Issue 2 -- bug
crates/rbitcoin-net/src/parent_req.rs:127
ParentSlot::wtxid is sticky for every announcer of those 32 bytes. schedule records orphan parents as txids, but take_due then sends MSG_WTX once any wtxid inv has set the bit. A MSG_WTX inv of a parent txid flips the slot, so the parent is requested as a wtxid and a segwit parent is not fetched. take_due_parent_getdata also stops using parent_already_have for that hash.
Suggestion: Key rows by announcement kind, or store the kind on the announcement select chooses. Txid parents stay WitnessTransaction. Wtxid-inv follow-ups stay WTx.
Issue 3 -- bug
crates/rbitcoin-net/src/peer.rs:1607 (IBD: crates/rbitcoin-net/src/ibd/peer_io.rs:251)
The first decoy that does not fit the window aborts the read with peer misbehavior threshold and drops the peer. Unknown types and ordinary frames only add RATE_LIMIT_BAN_SCORE (50) and disconnect at 100. A tip-follow peer already at the 4_000 msg/s budget stays up for an extra inv and is disconnected for a decoy. On IBD the generic error arm marks the peer dead with ban_score still 0.
Suggestion: Count the decoy, add RATE_LIMIT_BAN_SCORE when note returns false, and disconnect only at BAN_SCORE_THRESHOLD. Return Ok(()) from the decoy hook until then.
| mp.note_getdata_tx(gd_tx); | ||
| } | ||
| if parent_capped { | ||
| punish_disconnect(&mut follow.ban_score, session); |
There was a problem hiding this comment.
[bug] note_inv returns false for both the per-peer cap and the process-wide cap (ParentTracker::at_cap). This treats every false as misbehavior, disconnects, and returns before getheaders and before getdata already collected in want. The global cap is 200_000 rows, which is 40 peers at the 5_000 per-peer cap and fits in the inbound limit. Those rows stay until the holding peer's heartbeat expires the 60s in-flight entry, so an honest peer is disconnected for a table someone else filled. schedule hits the same cap and drops the orphan parent with no signal.
Suggestion: Disconnect only when this peer's own counter is at MAX_PARENT_ANN_PER_PEER. When the process-wide cap refuses a new row, skip that announcement, do not set parent_capped, and still send getheaders plus any getdata already accepted. note_inv has to tell the two caps apart.
| return false; | ||
| }; | ||
| if wtxid { | ||
| slot.wtxid = true; |
There was a problem hiding this comment.
[bug] ParentSlot::wtxid is one sticky bit for every announcer of these 32 bytes. This assignment runs before the function knows whether this peer already has a row, and nothing clears it. schedule always passes wtxid: false because orphan parents are txids, but take_due then emits Inventory::WTx for whichever peer is selected. A MSG_WTX inv whose hash is a parent txid flips the slot, so the parent getdata will not match a segwit parent's txid and the orphan is not fetched. Refreshing that inv sets the bit again. take_due_parent_getdata also stops using parent_already_have once the bit is set, so a parent already in the mempool by txid is requested again.
Suggestion: Key rows by announcement kind, or store the kind on the announcement select chooses. A scheduled orphan parent stays WitnessTransaction. A wtxid-inv follow-up stays WTx. One inv must not change the getdata type for other peers' txid rows. Point wtxid_followup_is_requested_as_wtx at the wtxid row's own retry.
| if rate.note(n) { | ||
| Ok(()) | ||
| } else { | ||
| Err(NetError::Protocol("peer misbehavior threshold")) |
There was a problem hiding this comment.
[bug] A decoy that does not fit the window fails the read with peer misbehavior threshold. The match arm below (Err(e) => return Err(e)) drops the peer on that first overflow. Unknown types in this loop, and ordinary frames, only add RATE_LIMIT_BAN_SCORE (50) and disconnect at BAN_SCORE_THRESHOLD (100). note returns false for a single message that does not fit, so a peer already at the 4_000 msg/s budget stays connected if the extra message is an inv and is disconnected if it is a decoy. The IBD reader has the same callback at ibd/peer_io.rs:251; that error hits the generic Err(e) arm and marks the peer dead while ban_score is still 0. Requested block bodies stay off the IBD window, which is right.
Suggestion: Keep counting decoys in this limiter. On note false, add RATE_LIMIT_BAN_SCORE and disconnect only at BAN_SCORE_THRESHOLD, same as invalid types. Return Ok(()) from the decoy hook until that threshold so one overflow does not abort the read.
| if rate.note(n) { | ||
| Ok(()) | ||
| } else { | ||
| Err(NetError::Protocol("peer misbehavior threshold")) |
There was a problem hiding this comment.
[bug] A decoy that does not fit the window fails this read with peer misbehavior threshold. The generic Err(e) arm below marks the peer dead on that first overflow, while ban_score is still 0. Unknown types in this same loop add RATE_LIMIT_BAN_SCORE (50) and disconnect only at BAN_SCORE_THRESHOLD (100). Tip-follow has the same decoy callback (peer.rs:1607) against the two-strike path used for ordinary frames. A single overflow is what note reports; it is not yet a disconnect under the score used everywhere else. Requested block bodies stay off this window, which is right.
Suggestion: Keep counting decoys in this limiter. On note false, add RATE_LIMIT_BAN_SCORE and disconnect only at BAN_SCORE_THRESHOLD, same as the invalid-type arm here. Return Ok(()) from the decoy hook until that threshold so one overflow does not abort the read.
df9afe5 to
10c8e6d
Compare
10c8e6d to
09d67ed
Compare
An inv flood grew the process-wide parent map, and each heartbeat copied every key. Announcements now stop at a per-peer and global cap, the heartbeat walks one peer's due index, and a wtxid follow-up is requested as WTx.
Inv-driven getdata, parent getdata, tx announcements, and filter checkpoints were queued without moving the per-peer send counter, and block getdata kept serving after that counter was already over.
A tumbling one-second reset granted a second full message and byte budget at the boundary. The limiter now weighs the previous second and does not allocate per message.
Decoy packets never left the v2 reader, and unknown types were logged and ignored, so neither counted toward the peer rate window. Tip-follow and the IBD reader now note those bytes and disconnect once the misbehavior score crosses the threshold. Requested block bodies stay off that window.
Each due transaction was its own inv, so a flush sent one message per tx. After the candidate filter, announcements go out in chunks of 1000 and each chunk still charges the send budget.
The cap tests already assert the next insert is refused. The counter existed only for those asserts, and clippy rejects it on the library build.
The index links 053-059 for the rows this branch closes. Each note names the severity, the behavior that changed, and the regression.
A process-wide parent cap skips the new row. Headers and getdata already collected still go out, and only a peer at its own cap is disconnected. Each announcement keeps its getdata kind, so a wtxid inv does not turn another peer's txid parent into WTx. One decoy past the rate window adds the same score as an unknown type. The IBD reader and writer are their own functions so spawn_peer stays under the CRAP gate.
09d67ed to
80e2d62
Compare
read_ibd_peer scored above the coverage CRAP gate. Ping, block, decode, and read-error handling now live in small functions the tests call directly.
A non-segwit wtxid announcement uses the same hash as the txid. Sending that hash again makes the parent getdata larger than the missing set. The kind of a later request stays the announcement's own.
A non-segwit wtxid request and the orphan parent share one hash. The parent stays out of getdata while that request is in flight, then goes out as a txid once the window ends. The wtxid follow-up stays WTx.
A txid schedule followed by a same-peer wtxid inv left the wtxid window indexed after it ended, so the txid parent was never requested.
1 participant
Why
A connected peer could grow process-wide parent-request state without a cap, and a wtxid announcement was later fetched as a txid. Outbound getdata and transaction announcements did not count against the per-peer send budget, so block serving could keep queueing after that budget was already over. BIP324 decoys and unknown message types never entered the rate window, and the window reset in a way that granted a second full budget at the one-second boundary. Mempool announcements went out as one
invper transaction.What
Index rows C1, N2, H1, N1, M1, L13, and M7 are marked fixed in
docs/external_findings/052-livera-review-index.md.Stacked on
docs/livera-index.