Repository navigation
mempool: allow parking local-origin orphans when enabled - #2126
Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 16 selected item(s). |
| // The rebuild above may have enqueued orphans for reprocessing (e.g. parked | ||
| // local-origin transactions whose parents have just been re-added from blocks). | ||
| // Pump the work queue now so they don't have to wait for an external trigger. | ||
| while self.has_work() { | ||
| self.perform_work_unit(); | ||
| } |
There was a problem hiding this comment.
This drain loop synchronously processes the entire work queue within a single on_new_tip invocation. WorkQueue was explicitly designed with round-based, randomized per-peer scheduling to ensure fairness and mitigate DoS (see its module docs); draining everything at once bypasses that guarantee. On a deep reorg that re-adds many transactions, an unbounded amount of orphan reprocessing (bounded only by the orphan pool hard limit of 50_000) runs inside one chainstate event handler, potentially stalling mempool event processing for a long time. Consider processing a bounded number of work units per event (or relying on the existing external trigger that pumps the queue after tx submission), so the round-based fairness and per-event latency are preserved.
Suggestion:
| // The rebuild above may have enqueued orphans for reprocessing (e.g. parked | |
| // local-origin transactions whose parents have just been re-added from blocks). | |
| // Pump the work queue now so they don't have to wait for an external trigger. | |
| while self.has_work() { | |
| self.perform_work_unit(); | |
| } | |
| // The rebuild above may have enqueued orphans for reprocessing (e.g. parked | |
| // local-origin transactions whose parents have just been re-added from blocks). | |
| // Pump a bounded number of work units now so they don't have to wait for an | |
| // external trigger, while keeping per-event processing latency bounded. | |
| const MAX_WORK_UNITS_PER_EVENT: usize = 64; | |
| for _ in 0..MAX_WORK_UNITS_PER_EVENT { | |
| if !self.has_work() { | |
| break; | |
| } | |
| self.perform_work_unit(); | |
| } |
There was a problem hiding this comment.
Both findings from this review are addressed in the current head (dbc492f65):
- Drain loop bound —
on_new_tipnow pumps at mostMAX_WORK_UNITS_PER_TIP = 16work units per event (reduced from 64 per this thread); the rest is drained by the round-based scheduler, keeping per-event latency bounded. - Configurable capacity —
local_orphan_pool_capacityis now aMempoolConfigsetting (default 25, validated non-zero inMempoolConfig::validate), exposed through the mempool RPC config and the node config file, mirroring the sibling limits.
Also note: pruning of deterministically-rejected pending transactions no longer triggers on a single snapshot in the companion wallet PR (#2127) — it requires the "parent present, tx missing, node-observed submission" signature on 3 consecutive reconcile passes.
976a308 to
763db5d
Compare
Address OCR findings on PR #2126: - local_orphan_pool_capacity becomes a MempoolConfig setting (default DEFAULT_LOCAL_ORPHAN_POOL_CAPACITY = 25), exposed through the mempool RPC config and the node config file, like the sibling limits; validated to be non-zero in MempoolConfig::validate; - OrphanPoolError::LocalCapacityExceeded carries the configured limit, mirroring TooLarge; - the on_new_tip work-queue drain is bounded to 64 work units per event to keep per-event latency bounded on deep reorgs; the rest of the queue is processed by the round-based scheduler.
9cf32a0 to
e05f896
Compare
| for _ in 0..MAX_WORK_UNITS_PER_TIP { | ||
| if !self.has_work() { | ||
| break; | ||
| } | ||
| self.perform_work_unit(); | ||
| } |
There was a problem hiding this comment.
Up to 64 perform_work_unit() calls run synchronously inside the tip-update handler. Each unit can trigger full transaction validation via add_transaction (including signature checks and chainstate queries), so under a large orphan backlog a single submit_block/tip event can block the caller for a substantial time. If the goal is bounded latency, consider a smaller bound here or delegating the pump to the existing round-based scheduler trigger that callers already use.
Suggestion:
| for _ in 0..MAX_WORK_UNITS_PER_TIP { | |
| if !self.has_work() { | |
| break; | |
| } | |
| self.perform_work_unit(); | |
| } | |
| // Same bounded pump, but with a cheaper per-event bound; the | |
| // round-based scheduler drains the remainder. | |
| for _ in 0..MAX_WORK_UNITS_PER_TIP { | |
| if !self.has_work() { | |
| break; | |
| } | |
| self.perform_work_unit(); | |
| } |
Transactions submitted locally whose inputs are not yet known (e.g. a child of a transaction that was dropped from the mempool, or that lost an ordering race during a burst of submissions) were previously rejected outright with 'Orphans not supported for transactions originating at local node', forcing the submitter to handle recovery themselves. Add a MempoolConfig option, allow_local_orphans (default: false, so the default behavior is unchanged), which parks such transactions in the orphan pool under the same bounds as remote orphans: - a separate capacity limit (DEFAULT_LOCAL_ORPHAN_POOL_CAPACITY = 25), so local orphans cannot crowd out remote peers' orphans; - the existing size, account-nonce-gap and RBF checks; - the existing expiry interval. Parked local orphans re-enter the mempool through the same full validation path as everything else: - when a parent is admitted to the main pool (enqueue_children; local orphans are enqueued under the sentinel peer id 0); - after a new-tip rebuild, via draining the work queue in on_new_tip. The orphan pool now stores entries with the unified TxOrigin instead of RemoteTxOrigin only. When the option is disabled, behavior is unchanged: local-origin orphans are still rejected with NotSupportedForLocalOrigin. The option is exposed through mempool RPC config and the node daemon's mempool config file (allow_local_orphans = true). It is intentionally not available as a CLI option. Also updates the exhaustive MempoolConfig literals in blockprod and p2p tests, and adds tests: parking of all local origins when enabled, rejection when disabled, parent-arrival reprocessing, and the local capacity limit.
e05f896 to
ce9f3be
Compare
Follow-up to the OCR review of #2126: 64 synchronous work units (each a full transaction validation) can stall the tip-update handler under a large orphan backlog. 16 per event keeps the handler latency tight; the rest of the queue is drained by the round-based scheduler.
Summary
Transactions submitted locally whose inputs are not yet known (e.g. a child of a transaction that was dropped from the mempool, or that lost an ordering race during a burst of submissions) are currently rejected outright with
Orphans not supported for transactions originating at local node, forcing submitters (wallets, RPC clients) to handle recovery themselves.This PR adds an opt-in mempool config,
allow_local_orphans(default: false — behavior unchanged), which parks such transactions in the orphan pool under the same bounds as remote orphans:DEFAULT_LOCAL_ORPHAN_POOL_CAPACITY = 25), so local orphans cannot crowd out remote peers' orphans;Parked local orphans re-enter the mempool through the same full validation path as everything else:
enqueue_children; local orphans are enqueued under a sentinel peer id);on_new_tip.The orphan pool now stores entries with the unified
TxOrigininstead ofRemoteTxOriginonly. With the option disabled, behavior is unchanged: local-origin orphans are still rejected withNotSupportedForLocalOrigin.Scope / safety
[mempool] allow_local_orphans = true). Intentionally not a CLI option.Tests
P2p,Mempool,PastBlock) when enabled; rejection when disabled;MempoolConfigliterals in blockprod and p2p tests.