Require both `KVStore` and `KVStoreSync` implementations, switch BP to be fully-async #633

tnull · 2025-09-12T11:00:30Z

As an intermediary step towards making our IO fully async, we now require any store to implement both KVStore and KVStoreSync, which allows us to switch over to the fully-async background processor and take further migration steps bit-by-bit when we make more and more of the core codebase async.

To this end, we refactor VssStore and SqliteStore to implement KVStore

TODOs:

Implement KVStore for TestStore upstream to fix tests
Implement write-order tracking for the VSS KVStore implementation

.. draft until then.

ldk-reviews-bot · 2025-09-12T11:00:32Z

👋 Thanks for assigning @joostjager 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-09-18T09:33:34Z

This now builds based on the just-merged lightningdevkit/rust-lightning#4069. We have yet to add write-version tracking for VSS.

tnull · 2025-09-29T13:39:24Z

Should be good for review. For the VssStore versioning I copied as much as possible from the already-reviewed approach over at lightningdevkit/rust-lightning#3931

tnull · 2025-09-29T14:58:16Z

Rebased to address conflicts post-#652.

joostjager · 2025-09-29T14:08:17Z

src/io/vss_store.rs

+		if primary_namespace.is_empty() {
+			key.to_owned()
+		} else {
+			format!("{}#{}#{}", primary_namespace, secondary_namespace, key)


Is it necessary to make a string out of this? It doesn't need to be mapped to a filename like for fs_store, so maybe it can also simply be a tuple?

I think so, as the HashMap needs to hold an owned value. We could have it be a (String, String, String), but that's worse.

Is that worse? The string concatenation looks a bit unnecessary. Or make it a struct that is used as the key?

Is that worse? The string concatenation looks a bit unnecessary. Or make it a struct that is used as the key?

It at least requires three individual allocations instead of one? I.e. more clutter on the heap, and probably also some slowdown?

FWIW, I mirrored what we do for the obfuscated key. Unfortunately it's not super easy to just reuse that.

Not ideal indeed, the heap allocs

src/io/vss_store.rs

joostjager · 2025-09-30T07:25:10Z

src/io/vss_store.rs

-
-		Ok(self.storable_builder.deconstruct(storable)?.0)
+
+		self.execute_locked_read(locking_key, async move || {


I think we don't need the lock for reading?

tnull · 2025-09-30T07:47:27Z

Addressed pending comments, probably still need to see why the CI job start hanging again.

joostjager · 2025-09-30T08:46:38Z

LGTM, can squash

tnull · 2025-09-30T08:47:51Z

LGTM, can squash

Squashed without further changes.

joostjager · 2025-09-30T09:07:23Z

src/io/sqlite_store/mod.rs

+		let secondary_namespace = secondary_namespace.to_string();
+		let key = key.to_string();
+		let inner = Arc::clone(&self.inner);
+		let fut = tokio::task::spawn_blocking(move || {


During final look, I am now again wondering if this function is actually preserving order?

Hmm, good point, it seems it would depend on how tokio exactly schedules the blocking tasks. I considered a few other options, but now simply also followed the tried and true 'write version locking' approach here, as we already use that in VSS and FS stores.

Hmm but isn't this completely unnecessary because sqlite has its own global lock? With FS and VSS there is the actual possibility of parallel execution.

Hmm but isn't this completely unnecessary because sqlite has its own global lock? With FS and VSS there is the actual possibility of parallel execution.

Well, I first thought so, too, but I think the issue is that tokio gives no guarantee in which order spawned tasks are executed/scheduled. I.e., AFAICT it could happen that we spawn to writes w1, w2 but the task for w2 gets polled first, acquiring the Mutex first. Same goes for the case where multiple writes wait on the same connection lock: say w1 currently holds the Mutex and two more writes w2, w3 would get get queued, AFAIU there is no guarantee that when w1 drops the lock w2 always acquires the lock next.

TLDR: it seems we unfortunately need to do the version dance here, too. Maybe there's an easier mechanism in the SQLite case (e.g., prepare should technically take care of that, but we can't lock the connection Mutex outside of the spawned task as the guard is not Send) we could lean on to guarantee the ordered writes, but applying the same approach seemed simplest for now?

I just refuse to believe that we need that version dance really for this problem. Isn't block_in_place meant for this? If we ensure that the write has happened before the fn returns, it doesn't matter that during that execution there may be other writes that get processed in a certain order?

Hmm? How do you imagine block_in_place to work here? block_in_place takes a future and drives it to completion, i.e., makes it a blocking operation. For the async KVStore we however exactly need a future, not a blocking operation, which is what spawn_blocking does for us.

Does it? I saw

pub fn block_in_place<F, R>(f: F) -> R where F: FnOnce() -> R

Does it? I saw

pub fn block_in_place<F, R>(f: F) -> R where F: FnOnce() -> R

Sorry, please replace block_in_place with block_on above. block_in_place is simply a wrapper that spawns a blocking task on the outer runtime context so that the inner block_on call doesn't starve (that is, if it is indeed called on the same runtime, which it isn't always).

Discussed offline that even though block_in_place may work with caveats, we do prefer to implement sqlite async. And that for that to work, we need the versioning.

src/io/sqlite_store/mod.rs

.. first step to make review easier.

.. as we're gonna reuse the `async` `_internal` methods shortly.

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

We implement the async `KVStore` trait for `VssStore`.

.. to be easier reusable via `KVStore` also

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

.. to be easier reusable via `KVStore` also

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

As an intermediary step, we require any store to implement both `KVStore` and `KVStoreSync`, allowing us to switch over step-by-step. We already switch to the fully-async background processor variant here.

tnull · 2025-10-07T08:49:55Z

Addressed remaining comments and changed base to main. Let me know if I can squash.

tnull · 2025-10-07T09:41:06Z

CI breakage is unrelated (#654)

joostjager · 2025-10-07T11:35:36Z

src/io/sqlite_store/mod.rs

+		&self, primary_namespace: &str, secondary_namespace: &str, key: &str,
+	) -> String {
+		if primary_namespace.is_empty() {
+			key.to_owned()


And secondary ns?

Is this actually used without a namespace?

joostjager · 2025-10-07T11:41:08Z

src/io/sqlite_store/mod.rs

 			io::Error::new(io::ErrorKind::Other, msg)
 		})?;
 		Ok(())
+        })


Indent formatting issue?

joostjager · 2025-10-07T11:43:11Z

Squash = ok

tnull marked this pull request as draft September 12, 2025 11:00

tnull force-pushed the 2025-09-async-vss-store branch from 2adf4a7 to 296b55c Compare September 12, 2025 11:01

This was referenced Sep 12, 2025

Add support for native async KVStore persist to ChainMonitor lightningdevkit/rust-lightning#4063

Merged

Implement KVStore for TestStore lightningdevkit/rust-lightning#4069

Merged

Update static invoice store for invoice requests #635

Merged

tnull force-pushed the 2025-09-async-vss-store branch from 296b55c to 85198d8 Compare September 18, 2025 09:32

tnull force-pushed the 2025-09-async-vss-store branch 3 times, most recently from c4251d4 to 7158653 Compare September 18, 2025 09:52

tnull added this to Weekly Goals Sep 18, 2025

tnull moved this to Goal: Merge in Weekly Goals Sep 18, 2025

tnull self-assigned this Sep 18, 2025

tnull mentioned this pull request Sep 24, 2025

process_events_async_with_kv_store_sync isn't actually usable lightningdevkit/rust-lightning#4111

Closed

tnull force-pushed the 2025-09-async-vss-store branch from 7158653 to 8c2ff8f Compare September 25, 2025 09:11

tnull mentioned this pull request Sep 25, 2025

Account for LiquidityManager persistence #650

Draft

tnull force-pushed the 2025-09-async-vss-store branch 2 times, most recently from 464e6b7 to 9035b71 Compare September 29, 2025 13:35

tnull requested a review from joostjager September 29, 2025 13:38

tnull marked this pull request as ready for review September 29, 2025 13:39

tnull force-pushed the 2025-09-async-vss-store branch from 9035b71 to 750ab87 Compare September 29, 2025 14:54

joostjager reviewed Sep 30, 2025

View reviewed changes

tnull force-pushed the 2025-09-async-vss-store branch from 750ab87 to 70404ed Compare September 30, 2025 07:45

tnull requested a review from joostjager September 30, 2025 07:46

tnull force-pushed the 2025-09-async-vss-store branch from 70404ed to 6c3fdf3 Compare September 30, 2025 08:47

joostjager reviewed Sep 30, 2025

View reviewed changes

src/io/sqlite_store/mod.rs Show resolved Hide resolved

tnull added 5 commits October 7, 2025 10:08

Move current VSS KVStoreSync logic to _internal methods

52a5f9a

.. first step to make review easier.

Make VSS internal methods async, move block_on to impl KVStoreSync

bc313f9

.. as we're gonna reuse the `async` `_internal` methods shortly.

Split VssStore into VssStore and VssStoreInner

2b40a80

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

Refactor infallible build_key to not return an error

679b6d3

Implement KVStore for VssStore

2b117f5

We implement the async `KVStore` trait for `VssStore`.

tnull force-pushed the 2025-09-async-vss-store branch from 6c3fdf3 to 1a55ab8 Compare October 7, 2025 08:45

tnull added 10 commits October 7, 2025 10:49

f No need to use RwLock if we ever only write it

ce73168

Move SqliteStore logic to _internal methods

76c75fe

.. to be easier reusable via `KVStore` also

Split SqliteStore into SqliteStore and SqliteStoreInner

c0473cc

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

Implement KVStore for SqliteStore

a3e9fed

f Newlines

4e50c14

f Use versioned write for SqliteStore

cc35230

Move TestStoreSync logic to _internal methods

c566e73

.. to be easier reusable via `KVStore` also

Split TestSyncStore into TestSyncStore and TestSyncStoreInner

9382235

.. where the former holds the latter in an `Arc` that can be used in async/`Future` contexts more easily.

Implement KVStore for TestSyncStore

42f7a9e

Require both types of KVStore

8dada08

As an intermediary step, we require any store to implement both `KVStore` and `KVStoreSync`, allowing us to switch over step-by-step. We already switch to the fully-async background processor variant here.

tnull force-pushed the 2025-09-async-vss-store branch from 1a55ab8 to 8dada08 Compare October 7, 2025 08:49

tnull changed the base branch from develop to main October 7, 2025 08:49

tnull requested a review from joostjager October 7, 2025 08:49

joostjager reviewed Oct 7, 2025

View reviewed changes


		Ok(self.storable_builder.deconstruct(storable)?.0)

		self.execute_locked_read(locking_key, async move \|\| {

Require both KVStore and KVStoreSync implementations, switch BP to be fully-async #633

Are you sure you want to change the base?

Require both KVStore and KVStoreSync implementations, switch BP to be fully-async #633

Uh oh!

Conversation

tnull commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

tnull commented Sep 18, 2025

Uh oh!

tnull commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Sep 29, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Sep 30, 2025

Uh oh!

joostjager commented Sep 30, 2025

Uh oh!

tnull commented Sep 30, 2025

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 Oct 7, 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!

joostjager Oct 7, 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!

Choose a reason for hiding this comment

Uh oh!

joostjager Oct 7, 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!

Uh oh!

tnull commented Oct 7, 2025

Uh oh!

tnull commented Oct 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Oct 7, 2025

Uh oh!

Uh oh!

Require both `KVStore` and `KVStoreSync` implementations, switch BP to be fully-async #633

Require both `KVStore` and `KVStoreSync` implementations, switch BP to be fully-async #633

tnull commented Sep 12, 2025 •

edited

Loading

ldk-reviews-bot commented Sep 12, 2025 •

edited

Loading

tnull commented Sep 29, 2025 •

edited

Loading

tnull Oct 7, 2025 •

edited

Loading

joostjager Oct 7, 2025 •

edited

Loading

joostjager Oct 7, 2025 •

edited

Loading