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.

[jkczyz]

If you want to avoid the clone you could placate the borrow checker like so:

asyncfnpersist_inner(latest_change_set_opt:&mutOption<ChangeSet>,kv_store:&Arc<DynStore>,logger:&Arc<Logger>,change_set:&ChangeSet,) -> Result<(), std::io::Error>{ ...}pub(super)asyncfnpersist_changeset(&mutself,change_set:ChangeSet,) -> Result<(), std::io::Error>{self.pending_change_set.merge(change_set);Self::persist_inner(&mutself.latest_change_set,&self.kv_store,&self.logger,&self.pending_change_set,).await?;let _ = std::mem::take(&mutself.pending_change_set);Ok(())}

[tnull]

Done in the last push

[jkczyz]

Feel free to squash.

[tnull]

Squashed and included the following changes:

diff --git a/src/wallet/persist.rs b/src/wallet/persist.rs
index e1e46338..9d33a09f 100644
--- a/src/wallet/persist.rs+++ b/src/wallet/persist.rs@@ -54,15 +54,17 @@ impl KVStoreWalletPersister {
}
-	async fn persist_inner(&mut self, change_set: &ChangeSet) -> Result<(), std::io::Error> {+	async fn persist_inner(+ latest_change_set_opt: &mut Option<ChangeSet>, kv_store: &Arc<DynStore>,+ logger: &Arc<Logger>, change_set: &ChangeSet,+	) -> Result<(), std::io::Error> {
if change_set.is_empty() {
return Ok(());
}
- let kv_store = Arc::clone(&self.kv_store);- let logger = Arc::clone(&self.logger);+ let kv_store = kv_store.as_ref();
// We're allowed to fail here if we're not initialized, BDK docs state: "This method can fail if the
// persister is not initialized."
- let latest_change_set = self.latest_change_set.as_mut().ok_or_else(|| {+ let latest_change_set = latest_change_set_opt.as_mut().ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::Other,
@@ -176,6 +178,11 @@ impl KVStoreWalletPersister {
) -> Result<(), std::io::Error> {
self.pending_change_set.merge(change_set);
- let pending_change_set = self.pending_change_set.clone();- self.persist_inner(&pending_change_set).await?;+ Self::persist_inner(+ &mut self.latest_change_set,+ &self.kv_store,+ &self.logger,+ &self.pending_change_set,+ )+ .await?;
let _ = std::mem::take(&mut self.pending_change_set);
Ok(())
@@ -201,5 +208,10 @@ impl AsyncWalletPersister for KVStoreWalletPersister {
Self: 'a,
{
- Box::pin(persister.persist_inner(change_set))+ Box::pin(Self::persist_inner(+ &mut persister.latest_change_set,+ &persister.kv_store,+ &persister.logger,+ change_set,+ ))
}
}

[tnull]

Also had to rebase after #660 landed.

[tnull]

Also added a minor optimization of pre-existing code in d5c60b1.

[joostjager]

@tnull, did you have any thoughts on #980 (comment)?

[tnull]

@tnull, did you have any thoughts on #980 (comment)?

Yes, as mentioned above, I don't think it's worth as we're about to drop the block_ons anyways.

[joostjager]

I didn't see that mentioned above, but if we drop all the block_ons, we don't need to bother indeed. await_holding_lock might still be useful in the post-situation?