Repository navigation
dos: bound remote sessions and replacement inputs - #1069
Conversation
There was a problem hiding this comment.
Summary
The remote bounds match the stated rules on the paths they touch. A history span wider than 8,064 blocks is rejected before the create scan, an SV2 idle deadline moves only after a handled frame, the invalid-txid set no longer clears at 4,096, and a replacement may bring an unconfirmed input only from its single direct conflict. Two of the new bounds do not cover the path a remote peer actually hits.
The findings index rows in this PR and in #1071 both append to the same README table, so the second merge needs both ranges kept.
Issue counts by severity
- bugs: 2
- suggestions: 0
- nits: 0
There was a problem hiding this comment.
Summary
Verdict: Changes requested
The PR caps remote work: Electrum and SV2 deadlines, an 8,064-block history window, a non-clearing invalid-txid set, overlay discourage keys, no GET /broadcast, script-pool panic release, and Libre's single-conflict unconfirmed-input rule. The Electrum status push now fails the connection on a write timeout, and a plain parked chain is admitted from one worklist frame (ChildPromote::Caller plus the depth pin). That pin misses a 1p1c admit inside the walk: the packaged parent is published with Caller, and the worklist never takes that parent's other orphans, so they stay parked.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
d261993 to
11052c4
Compare
There was a problem hiding this comment.
Summary
Verdict: Changes requested
The orphan walk, Electrum write deadline, SV2 idle timeout, replacement input rule, invalid-txid cap, script-pool panic release, and GET /broadcast removal behave as described. One new outbound gate is wrong: it treats an inbound slot-eviction netgroup as a dial refusal, so a single full-inbound eviction blocks outbound connections to that whole IPv4 /16 for ten minutes.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
11052c4 to
6cd2280
Compare
There was a problem hiding this comment.
Summary
Verdict: Changes requested
Outbound refusal of a misbehavior key is in the right place for addnode, --connect, block-relay, addr-fetch, and full-relay, and slot-eviction stays inbound-only. Two holes remain: feeler and IBD dials never consult that key, and an onion or I2P refusal is tied to the port as well as the identity, so the same peer is dialed again on another port.
Issue counts by severity
- bugs: 2
- suggestions: 0
- nits: 0
A parked chain no longer recurses once per descendant. The first child is admitted, then its descendants, then the next sibling. bobberb and muse, finding 092.
A from_height catch-up still scans that window and skips the create cap. The clipped span stops at 8,064 blocks before any input.body walk. bobberb and muse, finding 093.
A client that stops reading no longer holds the connection inside write_all. The write uses the same idle deadline as the next read. bobberb and muse, finding 094.
After the first CoinbaseOutputConstraints, the setup timeout is an idle deadline. It moves only when a frame is handled. bobberb and muse, finding 095.
An unconfirmed input must be a parent of the direct conflicts, and only one direct conflict is allowed. A single-conflict CPFP of that parent still replaces. Direct-set feerate is unchanged. bobberb and muse, finding 096.
A new id past 4096 no longer clears the set. The oldest id stays. bobberb and muse, finding 097.
Onion and I2P sessions are remembered for the in-memory day, not skipped because the version socket is 0.0.0.0. Loopback stays unrecorded. bobberb and muse, finding 098.
Production records a DiscourageKey. The IpAddr helper is what the clearnet tests call, so a non-test build does not need it.
A browser GET could admit a transaction. POST /tx remains. bobberb and muse, finding 099.
in_wave drops with the chunk, including on unwind, and the worker keeps running. wait_done returns. bobberb and muse, finding 100.
Changelog fragment and the review index. bobberb and muse, finding 101.
The subscribe response is the height map plus a done notification. A fresh BufReader could swallow that second line, so the next call parsed done as server.banner.
The replacement no longer spends the anchor. That input is not a parent of the conflict, so HasNoNewUnconfirmed rejects it. A package child with no conflicts spends the anchor; evicting the anchor drops the child and the failed package puts the below-floor conflicts back.
The relay fixtures spent the anchor from the replacement, which HasNoNewUnconfirmed rejects. The package child spends that anchor, and evicting it still drops the child so the conflicts come back.
DiscourageKey was inserted between the allow and the struct, so the attribute covered the enum and qc rejected PeerHub's addr cache.
A scripthash status notification uses the same write deadline as a response. A timeout ends the connection. The relay hub admits a parked chain on one stack frame. A nested admit does not start another walk.
A one-parent package inside the block-connect walk published the low-fee parent without taking the spends that waited only on that parent. The same walk now keeps every tx it publishes, including a conflict put back after a failed admit, and releases those orphans before the walk ends.
An outbound dial asked the inbound discourage check, so a prefix paused after one inbound eviction also refused dials to every other address in that prefix. Outbound now checks only the misbehavior key. The ten-minute netgroup wait still applies to the next inbound.
Onion and I2P refusals were the full address, so another port of the same identity was dialed again. Feeler and IBD connects never checked the key at all. The key is now the 32-byte identity, and those dials return before opening a socket. A slot-eviction netgroup stays inbound.
6cd2280 to
d5dafe3
Compare
There was a problem hiding this comment.
Summary
Verdict: Changes requested
Outbound dials (addnode, --connect, feeler, addr-fetch, full-relay, redial, and IBD) refuse the misbehavior key and do not apply the inbound slot-eviction netgroup. Onion and I2P rate-limit strikes use that same portless identity. The new orphan worklist in ActiveMempool::promote_orphans_of still drops a parked child: a package that contains that child rolls the child out of both the mempool and the orphanage, and a child that still misses another parent is not put back.
Issue counts by severity
- bugs: 2
- suggestions: 0
- nits: 0
Promoting one parent took the child out of the orphanage, and a child that still missed another parent was not put back. Co-authored-by: Cursor <cursoragent@cursor.com>
Promoting inside the package loop admitted a later member early, then the duplicate rollback dropped that child from the mempool and the orphanage. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Summary
Verdict: Changes requested
The limits from the last round hold where they were pinned. A child that still misses another parent is put back in the orphan pool. A package handled inside ActiveMempool admits a parked member before it promotes anyone else, and a failed package puts that member back. Feeler and IBD dials refuse the misbehavior key. Onion and I2P keys ignore the port. A netgroup paused after an inbound eviction still does not block outbound dials. Electrum writes and the SV2 idle deadline still drop a peer that goes quiet. The invalid-txid set still stops at 4,096 without clearing. A replacement still cannot add an unconfirmed input that is not a parent of its one direct conflict.
The package fix does not reach a wallet. submitpackage, Esplora, and Electrum broadcast_package call MempoolHub::accept_package. That path never takes a member out of the orphan pool. A child that was already parked is rejected as an orphan even when its parent was just admitted earlier in the same package, and that parent is then rolled back. package_admits_its_parked_child only calls ActiveMempool::accept_package.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
submitpackage and Electrum call the hub, which still rejected a child that was already parked and then rolled its parent back. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Summary
Verdict: Changes requested
The hub lift is in the right place. accept_package takes a parked member out before admit_staged, and a failed package rolls the admitted members back before it puts that child back. The new test covers both the success path (parked child and its sibling) and the failure path (parent not left in the graph, child back in the orphanage). submitpackage, Esplora POST /txs/package, and Electrum broadcast_package all enter through this hub path. prepare_admit and plan_after_script still refuse a txid that is sitting in the orphanage, so the lift is what makes the package able to admit its own child.
The other bounds from the earlier rounds still hold. A child that is still missing a parent is re-parked from the worklist with the parents it still does not have, and a conflict restored during that walk is pushed onto the same queue. The individual submitpackage pass uses defer_trim, so a child that is not parked yet is not parked by the first pass; one that is already parked stays there until the package lifts it. Clearnet and CJDNS are discouraged by address, onion and I2P by the 32-byte identity with the port left out, and the feeler and IBD dials check that key before they connect. Slot-eviction netgroups stay inbound-only. An Electrum status write that misses its deadline ends the connection. An SV2 session refreshes its idle deadline only after a handled frame. The invalid-txid set stops at 4096 without clearing. A replacement may add an unconfirmed input only when that input is a parent of its single direct conflict. A history window wider than 8064 blocks is refused when from_height > 0. GET /broadcast is gone. A script-pool worker that panics releases its wave slot.
Putting the lifted child back is wrong when a parent arrives while the child is out of the orphanage.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Summary
Verdict: Changes requested
The stuck-orphan hole from the last round is closed for a child that already pays the relay fee. repark_lifted ignores the missing-parent set saved at the lift and resolves the inputs again. A child that still misses a parent is parked under that fresh set, and the peers who announced it are put back. A child whose parents are already in the mempool, and whose raw fee clears the floor, is admitted. The new test covers both, and the earlier hub test still checks that a failed package rolls its own parent out and puts the parked child back.
The other bounds still hold. Orphan promotion is one worklist on both the mempool and the hub, and a child that still misses a parent stays parked. A package lifts a parked member before admit on the hub path used by submitpackage, Esplora, and Electrum. Clearnet and CJDNS are discouraged by address, onion and I2P by the 32-byte identity, and feeler and IBD dials check that key before they connect. Slot-eviction netgroups stay inbound-only. An Electrum write that misses its deadline ends the connection. An SV2 session refreshes its idle deadline only after a handled frame. The invalid-txid set stops at 4,096 without clearing. A replacement may add an unconfirmed input only from its one direct conflict. A history window wider than 8,064 blocks is refused when from_height > 0. GET /broadcast is gone. A panicking script-pool chunk releases its wave.
The new classifier drops a lifted child that a normal admit would keep.
Issue counts by severity
- bugs: 1
- suggestions: 0
- nits: 0
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Summary
Verdict: The changes hold.
The fee-delta hole from the last round is closed. repark_lifted takes the caller's prioritisetransaction map. The hub snapshots that map before the mempool write lock, and a child under the raw relay floor comes back ready for admit when the delta clears it. A child that still misses a parent is parked with the peers who announced it. A child the real admit still rejects is dropped. The new hub test is a 1-sat child with a 1_000_000 delta.
The two IBD stalls are closed on the same commits. After an invalid tip block, a sibling that exists only in the header store is planted, so the pump is not left with an empty in-flight set. A BadPrev whose parent is the current tip stays on the work path, and densify walks back to that height. A BadPrev whose parent is an earlier block still leaves the path. The remote bounds from the earlier rounds still hold.
Issue counts by severity
- bugs: 0
- suggestions: 0
- nits: 0
Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Bounds remote sessions and a few relay edges found by bobberb and muse (2026-10-18). Status board:
docs/external_findings/101-bobberb-remote-index.md.from_heightof 1 or more still does not use the create cap.CoinbaseOutputConstraintshits that same setup timeout. Tip and fee wakes do not refresh it (095).GET /broadcastis removed.POST /txremains (099).in_wave, sowait_donereturns (100).D2, D6, and D12 stay won't-fix. D11 is the window already covered by 093.
Findings rows 092–101 here and rows 102–108 on #1071 both append to the same README table. Whichever pull request merges second has to keep both ranges.
Test plan
cargo test -pfor each new pin, includingparked_chain_promotes_in_one_frame,package_inside_promote_admits_parent_orphans,slot_evict_does_not_block_an_outbound_dial,overlay_refusal_ignores_the_port,feeler_does_not_dial_a_discouraged_address,ibd_dial_refuses_a_discouraged_net, andstalled_scripthash_push_ends_the_connection(workspace suite not run locally; the VM was busy)promote_keeps_orphan_that_still_misses_a_parentandpackage_admits_its_parked_child, thencargo test --workspace --quietcargo fmton the touched crates andcargo clippy -p rbitcoin-net -p rbitcoin-electrum --all-targets -- -D warningsqc,test,windows,macos,coverage