Implement `KVStore` for `TestStore` by tnull · Pull Request #4069 · lightningdevkit/rust-lightning

tnull · 2025-09-12T12:15:51Z

We implement the async KVStore trait for TestStore. This is mostly useful in LDK Node tests, where we use TestStore and we now require all stores to implement both variants.

~~Moreover, we drop the Frankenstein-esque process_events_async_with_kv_store_sync which won't be necessary anymore for LDK Node.~~

ldk-reviews-bot · 2025-09-12T12:15:54Z

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

TheBlueMatt · 2025-09-12T12:31:55Z


-/// Async events processor that is based on [`process_events_async`] but allows for [`KVStoreSync`] to be used for
-/// synchronous background persistence.
-pub async fn process_events_async_with_kv_store_sync<


Hmm, not quite sure I understand why we want to drop this? ldk-node might not use it, but I imagine some others might? Its the equivalent of our previous async BP loop and keeping it makes upgrades easier for those who might not want to switch to partial-async-kvstore immediately?

IMO it's a very odd middleground API that was introduced when the KVStoreSyncWrapper wasn't public. I fear users might find it confusing, and just using KVStoreSyncWrapper when needed seems way more consistent (as they'd already need to do that for some of the other types they'd hand into that method anyways).

No comment. It's so little code that I think it's fine either way.

IMO we should prefer users not use KVStoreSyncWrapper directly. The docs even explicitly say "It is not necessary to use this type directly." (and I feel like we should #[doc(hidden)] it?).

Hmm, but at least in LDK Node using KVStoreSyncWrapper was unavoidable. I can drop the drop commit if you insist, but IMO it's a pretty awkward confusing API.

Now dropped the drop commit since you prefer to keep it.

TheBlueMatt · 2025-09-12T12:33:01Z

+	}
+}
+
+impl KVStore for TestStore {


I wonder if we shouldn't actually make these async? eg write the returned future to require two polls or something? Maybe its not worth it but seems like we should consider it. It feels a bit weird to have the test implementation do something that no real implementation should do.

Alright, now added a fixup to do just that.

Now force-pushed a further simplification, also allowing to drop the 'splitting' commit.

Oh I like this double poll thing for testing. A bit too fake otherwise.

codecov · 2025-09-12T12:38:15Z

Codecov Report

❌ Patch coverage is 61.29032% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.34%. Comparing base (867f084) to head (012bb7a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/test_utils.rs	50.00%	24 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4069 +/- ##
==========================================
- Coverage 88.39% 88.34% -0.05% 
==========================================
Files 177 177 Lines 131314 131922 +608 Branches 131314 131922 +608 ==========================================
+ Hits 116069 116550 +481 - Misses 12596 12708 +112 - Partials 2649 2664 +15

Flag	Coverage Δ
fuzzing	`21.60% <0.00%> (-0.41%)`	⬇️
tests	`88.18% <61.29%> (-0.05%)`	⬇️

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.

joostjager · 2025-09-12T13:06:04Z

+ let secondary_namespace = secondary_namespace.to_string();
+ let key = key.to_string();
+ let inner = Arc::clone(&self.inner);
+ Box::pin(async move {


Before returning this, I think we need to record the order?

Or spawn_blocking and await?

Or just call it because it isn't actually blocking?

I don't think so, as it calls through to the inner Mutex? Ah, actually, now that we made it actually async as of #4069 (comment), we might need to?

Or just call it because it isn't actually blocking?
You mean actually async? It is now, so we might need to do the whole write-tracking thing now..

Nevermind, I think now that we retrieve the result sync again (while acquiring the inner Mutex), we should be good ordering-wise?

Yes I think it's good now

This has regressed again - its no longer correct. You should be able to drop the second-to-last commit and just do the actual operation sync and then return a future of the result.

This has regressed again
Yes, I explicitly asked you whether to revert to this version though? So you don't want the intial version, but the intermediate version that implements the more complicated future logic, but doens't require polling twice. Okay.

This has regressed again - its no longer correct. You should be able to drop the second-to-last commit and just do the actual operation sync and then return a future of the result.
Pushed another fully-sync-under-the-hood variant. Let me know if that's the right amount of regression.

joostjager

I understand this is going to be used in ldk-node, but I don't think any of the async code is hit in ldk itself. It's basically dead code within that scope. Test coverage of test code is maybe a bit over the top?

joostjager · 2025-09-12T14:30:15Z


-/// Async events processor that is based on [`process_events_async`] but allows for [`KVStoreSync`] to be used for
-/// synchronous background persistence.
-pub async fn process_events_async_with_kv_store_sync<


No comment. It's so little code that I think it's fine either way.

joostjager · 2025-09-12T14:30:36Z

+ let secondary_namespace = secondary_namespace.to_string();
+ let key = key.to_string();
+ let inner = Arc::clone(&self.inner);
+ Box::pin(async move {


Yes I think it's good now

joostjager · 2025-09-12T14:31:02Z

+	}
+}
+
+impl KVStore for TestStore {


Oh I like this double poll thing for testing. A bit too fake otherwise.

TheBlueMatt

There's a ton of room to make TestStore a better test, dunno if it has to happen here but if you have a minute it would be nice to make it a really good test.

ldk-reviews-bot · 2025-09-15T00:00:53Z

🔔 1st Reminder

Hey @TheBlueMatt! 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 · 2025-09-16T05:17:14Z

Now reverted back to the initial approach 444f408.

tnull · 2025-09-16T12:08:06Z

Seems CI error is pre-existing?

joostjager

When reverting, did this also undo the changes that followed from #4069 (comment)?

I think currently the order is lost again?

.. to be easier reusable by different trait implementations.

We implement the async `KVStore` trait for `TestStore`. This is mostly useful in LDK Node tests, where we use `TestStore` and we now require all stores to implement both variants.

tnull · 2025-09-16T15:26:55Z

Pushed another simplified fully-sync variant. Let me know if that works for you guys.

TheBlueMatt · 2025-09-16T18:51:46Z

This is now quite trivial, just gonna land it.

tnull requested review from TheBlueMatt and joostjager September 12, 2025 12:16

tnull force-pushed the 2025-09-async-test-store branch from e636a74 to 6246c04Compare September 12, 2025 12:18

tnull mentioned this pull request Sep 12, 2025
Add support for native async KVStore persist to ChainMonitor#4063
Merged

tnull force-pushed the 2025-09-async-test-store branch from 6246c04 to 7faae86Compare September 12, 2025 12:22

TheBlueMatt reviewed Sep 12, 2025
View reviewed changes

joostjager reviewed Sep 12, 2025
View reviewed changes

tnull force-pushed the 2025-09-async-test-store branch from 7faae86 to 3ef76a5Compare September 12, 2025 13:34

tnull requested a review from TheBlueMatt September 12, 2025 13:35

graphite-appBot reviewed Sep 12, 2025
View reviewed changes

Comment threadlightning/src/util/test_utils.rs Outdated

tnull force-pushed the 2025-09-async-test-store branch from 3ef76a5 to 9df68fcCompare September 12, 2025 13:55

tnull requested a review from joostjager September 12, 2025 13:57

tnull force-pushed the 2025-09-async-test-store branch from 9df68fc to 3dc60fdCompare September 12, 2025 14:30

joostjager previously approved these changes Sep 12, 2025
View reviewed changes

TheBlueMatt reviewed Sep 12, 2025
View reviewed changes

Comment threadlightning/src/util/test_utils.rs Outdated
Comment threadlightning/src/util/test_utils.rs

tnull dismissed joostjager’s stale review via cd28a16September 12, 2025 16:25

tnull force-pushed the 2025-09-async-test-store branch from 3dc60fd to cd28a16Compare September 12, 2025 16:25

tnull requested a review from TheBlueMatt September 12, 2025 16:30

tnull force-pushed the 2025-09-async-test-store branch from cd28a16 to c47ca84Compare September 15, 2025 08:37

tnull requested a review from joostjager September 15, 2025 10:28

TheBlueMatt reviewed Sep 15, 2025
View reviewed changes

Comment threadlightning/src/util/test_utils.rs Outdated

tnull force-pushed the 2025-09-async-test-store branch from c47ca84 to 444f408Compare September 16, 2025 05:16

tnull requested a review from TheBlueMatt September 16, 2025 05:17

tnull changed the title ~~Implement KVStore for TestStore, drop process_events_async_with_kv_store_sync~~Implement KVStore for TestStoreSep 16, 2025

joostjager reviewed Sep 16, 2025
View reviewed changes

TheBlueMatt removed their request for review September 16, 2025 12:34

tnull added 2 commits September 16, 2025 17:25

Move TestStore logic to _internal methods
828e10d
.. to be easier reusable by different trait implementations.

Implement KVStore for TestStore
012bb7a
We implement the async `KVStore` trait for `TestStore`. This is mostly useful in LDK Node tests, where we use `TestStore` and we now require all stores to implement both variants.

tnull force-pushed the 2025-09-async-test-store branch from 444f408 to 012bb7aCompare September 16, 2025 15:25

tnull requested review from TheBlueMatt and joostjager September 16, 2025 15:26

TheBlueMatt approved these changes Sep 16, 2025
View reviewed changes

TheBlueMatt merged commit 3564646 into lightningdevkit:mainSep 16, 2025
24 of 25 checks passed

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!

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!

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

Uh oh!

Codecov Report

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!

Uh oh!

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

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Sep 15, 2025

Uh oh!

Uh oh!

tnull commented Sep 16, 2025

Uh oh!

tnull commented Sep 16, 2025

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Sep 16, 2025

Uh oh!

TheBlueMatt commented Sep 16, 2025

tnull commented Sep 12, 2025•
edited
Loading

ldk-reviews-bot commented Sep 12, 2025•
edited
Loading

codecovBot commented Sep 12, 2025•
edited
Loading