rpc: gate REST and bound listener and scan costs - #878
rearden-grok[bot] wants to merge 7 commits into
Conversation
Unauthenticated /rest/ was on every RPC listener and took an RPC work-queue permit before the body was read. The flag defaults off. The body is read first, and REST uses its own queue.
A nonzero start height walked from that height through the tip, and the scan kept running after the client hung up. Every start now stays inside the existing 256-block window, and each chunk checks the socket once before it runs.
The chunk count was only read by the test, so a library build rejected it. The subscribe path now uses that count: if the client hung up before any chunk, it does not write a history notification.
A waitfor* call held the only work-queue permit for the whole sleep, so other RPC calls got 503. The permit is taken after the wait. Requested timeouts and getblocktemplate long-poll stop at two minutes, and the listener uses a 256-connection cap and the same request timeout.
Clippy rejects `&waker` because `Waker::noop` already returns a reference.
Silent-payment unsubscribe and scantxoutset logged the scan key and xprv/tprv text. api_call strips those before the trace line and the JSONL file, so every caller is covered.
The index links 063-066 for REST, log redaction, the scan window, and RPC wait limits.
b60f86f to
787b4d9
Compare
e41fb19 to
a2a607b
Compare
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Gating REST, giving it its own queue, reading the body before that permit, clamping silent-payment scans to the existing 256-block window, and redacting scan secrets and xprv/tprv material in api_call are the right fixes. NixOS defaults rpc.rest to false and only passes --rest when RPC is also on. The new tower layers do not implement the accept cap or the wait result they describe: a full-length waitfor* / getblocktemplate long-poll is turned into an empty HTTP 408, and ConcurrencyLimitLayer does not cap accepts. The REST default also leaves the cross-surface REST pins returning 404.
Issue counts by severity
- bugs: 3
- suggestions: 1
- nits: 0
| .route("/rest/{*path}", get(rest_entry).post(rest_entry)) | ||
| .layer(DefaultBodyLimit::max(RPC_MAX_HTTP_BODY)) | ||
| .layer(from_fn_with_state(state.clone(), reject_unauthorized)) | ||
| .layer(TimeoutLayer::with_status_code( |
There was a problem hiding this comment.
[bug] TimeoutLayer starts its 120s clock at call, which is before satisfy_http_wait arms the same RPC_WAIT_TIMEOUT_MS deadline, and on expiry it returns an empty 408 and drops the handler future. tower_http's timeout poll checks the sleep first, so a waitfor* or getblocktemplate long-poll that runs out the cap never reaches exec_http_rpc. The handler was written to return true at that deadline and then emit the JSON-RPC tip or template; clients now get a body-less 408 instead. The same drop detaches spawn_blocking (JoinHandle drop does not abort) while releasing the work-queue permit, so a method that is already inside the blocking task keeps running after the slot is free.
Suggestion: Do not wrap the router in TimeoutLayer. Keep the two-minute cap inside satisfy_http_wait so the method still returns a JSON-RPC body. If a slow header or body needs a bound, use a read timeout that ends when the request is fully received, not a timeout of the whole service future. If a handler timeout stays, it has to outlive the wait cap, and dropping the handler must not detach the blocking task while freeing the permit.
| StatusCode::REQUEST_TIMEOUT, | ||
| std::time::Duration::from_millis(crate::methods::RPC_WAIT_TIMEOUT_MS), | ||
| )) | ||
| .layer(ConcurrencyLimitLayer::new(RPC_MAX_CONNECTIONS)) |
There was a problem hiding this comment.
[bug] RPC_MAX_CONNECTIONS is documented as an accept cap copied from the Electrum listener, but ConcurrencyLimitLayer is not that. axum::serve accepts the TCP connection and spawns handle_connection before this layer runs. Hyper reads the request head without calling tower poll_ready; only TowerToHyperService's oneshot then waits on the semaphore. Extra connections are not dropped, and that wait sits outside TimeoutLayer (the timeout starts in call, after the permit). Electrum try_acquire_owneds and drops the stream at max_connections. This layer only queues in-flight requests that already sent headers, and it holds one of those 256 slots for the whole long-poll.
Suggestion: Cap accepts the way Electrum does: a shared semaphore around accept, try_acquire_owned, and drop the socket when it is full. Do not use ConcurrencyLimitLayer as that cap. If the in-flight limit stays, put the timeout outside it or the queued request never hits the two-minute bound.
| async fn rest_entry(State(state): State<AppState>, req: axum::extract::Request) -> Response { | ||
| let _permit = match state.work_queue.try_acquire() { | ||
| if !state.rest { | ||
| return StatusCode::NOT_FOUND.into_response(); |
There was a problem hiding this comment.
[bug] REST is 404 unless RpcConfig.rest is set, and RpcOpts::rest defaults to false. esplora_broadcast_visible_in_rpc_and_electrum in crates/rbitcoin-test/tests/cross_surface.rs sets cfg.rpc.listen and then calls pin_rest_chaininfo_and_blockhash, pin_rest_headers, pin_rest_block_and_tx, pin_rest_mempool_and_utxos, and pin_rest_deployment_and_filter, all expecting HTTP 200. Nothing in that journey sets rpc.rest, so those pins 404. Docs and the NixOS module were updated; this in-repo REST journey was not.
Suggestion: Set cfg.rpc.rest = true on that journey (it is the REST pin, so it should opt in). Any other node spawn that GETs /rest/ on the RPC listener needs the same flag.
| err: Option<&str>, | ||
| ) { | ||
| let params = compact_params(params); | ||
| let params = compact_params(&redact_secrets(method, params)); |
There was a problem hiding this comment.
[suggestion] api_call now always runs redact_ext_privkeys, which allocates and copies the entire params string, and then compact_params copies again. exec_one already built that string with serde_json::to_string and calls api_call on the blocking RPC thread while rpc_post still holds the work-queue permit. compact_params used to keep at most 384 bytes; a submitblock body (up to the 2 MiB HTTP cap) is now copied in full before truncation. Redaction itself is in the right place (before the trace line and the JSONL write) and covers unsubscribe via method.contains("silentpayment") plus xprv/tprv/yprv/zprv on every method.
Suggestion: Scan for the tags and a quoted 64-hex secret, and allocate a second string only when one matches. Truncate to PARAMS_MAX before the copy when the line will not contain a secret past the cap, so a large RPC body is not duplicated on the work-queue thread.
Why
REST on the RPC port answered without a flag and shared the RPC work queue, taking a slot before the body was read. A long-poll then held that slot for the whole wait. Silent-payment unsubscribe wrote the scan secret into the API log, and a nonzero start height scanned from that height through the tip.
What
/rest/is off unless--restorrest=is set. It has its own queue, and the body is read before that permit. NixOSservices.rbitcoin.rpc.rest(default false) passes--restonly when RPC is also on.getblocktemplatelong-poll stop at two minutes. The listener caps connections at 256 and uses that same request timeout. The work-queue permit is taken after the wait, not during it.xprv/tprv/yprv/zprvkey material and a silent-payment scan secret.--sh-indexis not required.Stacked on
net/peer-lifecycle.