Avoid wallet persistence deadlocks

[tnull]

Fixes #978.

Release the synchronous wallet lock before awaiting store I/O while serializing wallet mutations with an asynchronous persistence gate. Retain failed change sets so retries preserve BDK persistence semantics.

[ldk-reviews-bot]

👋 Thanks for assigning @jkczyz 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.

[joostjager]

Could it be useful to enable await_holding_lock in clippy to catch these issues?

[tnull]

Could it be useful to enable await_holding_lock in clippy to catch these issues?

No, wouldn't have caught it, the issue (as usual) are block_ons

main ~/workspace/ldk-node> cargo clippy -- -A warnings -W clippy::await_holding_lock

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.21s
main ~/workspace/ldk-node>

[joostjager]

Or maybe a Send bound on block_on to avoid the mutex from being captured? Or some other way to install a trip wire for this class of bugs?

Perhaps the clippy switch is useful independently as a future guard for other async issues?

[jkczyz]

Some Fable findings below, though none critical. See inline comment for one bug that was identified.

🤖 A few smaller observations (none blocking):

• select_confirmed_utxos skips retry flushes: when no change output exists, persist_changeset isn't called at all (src/wallet/mod.rs:1004-1013), so a pending_change_set retained from an earlier failure isn't retried at that opportunity. Correct as written — just a missed retry point.
• Persister lock scope: apply_update/apply_mempool_txs/block_connected hold the persister lock across update_payment_store's payment-store I/O, so user-facing calls like new_address() queue behind a full sync round's store writes. Seems acceptable (it preserves event ordering against wallet state), but worth being deliberate about.
• Relaxed atomicity is intentional but observable: readers (e.g. list_balances) can now see wallet state whose payment-store counterpart hasn't been written yet — previously the wallet lock made these atomic (that atomicity was the deadlock). I didn't find any code relying on the old atomicity.
• Commit structure: one commit mixes the mechanical sync→async conversion, the semantic persistence-model change (take_staged + retention), and tests. Two or three commits (persister model → wallet/caller conversion → tests) would be easier to review safely, since the risky part is a few dozen lines inside ~900 changed ones.
• API/compatibility: public API signatures unchanged; on-disk format untouched (persist_inner unchanged); constructor plumbing is pub(crate). No FFI or migration concerns that I could find.

[jkczyz]

Fable caught a bug here:

Stopping the node while a wallet persist is in flight — routinely in bitcoind mode, where stop() drops the in-progress poll, or on any backend when a slow store (e.g. VSS on flaky mobile connectivity) makes shutdown hit its 5s/30s abort timeouts — silently destroys the changeset that was already taken from the wallet, so after restart the wallet can, for example, hand out an already-used address.

Here's a failing test:

jkczyz@161c6c2

[tnull]

My bad, I think I introduced this bug while trying to avoid some clones. I guess we can't get away with not cloning the pending changeset here. Will shortly push a fixup.