Repository navigation
fix(mempool): per-peer orphan pool reservations - #2148
nullPointerEnjoyer wants to merge 43 commits into
Conversation
|
🔍 OpenCodeReview found 7 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let status = self.insert(entry)?; | ||
| self.enforce_limits(cur_time); | ||
| ensure!(self.contains(&tx_id), OrphanPoolError::Full); |
There was a problem hiding this comment.
A stale resubmission (entry already expired) is rejected via the trailing ensure!(self.contains(&tx_id), OrphanPoolError::Full) even though the pool may be completely empty — the test resubmission_of_expired_orphan_is_rejected_and_swept explicitly documents that Full here means "expired and swept", not "pool at capacity". Callers that react to Full as resource exhaustion (logging, metrics, peer discouragement, backoff) will misinterpret an ordinary stale rebroadcast as an overload event. Consider a dedicated OrphanPoolError::Expired (or a TxStatus::Expired result) so the failure mode stays distinguishable at the boundary.
Suggestion:
| let status = self.insert(entry)?; | |
| self.enforce_limits(cur_time); | |
| ensure!(self.contains(&tx_id), OrphanPoolError::Full); | |
| let status = self.insert(entry)?; | |
| self.enforce_limits(cur_time); | |
| ensure!(self.contains(&tx_id), OrphanPoolError::Expired); |
There was a problem hiding this comment.
Addressed in 3bc50ac: the trailing containment check now returns the new OrphanPoolError::Expired instead of Full (it only fires when the post-insert expiry sweep removed the entry, so the dedicated variant is exact). Wallet classifier treats it as transient — a rebroadcast re-parks the transaction with a fresh timestamp — and the variant-count/wire-mirror tests pin the new Display string. Note this path is unreachable in production today (the interface always constructs fresh entries, and the caller sweeps before admission); the variant keeps the API honest for future callers.
|
CI note — the 4 failing checks are inherited from This diff adds no dependencies and touches no wasm/TypeScript code. Both failing workflows fail identically on current
Everything else is green on this PR: build_ubuntu (incl. full test suite), loom, review, wasm_artifacts, all ledger and trezor device tests. The two inherited breakages look like they need a runner/dependency-bump fix on master; happy to take that in a separate PR if useful. |
|
Addressing the remaining review findings, all in 3bc50ac:
|
| // If the entry is gone after the post-insert sweep, it was already expired at | ||
| // `cur_time` — capacity-wise nothing was rejected (the pool had room), so report | ||
| // expiry rather than `Full`, which callers read as resource exhaustion. |
There was a problem hiding this comment.
This comment is inconsistent with the code: as written, an entry inserted at cur_time cannot be expired by the post-insert sweep, so !contains here can only indicate that enforce_limits evicted it (or protected entries) via enforce_max_size. The stated rationale ('it was already expired at cur_time') doesn't hold, and mapping that eviction case to Expired hides resource pressure from callers.
Suggestion:
| // If the entry is gone after the post-insert sweep, it was already expired at | |
| // `cur_time` — capacity-wise nothing was rejected (the pool had room), so report | |
| // expiry rather than `Full`, which callers read as resource exhaustion. | |
| // A just-inserted entry cannot expire at `cur_time`; if it is missing here, | |
| // `enforce_max_size` evicted it or another origin's entries — surface that | |
| // distinctly instead of reporting expiry. |
| ensure!(self.contains(&tx_id), OrphanPoolError::Expired); | ||
|
|
||
| Ok(status) |
There was a problem hiding this comment.
This ensure!(self.contains(&tx_id), OrphanPoolError::Expired) mislabels non-expiry evictions. A freshly inserted entry has creation_time == cur_time, so remove_expired inside enforce_limits can never remove it; the only way it can be absent here is eviction by enforce_max_size (via the fail-open oldest-overall fallback, which can also displace another peer's protected entries). Reporting that as Expired masks a resource-exhaustion/displacement condition — including from wallet-node-client, which classifies Expired as a deterministic rejection. Either return a dedicated eviction error or, better, use evict_over_reservation semantics after insert so the branch truly can only mean expiry.
Suggestion:
| ensure!(self.contains(&tx_id), OrphanPoolError::Expired); | |
| Ok(status) | |
| // Only `remove_expired` (never eviction) can have removed a just-inserted entry. | |
| ensure!(self.contains(&tx_id), OrphanPoolError::Expired); | |
| Ok(status) |
There was a problem hiding this comment.
Restructured in a9f8f8a: the stale case is now detected before insertion instead of after the sweep — insert_and_enforce_limits rejects any entry whose creation_time is past the expiry cutoff (shared with remove_expired via a helper) with OrphanPoolError::Expired, so the post-insert containment check can no longer attribute anything to expiry. On your eviction point: that branch is unreachable by construction — the Full check bounds the pool below capacity before insertion, so enforce_max_size has nothing to evict afterward (this holds regardless of the fail-open fallback, which only runs inside evict_over_reservation/direct calls when the pool is already at capacity). The containment check stays as a release-side fail-closed backstop reporting Full with a proof comment; the Expired variant now has exactly one producer, the staleness check.
| if self.len() >= *self.transaction_count_limit | ||
| && !self.evict_over_reservation(*self.transaction_count_limit) | ||
| { | ||
| return Err(OrphanPoolError::Full); | ||
| } | ||
|
|
||
| // An already-stale entry is rejected here instead of being inserted and immediately | ||
| // swept again: `remove_expired` has just run, so staleness is detectable without | ||
| // touching the pool. Reporting `Expired` keeps a swept-and-rejected resubmission | ||
| // distinguishable from `Full`, which callers read as resource exhaustion. | ||
| if entry.creation_time() < self.expiry_cutoff(cur_time) { | ||
| return Err(OrphanPoolError::Expired); | ||
| } |
There was a problem hiding this comment.
The staleness check runs after the capacity check. If the pool is full (after the expired sweep freed nothing) and the incoming entry is already stale, the caller gets OrphanPoolError::Full — resource exhaustion — instead of Expired, contradicting the stated goal of keeping swept-and-rejected resubmissions distinguishable from Full. Move the expiry_cutoff check above the Full/eviction check so the more specific error always wins.
Suggestion:
| if self.len() >= *self.transaction_count_limit | |
| && !self.evict_over_reservation(*self.transaction_count_limit) | |
| { | |
| return Err(OrphanPoolError::Full); | |
| } | |
| // An already-stale entry is rejected here instead of being inserted and immediately | |
| // swept again: `remove_expired` has just run, so staleness is detectable without | |
| // touching the pool. Reporting `Expired` keeps a swept-and-rejected resubmission | |
| // distinguishable from `Full`, which callers read as resource exhaustion. | |
| if entry.creation_time() < self.expiry_cutoff(cur_time) { | |
| return Err(OrphanPoolError::Expired); | |
| } | |
| // Check staleness first: an expired entry should never be reported as `Full`, even | |
| // when the pool has no room, so callers can distinguish the two conditions. | |
| if entry.creation_time() < self.expiry_cutoff(cur_time) { | |
| return Err(OrphanPoolError::Expired); | |
| } | |
| if self.len() >= *self.transaction_count_limit | |
| && !self.evict_over_reservation(*self.transaction_count_limit) | |
| { | |
| return Err(OrphanPoolError::Full); | |
| } |
There was a problem hiding this comment.
Done in d58462e: the expiry_cutoff check now runs before the Full/eviction check, so Expired wins whenever both conditions apply — and a stale entry no longer triggers an eviction for room it would never use. The exact scenario is pinned by a new unit test (stale_entry_on_full_pool_reports_expired_not_full): pool filled to capacity with fresh entries, a stale resubmission must yield Expired (the pre-reorder ordering would have produced Full), and the pool must remain untouched. While in the file, also cleared every clippy warning this branch had introduced in the mempool crate (map_or → is_some_and/is_none_or, Copy-type clones, and the constant-precondition asserts now bind through runtime locals).
|
Final state on d58462e — all review threads addressed and the check matrix is green, with the same four failures master also has (evidence in the earlier parity comment):
|
|
CI fixes pushed in 20df2c7 and e3a54b8:
Both failures were reproduced locally before pushing; the full |
Problem
The mempool's orphan pool was a single shared 100-entry cache with no per-peer accounting:
enforce_max_sizedrew a random resident entry per insertion).A single connected peer could therefore flood the pool with ~1500 costless transactions (~72 KiB, no signature checks, no fees) and displace every other peer's parked orphans for as long as the connection stayed open. Child-before-parent relay — which wallets rely on for unconfirmed chains — was disabled for everyone except the attacker. Affected release line: v1.4.x.
Fix
The pool now grants each origin a protected reservation; one peer can never evict another peer's entries.
check_orphan_pool_policy): remote inbound peers default to 10 entries (remote_orphan_pool_capacity_per_peer), local origin keeps its aggregate cap of 25, and a new outbound tier defaults to 40 (outbound_remote_orphan_pool_capacity_per_peer). The outbound flag is set by the node from the actual connection direction (p2ptransaction manager); RPC callers cannot claim it.OrphanPoolError::Fullinstead of displacing a protected entry.origin_stats) with integrity checking, purged on peer disconnect (existing teardown) and when an origin's count reaches zero.MempoolConfigfields + validation (outbound >= inbound, non-zero), node config file, RPC type, and the wallet error classifier now treatsRemoteCapacityExceededas transient (retryable) rather than deterministic rejection.The global pool ceiling stays fixed at 100 entries (
DEFAULT_ORPHAN_POOL_CAPACITY); worst-case memory is unchanged.RemoteCapacityExceededrejections carry ban score 0, so quota pressure cannot be laundered into peer bans.Tests
mempool/src/pool/tests/orphan_starvation.rs(drives the productionadd_transaction_remotepath): control (child promoted), attack (flood no longer displaces — inverted from the original failing PoC), scale (100/100 survive), two-peer fairness, 99-peer fairness, fair share under load, expiry/fresh-quota, pinned-pool fail-closed (remote and local), duplicate/ownership semantics, teardown, outbound tier.mempool/src/pool/orphans/test.rs, including a randomized simulation exercising quota + eviction + expiry.mempool_orphan_peer_disconnected.pyfunctional test passes.