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:

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> { ... }

pub(super) async fn persist_changeset(
	&mut self, change_set: ChangeSet,
) -> Result<(), std::io::Error> {
	self.pending_change_set.merge(change_set);
	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(())
}

[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)?