Add PaginatedKVStore traits upstreamed from ldk-server #4347

benthecarman · 2026-01-26T15:00:16Z

Allows for a paginated KV store for more efficient listing of keys so you don't need to fetch all at once. Uses monotonic counter or timestamp to track the order of keys and allow for pagination. The traits are largely just copy-pasted from ldk-server.

This also adds a PaginatedKVStoreSyncAdapter/PaginatedKVStoreAdapter so you can use a paginated kv store in contexts that expect a regular key value store.

Adds some basic tests that were generated using claude code.

ldk-reviews-bot · 2026-01-26T15:00:20Z

👋 I see @tnull was un-assigned.
If you'd like another reviewer assignment, please click here.

codecov · 2026-01-26T16:41:29Z

Codecov Report

❌ Patch coverage is 58.13953% with 72 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.03%. Comparing base (3fee76b) to head (95b82f8).

Files with missing lines	Patch %	Lines
lightning/src/util/persist.rs	50.00%	42 Missing ⚠️
lightning/src/util/test_utils.rs	65.90%	29 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4347      +/-   ##
==========================================
- Coverage   86.08%   86.03%   -0.05%     
==========================================
  Files         156      156              
  Lines      102416   102588     +172     
  Branches   102416   102588     +172     
==========================================
+ Hits        88165    88263      +98     
- Misses      11759    11830      +71     
- Partials     2492     2495       +3

Flag	Coverage Δ
tests	`86.03% <58.13%> (-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.

TheBlueMatt · 2026-01-26T16:45:11Z

lightning/src/util/persist.rs

+
+	/// A token that can be used to retrieve the next set of keys.
+	/// The `String` is the last key from the current page and the `i64` is
+	/// the associated `time` used for ordering.


I don't think most DBs are going to support this? "sort by the last-changed as of time X" isn't possible without storing what the last-updated time was prior to X when something is updated after X.

ldk-server does it based on creation_at not updated_at, will update the docs to make that more clear

Hmm, do all of our backend storage things have a created-at time? I guess if you want it time-ordered you have to do it that way but it adds quite a requirement :/

for ldk-server's we do

https://github.com/lightningdevkit/ldk-server/blob/9ae5ff8e2d2faf19540ce1be7af973ea134f9465/ldk-server/src/io/persist/sqlite_store/mod.rs#L97

Hmm, presumably that needs to be upstreamed to ldk-node and maybe also vss-server? I imagine we want pagination there too?

TheBlueMatt · 2026-01-26T18:08:47Z

lightning/src/util/persist.rs

+	pub keys: Vec<String>,
+
+	/// A token that can be used to retrieve the next set of keys.
+	/// The `String` is the last key from the current page and the `i64` is


I'm not sure I understand why this field is here, honestly. This doc indicates that the first element is just repeating an entry from keys (why can't I just call keys.last()?) and the second element doesn't seem super useful either (is there some DB where we need it for some reason?)

keys.last() doesn't give you the same info because it won't be None if it is the last page, you would need to do another list call to see that there are actually no elements left.

Having both elements is pretty important, the time is important so you can just do a select * where t.time > time to easily get the next set of elements. Having the key is less important but you need it to differentiate if there are multiple values with the same time

keys.last() doesn't give you the same info because it won't be None if it is the last page, you would need to do another list call to see that there are actually no elements left.

Sure, but we can have a bool instead :)

Having both elements is pretty important, the time is important so you can just do a select * where t.time > time to easily get the next set of elements. Having the key is less important but you need it to differentiate if there are multiple values with the same time

SELECT * FROM entries WHERE key > $1 ORDER BY key LIMIT 10 should do just fine in any SQL-based store. If we're worried about support for something else we shouldn't use the key or time but rather an opaque "next request token" (and maybe make the API take the last key anyway so that an SQL implementation can return an empty string for the token).

I don't think key ordering would work. If you got a payment between calls of list, it could screw up the pagination. Also while it does give you pagination, I am not sure if that is useful at all. Normally you are iterating through payments sequentially, if you are sorting by key, you are essentially just getting a random sample of payments each page.

Oops sorry I didn't think hard on that one lol. Its a trivial subquery though. Something like SELECT * FROM entries WHERE created_at >= (SELECT created_at FROM entries WHERE key = $1) ORDER BY created_at LIMIT 12.

Yeah I mean that works but it slightly less efficient. Would replace the page_token with something like has_more_pages: bool, but imo feels worse. Seems more of the complaint is that the type Option<(String, i64)> is confusing, maybe just creating a PageToken type would make things better

Indeed we should definitely switch to a pagetoken (at a minimum cause we don't want to require every implementation store the creation time!).

TheBlueMatt · 2026-01-26T18:09:37Z

lightning/src/util/persist.rs

+/// when dealing with a large number of keys that cannot be efficiently retrieved all at once.
+///
+/// For an asynchronous version of this trait, see [`PaginatedKVStore`].
+pub trait PaginatedKVStoreSync: Send + Sync {


Why doesn't this just extend KVStoreSync? Seems weird to duplicate the interface.

I am too used to having to create wrappers for ldk traits, I am actually in ldk now though! Fixed

You can add extension traits in downstream crates too?

You can add extension traits in downstream crates too?

To give some 'historic' context here, there were discussions on whether or not to make this an extension trait (cf. lightningdevkit/ldk-server#22). In the end, the PR author decided against it, also because it was anticipated that pagination might require two separate implementations.

However, IMO, now would be a good time to at least consider rethinking and double-checking some the design choices made, as there is still time to fundamentally LDK Server's persistence model, before we cut the first release (cc @jkczyz as he was also part of the original discussions, not sure if he has an opinion now).

In particular, I want to throw out some questions to consider:

Do we really want to maintain separate implementations for all backends and have LDK Node take different store types for 'general purpose' vs 'payment data'?

Either way, couldn't we still make PaginatedKVStore an extension trait?

If we will require it in LDK Node for payment data, will we be able to add pagination to all supported backends? if so, should this even be just a breaking change to KVStore::list rather than making it an extension trait?

Somewhat relatedly, note that from my point of view our endgoal should be that also all backend implementations (incl. VSS, SQLite, Postgres) eventually live in lightning-persister, and that preferably very little or no custom logic would live on the LDK Node/Server side. IMO this top-down approach would give clear interfaces and implementations across the board and avoid risking running into edge cases when making conflicting decisions on implementation details in different crates.

Do we really want to maintain separate implementations for all backends and have LDK Node take different store types for 'general purpose' vs 'payment data'?

Ideally not indeed.

Either way, couldn't we still make PaginatedKVStore an extension trait?

Definitely should be (it is now in this PR).

If we will require it in LDK Node for payment data, will we be able to add pagination to all supported backends?

Yea, see above discussion. Requiring that the paginated list be in created_at order means that most of our backends will have to actually store a created_at time (or have an incrementing row id at least). At least the only one in lightning-persister (filesystem store) already has it, but the SQLite/Postgres/VSS backends will all need it.

For that reason I'm really not a fan of sorting by created_at but it does certainly make the client easier. Another option would be to only require that things be listed in key-order and then make sure that when we want pagination we store a key that is ordered, but that doesn't work for VSS cause the key is encrypted garbage (so we'd have to have some crazy abstraction-break there). Dunno if anyone has any more creative ideas.

Somewhat relatedly, note that from my point of view our endgoal should be that also all backend implementations (incl. VSS, SQLite, Postgres) eventually live in lightning-persister, and that preferably very little or no custom logic would live on the LDK Node/Server side. IMO this top-down approach would give clear interfaces and implementations across the board and avoid risking running into edge cases when making conflicting decisions on implementation details in different crates.

👍

Allows for a paginated KV store for more efficient listing of keys so you don't need to fetch all at once. Uses monotonic counter or timestamp to track the order of keys and allow for pagination. The traits are largely just copy-pasted from ldk-server. Adds some basic tests that were generated using claude code.

benthecarman requested a review from tnull January 26, 2026 15:00

TheBlueMatt reviewed Jan 26, 2026

View reviewed changes

benthecarman force-pushed the paginated-kv-store branch from 5160a11 to 3fb869f Compare January 26, 2026 17:56

TheBlueMatt reviewed Jan 26, 2026

View reviewed changes

benthecarman force-pushed the paginated-kv-store branch from 3fb869f to ebb2e5d Compare January 26, 2026 20:42

benthecarman force-pushed the paginated-kv-store branch from ebb2e5d to 95b82f8 Compare January 26, 2026 20:45

tnull removed their request for review January 27, 2026 08:30

Add PaginatedKVStore traits upstreamed from ldk-server #4347

Are you sure you want to change the base?

Add PaginatedKVStore traits upstreamed from ldk-server #4347

Conversation

benthecarman commented Jan 26, 2026

Uh oh!

ldk-reviews-bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 26, 2026 • 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!

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!

tnull Jan 27, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Jan 26, 2026 •

edited

Loading

codecov bot commented Jan 26, 2026 •

edited

Loading

tnull Jan 27, 2026 •

edited

Loading