[bobberb]

Batch items used to run one after another, so a 100-address get_history page cost the sum of 100 lookups.

• Read-only items (scripthash history/balance/unspent/mempool, tx get/merkle/id_from_pos, block header(s), fee estimates, outpoint status) go to the blocking pool, up to ElectrumConfig::batch_window per connection (half the node's blocking pool, at least 1: 2 on a 4-thread pool), and are written in request order.
• Slot i is written before read i + window starts, so a client that stops reading still stops the work and holds at most a window of unwritten replies. Each read reports its own dispatch time.
• Anything stateful (subscribe, unsubscribe, server.version, broadcast, ...) waits for the reads before it, then runs alone through answer_one. Notifications, non-objects, streaming methods and the 345 cap are unchanged.
• The single-request path is the same code, split into spawn_dispatch and finish_reply.
• Memory: an unpaged scripthash join above --heavy-sh-creates (default 10000, 0 off) takes a node-wide turn, so only one heavy join runs at a time across all Electrum and Esplora clients, whatever the batch window or connection count. Lighter joins stay parallel. A waiting heavy join holds a blocking thread.

Numbers, 100 get_history lookups, median of 7, warm cache:

	one batch	2 conns	4 conns	8 conns
mainnet node (4 vCPU), small set	191 ms	112	69	52
mainnet node, mixed set (20 with 100 to 735 entries)	669 ms	345	244	236

The mainnet split across connections stands in for in-batch concurrency; it flattens at 4. Local regtest, this branch vs master, one batch line (12 cores, tiny store): small 8.9 ms to 3.4 ms, mixed 32.9 ms to 15.5 ms.

Tested: new suite tests use a debug-only, opt-in trace of spawn/serial/write steps to check the window never goes past 4, that a subscribe drains earlier reads and is seen by a later unsubscribe, order, and that a stalled reader stops reads at slot + 4. Hand-checked mutants (window bound off by one, unbounded, serial, barrier without drain, pop_back order, subscribe or unsubscribe treated as a read) all fail. cargo test -p rbitcoin-electrum and the rbitcoin-test electrum journeys pass, plus fmt, clippy, ast-grep and cargo check --release.

Closes #1174

🤖 Generated with Claude Code

[rearden-grok]

Verdict

APPROVE. I would merge 55d4712c9b78f51aab29fed6a38fbe5131869290 as-is.

Read-only batch items go to the blocking pool, at most four per connection, and their replies are written in request order. Anything else (subscribe, unsubscribe, server.version, broadcast, a streaming method, a non-object) waits until those reads are written, then runs alone. Slot i is written before read i + 4 is spawned. Notifications still produce no slot.

Findings

Blocking: none.

Nit. land always installs that worker's sh_join. A later read that never touches the slot (header, tx, fee, mempool histogram) still holds the Arc cloned at spawn, so landing it puts the pre-window snapshot back and drops a join an earlier item in the same window just built. Results stay correct: a slot is reused only when the scripthash and tip hash match, and a miss recomputes. The comment on land overstates this as sequential cache behavior. A page of get_history calls, which is the change this PR is for, only keeps the last scripthash anyway.

Nit. A batch read's timer starts in Asked::of and stops when the slot is written, so the API log and rbitcoin_electrum_request_seconds_total include window wait and head-of-line delay. Single-request timing is unchanged.

Nit. The trace hooks are cfg(debug_assertions) and the new tests call them directly, so cargo test --release would fail to compile this suite. Debug #[tokio::test] is current-thread, which is why the thread-local trace is stable.

What I checked

The read-only allowlist matches the methods that only read the store, the mempool, the negotiated protocol, and the join cache. Subscribe, unsubscribe, server.version, header and outpoint subscriptions, silent-payments unsubscribe, and broadcast stay on the serial path. Reads get an empty subscription set.

ShJoinSlot is not mutated in place across tasks. Each read gets its own Arc clone, Arc::make_mut copies when the refcount is above one, and the connection keeps its own clone until land.

JSON-RPC behavior holds: request order, skipped notifications, "id": null still answered, non-objects and in-batch streams get -32600, and the empty-batch and 345-item responses are unchanged. Subscription notifications cannot land in the middle of the array. answer_batch runs on the read branch and holds the writer until the closing ]\n.

A connection holds at most four blocking tasks and four unwritten replies. A stalled write stops further spawns. If that write fails, the remaining JoinHandles are dropped and those tasks run to completion. The node blocking pool is still max(cores, 4), so one connection can occupy every blocking thread. That matches the operator note. BlockingRegion is a thread-local flag so store IO is allowed on those threads. It is not a permit, and the four reads do not nest it on one thread.

No new unwrap. No dependency, lockfile, or auth changes.

Build and test

On that commit, rbitcoin-electrum only:

• cargo fmt -p rbitcoin-electrum -- --check: exit 0
• cargo test -p rbitcoin-electrum --test suite -- --test-threads=4 --quiet: 49 passed
• the two new tests, 40 more times: passed
• cargo clippy -p rbitcoin-electrum --all-targets -- -D warnings: exit 0

Skipped: workspace cargo test, cargo test --release, cargo mutants, ast-grep, and node/functional harnesses. The in-crate suite includes the scripthash journey.

[rearden-grok]

Nit: this always installs the worker's slot. A read that never touches sh_join (header, tx, fee, mempool histogram) still holds the Arc cloned at spawn, so landing it restores that pre-window snapshot and drops a join computed earlier in the same window. Results stay correct (sh_join_slot_hit requires scripthash and tip hash, then a miss recomputes). The comment above overstates this as sequential cache behavior. Installing the slot only when this worker replaced it would match that comment. Homogeneous get_history pages only keep the last scripthash either way.

[bobberb]

Added a node-wide limit in 1f43db7: an unpaged scripthash join with more than --heavy-sh-creates creates (default 10000, 0 off) now waits until no other heavy join is running, across every Electrum and Esplora client. So a batch or a handful of connections can no longer stack several 2 GiB joins. Lighter joins and paged history stay parallel. The journey checks heavy joins never overlap (two connections and one batch) while light ones do.

[coreyphillips]

Two independent reviews, nothing blocking a merge.

worth doing, does not block

• One batch can occupy every blocking thread on a 4-core node (crates/rbitcoin-electrum/src/server.rs:686). On a host with 4 or fewer cores, one batch connection can fill the node's whole blocking pool, which the issue asked the cap to prevent. BATCH_CONCURRENCY = 4 equals the smallest pool blocking_pool_size() builds, max(cores, 4) (crates/rbitcoin-node/src/cli.rs:571). answer_batch spawns four reads before it lands any. Raspberry Pi class hardware and the 4 vCPU box in the PR's table are in this case. That pool is not only Electrum's. Per docs/concurrency.md:198-201, block decode, compact reconstruct, header jobs and mempool accept all use it. Before this PR, a single wallet held one thread. Now block relay and tx accept wait for an Electrum read to finish, and a read near --max-sh-creates can take a long time. The delay is bounded, because tokio's queue is FIFO and a freed thread goes to the waiting job before read i+4 is spawned. Four connections could already do this before. Still, the issue asked for a cap "so one batch can't take every blocking thread", and the new doc comment says this is not met. One fix is to derive the cap from the pool size, for example leaving at least half the pool free, and pass it in through ElectrumConfig. Confirmed by reading answer_batch and node_tokio_runtime. I did not measure the relay latency.
• Batch read wall time now includes queueing and earlier slot writes (crates/rbitcoin-electrum/src/server.rs:834). For batch reads, the wall_ms in the API log and the METER totals now count time spent waiting for other items, not only the read itself. Asked::of (line 802) starts t0 when batch_item classifies the item (line 834). That happens before answer_batch waits on land(oldest) for a free window place. finish_reply stops the clock only when the read is landed (line 1070). By then the earlier reads in the window have landed and their slots have been written, and each write may take up to idle (120 s by default). Before this PR, answer_one started t0 after the previous slot was written, and stopped it before its own write. Two examples: - A 1 s read at slot 0 makes reads 1 to 4 each report about 1 s, even if they took 1 ms. - With a wallet that reads slowly, cheap reads report the socket stall. This inflates rbitcoin_electrum_request_seconds_total, the Electrum avg/max fields in tip: perf, and per-request wall_ms. Operators use those to find slow queries. A fix is to time the dispatch inside the blocking closure and return the elapsed time with Dispatched. Confirmed by reading the code path; not measured.
• land can roll the join cache back to a stale copy (crates/rbitcoin-electrum/src/server.rs:965). land copies every read's sh_join back into conn (line 965). That includes reads that never touch the cache: transaction.get, headers, get_mempool, estimatefee. Those return the copy read_conn took when they were spawned. So in a mixed batch, a later non-scripthash read undoes the slot an earlier get_history filled, and the comment "as if the reads had run one after another" is not accurate. Separately, read_conn clones the Arc (line 948) while conn keeps its copy. On a cache hit, Arc::make_mut in rbitcoin-query/src/scripthash.rs:1816 and :2126 then deep-copies the whole joined slot. The serial path take()s the slot and fills it in place. Answers stay correct because this is only a cache. Copying back only when the slot pointer changed would keep the comment true.

[bobberb]

Thanks, all addressed in ccdb57a:

• Blocking pool: the window is now ElectrumConfig::batch_window, half the node's blocking pool and at least 1 (2 on a 4-thread pool), set by the node from blocking_pool_size.
• Timing: batch reads are timed inside the blocking closure, so the API log and meter see only their own dispatch. Single requests are timed as before.
• land: the cache is installed only when a read hands one back, and scripthash reads take the slot instead of sharing a clone, so hits fill it in place. A test checks a get_history after a mixed batch is a cache hit.
• Release: the new tests and hooks are gated on debug_assertions. cargo test --release -p rbitcoin-electrum still doesn't build on master (20 existing errors, as TESTING.md notes); this PR adds none.

Flaky tests from CI are in #1190.

[bobberb]

@reardencode this one's green and the review points are addressed in ccdb57a. Ready for a look when you have time.