Skip to content

api: budget store reads and keep connect off the blocking pool - #1333

Merged
reardencode merged 4 commits into
masterfrom
api/thread-budget
Nov 3, 2026
Merged

reardencode merged 4 commits into
masterfrom
api/thread-budget

Conversation

@rearden-grok

@rearden-grok rearden-grok Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

getblockchaininfo size_on_disk can be 30 seconds stale. The node reuses the last datadir walk for 30 seconds. A new or grown store file shows up on the next walk after that interval.

API store reads share one budget. RPC, REST, Electrum, Esplora, and BlindBit take permits from one semaphore. The default is 1 when --datadir-hdd is set and --store-read-budget is omitted or 0, otherwise one less than the core count (minimum 1). --store-read-budget N / store_read_budget= overrides that, including on rotating media. A full RPC work queue is HTTP 503 before any call in that POST runs, and before a long-poll's tip poll. A batch that already ran a call keeps those results when the work queue cannot be retaken after a later long-poll; calls that never started return a JSON-RPC error (-32603, "Work queue depth exceeded"). A long-poll does not hold a disk permit or a work-queue slot while it waits.

Block connect and mempool script verification no longer hold a Tokio blocking-pool thread. Connect stays on the tip-accept thread. Scripts stay on rbtc-scripts-*. With every blocking-pool thread parked, a connect and a script-bearing admit still finish. This covers submitblock, generate, generatetoaddress, generatetodescriptor, invalidateblock, reconsiderblock, preciousblock, an SV2 solution, sendrawtransaction, testmempoolaccept, submitpackage, Electrum broadcast and package, Esplora POST /tx and package, and BlindBit forward-tx. A package, a one-parent-one-child admit, submitpackage, and a multi-tx testmempoolaccept finish rollback, trim, and removal of test inserts when the request is dropped. That work keeps the caller's store-read budget. The coin re-read after script verify takes a permit again. A single-tx broadcast awaits a task that still publishes or restores after that write when the request is dropped. Orphan promotion, putting a package member back after a failed package, putting a replaced conflict back, and the orphan walk after a multi-tx testmempoolaccept removes its inserts take a permit only for the coin read. They do not hold it across script checks.

SV2 builds one template at a time for the same tip and coinbase budget. A second session with the same previous-block hash, coinbase size, coinbase sigops, and mempool template-update counter waits and reuses that build. A different budget or a template-update bump builds again. A build that does not produce a template is not reused. The build waits for the connect-held tip on rbtc-off-pool, not on the blocking pool. getblocktemplate and Esplora GET /block-template sample that tip before taking a disk permit. Esplora's 15-second cache TTL is unchanged. A template whose previous block is not the requested tip is not cached.

P2P block-decode permits match the blocking-pool size. Header-decode permits do not exceed that size or twice the core count (minimum 8). A reader does not wait for a decode permit before the next TCP read, and does not apply a later message before an earlier one. While a frame is queued for decode, a heartbeat does not expire getdata, compact-block asks, the block-download window, or the ping timeout. Ping and pong wait behind that frame. A frame already read is applied before the session forgets its getdata, including when the peer is disconnected.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head reviewed: d0e010c
Disposition: change (blocking; posted as COMMENT because the bot authored this PR)
2 bugs, 2 suggestions, 2 nits

*hold = Some(Arc::clone(work_queue).try_acquire_owned().map_err(|_| ())?);
}
drop(hold.take());
let prep = satisfy_one_http_wait(ctx, req).await;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(prepare_ordered_call): a long-poll waits in satisfy_one_http_wait, then re-acquires the work-queue slot with try_acquire_owned().map_err(|_| ())?. If the queue is full at that point, the wait result is discarded. A lone call returns 503 "Work queue depth exceeded". A batched call gets -32603 for itself and every later call. The tip it waited for is lost. Re-acquire with a blocking acquire_owned (the queue is bounded by the same semaphore), or return the prep result alongside the error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the long-poll, the next call waits for a work-queue slot. A POST that has not started a call is still 503, and that 503 is still before the tip poll.

Comment thread crates/rbitcoin-rpc/src/server.rs Outdated
let ctx = Arc::clone(ctx);
// Sample before the permit. The build then uses the sample, so the
// permit covers header and mempool reads and not the connect mutex.
let template_prev = if method == "getblocktemplate" {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(exec_blocking_method): template_prev is sampled before acquire_armed_store_budget().await, and nothing re-checks it after the permit wait. A connect that completes while the call waits for the budget leaves gbt_tip on the pre-connect hash, so previousblockhash is superseded. Master read the tip under the connect lock at build time, so this is a regression. Sample after the permit, or re-sample before exec_one. The same pattern is at server.rs:526 and esplora handlers.rs block_template.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build reads the tip on rbtc-off-pool with no store permit, takes one permit, then reads the tip again with try_lock. It builds only when both reads match and the pool is settled. Esplora GET /block-template does the same and does not take a second permit. A connect during the wait is the tip that gets built.

tokio::task::spawn_blocking(move || match gbt_request(&ctx, &params) {
Ok(GbtRequest::Template { longpoll, .. }) => longpoll,
_ => None,
rbitcoin_net::off_pool_budgeted(move || {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(long-poll loops, also 555, 595, 613): each 50 ms tick calls off_pool_budgeted, and reactor.rs:51 off_pool spawns a fresh OS thread per call. A waiting client costs ~20 thread spawns per second for up to 120 s. Run the tick on a shared worker, or lengthen the interval when the tip has not moved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 50 ms tick stays. It already runs on the one rbtc-off-pool thread, so a waiting client does not start a thread per tick.

/// Sync generate runs on stack hubs, `dispatch()` tests, and some
/// `#[tokio::test]` tasks. `blocking_lock` panics on a Tokio worker even
/// when the lock is free, so a worker only uses `try_lock`.
fn lock_generate_sync(&self) -> tokio::sync::MutexGuard<'_, ()> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(lock_generate_sync): on a Tokio worker it spins try_lock with sleep(1ms). On a current-thread runtime, if the async generate holds generate_lock, the sync caller blocks the only worker and deadlocks. Reachable from #[tokio::test] and in-process dispatch(), not from production sync generate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Production HTTP generate uses the async path. The sync lock stays for in-process dispatch.

Comment thread nix/modules/rbitcoin.nix
};

storeReadBudget = mkOption {
type = types.nullOr types.ints.positive;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(storeReadBudget): types.ints.positive rejects 0, but --store-read-budget documents 0 as "use the default". Null already gives that. Use types.ints.unsigned or note that null is the default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null omits the flag, and that is how an operator keeps the default. The NixOS type still rejects 0.

Comment thread crates/rbitcoin-net/src/reactor.rs Outdated
let _ = tx.send(f());
})
.expect("rbtc-off-pool");
rx.await.expect("rbtc-off-pool dropped")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(off_pool): a panic in f shows as "rbtc-off-pool dropped" on the request task instead of a join error. Surface the panic as an error response.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A panic on rbtc-off-pool is resumed on the caller. It stays a panic, so a mempool bug is not reported as a JSON-RPC error.

@reardencode

Copy link
Copy Markdown
Owner

@coreyphillips would love your (agents') review of this - I'm not confident it's the right direction to go even

@coreyphillips coreyphillips left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two independent reviews.

needs changing before merge

  • disconnect_id unregisters a session that keeps running, leaking hub counters (crates/rbitcoin-net/src/peers.rs:3563-3573). disconnect_id now calls wake_for_disconnect and then unregister(id), but it no longer aborts the session. unregister releases cmpct slots, the block-download window and headers sync only when live.remove returns the peer (peers.rs:2790-2794). The session then keeps running the in-flight handler and drains decode_q. Those paths take hub-wide state again, with no stop check: - A queued ping runs on_ping, then maybe_queue_initial_getheaders, then try_start_headers_sync (peers.rs:2243-2267). - drain_after_accept calls queue_block_getdata, then note_blocks_requested, which bumps peers_downloading_from (peers.rs:1223-1227). When the session returns, service.rs:595 / :776 call unregister(id) again. live.remove finds nothing, so nothing is released. How I confirmed it: I wrote a scratch copy of disconnect_applies_a_block_already_queued in a /tmp copy of the tree. 1. Hold every block-decode permit. 2. Send a block, then a ping. 3. Call disconnect_id, then release the permits. 4. Close the socket, await the session, then call unregister the way service.rs does. REVIEW before: headers_sync_peers=1 sync_started=true REVIEW after disconnect_id: headers_sync_peers=0 REVIEW after session end + unregister: headers_sync_peers=1 sync_started=true assertion `left == right` failed left: 1 right: 0 Why it matters: - Until the tip is within 24h, try_start_headers_sync does compare_exchange(0, 1), so a stuck 1 refuses initial headers sync from every other peer. - check_headers_sync_timeouts walks only live peers, so it never clears the stuck slot. - The peers_downloading_from variant adds half a block interval to every peer's stall timeout per leak. Every disconnect_id caller can trigger it: disconnectnode, inbound eviction, stale-tip outbound replacement, and handshake timeouts. A fix could release on session exit, or keep the post-disconnect drain from taking hub-wide state. Either way, add a test that checks the counters after a disconnect.
  • A budget wait can produce a template with missing parents (crates/rbitcoin-rpc/src/server.rs:1096-1100)

worth doing, does not block

  • Old sync paths stay beside the new async ones, and most tests exercise the old ones (crates/rbitcoin-rpc/src/methods/mod.rs:678-712). Code-shape rule 9 is an AGENTS.md hard stop. What is still in place: - dispatch_inner still routes sendrawtransaction, testmempoolaccept, submitpackage, submitblock, generate*, generateblock and invalidate/reconsider/precious to the sync implementations (methods/mod.rs:678-712). HTTP always goes through dispatch_tip_connect instead. - Electrum dispatch_pinned keeps broadcast_raw_tx / broadcast_package (electrum/src/server.rs:2746-2747) beside the async copies. - MempoolHub keeps sync accept_tx_from plus sync twins for promote, restore and abandon. - satisfy_http_wait and the waited slice in exec_http_rpc are test-only; production passes &[]. About 240 test call sites drive these methods through in-process dispatch, so they test code that production no longer runs. The twins have already drifted: - the generateblock text above; - sync submitblock falls back to ctx.regtest, while async requires ctx.chain; - async promotion runs after expire_stale / publish_fee_floor, sync before.
  • Every plain RPC POST holds a store permit, including tip waits and non-store calls (crates/rbitcoin-rpc/src/server.rs:708)
  • SV2 flight serializes builds for every key, not per tip and budget (crates/rbitcoin-sv2/src/template.rs:198)
  • SV2 SubmitSolution still waits on the blocking pool (crates/rbitcoin-sv2/src/session.rs:578). The description says an SV2 solution still finishes with every blocking-pool thread parked. However, the bits pre-check before accept_block_async runs in tokio::task::spawn_blocking (session.rs:578-583). With a parked pool, a solved block waits for a pool thread before it can reach tip-accept. Moving submit_timestamp_needs_other_bits to off_pool would make the claim hold.
  • Queued frames skip the send-budget pause and the disconnect verdict (crates/rbitcoin-net/src/peer.rs:2027-2032)
  • off_pool spawns a new OS thread per step and panics if the spawn fails (crates/rbitcoin-net/src/reactor.rs:56-65)
  • Store permits are held while parked on heavy_sh_join and gbt_cache (crates/rbitcoin-electrum/src/server.rs:1036-1052)

nits

  • Leftover handles after removing the session abort (crates/rbitcoin-net/src/peers.rs:730). - take_session_abort (peers.rs:730) is no longer called in production, while set_session_abort still runs in service.rs. - decode_tasks in peer_session_with is only pushed and retained, never read. - TemplateFlight::inflight is written and never read. A quick build can also overwrite a newer handle.

the reviewers disagree, your call

  • HTTP generateblock reports "missing prevout" instead of Core's reject reason (crates/rbitcoin-rpc/src/methods/mine.rs:322)
  • Decode queue can hold 68 MB per peer, with no global bound (crates/rbitcoin-net/src/peer.rs:270-272)
  • Sampling the template tip before the permit can pair an old tip with a newer mempool (crates/rbitcoin-rpc/src/server.rs:1091-1096)
  • Shutdown can persist unfinished dry-run inserts (crates/rbitcoin-rpc/src/server.rs:1171-1172)

Comment thread crates/rbitcoin-rpc/src/methods/mine.rs Outdated
let hashes = chain
.generate_to_script_for_rpc_async(1, script, extra, false)
.await
.map_err(|e| generateblock_missing_input_error(&e.to_string(), &extra_for_err))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reviewers disagree, your call: HTTP generateblock reports "missing prevout" instead of Core's reject reason

generateblock is now a tip_connect_method, so every HTTP call runs generateblock_async. That path passes NetError::to_string() straight into generateblock_missing_input_error and generateblock_validity_error (mine.rs:310, :322). The sync path it replaces first maps the error through submit_reject_reason (HubRegtest, methods/mod.rs:267-268). That mapping turns Rules(MissingPrevout) into bad-txns-inputs-missingorspent. How I confirmed it: a scratch test in a /tmp copy. Regtest hub, 130 blocks, then generateblock raw(51) [child_hex, parent_hex] (out of order): sync dispatch: {"code":-25,"message":"TestBlockValidity failed: bad-txns-inputs-missingorspent"} dispatch_tip_connect: {"code":-25,"message":"TestBlockValidity failed: missing prevout"} - Core's rpc_generate.py is listed as run in scripts/core-functional/inventory.toml and asserts the first string. - The in-repo assert at regtest_chain_ops_journey.rs:2670 goes through in-process dispatch, so it never covers the HTTP path. - core-functional was skipped on this PR's CI. - Every Rules variant diverges the same way. Mapping with submit_reject_reason(&e) at both call sites fixes it.

Objection: Real, but non-blocking rather than blocking: The async conversions use Display, which renders MissingPrevout as "missing prevout" instead of the mapped Core reject token. HTTP selects this path, so the response regression is real.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP generateblock maps the error through submit_reject_reason. A missing input comes back as bad-txns-inputs-missingorspent.

.into_response();
}
};
let store_permit = budget.acquire_owned().await.expect("store budget");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth doing, does not block: Every plain RPC POST holds a store permit, including tip waits and non-store calls

rpc_post takes one budget permit for the whole plain path, whatever the method (server.rs:708). - getblockcount and getbestblockhash call wait_chain_tip (chain.rs:12-25) while holding it. That blocks until the in-flight tip-accept job finishes. - With --datadir-hdd (budget 1), a client polling getblockcount during a connect stalls every Electrum, Esplora, REST and BlindBit read for the length of that connect. - stop, uptime, help, getpeerinfo and getnetworkinfo also queue behind disk reads. - A whole batch holds one permit for its full run. Electrum already skips non-store methods via method_reads_store. RPC has no such filter. Separately, request-task methods bypass ActiveCall::enter: dispatch_tip_connect (mod.rs:56-76) does not call it, so getrpcinfo.active_commands no longer lists them over HTTP.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A plain RPC POST still takes one store permit for every method, including getblockcount. Leaving that for a follow-up.

Comment thread crates/rbitcoin-sv2/src/template.rs Outdated
impl TemplateFlight {
pub fn new() -> Self {
Self {
one: Arc::new(Semaphore::new(1)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth doing, does not block: SV2 flight serializes builds for every key, not per tip and budget

TemplateFlight has one Semaphore::new(1) per listener (template.rs:198) and a single (key, template) slot. The changelog and docs say builds are one at a time "for the same tip and coinbase budget". In fact sessions with different budgets queue behind each other, where before they built in parallel. Interleaved budgets (A, B, A, B) also evict each other's cached template, so a second A builds again. On a new tip, the last session's template waits up to N build times. It also waits behind fee-check builds keyed to the old tip; those are built and then thrown away. The key also has no time component. header_timestamp = max(mintime, now), and on min-difficulty networks n_bits depends on it (template.rs:115-124). So on testnet a late-joining session can reuse an old timestamp and full-difficulty bits. inflight is written but never read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each key has its own gate. The key is the previous-block hash, coinbase size, coinbase sigops, template-update counter, nBits, and header time. A different key builds on its own. Idle entries past eight are dropped, and a live build is kept. The caller waits on the build task. Dropping that wait leaves the permit in the task, so a second build of the same key still waits.

Comment on lines +270 to +272
pub const DECODE_Q_MAX_FRAMES: usize = 32;
pub const DECODE_Q_MAX_BYTES: usize =
(MAX_SERVE_BLOCKS + 1) * crate::codec::MAX_PROTOCOL_MESSAGE_LENGTH;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reviewers disagree, your call: Decode queue can hold 68 MB per peer, with no global bound

DECODE_Q_MAX_BYTES is (MAX_SERVE_BLOCKS + 1) * 4 MB, about 68 MB, plus 32 frames per session (peer.rs:270-272). Before this PR, the reader waited on the global decode semaphore, so each peer held about one frame. Now any peer, including one sending unsolicited block or headers frames, can fill its queue while decode permits are contended. At the default 200 inbound slots, the worst case is several GB. docs/concurrency.md says "the getdata window, not the semaphore, bounds that RAM". That only holds for requested frames. The 32-frame cap also counts cheap frames queued behind one slow heavy decode. An honest peer that sends a block followed by a burst of more than 32 tx or inv messages is disconnected with "decode queue full".

Objection: Real, but blocking rather than non-blocking: Heavy frames accumulate before acquiring decode permits, without a requested-block filter or shared byte budget. The per-peer allowance permits several GB across inbound sessions, and the frame cap can also disconnect honest bursts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each peer still queues one getdata window of block, headers, and notfound frames. Those heavy bytes also share a process cap of the blocking-pool size times that window, so unsolicited bodies are included. tx, inv, and ping use a separate cap of 1024 and do not count toward the heavy bytes.

Comment thread crates/rbitcoin-net/src/peer.rs Outdated
Comment on lines +2027 to +2032
decoded = std::future::poll_fn(|cx| {
let Some(rx) = decode_q.front_mut() else {
return Poll::Pending;
};
std::pin::Pin::new(rx).poll(cx)
}), if !decode_q.is_empty() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth doing, does not block: Queued frames skip the send-budget pause and the disconnect verdict

The decode arm is gated only by !decode_q.is_empty() (peer.rs:2027-2032), not by reading. Two consequences: - Paused peer, dropped messages. Queued getheaders, getblocks and inventory messages applied after a getdata reply crosses the send budget are dropped by the over-budget early returns (peer.rs:3478-3487, :3741). Core delays them under fPauseSend; it does not drop them. - Banned peer, still processed. apply_queued_decodes (peer.rs:3395) keeps applying every queued frame after a misbehavior threshold, a rate-limit strike or an error. We still admit its tx, store its addr and validate its blocks after deciding to drop it. Only block, cmpctblock and blocktxn frames need applying to keep the getdata bookkeeping right.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the way out, only block, compact-block, and blocktxn frames are applied. While the send budget is spent, getheaders, getblocks, and inventory wait at the front of the queue. A later frame cannot pass them.

Comment thread crates/rbitcoin-net/src/reactor.rs Outdated
Comment on lines +56 to +65
let _ = tx.send(f());
})
.expect("rbtc-off-pool");
rx.await.expect("rbtc-off-pool dropped")
}

/// Run `fut` with the API store-read budget armed. Prepare takes one permit
/// and drops it before script verify. The commit coin re-read takes one
/// again. A task that never calls this does not take the budget (P2P decode,
/// and admits that are not an API read).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth doing, does not block: off_pool spawns a new OS thread per step and panics if the spawn fails

off_pool creates a fresh rbtc-off-pool thread on every call and expects the spawn (reactor.rs:56-65). Before, these steps ran on pooled spawn_blocking. - One relayed tx admit spawns at least five threads: spec, prepare, commit, publish, and take-orphans. - Each long-poll waiter spawns about two threads every 50 ms, and each of those ticks also takes a store permit. - Detached admits (spawn_with_store_budget) outlive the HTTP work-queue slot, so a client that submits and disconnects in a loop has no concurrency cap. When a spawn fails (e.g. under a task limit), or f panics, the panic surfaces through .expect("tx admit") and similar calls. Over P2P that kills the session task; over HTTP it resets the connection. Previously these were join errors with a 500 or -1 reply.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off_pool is one rbtc-off-pool thread for the process. A call queues on that thread.

Comment thread crates/rbitcoin-rpc/src/server.rs Outdated
Comment on lines +1091 to +1096
let template_prev = if method == "getblocktemplate" {
Some(sample_template_prev(&ctx).await)
} else {
None
};
let store_permit = rbitcoin_net::acquire_armed_store_budget().await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reviewers disagree, your call: Sampling the template tip before the permit can pair an old tip with a newer mempool

getblocktemplate (server.rs:1091-1096) and Esplora GET /block-template (handlers.rs:817-825) sample template_prev, then wait for a store permit. Only after that do they build with the armed sample. If block P2 connects during that wait, gbt_tip (mine.rs:891-910) still accepts P1, because P1 is still the header at its height. Selection then reads the mempool after P2 removed its transactions, so the template can include a child whose parent confirmed in P2. That template is invalid on P1, not just stale. The same race existed before but was microseconds wide. With budget 1 the wait can now be seconds. Esplora does not cache such a body, but it is still returned.

Objection: Real, but blocking rather than non-blocking: The armed P1 sample survives a permit wait during which P2 can connect and strip the mempool. gbt_tip accepts P1 as an ancestor, allowing selection of a child whose parent exists only in P2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same confirm as the stale template sample. The test holds the only store permit, connects a block while the template request waits, then releases the permit. previousblockhash is the new tip.

Comment on lines +1036 to +1052
tokio::spawn(async move {
let permit = if reads {
Some(
q.api_read_budget()
.acquire_owned()
.await
.expect("store budget"),
)
} else {
None
};
tokio::task::spawn_blocking(move || {
let _permit = permit;
let _g = BlockingRegion::enter();
let t0 = Instant::now();
let (r, work, view) =
dispatch_off_worker(&q, &cfg, &p, mp.as_deref(), &method, &params, work);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth doing, does not block: Store permits are held while parked on heavy_sh_join and gbt_cache

Electrum spawn_dispatch (electrum/src/server.rs:1036-1052) and Esplora spawn_join take a budget permit before spawn_blocking. The closure can then park on heavy_sh_join (query/src/scripthash.rs:750-755) or on the Esplora gbt_cache mutex, which is held across the build (handlers.rs:849-855). With budget B, B heavy joins leave B-1 permits idle while one walk runs. Every other API read, including broadcast prepare and commit, waits for the whole heavy queue. Before, those waiters held pool threads but still left other threads free. On a public Electrum server, any client can trigger this by asking for a few heavy addresses.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store permit is still held across a heavy scripthash join and the Esplora template-cache lock. Leaving that for a follow-up.

Comment thread crates/rbitcoin-rpc/src/server.rs Outdated
Comment on lines +1096 to +1100
let store_permit = rbitcoin_net::acquire_armed_store_budget().await;
match tokio::task::spawn_blocking(move || {
let _g = BlockingRegion::enter();
let _store = store_permit;
let _prev = template_prev.map(rbitcoin_net::arm_template_prev_sample);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs changing before merge: A budget wait can produce a template with missing parents

getblocktemplate can combine an old tip with transactions from a newer tip because the saved sample survives the permit wait. I held the sole store permit and started a template request. While it waited, I confirmed a parent transaction and admitted its child, then released the permit. The response named the old tip and contained the child alone. A block built from it failed validation with bad-txns-inputs-missingorspent. Esplora uses the same sampling pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That pairing was the stale tip sample. The build now confirms the tip after the permit, so a child admitted against the new tip is not returned with the old previousblockhash.

Comment on lines +1171 to +1172
let t0 = Instant::now();
let dispatched = crate::methods::dispatch_tip_connect(ctx, method, params).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reviewers disagree, your call: Shutdown can persist unfinished dry-run inserts

Graceful shutdown can persist transactions inserted only for a testmempoolaccept dry run. This dispatch bypasses ActiveCall, so finish_tip_follow_wake sees no active request. Runtime teardown then cancels the detached cleanup task. I confirmed this through rpc_post by parking script verification after the first dry-run insert, completing stop, and running the shutdown flush and runtime teardown. Reopening the mempool still contained that transaction.

Objection: Real, but non-blocking rather than blocking: exec_one_async dispatches request-task methods without ActiveCall (the only enter sites are dispatch and the wait prep), so the RPC-stop drain in finish_tip_follow_wake sees zero and the mempool flush at run.rs:1587 can persist a package dry-run insert made by the detached spawn_with_store_budget task. The window is a package's later script verifies racing stop, master already has this race on signal shutdown with only a 2s drain, and the fix is to enter ActiveCall around the dispatch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testmempoolaccept, submitpackage, sendrawtransaction, and the tip commands stay in the active-command list until the call returns. Shutdown waits on that list.

reardencode pushed a commit that referenced this pull request Nov 2, 2026
note_admit decayed the bucket it updated, then applied the EMA's 1 - alpha
to it again, so a steady stream settled near half its rate. Reads also moved
the clock the next sample's gap was measured from. Keep a separate admit
clock for the gap and add only the new sample after the decay.

Part of #1274. The CPFP chunk rate is left for after the package admit
rework in #1333.
@rearden-grok
rearden-grok Bot force-pushed the api/thread-budget branch 2 times, most recently from 4e75090 to 09ff01d Compare November 3, 2026 05:53
@rearden-grok

rearden-grok Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

disconnect_id keeps a handshaken peer in the hub until the session returns and unregisters. The drain does not start a new headers sync or a new block-download window. A handshake that never finished still unregisters inside disconnect_id. The post-handshake make-room failure unregisters explicitly, because disconnect_id no longer does that itself.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 bugs, 0 suggestions, 0 nits

This is the right split for a family node and a public explorer. Connect and mempool admit no longer occupy the Tokio blocking pool, and wallet reads share one store-read budget. The remaining stall is the one already left open: an Electrum or Esplora history join holds that permit for the whole scripthash walk, so on a budget of 1 it blocks every other wallet read.

getblockchaininfo walks the datadir for a byte total. The same total
stands for 30 seconds, and a newer total replaces it.
Block connect stays on the tip-accept thread. Mempool script verify
stays on rbtc-scripts. Prepare and commit queue on one rbtc-off-pool
thread, started on first use, so a full Tokio blocking pool cannot
stall either. A job on that thread must not enter it again.

RPC, REST, Electrum, Esplora, and BlindBit share one store-read
budget fixed at listen. Waiters do not occupy a pool thread, and a
permit stays held until that read finishes. Omitted or 0 with
--datadir-hdd is 1. Otherwise the default is one less than the core
count, minimum 1. An explicit N overrides the disk flag. P2P decode
does not take the budget.

Decode permits match the blocking pool. The decode queue holds one
serve window, and the reader still takes the next TCP frame without
waiting for a permit. A dropped request still publishes, notes
unbroadcast, rolls a package back, and commits the transactions a
generate already connected. Each of those calls detaches once.

getblocktemplate and Esplora GET /block-template read the tip, take
one permit, and read it again. They build only when both reads match.
A long-poll does not hold a work-queue slot or a disk permit while it
waits. The next call in that POST waits for a slot. HTTP generateblock
reports Core's reject reason. Request-task tip commands stay active
until they return.
@rearden-grok
rearden-grok Bot force-pushed the api/thread-budget branch from 482446a to 3894383 Compare November 3, 2026 17:53
A second session with the same previous block, coinbase size, sigop
budget, nBits, header time, and template-update counter waits and
reuses the build. A different key builds on its own. Idle entries past
eight are dropped. A live build is not. The caller waits on the join
handle. Dropping that wait still detaches the task and leaves the
permit inside it, so a second build of the same key waits. A template
whose previous block is not that key is not cached.
…urns

disconnect_id leaves a handshaken peer registered until its session
returns. The drain applies block bodies already read and does not start
a headers sync or a download window.

Each peer queues one getdata window of block, headers, and notfound
frames. Those heavy bytes also share a process cap of the blocking-pool
size times that window. tx, inv, and ping use a separate cap of 1024
and do not count toward the heavy byte cap. The byte counter uses
try_update.
@rearden-grok
rearden-grok Bot force-pushed the api/thread-budget branch from 3894383 to 80bcd01 Compare November 3, 2026 17:59
@reardencode
reardencode merged commit 3bdaaa4 into master Nov 3, 2026
21 checks passed
average-gary added a commit to average-gary/rbitcoin that referenced this pull request Nov 4, 2026
reardencode#1333 moved connect off the Tokio blocking pool so a parked pool cannot
hold a found block, and SubmitSolution now awaits the tip-accept thread.
The nBits read that precedes that connect (ChainHub::expected_child_bits,
asked for every solution, pushed template or custom job) still took a
blocking-pool thread, so a full pool still delayed a solution.

It now runs on rbtc-off-pool, where the template build and the mempool
prepare and commit already run. Journey: with the pool's one thread
parked, a solution for a pushed template still becomes the tip and pushes
the next NewTemplate. Red was no tip inside five seconds.

D14 no longer lists template builds and SubmitSolution among the users
that can fill the pool, and says why a validation takes no
--store-read-budget permit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants