fix(rpc): bound async operation queue to prevent memory DoS

[ouicate]

Summary

• Confirms and fixes GHSA-3wr9-v982-59fj: WalletRpcImpl::async_ops was an unbounded Vec<AsyncOperation> with no automatic pruning.
• Prunes finished operations (success, failed, cancelled) before enqueueing new async RPCs.
• Rejects new async RPCs when 64 in-flight operations are already queued, matching Bitcoin Core's DEFAULT_HTTP_WORKQUEUE used by zcashd's RPC layer.
• Checks the limit before spawning the background Tokio task, so rejected calls do not leak work.

Vulnerability analysis

An authenticated JSON-RPC caller (HTTP basic auth) could call z_sendmany or z_shieldcoinbase repeatedly without calling z_getoperationresult. Each call appended an AsyncOperation retaining full RPC parameters (ContextInfo.params as JsonValue) until explicitly removed. There was no maximum size, expiration, or background cleanup — enabling linear memory growth until OOM.

[ouicate]

Please review @nullcopy

[nuttycom]

Bounding the queue is the right goal, but as written this trades the memory-DoS for a worse defect: it silently destroys the results of completed operations before the caller can collect them, which for a wallet means losing the txid of a broadcast transaction. Summary of the issues detailed in the inline comments:

1. Result loss (most severe): prune_finished runs on every new async RPC and deletes all finished operations, collected or not. zcashd retains a finished operation until z_getoperationresult explicitly collects it (asyncrpcqueue.cpp removes entries only via popOperationForId()). A batch script that submits N sends and then polls loses every result except the last, and z_getoperationstatus polling becomes non-idempotent.
2. No timeout or eviction for in-flight ops: an operation that never finishes occupies a slot forever; there is no cancellation RPC (OperationState::Cancelled is unreachable), so 64 wedged operations permanently reject all future async RPCs.
3. The 64 constant's justification is factually wrong: zcashd's DEFAULT_HTTP_WORKQUEUE is 16, not 64 (64 is Bitcoin Core ≥29.0, via bitcoin/bitcoin#31215, which zcashd never took), and that constant bounds the transient HTTP request queue, not the async operation table — which in zcashd is unbounded. The limit is also hardcoded rather than joining the existing configurable-limits surface (cf. BuilderLimitsSection).
4. The cap bounds memory but not work: propose_transfer and keystore.decrypt_seed run eagerly in call() before start_async checks the cap, so each rejected flood request still pays full input selection plus a keystore decryption.
5. Lock-hold regression: the global async_ops write lock is now held across up to 64 sequential per-op .awaits during pruning, blocking all status/result/list RPCs for the duration.

A sound version separates the memory bound from the retention contract: keep finished operations until collected (zcashd semantics), and when the table exceeds a larger, configurable cap, evict only the oldest finished entries — optionally with a retention TTL after completion, and/or shrink completed entries by dropping ContextInfo.params (the actual memory hog cited in the advisory) while keeping the result. The in-flight count then needs its own story (execution timeout or a cancellation RPC), or issue 2 remains.

---

🤖 Review performed with Claude Code assistance.

[nuttycom]

Result loss: this prunes all finished operations whenever any new async RPC starts, whether or not the caller has collected their results. Previously (and in zcashd) a finished operation persisted until z_getoperationresult explicitly consumed it.

Concrete failure: a client calls z_sendmany (op A), it completes with the txid in its result; the client then calls z_sendmany again (op B). This prune drops A before B is pushed, so a subsequent z_getoperationresult([A]) / z_getoperationstatus([A]) returns an empty array and the txid of an already-broadcast transaction is permanently unretrievable over RPC. Any "submit N sends, then collect all opids" workflow loses every result but the last, and status polling can no longer distinguish "never existed" from "finished and pruned".

Bounding memory shouldn't require discarding uncollected results — see the review summary for an eviction policy that preserves the retention contract.

[nuttycom]

Permanent liveness wedge: there is no execution timeout, age-based eviction, or cancellation path for in-flight operations (OperationState::Cancelled is unreachable — no RPC sets it). If 64 operations get stuck in Ready/Executing (e.g. futures blocked on a resource that never resolves), prune_finished never reclaims them and every subsequent async RPC is rejected for the lifetime of the process. The memory-DoS fix becomes a permanent liveness-DoS with no recovery path short of a restart.

[nuttycom]

The cited justification is factually wrong on both counts:

1. zcashd's DEFAULT_HTTP_WORKQUEUE is 16, not 64 (zcash/zcash src/httpserver.h). 64 is Bitcoin Core's value only since 29.0 (rpc: increase the defaults for -rpcthreads and -rpcworkqueue bitcoin/bitcoin#31215, 2025), which zcashd never took.
2. DEFAULT_HTTP_WORKQUEUE bounds the transient queue of pending HTTP requests awaiting worker threads — drained in milliseconds — not a table of long-lived operations that can each run for minutes. zcashd's actual async operation table (asyncrpcqueue.cpp) has no size limit at all; entries are removed only when z_getoperationresult collects them.

So 64 is essentially arbitrary here. Given ops can be long-lived, consider whether 64 concurrent in-flight sends is enough for batch workloads, and consider making it configurable — BuilderLimitsSection in config.rs is the existing precedent for exactly this category of anti-memory-exhaustion knob.

[nuttycom]

The cap bounds memory but not work: z_send_many::call(...) runs propose_transfer (full input selection over the wallet DB) and keystore.decrypt_seed eagerly, before start_async ever checks the cap. So when the queue is full, each rejected flood request has already paid the full proposal cost plus a keystore decryption — the PR description's "rejected calls do not leak work" only covers the tokio spawn. A cheap queue-fullness pre-check before invoking call() (accepting the small TOCTOU window) would make rejection actually cheap.

[nuttycom]

Lock held across awaits: the exclusive async_ops write lock is now held across prune_finished, which awaits op.state() sequentially for up to 64 ops (each acquiring that op's data RwLock, contending with the executing task's own state-transition writes). While parked at any of those yields, z_getoperationstatus, z_listoperationids, z_getoperationresult, and all other submissions block on this lock — a single submission can stall all status polling, and rejected flood requests still force the full scan under the write lock.

Cheaper shape: under a read lock, snapshot each op's Arc data handle and compute the finished-id set outside the global lock; then take the write lock briefly for a synchronous retain + capacity check + push. That also avoids the unconditional Vec reallocation in the common nothing-to-prune case.

[nuttycom]

Queue invariants owned by nobody: this duplicates the eviction semantics already in result() below (gather finished via is_finished, then remove) with a divergent idiom, and it lives in the z_getoperationstatus/z_getoperationresult method module even though it's queue maintenance called only from start_async. The capacity/retention rules are now enforced ad hoc across three sites on a raw RwLock<Vec<AsyncOperation>>, so a future method that touches async_ops can silently regress the bound. Consider a dedicated queue type (in asyncop.rs) that owns capacity, pruning policy, and result retention as invariants.

[nuttycom]

This test is tautological — it restates the constant's definition, so it can't catch a real regression; it just has to be edited in lockstep whenever the cap is retuned (and the name asserts a relationship to Bitcoin's workqueue that the code doesn't actually have — see the comment on the constant). Suggest deleting it; the prune_finished test above is the one carrying weight.

[nuttycom]

Minor: LegacyCode::Misc (-1) makes queue-full indistinguishable from any generic failure, so clients can't implement backoff-and-retry for what is a transient, retriable condition. A more specific code from the existing LegacyCode enum would let callers tell them apart.