Skip to content

esplora: one request limit for the whole listener, and --esplora-max-inflight - #1180

Open
bkeroack wants to merge 6 commits into
reardencode:masterfrom
bkeroack:esplora/global-conn-limit
Open

bkeroack wants to merge 6 commits into
reardencode:masterfrom
bkeroack:esplora/global-conn-limit

Conversation

@bkeroack

@bkeroack bkeroack commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #1177. The Esplora listener's request limit (ServeLimits::max_connections, 256) bounded each route on its own, not the listener. axum's Router::layer applies a layer to every route endpoint, the fallback and the catch-all separately (axum 0.8.9 src/routing/mod.rs:311-316, src/routing/path_router.rs:293-298). Each ConcurrencyLimitLayer::layer call makes a new Semaphore (tower 0.5.3 src/limit/concurrency/service.rs:27-28). With 63 routes, the real ceiling was in the thousands.

  • Fix: GlobalConcurrencyLimitLayer shares one semaphore between every clone, so the limit covers the whole listener (crates/rbitcoin-esplora/src/server.rs). Over the limit, a request waits for a slot; it is not refused.
  • The timeout now covers that wait. TimeoutLayer moves outside the limit, so one deadline covers the wait for a slot and the work (408 when it runs out). Before, the queue hardly ever engaged. Once it does, a request queued behind slow ones could wait about one timeout per request ahead of it, because each holder keeps its slot until its own timeout.
  • New knob: --esplora-max-inflight N (conf esplora_max_inflight=, NixOS services.rbitcoin.esplora.maxInflight), >= 1 and at most tokio's Semaphore::MAX_PERMITS (above it Semaphore::new panics; apply_kv refuses it as a config error, and the NixOS type stops at the 64-bit ceiling), default 256. Until now the value had no setting at all. DEFAULT_MAX_CONNECTIONS is now re-exported from rbitcoin-electrum for the default.
  • Docs: a row in docs/operator/operations.md and in the interfaces.md limits table; --help; changelog.d/esplora-global-conn-limit.md.

Evidence

  • esplora_max_connections_spans_every_route (tests/esplora_http_journey.rs), with max_connections = 4:
    • It holds 4 POST /tx requests whose bodies never arrive, then sends one GET /blocks/tip/height and requires that it is still unanswered.
    • It drops one held request and requires that same GET to be answered 200.
    • Before the fix it fails at the "waits" assertion (line 848): the GET is answered at once because it has its own 4 permits.
  • esplora_request_timeout_covers_the_wait_for_a_slot uses limit 1, a 1 s timeout, and five slow-body POST /tx queued ahead, then sends a GET.
    • The GET must be answered within 2.5 s, with 200 or 408. The held requests' deadlines started first, so their 408s can free the slot just in time; either answer is within one timeout. Before the timeout move it took 4.9 s and failed.
  • esplora_max_inflight_cli_conf_and_bounds (tests/cli.rs): the default, --esplora-max-inflight N and =N, refusal of 0, of a non-number, of MAX_PERMITS + 1 and of 9999999999999999999, and that a refused value leaves the default. Modelled on electrum_max_subs_cli_conf_and_bounds.
  • NixOS eval: --esplora-max-inflight 1024 is set when Esplora is enabled, absent when Esplora is off but maxInflight is set, and the option defaults to null. Checked by CI's qc job (nix build .#checks.x86_64-linux.nixos-module-eval); not run locally.
  • Wiring, checked by hand on regtest with the release build. The startup line reads esplora HTTP on … (max_conn=7 …) with --esplora-max-inflight 7, and max_conn=256 without it. As with --electrum-max-subs, there is no automated test that run.rs passes the value through.
  • cargo fmt --all --check and clippy -p rbitcoin-node -p rbitcoin-esplora -p rbitcoin-electrum --all-targets -D warnings pass. Tests for those three crates pass, except cli::trust_peer_addr, which passes when rerun alone (it races on process env).

Merge Danger

Door: two-way

Blast Radius: every Esplora listener. This tightens behaviour on purpose. The documented 256 now actually applies, so a node that used to spread several thousand concurrent requests over its routes will queue past 256. One heavy client, such as a wallet rescan or the ~200 parallel asks spdk sends, can hold most of the slots, and other Esplora clients then wait for a slot rather than being answered at once. They wait at most one request timeout (120 s by default) before a 408.

  • /health and /metrics are on the separate health listener and are not affected.
  • Operators who relied on the old ceiling can raise --esplora-max-inflight. If you would rather keep a higher default (for example 1024) so that upgrades change nothing for existing users, it is a one-line change to the default, and I'm happy to make it.
  • Electrum is untouched: it limits connections with its own counter in the accept loop.
  • With esplora: serve the BlindBit oracle v1 API for silent-payment wallets #1169: as reviewed, a trailing .layer() would take this permit before BlindBit's and hold it while waiting on BlindBit's. esplora: serve the BlindBit oracle v1 API for silent-payment wallets #1169 now takes the BlindBit permit first, in a middleware mounted outside the listener layers, and applies the listener layers to its router once before merging. Whichever lands second gets a rebase so both routers share this one GlobalConcurrencyLimitLayer.

🤖 Generated with Claude Code

@rearden-grok rearden-grok Bot 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.

Verdict

REQUEST_CHANGES. GlobalConcurrencyLimitLayer is the correct fix for axum cloning a layer onto every route, and the new journey test fails without that one-line change. I would not merge this as-is. Requests waiting for a permit sit outside the request timeout, with no cap on how many can wait. A config value the parser accepts panics the process. Combined with open PR 1169, BlindBit’s 32-permit sub-limit holds a listener permit while it waits, so a rescan fills the listener instead of staying inside 32.

Findings

High — a permit is held for the whole wait underneath it

GlobalConcurrencyLimitLayer is the outermost limit on the Esplora router (server.rs). Tower takes the permit in poll_ready and keeps it until the response future drops (tower 0.5.3 limit/concurrency/service.rs). TimeoutLayer starts its sleep in call, so time spent waiting for a permit is outside the deadline. That deadline is idle_timeout floored at 1s, default 120s. Tower does not cap the waiter list.

An incomplete POST /tx (Content-Length: 64, body never sent) holds a permit for that whole inner deadline, or until the peer drops. RequestBodyLimit returns 413 only when Content-Length is already over the cap; otherwise it forwards the stream. post_tx extracts Bytes, so the handler does not finish until the body arrives. The new journey test depends on this. One client can occupy every --esplora-max-inflight slot (default 256) and refresh them each 120s. Every other route on that listener waits, with no 408. Startup raises the soft RLIMIT_NOFILE to 16384 (NOFILE_SOFT_TARGET), so the waiter set can grow to that many TCP connections. axum’s default features are HTTP/1 only, so this is one request per connection. /healthz, /readyz, /progress, and /metrics are a different listener (health.rs) and stay up. Esplora has no route outside this semaphore.

health.rs already refuses this pattern (“tower’s queues without a bound”) and try_acquires so the extra probe gets 503. Electrum rejects in the accept loop. This listener waits.

The same nesting breaks BlindBit once #1169 lands. That PR merges /blindbit/v1 with its own GlobalConcurrencyLimitLayer::new(32) immediately before this .layer(...). axum 0.8.9 wraps each existing endpoint when Router::layer runs (path_router.rs maps endpoint.layer), so the listener semaphore is outside the 32. A BlindBit request takes a listener permit, then blocks in the inner poll_ready while still holding it. #1169 says the 32 exists so a rescan queues behind itself and leaves the rest of the listener alone, and that spdk keeps about 200 filter asks open. Those ~200 requests would pin ~200 of the 256 listener slots (32 running, the rest queued on the sub-limit). The PR text says the sub-limit still isolates the listener. This order does the opposite. The scarcer semaphore has to be acquired first. One trailing Router::layer on the merged router cannot express that.

Put the request deadline outside the semaphore, or fail with 503 when the semaphore is exhausted (and cap waiters if you keep the queue). For BlindBit, apply the 32-limit outside the shared listener semaphore, and do not wrap those routes again afterwards.

Medium — a parsed esplora_max_inflight can panic the node

apply_kv accepts any usize >= 1. tokio 1.53 Semaphore::new asserts permits <= usize::MAX >> 3 and panics. On 64-bit that ceiling is 2^61−1. esplora_max_inflight=9999999999999999999 parses, then panics in run_esplora, which start_esplora_if_ready awaits on the run_p2p task. Nix types.ints.positive allows any positive Nix integer, up to 2^63−1, which is past that ceiling. run_esplora turns 0 into 1 with .max(1) and does not clamp the top. esplora_max_inflight_cli_conf_and_bounds checks 0 and a non-number only.

Reject values above Semaphore::MAX_PERMITS in apply_kv (and in the Nix type) before the listener starts.

Low — the DoS-floor paragraph still says excess work is rejected

docs/operator/interfaces.md still says excess connections are rejected immediately and do not hang, in the paragraph directly above the new row that says Esplora requests wait. Electrum does reject. The changelog says a listener “serving silent-payment rescans” may want a higher --esplora-max-inflight. Those scans are Electrum (SP_SCAN_PERMITS = 3 in silent_scan.rs). BlindBit’s cap is the 32 above. Raising this knob does not widen either one. operations.md is right that one Esplora wallet rescan can fill the listener.

Nit — the journey check is “some GET exceeded 300ms”

Without the fix the test failed at esplora_http_journey.rs:849 in 2.65s, so the cross-route claim holds in this environment. waited becomes true if any GET /blocks/tip/height exceeds 300ms, and the follow-up only checks that a later GET returns 200 within 10s. One slow response would pass under per-route semaphores. Hold one in-flight GET, require it to stay unanswered, drop one stalled body, and require that same request to complete.

There is no test that run.rs copies the flag onto EsploraConfig. Both call sites pass config.esplora_max_inflight. The assignment is one line.

What holds up

  • axum 0.8.9 Router::layer clones the layer onto every route, the fallback router, and the catch-all (routing/mod.rs 311–316, path_router.rs 293–298). ConcurrencyLimitLayer::layer builds a new semaphore per call. GlobalConcurrencyLimitLayer shares one Arc<Semaphore> (tower 0.5.3). A request enters one of those services, so a 404 costs one permit. with_state does not rebuild a Route, so it does not mint a second semaphore.
  • /health and /metrics are not on this router.
  • 0 is rejected by apply_kv (CLI and conf share that path) and clamped again in run_esplora.
  • The Nix option follows electrum.maxSubs: the flag is emitted only when Esplora is enabled and maxInflight is set. The eval assertions cover default null, --esplora-max-inflight 1024 on the unit, and absence when Esplora is off.
  • No Cargo.toml or lockfile changes. ConcurrencyLimitLayer is removed, not left beside the new layer.
  • Re-exporting DEFAULT_MAX_CONNECTIONS matches that const’s existing “Electrum / future Esplora” note.

What I ran

Head d919cc8c648790bf329ab9d547b3e65a75b8ddc0. Merge-base with origin/master: 8e62e4ce4d60c30abc851cb0893e4b80149ac9e4.

  • cargo fmt --all -- --check — exit 0
  • cargo test -p rbitcoin-esplora -p rbitcoin-node -p rbitcoin-electrum --test suite esplora_max -- --quiet — exit 0. esplora_max_connections_spans_every_route passed in 0.39s. esplora_max_inflight_cli_conf_and_bounds passed. The Electrum suite compiled; the filter matched 0 tests.
  • cargo test -p rbitcoin-esplora --test suite -- --quiet — exit 0, 35 passed
  • cargo test -p rbitcoin-node --test suite cli:: -- --quiet — exit 0, 13 passed
  • cargo test -p rbitcoin-electrum --lib -- --quiet — exit 0, 0 tests ([lib] test = false)
  • cargo test -p rbitcoin-electrum --test suite -- --quiet — exit 0, 47 passed
  • cargo clippy -p rbitcoin-node -p rbitcoin-esplora -p rbitcoin-electrum --all-targets -- -D warnings — exit 0
  • Red check: server.rs only, GlobalConcurrencyLimitLayer restored to ConcurrencyLimitLayer. esplora_max_connections_spans_every_route failed at line 849 (a full listener makes a request on another route wait) in 2.65s. File restored. Worktree clean at the reviewed SHA.
  • Nix eval not run. nix is not installed. The module and nix/tests/nixos-module-eval.nix were read.

Comment thread crates/rbitcoin-esplora/src/server.rs Outdated
.layer(ConcurrencyLimitLayer::new(max_conn));
// `Router::layer` wraps each route on its own; a global limit shares one
// semaphore, so `max_conn` bounds the listener rather than each route.
.layer(GlobalConcurrencyLimitLayer::new(max_conn));

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.

This semaphore is outside TimeoutLayer, and ConcurrencyLimit::poll_ready holds the permit while the inner service is pending. A queued request therefore never starts the 120s deadline, and tower does not cap the waiter list. An incomplete POST /tx body (what the new test uses) occupies a slot until that inner deadline. One client can pin every slot and leave every other route on this listener waiting. health.rs avoids this layer for that reason and try_acquires.

The same order breaks #1169. That PR merges /blindbit/v1 with GlobalConcurrencyLimitLayer::new(32) before this .layer(). axum wraps the existing endpoint, so the listener permit is taken first and held while the request waits for one of the 32. spdk's ~200 open filter asks would then occupy ~200 listener slots. The 32 has to be the outer semaphore, and these routes must not be wrapped again.

Put the deadline outside this layer, or return 503 when the semaphore is exhausted.

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.

Addressed in 63a161b.

"conf esplora_max_inflight must be >= 1".into(),
));
}
self.esplora_max_inflight = n;

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.

usize parse plus n == 0 still accepts values Semaphore::new rejects. tokio 1.53 panics when permits > usize::MAX >> 3 (2^61-1 on 64-bit, 2^29 on 32-bit). 9999999999999999999 parses and then panics in run_esplora on the run_p2p task. Nix types.ints.positive allows 2^63-1, which is past that ceiling. Reject anything above Semaphore::MAX_PERMITS here, and cover it in esplora_max_inflight_cli_conf_and_bounds.

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.

Addressed in 5ca9103.

Comment thread docs/operator/interfaces.md Outdated
| Limit | Default | Role |
|-------|---------|------|
| Max connections | 256 | Concurrent Electrum TCP clients |
| Max Esplora in flight | 256 (`--esplora-max-inflight`) | Esplora requests in flight across the listener, every route together. Further requests wait for a slot |

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 paragraph above this table still says excess connections are rejected immediately and do not hang. This new row says Esplora requests wait. Electrum rejects; Esplora now waits, outside the 120s timeout. Split those two behaviors in the intro. Silent-payment rescans are not this knob (SP_SCAN_PERMITS is 3 on the Electrum path; BlindBit's cap is 32).

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.

Addressed in 63a161b.

@rearden-grok rearden-grok Bot 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.

Verdict

REQUEST_CHANGES. Moving TimeoutLayer outside the listener semaphore does bound the queue wait by one request timeout, and the new journey test fails if that layer is put back inside. I would not merge while a parsed --esplora-max-inflight panics the process, or while this semaphore is acquired before the BlindBit limit in open PR 1169.

What changed since d919cc8

One commit, 63a161bc. TimeoutLayer is now the outer layer in run_esplora, ahead of GlobalConcurrencyLimitLayer. esplora_request_timeout_covers_the_wait_for_a_slot queues five stalled POST /tx bodies at limit 1 and a 1s timeout. docs/operator/interfaces.md now says Electrum rejects extra connections and Esplora waits inside the request timeout. operations.md and the changelog say the same about that wait. The config parser, Nix type, run.rs wiring, and the silent-payment changelog sentence are unchanged.

Prior findings

High — permit wait outside the deadline, and BlindBit nested inside

The deadline half is fixed. tower-http 0.7.1 Timeout::poll_ready still forwards, and the sleep still starts in call. ConcurrencyLimit::poll_ready still takes the permit. It works because axum wraps each layer in a Route whose poll_ready is always ready, and Route::call oneshots the inner service from inside Timeout::call. The semaphore wait runs while the sleep is already running.

Measured at this head: five stalled POST /tx requests, max_connections = 1, idle_timeout = 1s, then GET /blocks/tip/height returned 200 in 981ms. With TimeoutLayer moved back inside the semaphore, that GET returned 200 in 4.899s and the new test failed. A stalled body still holds its slot until that request's own timeout. The waiter list is still only bounded by the raised file-descriptor limit. Each waiter now ends in 408 after one timeout.

BlindBit is not fixed. PR 1169 at b4369771 applies GlobalConcurrencyLimitLayer::new(32) inside blindbit_routes() and merges that router before this .layer() chain. Router::layer wraps the existing endpoint, so a BlindBit request takes a listener permit and then blocks in the inner poll_ready while holding it. About 200 open spdk filter requests would occupy about 200 of the 256 listener slots. The outer timeout bounds that occupation; it does not keep those queued requests off the listener semaphore. The 32-permit semaphore has to be acquired first, on those routes only, and they must not be wrapped again.

Medium — parsed esplora_max_inflight panics

Not fixed. apply_kv accepts any usize >= 1. tokio 1.53.3 Semaphore::new asserts permits <= Semaphore::MAX_PERMITS (usize::MAX >> 3). esplora_max_inflight=3000000000000000000 parses, then GlobalConcurrencyLimitLayer::new panics in run_esplora. That is awaited from run_p2p, which cli_main block_ons, so the process panics instead of returning a config error. The same path runs later, when the scripthash index becomes ready. Nix types.ints.positive allows 2^63-1, which is past that ceiling. esplora_max_inflight_cli_conf_and_bounds still checks only 0 and a non-number.

Low — DoS-floor wording and silent-payment rescans

The DoS-floor paragraph is fixed: Electrum rejects, Esplora waits, and the wait counts against the request timeout. The changelog still says a listener serving silent-payment rescans may want a higher --esplora-max-inflight. Those scans use Electrum SP_SCAN_PERMITS (3). This tree has no Esplora tweak routes. BlindBit's cap is the 32 above.

Nit — journey check and run.rs wiring

esplora_request_timeout_covers_the_wait_for_a_slot does prove the layer order. It does not assert the status code. esplora_max_connections_spans_every_route still passes if any GET /blocks/tip/height exceeds 300ms. There is still no test that run.rs copies the flag onto EsploraConfig. Both call sites pass config.esplora_max_inflight.

New findings

No new high or medium defect in the timeout move. with_state does not build a second semaphore. /health and /metrics stay on the other listener.

Low: --help and the Nix maxInflight description still omit that the wait ends in 408 at the request timeout.

What I ran

Head 63a161bcca23dbb55b6a94e234a6cfead5698cc6. Merge-base with origin/master: 8e62e4ce4d60c30abc851cb0893e4b80149ac9e4.

  • cargo fmt --all -- --check — exit 0
  • cargo test -p rbitcoin-esplora --test suite -- --quiet — exit 0, 36 passed
  • cargo test -p rbitcoin-node --test suite cli:: -- --quiet — exit 0, 13 passed
  • cargo test -p rbitcoin-electrum --test suite -- --quiet — exit 0, 47 passed
  • cargo clippy -p rbitcoin-node -p rbitcoin-esplora -p rbitcoin-electrum --all-targets -- -D warnings — exit 0
  • Layer-order check, then the file restored: timeout inside the semaphore, esplora_request_timeout_covers_the_wait_for_a_slot failed at 4.899s (HTTP 200). Worktree clean at the reviewed SHA.
  • Nix eval not run. nix is not installed. The module and nix/tests/nixos-module-eval.nix were read.

"conf esplora_max_inflight must be >= 1".into(),
));
}
self.esplora_max_inflight = n;

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.

usize parse plus n == 0 still accepts values Semaphore::new rejects. tokio 1.53.3 panics in batch_semaphore::Semaphore::new when permits > Semaphore::MAX_PERMITS (usize::MAX >> 3). esplora_max_inflight=3000000000000000000 parses, then GlobalConcurrencyLimitLayer::new panics in run_esplora. That is awaited from run_p2p, which cli_main block_ons, so the process panics instead of returning a config error, including the later start once the scripthash index is ready. Nix types.ints.positive on services.rbitcoin.esplora.maxInflight allows 2^63-1, which is past that ceiling. Reject anything above Semaphore::MAX_PERMITS here, cap the Nix type the same way, and cover it in esplora_max_inflight_cli_conf_and_bounds.

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.

Addressed in 5ca9103.

Comment thread crates/rbitcoin-esplora/src/server.rs Outdated
.layer(RequestBodyLimitLayer::new(max_body))
// `Router::layer` wraps each route on its own; a global limit shares one
// semaphore, so `max_conn` bounds the listener rather than each route.
.layer(GlobalConcurrencyLimitLayer::new(max_conn))

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.

Putting TimeoutLayer outside this semaphore does cover the queue wait. axum wraps each layer in a Route whose poll_ready is always ready, and Route::call oneshots the inner service from inside Timeout::call, where the sleep is already running. ConcurrencyLimit still acquires in poll_ready and holds the permit until that response future drops. I measured GET /blocks/tip/height return 200 in 981ms behind five stalled POST /tx bodies at limit 1 and a 1s timeout, and 4.899s with the timeout moved back inside this layer.

That does not fix #1169. blindbit_routes() applies GlobalConcurrencyLimitLayer::new(32) and is merged before this .layer(). Router::layer wraps that endpoint, so a BlindBit request takes a listener permit and then blocks on the 32 while still holding it. spdk's ~200 open filter asks would pin ~200 of the 256 listener slots. The 32 has to be the outer semaphore, on those routes only, and they must not be wrapped again.

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 #1169 half is addressed in #1169 at fdcf3d0: BlindBit requests take their permit before a listener slot. Once both land, the second to merge is rebased so those routes share this listener limit.


- **`--esplora-max-inflight N`** (conf `esplora_max_inflight=`, NixOS
`services.rbitcoin.esplora.maxInflight`) sets that limit. The default stays
256. A busy public listener, or one serving silent-payment rescans, may

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.

Electrum silent-payment rescans are capped by SP_SCAN_PERMITS (3) in silent_scan.rs. This tree has no Esplora tweak routes, and BlindBit's cap in #1169 is 32. Raising --esplora-max-inflight widens neither scan. The DoS-floor paragraph now matches the wait; this sentence still gives the wrong reason to turn the knob.

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.

Addressed in 5ca9103.

@bkeroack

Copy link
Copy Markdown
Contributor Author

Addressed in 5ca9103:

  • Medium: apply_kv refuses values above Semaphore::MAX_PERMITS, the Nix type stops at the 64-bit ceiling, and the CLI test covers MAX+1 and an oversized value.
  • High (BlindBit nesting): fixed on esplora: serve the BlindBit oracle v1 API for silent-payment wallets #1169's side. It takes its own permit first and applies the listener layers once. Whichever PR lands second rebases so both share this GlobalConcurrencyLimitLayer.
  • Low: the silent-payment sentence is gone from the changelog. --help and the Nix description say the wait ends in 408.
  • Nits: the cross-route test now holds one specific GET, requires it to stay unanswered, then requires that same request to complete once a slot frees. The timeout test checks the status (200 or 408: the held requests' deadlines start first, so their 408s can free the slot just in time). run.rs wiring stays untested, like --electrum-max-subs; it was checked by hand from the startup line.
  • The coverage failure on this head is p2p_timeout_getaddr_and_keepalive_ping, which doesn't touch Esplora; the same suite passed in test. It needs a rerun, which I can't trigger.

@reardencode

Copy link
Copy Markdown
Owner

rebase and then I'll make sure my bot re-reviews

@bkeroack
bkeroack force-pushed the esplora/global-conn-limit branch from 5ca9103 to 60d03e3 Compare October 24, 2026 19:17
@bkeroack

Copy link
Copy Markdown
Contributor Author

Rebased onto master (60d03e3).

@bkeroack

Copy link
Copy Markdown
Contributor Author

The red coverage is integration_multinode::p2p_timeout_getaddr_and_keepalive_ping, which also fails master's coverage job at 27e22a5 (run 40124172047) and e5079de (run 40106065041). Every other job is green.

@reardencode

Copy link
Copy Markdown
Owner

oh dear, rebase again :)

@rearden-grok rearden-grok Bot 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.

REQUEST_CHANGES. This head conflicts with current master, and the conflict is the listener semaphore.

Earlier findings:

  1. Medium, esplora_max_inflight above Semaphore::MAX_PERMITS: fixed.
  2. High, BlindBit permit before a listener slot: still open. Merging 57974e2f conflicts in run_esplora.
  3. Low, silent-payment changelog sentence: fixed.
  4. Nits on the timeout status, the 300ms check, and 408 in --help and the Nix description: fixed. No test that run.rs copies the flag onto EsploraConfig: still open.

1 bug, 0 suggestions, 1 nit.

Comment thread crates/rbitcoin-esplora/src/server.rs Outdated
.layer(RequestBodyLimitLayer::new(max_body))
// `Router::layer` wraps each route on its own; a global limit shares one
// semaphore, so `max_conn` bounds the listener rather than each route.
.layer(GlobalConcurrencyLimitLayer::new(max_conn))

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.

run_esplora conflicts with master at 57974e2f. listener_layers is called for the rest router and again for blindbit_routes(), with blindbit_permit_mw and a timeout outside it. GlobalConcurrencyLimitLayer::new allocates a semaphore, and a clone shares that one (tower 0.5.3). Calling new inside the closure gives /blindbit/v1 its own max_conn permits, so a scan does not consume --esplora-max-inflight. blindbit_v1_routes_share_one_inflight_limit still sees 200 on /blocks/tip/height then, because max_connections is one above the BlindBit cap. Leaving ConcurrencyLimitLayer outside this TimeoutLayer holds a per-route permit across the wait.

Build the layer once and clone it into both routers. Keep this timeout outside the layer, and keep the BlindBit permit and its timeout outside listener_layers.

bkeroack and others added 6 commits October 24, 2026 16:58
axum's Router::layer applies a layer to every route on its own, and each
ConcurrencyLimitLayer::layer call makes a new semaphore, so each route had
its own max_connections permits. GlobalConcurrencyLimitLayer shares one.

Fixes reardencode#1177.
The limit used ServeLimits::max_connections, fixed at 256, with no knob.
Now that it bounds the whole listener rather than each route, operators
of a busy public listener need to be able to raise it. Default stays 256.
Conf esplora_max_inflight=, NixOS services.rbitcoin.esplora.maxInflight.
With one limit for the listener the queue engages. TimeoutLayer sat
inside the limit, so a request queued behind slow ones waited up to one
timeout per request ahead. It is now outermost: one deadline covers the
wait and the work. Docs no longer say excess Esplora requests are
rejected.
…e limit tests

A value above Semaphore::MAX_PERMITS parsed and then panicked in run_esplora;
apply_kv now refuses it, and the NixOS type stops at the 64-bit ceiling.
The cross-route test holds one GET and requires that same request to wait and
then complete; the timeout test checks the status. Help, NixOS and docs say
the wait ends in 408; the changelog drops the silent-payment reason, which
this knob does not widen.
With one listener-wide limit, the last listener slot is free only if
asks queued on the BlindBit permit hold none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bkeroack
bkeroack force-pushed the esplora/global-conn-limit branch from 60d03e3 to 9a88526 Compare October 24, 2026 23:00
@bkeroack

Copy link
Copy Markdown
Contributor Author

Rebased onto master after #1169 (9a88526). The BlindBit routes now share the listener-wide limit: one cloned GlobalConcurrencyLimitLayer wraps both routers, and each router has a single outermost timeout. The BlindBit permit is still taken before a listener slot. With the permit moved inside the slot, blindbit_v1_routes_share_one_inflight_limit now fails at its tip-height check, which it couldn't do while the limit was per route (257f253).

@rearden-grok rearden-grok Bot 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.

REQUEST_CHANGES

  1. High, GlobalConcurrencyLimitLayer::new inside listener_layers: fixed. run_esplora builds one layer and clones it into both routers; the BlindBit permit is taken before a listener slot, under one timeout. Merges with master.
  2. Nit, no test that run.rs copies the flag onto EsploraConfig: still open.

The Semaphore::MAX_PERMITS bound, the silent-payment changelog sentence, and the nits on the timeout status, the 300ms check, and 408 in --help and the Nix description did not regress.

0 bugs, 1 suggestion, 1 nit.

// While the listener limit is per route, this route's slots are its own
// and this passes either way. It checks the permit order once the
// listener limit spans every route.
// The listener's last slot is free only if the queued asks hold none.

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.

blindbit_v1_routes_share_one_inflight_limit returns 200 for GET /blocks/tip/height if GlobalConcurrencyLimitLayer::new moves back inside listener_layers. max_connections is MAX + 1, and the three queued asks are the ones that must not hold a slot, so this GET still has a permit when /blindbit/v1 has its own semaphore. The check fails only when those asks hold a shared slot, which is the permit order.

Set max_connections to the number of admitted BlindBit bodies and require this GET to stay unanswered until one body is dropped. Keep a spare slot only for the order check.

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.

esplora: max_conn limits each route separately, not the listener

2 participants