Async background persistence #3905

joostjager · 2025-07-02T14:55:33Z

Stripped down version of #3778. It allows background persistence to be async, but channel monitor persistence remains sync. This means that for the time being, users wanting async background persistence would be required to implement both the sync and the async KVStore trait. This model is available through process_events_full_async.

process_events_async still takes a synchronous kv store to remain backwards compatible.

Usage in ldk-node: lightningdevkit/ldk-node@main...joostjager:ldk-node:upgrade-to-async-kvstore

ldk-reviews-bot · 2025-07-02T14:55:35Z

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

lightning-background-processor/src/lib.rs

lightning/src/util/sweep.rs

graphite-app · 2025-07-09T09:54:35Z

lightning/src/util/sweep.rs

+	fn persist_state<'a>(
+		&self, sweeper_state: &SweeperState,
+	) -> Pin<Box<dyn Future<Output = Result<(), io::Error>> + 'a + Send>> {
+		let encoded = &sweeper_state.encode();
+
+		self.kv_store.write(
+			OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE,
+			OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE,
+			OUTPUT_SWEEPER_PERSISTENCE_KEY,
+			encoded,
+		)


The encoded variable is captured by reference in the returned future, but it's a local variable that will be dropped when the function returns. This creates a potential use-after-free issue. Consider moving ownership of encoded into the future instead:

fn persist_state<'a>( &self, sweeper_state: &SweeperState, ) -> Pin<Box<dyn Future<Output = Result<(), io::Error>> + 'a + Send>> { let encoded = sweeper_state.encode(); self.kv_store.write( OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE, OUTPUT_SWEEPER_PERSISTENCE_KEY, &encoded, ) }

This ensures the data remains valid for the lifetime of the future.

Suggested change

fn persist_state<'a>(

&self, sweeper_state: &SweeperState,

) -> Pin<Box<dyn Future<Output = Result<(), io::Error>> + 'a + Send>> {

let encoded = &sweeper_state.encode();

self.kv_store.write(

OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE,

OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE,

OUTPUT_SWEEPER_PERSISTENCE_KEY,

encoded,

)

fn persist_state<'a>(

&self, sweeper_state: &SweeperState,

) -> Pin<Box<dyn Future<Output = Result<(), io::Error>> + 'a + Send>> {

let encoded = sweeper_state.encode();

self.kv_store.write(

OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE,

OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE,

OUTPUT_SWEEPER_PERSISTENCE_KEY,

&encoded,

)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Is this real?

I don't think so as the compiler would likely optimize that away, given that encoded will be an owned value (Vec returned by encode()). Still, the change that it suggests looks cleaner.

In general it will be super confusing that we encode at the time of creating the future, but would only actually persist once we dropped the lock. Starting from now we'll need to be super cautious about the side-effects of interleaving persist calls.

The idea is that an async kv store store encodes the data and stores the write action in a queue at the moment the future is created. Things should still happen in the original order.

Can you show a specific scenario where we have to be super cautious even if we have that queue?

The idea is that an async kv store store encodes the data and stores the write action in a queue at the moment the future is created. Things should still happen in the original order.

If that is the idea that we start assuming in this PR, we should probably also start documenting these assumptions in this PR on KVStore already.

Added this requirement to the async KVStore trait doc

tnull

Did a first pass. Changes look mostly good, although I'm not the biggest fan of the process_events_full_async variant as well as the SyncOutputSweeperSyncKVStoreSync in the last commit. I hope we can avoid them?

For the former, it would be useful to wait for #3688 (given it's close), and then the user could use the builder pattern to either configure an async or sync KVStore.

lightning-background-processor/src/lib.rs

tnull · 2025-07-11T10:26:37Z

lightning-background-processor/src/lib.rs

@@ -769,7 +789,7 @@ use futures_util::{dummy_waker, OptionalSelector, Selector, SelectorOutput};
 #[cfg_attr(feature = "std", doc = "	handle.await.unwrap()")]
 ///	# }
 ///```
-pub async fn process_events_async<
+pub async fn process_events_full_async<


I dislike to solve this by just doubling the API surface here. IMO, we should finally move forward with #3688 first, and then utilize the builder pattern to allow the user to plugin a sync or async KVStore variant.

I don't think we should do #3688 in the form that it currently has. My opinion is that it is too much code for what it gets us.

Okay, we may need to conclude on that debate soon then.

Note that a) we had already decided to go this way on #3612 b) have a contributor that already did the work and c) this PR is a pretty good example why we'd want the builder pattern over just adding more constructors or optional fields on them.

But probably best to discuss elsewhere.

This is also temporary, because we want to move over to full async eventually.

I'd say that to solve #3612 we can also use the dummy implementations and don't necessarily need a builder. But indeed best to move that discussion to #3688.

I'd say that to solve #3612 we can also use the dummy implementations and don't necessarily need a builder. But indeed best to move that discussion to #3688.

I still think forcing the user to supply all the dummy implementation doesn't significantly improve the API situation for the user. But yeah, lets move the discussion elsewhere.

lightning-background-processor/src/lib.rs

lightning/src/util/persist.rs

tnull · 2025-07-11T10:33:46Z

lightning/src/util/persist.rs

@@ -121,6 +121,58 @@ pub trait KVStoreSync {
 	) -> Result<Vec<String>, io::Error>;
 }

+/// A wrapper around a [`KVStoreSync`] that implements the [`KVStore`] trait.
+pub struct KVStoreSyncWrapper<K: Deref>(pub K)


As discussed yesterday, we'll also need access to this type in LDK Node as it will be part of the OutputSweeper type signature. Not sure if it then would preferable to have the user implement this wrapper. If we leave it here, it might make sense to split out all these wrappers into a separate util.rs or helpers.rs file, so they don't clutter up the codebase.

tnull · 2025-07-11T10:37:09Z

lightning/src/util/sweep.rs

-			log_error!(self.logger, "Error persisting OutputSweeper: {:?}", e);
-		})?;
-		state_lock.dirty = false;
+			state_lock.dirty = false;


No, I think we should really unset the dirty flag once we're sure we persisted, not before. I.e., rather than unsetting and re-setting it, just set it to false in the success case.

If we do it after persisting, another thread may also persist because it thinks the data is still dirty? What I've implemented is the outcome of a chat with @TheBlueMatt about it. I agree it isn't pretty, so open to other suggestions.

If we do it after persisting, another thread may also persist because it thinks the data is still dirty?

Hmm, but this way we have the same issue around interleaving threads, just the other way around? I.e., we could have another thread come in acquiring the lock, making changes, persisting, at which point the failed persistence task would be blocked on re-acquiring the mutex, then would set dirty = true, even though the state was just re-persisted in the meantime, also resulting in another unnecessary persist?

That is true, but I think the way the PR is currently written optimizes for the happy flow where the persist doesn't fail. So that in the happy flow, there are no unnecessary persists.

tnull · 2025-07-11T10:40:16Z

lightning/src/util/sweep.rs

@@ -922,6 +930,173 @@ where
 	}
 }

+/// A wrapper around [`OutputSweeper`] to be used with a sync kv store.
+pub struct OutputSweeperSyncKVStore<


Do we really need yet another wrapper type for this? Do we then also need an OutputSweeperSyncKVStoreSync to complete the matrix?

I think this is really messy. If this is only added to avoid exposing KVStoreSyncWrapper, I'd much rather go this way or implement the wrapper in LDK Node.

OutputSweeperSyncKVStoreSync is what OutputSweeperSync is, so that is already present.

Don't like this either, but we wanted to guarantee that users can't misuse the wrappers, and this is the result of that. If we have consensus on another approach, I am fine with that too.

I'm also not clear on why this needs a separate wrapper. We can have a second constructor on OutputSweeper that does the KVStore sync wrapping before returning a fully-async OutputSweeper, the only difference between this and that is that track_spendable_outputs would go from async to sync, but in this case its a user who already has support for calling most things async, so not sure why we really care.

I first had the second constructor, but that required exporting the wrapper type too which we wanted to keep private.

Discussed offline. As long as we don't poll the future and expect it to be ready immediately, there is no risk in exposing the wrapper. Added secondary constructor and secondary read method and removed OutputSweeperSyncKVStoreSync

tnull · 2025-07-11T10:41:41Z

lightning/src/util/sweep.rs

+	}
+
+	/// Returns a reference to the underlying [`OutputSweeper`].
+	pub fn sweeper_async(


Can't you implement deref to solve this?

tnull · 2025-07-11T11:24:21Z

lightning/src/util/sweep.rs

+	fn persist_state<'a>(
+		&self, sweeper_state: &SweeperState,
+	) -> Pin<Box<dyn Future<Output = Result<(), io::Error>> + 'a + Send>> {
+		let encoded = &sweeper_state.encode();
+
+		self.kv_store.write(
+			OUTPUT_SWEEPER_PERSISTENCE_PRIMARY_NAMESPACE,
+			OUTPUT_SWEEPER_PERSISTENCE_SECONDARY_NAMESPACE,
+			OUTPUT_SWEEPER_PERSISTENCE_KEY,
+			encoded,
+		)


I don't think so as the compiler would likely optimize that away, given that encoded will be an owned value (Vec returned by encode()). Still, the change that it suggests looks cleaner.

In general it will be super confusing that we encode at the time of creating the future, but would only actually persist once we dropped the lock. Starting from now we'll need to be super cautious about the side-effects of interleaving persist calls.

ldk-reviews-bot · 2025-07-11T11:28:39Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

lightning/src/util/sweep.rs

joostjager · 2025-07-14T09:02:50Z

@tnull changes made diff and replied to open threads.

tnull

As mentioned above, changes basically look good to me, although I'd prefer to avoid process_events_full_async by using the builder introduced in #3688.

But, generally this should be good for a second reviewer, so pinging @TheBlueMatt.

lightning/src/util/persist.rs

TheBlueMatt · 2025-07-15T15:09:51Z

lightning/src/util/persist.rs

+/// Trait that handles persisting a [`ChannelManager`], [`NetworkGraph`], and [`WriteableScore`] to disk.
+///
+/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+pub trait Persister<'a, CM: Deref, L: Deref, S: Deref>


Given Persister is only used in lightning-background-processor and we've been migrating to just using KVStore everywhere (eg its now required to use Sweeper), maybe we just kill off Persister entirely? We had Persister before we had KVStore as a way to persist the objects that the BP wanted to persist. To simplify the interface, we added the KVStore as a pseudo-wrapper around Persister. But since Sweeper now requires a KVStore explicitly, users can no longer only implement Persister, making it basically useless.

The only reason to keep it would be to avoid building the encoded Vec<u8> of the network graph and scorer for users who are avoiding persisting those objects, but I'm not entirely sure avoiding the ~50MiB memory spike during write is worth it.

Added first commit that removes Persister. Rebased the rest. Lots of simplication.

TheBlueMatt · 2025-07-15T16:25:39Z

lightning/src/util/sweep.rs

@@ -922,6 +930,173 @@ where
 	}
 }

+/// A wrapper around [`OutputSweeper`] to be used with a sync kv store.
+pub struct OutputSweeperSyncKVStore<


I'm also not clear on why this needs a separate wrapper. We can have a second constructor on OutputSweeper that does the KVStore sync wrapping before returning a fully-async OutputSweeper, the only difference between this and that is that track_spendable_outputs would go from async to sync, but in this case its a user who already has support for calling most things async, so not sure why we really care.

In preparation for the addition of an async KVStore, we here remove the Persister pseudo-wrapper. The wrapper is thin, would need to be duplicated for async, and KVStore isn't fully abstracted anyway anymore because the sweeper takes it directly.

lightning/src/util/persist.rs

TheBlueMatt

Almost LGTM, just one real comment and a doc nit.

TheBlueMatt · 2025-07-18T23:31:01Z

lightning/src/util/persist.rs

-	) -> Result<Vec<u8>, io::Error>;
-	/// Persists the given data under the given `key`.
+	) -> Pin<Box<dyn Future<Output = Result<Vec<u8>, io::Error>> + 'static + Send>>;
+	/// Persists the given data under the given `key`. Note that the order of multiple writes calls needs to be retained


nit: It may be useful to clarify what this means with an example or a suggested approach to working around it.

TheBlueMatt · 2025-07-18T23:35:55Z

lightning/src/util/sweep.rs

 				}

-				output_info.status.broadcast(cur_hash, cur_height, spending_tx.clone());
+				self.broadcaster.broadcast_transactions(&[&spending_tx]);


Hmm, it used to be the case that we'd first persist, wait for that to finish, then broadcast. I don't think its critical, but it does seem like we should retain that behavior.

joostjager force-pushed the async-persister branch from 1b95d30 to 21dc34c Compare July 2, 2025 15:52

joostjager commented Jul 2, 2025

View reviewed changes

lightning-background-processor/src/lib.rs Show resolved Hide resolved

joostjager force-pushed the async-persister branch 7 times, most recently from 3fb7d6b to 1847e8d Compare July 3, 2025 09:57

joostjager added this to Weekly Goals Jul 3, 2025

joostjager self-assigned this Jul 3, 2025

joostjager force-pushed the async-persister branch 2 times, most recently from 1f59bbe to 723a5a6 Compare July 3, 2025 11:52

joostjager mentioned this pull request May 12, 2025

Async Persistence TODOs #3052

Open

24 tasks

TheBlueMatt linked an issue Jul 7, 2025 that may be closed by this pull request

Async KV Store Persister #1470

Open

TheBlueMatt mentioned this pull request Jul 7, 2025

Async KV Store Persister #1470

Open

joostjager force-pushed the async-persister branch 10 times, most recently from bc9c29a to 90ab1ba Compare July 9, 2025 09:52

joostjager marked this pull request as ready for review July 9, 2025 09:52

joostjager requested a review from tnull July 9, 2025 09:52

graphite-app bot reviewed Jul 9, 2025

View reviewed changes

joostjager changed the title ~~Async Persister trait~~ Async Persister trait and async OutputSweeper persistence Jul 10, 2025

tnull reviewed Jul 11, 2025

View reviewed changes

joostjager force-pushed the async-persister branch from 32e17b3 to 9cf8375 Compare July 14, 2025 08:56

graphite-app bot reviewed Jul 14, 2025

View reviewed changes

lightning/src/util/sweep.rs Outdated Show resolved Hide resolved

joostjager force-pushed the async-persister branch from 9cf8375 to c3a4c24 Compare July 14, 2025 09:01

joostjager requested a review from tnull July 14, 2025 09:03

tnull reviewed Jul 15, 2025

View reviewed changes

tnull requested a review from TheBlueMatt July 15, 2025 09:16

joostjager mentioned this pull request Jul 15, 2025

Async FilesystemStore #3931

Draft

TheBlueMatt reviewed Jul 15, 2025

View reviewed changes

joostjager force-pushed the async-persister branch 5 times, most recently from 5988195 to cc5703e Compare July 18, 2025 06:06

joostjager changed the title ~~Async Persister trait and async OutputSweeper persistence~~ Async background persistence Jul 18, 2025

joostjager force-pushed the async-persister branch from cc5703e to e83affa Compare July 18, 2025 08:07

joostjager added 2 commits July 18, 2025 10:19

Remove Persister

90411b1

In preparation for the addition of an async KVStore, we here remove the Persister pseudo-wrapper. The wrapper is thin, would need to be duplicated for async, and KVStore isn't fully abstracted anyway anymore because the sweeper takes it directly.

Rename KVStore trait to KVStoreSync

d6b7ddd

joostjager force-pushed the async-persister branch 3 times, most recently from 98cdc61 to fda8a58 Compare July 18, 2025 09:20

graphite-app bot reviewed Jul 18, 2025

View reviewed changes

lightning/src/util/persist.rs Outdated Show resolved Hide resolved

Add async KVStore

1809349

joostjager force-pushed the async-persister branch from fda8a58 to 1809349 Compare July 18, 2025 09:41

joostjager requested review from tnull and TheBlueMatt July 18, 2025 09:52

TheBlueMatt reviewed Jul 18, 2025

View reviewed changes

Async background persistence #3905

Are you sure you want to change the base?

Async background persistence #3905

Uh oh!

Conversation

joostjager commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graphite-app bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jul 11, 2025

Uh oh!

Uh oh!

joostjager commented Jul 14, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

joostjager commented Jul 2, 2025 •

edited

Loading

ldk-reviews-bot commented Jul 2, 2025 •

edited

Loading

joostjager Jul 14, 2025 •

edited

Loading