Derive shutdown scripts without blocking on wallet persistence

[jkczyz]

Fixes #1010

LDK invokes the sync SignerProvider::get_shutdown_scriptpubkey and get_destination_script callbacks on runtime worker threads while holding channel locks, e.g. from the event handler when accepting an inbound channel. These callbacks called Runtime::block_on to await wallet persistence, which can deadlock the runtime:

1. The event-handler task parks in block_on while holding the per-peer channel Mutex (its worker core is handed off via block_in_place).
2. The replacement worker picks up a lightning-net-tokio task whose PeerManager::process_events blocks synchronously on that same Mutex, capturing the core.
3. The task holding the wallet persister lock can then never be polled, so the awaited future never completes and no party can make progress.

Observed as permanent hangs of integration_tests_rust runs under load (the tests run the node on a single-worker runtime, where one captured core is fatal). See #1010 for full thread samples and analysis.

Instead of waiting on persistence, the wallet now keeps a small pool of pre-revealed addresses whose reveal is already persisted. The sync callbacks pop from the pool — no persistence work at all — and schedule a background refill. The refill reveals replacement addresses, persists the wallet change set, and only then publishes them for handout, so a handed-out script is always covered by persisted wallet state and a crash can never leave it unwatched by incremental chain syncs. The pool's derivation indices are persisted alongside the wallet, so a restart reloads the pooled addresses instead of revealing (and burning) fresh indices on every run.

Semantics worth calling out:

• If wallet persistence fails for long enough, the pool drains and the callbacks fail closed: the channel open is rejected rather than handing out an address whose reveal isn't durable. The pool holds 16 addresses and is refilled after every handout; each channel open consumes two (destination + shutdown script), so persistence must stay unavailable across 8 consecutive opens before that happens.
• The pooled addresses are revealed-but-unused, which widens what incremental syncs must watch by the pool size. Keeping the pool small and reloading it across restarts bounds that cost.

The gated-store test reproduces the deadlock: without the fix the acceptor wedges in get_shutdown_scriptpubkey and the open times out; with it the open completes from the pool and the refill's persistence lands once the store recovers. A restart test asserts that rebuilding a node from the same store performs no wallet writes (the pool is reloaded, not re-revealed) and that a channel open is served from the reloaded pool. Unit tests cover the publish-only-after-persist ordering, refill retry after a persistence failure without burning an extra index, validation of the persisted record on reload, crash replay from every store-write boundary, degradation to an empty pool on an undecodable record, and the fail-closed callbacks.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tnull]

Looks good I think, feel free to undraft.

[tnull]

Hmm, so looking at this again one issue is that BDK's lookahead is not used for incremental syncs - there they really just check (all) previously revealed spks. So, if we fail persistence in the background task, we will miss a derived SPK, leading to address reuse (maybe acceptable in this narrow case) and only discovering transactions once it has been reused (maybe not acceptable as it could be considered funds loss, even if temporarily/recoverable - the user might not realize it's recoverable).

An alternative approach would be to create an AddressCache that is refreshed and persisted in the background, and returns Err(()) if no addresses are available. Something like:

• At startup, derive perhaps 64 external addresses and persist the entire derivation range.
• Only after persistence succeeds, place those addresses in an in-memory queue.
• The synchronous signer callbacks pop an address without blocking.
• Refill in the background below a low-water mark, publishing new addresses only after persistence succeeds.
• If storage remains unavailable and the pool empties, return Err(()) and fail closed.

On top, as an additional robustness measure we might want to consider adding a wallet_persistence_pending dirty marker than would trigger a full scan on next restart whenever we're not certain all persistence operations have cleanly succeeded before stopping?

Thoughs?

[jkczyz]

Yeah, the cache is a better approach. Claude implemented it with some deviations:

🤖 Good catch — you're right, incremental syncs only query the revealed SPKs of the persisted wallet, so a crash before the deferred flush lands would leave the handed-out script unwatched. That's not acceptable, even if recoverable. I reworked the PR along the lines you sketched.

The callbacks now pop from a pool of addresses whose reveal is already persisted, and fail closed (Err(())) if the pool is empty. Newly revealed addresses are only published for handout after their change set persists; on failure they're retained and retried by the next refill, so no index is burned.

Two deviations from your sketch, both aimed at keeping the revealed-but-unused window small, since every pooled address widens what incremental syncs must watch:

• Pool size 16 rather than 64, refilled after every handout rather than at a low-water mark. Since the refill runs after each pop, the pool size only bounds how many channel opens persistence can miss in a row before opens fail closed — 16 covers 8 consecutive opens (destination + shutdown script each), which seems plenty for an outage budget.
• The pool's derivation indices are persisted (under bdk_wallet/address_pool) and reloaded on startup, after validating them against the wallet's last revealed index. Without that, every restart would burn a pool's worth of fresh indices, permanently growing the watched set. The record is written before the reveals' change set, so a crash between the two writes just re-derives the same indices on restart instead of stranding them (a unit test replays a reload from every store-write boundary to check this).

On the wallet_persistence_pending dirty marker: with the pool, unclean shutdown no longer risks an unwatched handed-out script, so I left it out of this PR — but it could still make sense as a general robustness measure for other in-flight wallet writes. Happy to explore it as a follow-up if you think it's worth it.

[tnull]

Codex:

The safer design is one FIFO external-address allocator with two consumption modes:

• Synchronous signer callbacks pop immediately and accept the documented crash-reuse window.
• Async public address requests pop from the same pool but persist the dequeue/refill before returning, preserving their current no-reuse guarantee.

[jkczyz]

🤖 Implemented exactly this split: the signer callbacks only pop and accept the documented bounded reuse across a crash, while get_new_address pops from the same pool but persists the dequeue before returning. Both consume from a single FIFO pool in reveal order.

[tnull]

I think if we going to do the address pool, we should redirect all calls to get_new_address through it, to avoid unnecessarily advancing the descriptor while still having unused ones lying around.

[tnull]

Codex:

1. [P1] Address pool can hide funds during seed recovery — /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/wallet/mod.rs:96

   The pool reveals 16 unused external addresses. A configured stop gap of 16 or less will therefore miss the first ordinary receive address during seed-only recovery. Even with the default gap of 20, two channel opens consume four still-unused scripts and refill through index 19; the next
   receive address is index 20, beyond the full scan. The recovery strategy needs to account for pooled and handed-out-but-unused scripts, with a seed-restoration regression test.

[jkczyz]

🤖 Good catch. Full scans now extend the configured stop gap by the pool size (ADDRESS_POOL_SIZE), and get_new_address serves the oldest revealed address first. The seed-restore test isn't new — it failed against the unfixed pool and now pins the extended gap on both the Esplora and Electrum paths.

There's still a residual on the restore path: a wallet restored from seed can hand out addresses a previous installation already served. That's bounded address reuse, not a loss-of-funds issue, and it's documented. Happy to make the pool skip used indices after the first scan as a follow-up if you'd prefer.

[tnull]

Codex:

2. [P2] Refill tasks accumulate for the node’s lifetime — /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/wallet/mod.rs:612

   Every callback adds a task to the runtime’s JoinSet, but that set is only drained during shutdown at /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/runtime.rs:178. Completed refills therefore remain tracked—two per channel—and empty-pool failures still schedule more tasks.
   Refill work should be coalesced into one in-flight task or completed tasks should be reaped.

This could probably be a spawn_cancellable_background_task? (for which we'll land a fix for the accumulation in #997)

[jkczyz]

🤖 Done — refills now spawn on the cancellable set. This needed one accompanying fix: an abort could drop a refill's taken reveals, so the refill now stages them with the persister in the same synchronous critical section that takes them from the wallet, with a regression test covering it. Note that completed tasks still sit in the cancellable set until shutdown (#997 will reap them continuously) — so the practical gain today is that shutdown aborts an in-flight refill immediately instead of waiting on it.

[jkczyz]

I think if we going to do the address pool, we should redirect all calls to get_new_address through it, to avoid unnecessarily advancing the descriptor while still having unused ones lying around.

🤖 Done — get_new_address now serves from the front of the pool and persists the dequeue before returning. One caveat: every handout still nets one fresh reveal; what this buys is allocation in reveal order, which keeps the unused window roughly pool-sized.

[jkczyz]

🤖 Good catch. Full scans now extend the configured stop gap by the pool size (ADDRESS_POOL_SIZE), and get_new_address serves the oldest revealed address first. The seed-restore test isn't new — it failed against the unfixed pool and now pins the extended gap on both the Esplora and Electrum paths.

There's still a residual on the restore path: a wallet restored from seed can hand out addresses a previous installation already served. That's bounded address reuse, not a loss-of-funds issue, and it's documented. Happy to make the pool skip used indices after the first scan as a follow-up if you'd prefer.

[jkczyz]

🤖 Done — refills now spawn on the cancellable set. This needed one accompanying fix: an abort could drop a refill's taken reveals, so the refill now stages them with the persister in the same synchronous critical section that takes them from the wallet, with a regression test covering it. Note that completed tasks still sit in the cancellable set until shutdown (#997 will reap them continuously) — so the practical gain today is that shutdown aborts an in-flight refill immediately instead of waiting on it.

[jkczyz]

🤖 Implemented exactly this split: the signer callbacks only pop and accept the documented bounded reuse across a crash, while get_new_address pops from the same pool but persists the dequeue before returning. Both consume from a single FIFO pool in reveal order.