Async FilesystemStore #3931

joostjager · 2025-07-15T13:12:41Z

Async filesystem store with eventually consistent writes. It is just using tokio's spawn_blocking, because that is what tokio::fs would otherwise do as well. Using tokio::fs would make it complicated to reuse the sync code.

ldk-node try out: lightningdevkit/ldk-node@main...joostjager:ldk-node:async-fsstore

ldk-reviews-bot · 2025-07-15T13:12:44Z

👋 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.

tnull · 2025-07-15T13:49:38Z

lightning-persister/src/fs_store_async.rs

+		let this = Arc::clone(&self.inner);
+
+		Box::pin(async move {
+			tokio::task::spawn_blocking(move || {


Mhh, so I'm not sure if spawning blocking tasks for every IO call is the way to go (see for example https://docs.rs/tokio/latest/tokio/fs/index.html#tuning-your-file-io: "To get good performance with file IO on Tokio, it is recommended to batch your operations into as few spawn_blocking calls as possible."). Maybe there are other designs that we should at least consider before moving forward with this approach. For example, we could create a dedicated pool of longer-lived worker task(s) that process a queue?

If we use spawn_blocking, can we give the user control over which runtime this exactly will be spawned on? Also, rather than just doing wrapping, should we be using tokio::fs?

Mhh, so I'm not sure if spawning blocking tasks for every IO call is the way to go (see for example https://docs.rs/tokio/latest/tokio/fs/index.html#tuning-your-file-io: "To get good performance with file IO on Tokio, it is recommended to batch your operations into as few spawn_blocking calls as possible.").

If we should batch operations, I think the current approach is better than using tokio::fs? Because it already batches the various operations inside kvstoresync::write.

Further batching probably needs to happen at a higher level in LDK, and might be a bigger change. Not sure if that is worth it just for FIlesystemStore, especially when that store is not the preferred store for real world usage?

For example, we could create a dedicated pool of longer-lived worker task(s) that process a queue?

Isn't Tokio doing that already when a task is spawned?

If we use spawn_blocking, can we give the user control over which runtime this exactly will be spawned on? Also, rather than just doing wrapping, should we be using tokio::fs?

With tokio::fs, the current runtime is used. I'd think that that is then also sufficient if we spawn ourselves, without a need to specifiy which runtime exactly?

More generally, I think the main purpose of this PR is to show how an async kvstore could be implemented, and to have something for testing potentially. Additionally if there are users that really want to use this type of store in production, they could. But I don't think it is something to spend too much time on. A remote database is probably the more important target to design for.

With tokio::fs, the current runtime is used. I'd think that that is then also sufficient if we spawn ourselves, without a need to specifiy which runtime exactly?

Hmm, I'm not entirely sure, especially for users that have multiple runtime contexts floating around, it might be important to make sure the store uses a particular one (cc @domZippilli ?). I'll also have to think through this for LDK Node when we make the switch to async KVStore there, but happy to leave as-is for now.

tnull · 2025-07-15T13:50:24Z

lightning/src/util/persist.rs

 }

 /// Provides additional interface methods that are required for [`KVStore`]-to-[`KVStore`]
 /// data migration.
-pub trait MigratableKVStore: KVStore {
+pub trait MigratableKVStore: KVStoreSync {


How will we solve this for an KVStore?

I think this comment belongs in #3905?

We might not need to solve it now, as long as we still require a sync implementation alongside an async one? If we support async-only kvstores, then we can create an async version of this trait?

lightning-persister/src/fs_store.rs

joostjager · 2025-07-15T15:38:14Z

Removed garbage collector, because we need to keep the last written version.

codecov · 2025-07-23T19:56:31Z

Codecov Report

❌ Patch coverage is 87.83069% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.94%. Comparing base (61e5819) to head (f4e8d62).

Files with missing lines	Patch %	Lines
lightning-persister/src/fs_store.rs	87.83%	14 Missing and 9 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3931    +/-   ##
========================================
  Coverage   88.94%   88.94%            
========================================
  Files         174      174            
  Lines      124201   124353   +152     
  Branches   124201   124353   +152     
========================================
+ Hits       110472   110610   +138     
- Misses      11251    11262    +11     
- Partials     2478     2481     +3

Flag	Coverage Δ
fuzzing	`22.63% <ø> (ø)`
tests	`88.77% <87.83%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lightning-persister/src/fs_store_async.rs

joostjager · 2025-07-25T13:48:12Z

Updated code to not use an async wrapper, but conditionally expose the async KVStore trait on FilesystemStore.

I didn't yet update the ldk-node branch using this PR, because it seems many other things broke in main again.

ldk-reviews-bot · 2025-07-28T00:00:57Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Approach ACK I think, but I find it really concerning that we'd double our memory footprint while writing here. I don't think that is really acceptable if the async KVStore is going to be our default. I think we'll need to find a solution before moving on.

This also needs a rebase now that #3799 landed.

tnull · 2025-07-29T13:19:02Z

lightning-persister/src/fs_store.rs

+		let this = Arc::clone(&self.inner);
+
+		// Obtain a version number to retain the call sequence.
+		let version = self.version_counter.fetch_add(1, Ordering::SeqCst);


Very, very unlikely, but should we still panic if we'd ever hit u64::MAX to avoid we'd ever silently start skipping all writes? Also, I'm not sure we really need SeqCst here.

Relaxed and added overflow panic.

tnull · 2025-07-29T13:40:45Z

lightning-persister/src/fs_store.rs

+		let primary_namespace = primary_namespace.to_string();
+		let secondary_namespace = secondary_namespace.to_string();
+		let key = key.to_string();
+		let buf = buf.to_vec();


Ugh, all these reallocations are not great, in particular not for buf, which we then give as a reference to write_version. So we'll then hold the full encoded data in memory at least twice. If we don't find a way to deal with the references, I wonder if we should switch the async version of KVStore to take owned primary_namespace, secondary_namespace, key, buf args.

I think the problem is that those args should then have static lifetime, because it isn't clear when they will be used by the runtime? Open to suggestions how to do that.

Agreed that copy is not ideal. Not sure if it actually matters if we also need to hit the disk with the same data.

Why don't we just pass a Vec in to the (async) KVStore API? That's basically what we have at all the callsites, and now that we have a real use for it on the other end we should change the API, IMO.

Why don't we just pass a Vec in to the (async) KVStore API? That's basically what we have at all the callsites, and now that we have a real use for it on the other end we should change the API, IMO.

Right, that's why I meant with 'owned args' above.

Got it now.

ldk-reviews-bot · 2025-07-29T13:42:47Z

👋 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.

joostjager · 2025-07-29T14:57:41Z

Rebased. Only diff is the counter overflow change

tnull

This is blocked on #3974 now.

graphite-app · 2025-07-31T14:33:12Z

lightning-persister/src/fs_store.rs

+		let version = self.version_counter.fetch_add(1, Ordering::Relaxed);
+		if version == u64::MAX {
+			panic!("FilesystemStore version counter overflowed");


The overflow check occurs after fetch_add has already executed, which means if version_counter was at u64::MAX, it would wrap to 0 before the check detects the overflow. Consider either:

Using checked_add before incrementing:

let current = self.version_counter.load(Ordering::Relaxed); if current == u64::MAX { panic!("FilesystemStore version counter would overflow"); } let version = self.version_counter.fetch_add(1, Ordering::Relaxed);

Or using a wrapping increment with a separate check that compares the new value with the old:

let old_version = self.version_counter.fetch_add(1, Ordering::Relaxed); if old_version > self.version_counter.load(Ordering::Relaxed) { panic!("FilesystemStore version counter overflowed"); } let version = old_version;

Suggested change

let version = self.version_counter.fetch_add(1, Ordering::Relaxed);

if version == u64::MAX {

panic!("FilesystemStore version counter overflowed");

let current = self.version_counter.load(Ordering::Relaxed);

if current == u64::MAX {

panic!("FilesystemStore version counter would overflow");

}

let version = self.version_counter.fetch_add(1, Ordering::Relaxed);

Spotted by Diamond

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

I think this is okay. Some thread is going to hit max and panic.

joostjager · 2025-07-31T14:33:27Z

Rebased on the latest #3974. No more unnecessary copies.

joostjager · 2025-07-31T15:16:01Z

Updated ldk-node try out so that it again compiles. Need to think about what this is going to look like in the final version. A parent trait that combines Sync and Async might be useful?

tnull

LGTM, mod one comment that should be addressed before we merge this.

tnull · 2025-08-01T08:26:39Z

lightning-persister/Cargo.toml

 [dependencies]
 bitcoin = "0.32.2"
 lightning = { version = "0.2.0", path = "../lightning" }
+tokio = { version = "1.35", optional = true, features = [ "macros", "rt-multi-thread" ] }


Please add this with default-features = false. Also, we need macros only in tests, so could be added on a dev dependency only:

diff --git a/lightning-persister/Cargo.toml b/lightning-persister/Cargo.toml index 5a515fbab..593e19a95 100644 --- a/lightning-persister/Cargo.toml +++ b/lightning-persister/Cargo.toml @@ -19,7 +19,7 @@ tokio = ["dep:tokio"] [dependencies] bitcoin = "0.32.2" lightning = { version = "0.2.0", path = "../lightning" } -tokio = { version = "1.35", optional = true, features = [ "macros", "rt-multi-thread" ] } +tokio = { version = "1.35", optional = true, default-features = false, features = ["rt-multi-thread"] } [target.'cfg(windows)'.dependencies] windows-sys = { version = "0.48.0", default-features = false, features = ["Win32_Storage_FileSystem", "Win32_Foundation"] } @@ -30,6 +30,7 @@ criterion = { version = "0.4", optional = true, default-features = false } [dev-dependencies] lightning = { version = "0.2.0", path = "../lightning", features = ["_test_utils"] } bitcoin = { version = "0.32.2", default-features = false } +tokio = { version = "1.35", default-features = false, features = ["macros"] } [lints] workspace = true

Thanks for the diff

tnull · 2025-08-01T08:38:22Z

lightning-persister/src/fs_store_async.rs

+		let this = Arc::clone(&self.inner);
+
+		Box::pin(async move {
+			tokio::task::spawn_blocking(move || {


With tokio::fs, the current runtime is used. I'd think that that is then also sufficient if we spawn ourselves, without a need to specifiy which runtime exactly?

Hmm, I'm not entirely sure, especially for users that have multiple runtime contexts floating around, it might be important to make sure the store uses a particular one (cc @domZippilli ?). I'll also have to think through this for LDK Node when we make the switch to async KVStore there, but happy to leave as-is for now.

tnull

LGTM

TheBlueMatt · 2025-08-01T16:31:56Z

lightning-persister/src/fs_store.rs

@@ -30,47 +39,100 @@ fn path_to_windows_str<T: AsRef<OsStr>>(path: &T) -> Vec<u16> {
 	path.as_ref().encode_wide().chain(Some(0)).collect()
 }

-// The number of read/write/remove/list operations after which we clean up our `locks` HashMap.


Hmm, I don't really see why its okay to drop the lock GC'ing. We'll end up slowly increasing memory over time for ChannelMonitorUpdate-writing clients without ever cleaning it up.

I removed the garbage collection because we need to keep track of the version number for each file. Is there a way to avoid that?

We can also track the number of in-flight writes and drop the tracking when there are no writes, no?

I think we can do that. But is it really a problem that we use a few bytes for each file that we have on disk?

TheBlueMatt · 2025-08-01T16:35:14Z

lightning-persister/src/fs_store.rs

+		})
+	}
+
+	fn remove(


Seems like we should really care about ordering for remove, no? I mean I guess we could say that the remove operation happens in the future unlike the write, which we consider to happen before the future in the method, but it seems more consistent to just have remove have ordering, how hard would it be to add?

Yes you are right. Remove is also a sort of write. I've added a fix up commit that uses the same version counter for writes and removes.

TheBlueMatt · 2025-08-01T16:37:06Z

lightning-persister/Cargo.toml

 [dependencies]
 bitcoin = "0.32.2"
 lightning = { version = "0.2.0", path = "../lightning" }
+tokio = { version = "1.35", optional = true, default-features = false, features = ["rt-multi-thread"] }


While its true that we're relying on a multi-threaded runtime here, just setting the feature doesn't ensure we get one (you can still create a single-threaded runtime). I'm not entirely sure what the right answer is, but we need to at least document this requirement.

Is it a problem if it would be used with a single-threaded runtime, because isn't everything then just executed synchronously?

I tried the test with #[tokio::test(flavor = "current_thread")] and that seemed to work.

Ah, I figured it might be an issue but if you tested its definitely not. We can drop the feature here, then :)

We can't drop the feature, because spawn_blocking is only available with rt-multi-thread I believe.

joostjager changed the title ~~Async fsstore~~ Async FilesystemStore Jul 15, 2025

joostjager force-pushed the async-fsstore branch 4 times, most recently from 29b8bcf to 81ad668 Compare July 15, 2025 13:40

tnull reviewed Jul 15, 2025

View reviewed changes

joostjager force-pushed the async-fsstore branch from 81ad668 to e462bce Compare July 15, 2025 15:22

joostjager added this to Weekly Goals Jul 17, 2025

joostjager self-assigned this Jul 17, 2025

joostjager mentioned this pull request Jul 17, 2025

Async Persistence TODOs #3052

Open

24 tasks

joostjager force-pushed the async-fsstore branch 2 times, most recently from 97d6b3f to 02dce94 Compare July 23, 2025 18:11

joostjager force-pushed the async-fsstore branch 2 times, most recently from c061fcd to 2492508 Compare July 24, 2025 08:31

joostjager marked this pull request as ready for review July 24, 2025 08:32

ldk-reviews-bot requested a review from tankyleo July 24, 2025 08:32

joostjager force-pushed the async-fsstore branch 2 times, most recently from 9938dfe to 7d98528 Compare July 24, 2025 09:39

joostjager commented Jul 24, 2025

View reviewed changes

lightning-persister/src/fs_store_async.rs Outdated Show resolved Hide resolved

graphite-app bot reviewed Jul 24, 2025

View reviewed changes

lightning-persister/src/fs_store_async.rs Outdated Show resolved Hide resolved

joostjager force-pushed the async-fsstore branch 5 times, most recently from 38ab949 to dd9e1b5 Compare July 25, 2025 13:39

joostjager requested a review from tnull July 25, 2025 13:51

joostjager removed the request for review from tankyleo July 25, 2025 13:51

tnull reviewed Jul 29, 2025

View reviewed changes

joostjager force-pushed the async-fsstore branch from dd9e1b5 to 05a4bb4 Compare July 29, 2025 14:56

joostjager requested a review from tnull July 30, 2025 07:10

joostjager mentioned this pull request Jul 30, 2025

Update KVStore and KVStoreSync arguments to being owned #3974

Merged

tnull reviewed Jul 31, 2025

View reviewed changes

joostjager force-pushed the async-fsstore branch 2 times, most recently from c824b41 to 460c716 Compare July 31, 2025 14:23

graphite-app bot reviewed Jul 31, 2025

View reviewed changes

joostjager force-pushed the async-fsstore branch from 460c716 to c8b45ea Compare August 1, 2025 08:29

joostjager requested a review from tnull August 1, 2025 08:30

tnull reviewed Aug 1, 2025

View reviewed changes

Add async implementation of FilesystemStore

9866309

joostjager force-pushed the async-fsstore branch from c8b45ea to 9866309 Compare August 1, 2025 10:18

joostjager requested a review from tnull August 1, 2025 10:32

tnull previously approved these changes Aug 1, 2025

View reviewed changes

joostjager requested a review from TheBlueMatt August 1, 2025 11:31

TheBlueMatt reviewed Aug 1, 2025

View reviewed changes

f: versioned remove

f4e8d62

joostjager dismissed tnull’s stale review via f4e8d62 August 1, 2025 18:21

joostjager requested a review from TheBlueMatt August 1, 2025 18:23

Async FilesystemStore #3931

Are you sure you want to change the base?

Async FilesystemStore #3931

Conversation

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

Uh oh!

Uh oh!

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

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!

Uh oh!

joostjager commented Jul 15, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 28, 2025

Uh oh!

tnull left a comment

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 29, 2025

Uh oh!

joostjager commented Jul 29, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jul 31, 2025

Uh oh!

joostjager commented Jul 31, 2025

Uh oh!

tnull left a comment

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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

joostjager commented Jul 15, 2025 •

edited

Loading

ldk-reviews-bot commented Jul 15, 2025 •

edited

Loading

codecov bot commented Jul 23, 2025 •

edited

Loading

joostjager commented Jul 25, 2025 •

edited

Loading