Skip to content

Conversation

@benthecarman
Copy link
Contributor

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.

@benthecarman benthecarman requested a review from tnull January 26, 2026 15:00
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 26, 2026

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

@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

❌ Patch coverage is 78.72340% with 200 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.02%. Comparing base (caf0aac) to head (22655f6).

Files with missing lines Patch % Lines
lightning-persister/src/fs_store_v2.rs 82.73% 49 Missing and 38 partials ⚠️
lightning-persister/src/fs_store_common.rs 73.39% 35 Missing and 19 partials ⚠️
lightning/src/util/persist.rs 58.33% 30 Missing ⚠️
lightning/src/util/test_utils.rs 75.00% 17 Missing and 1 partial ⚠️
lightning-persister/src/fs_store.rs 87.64% 5 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4347      +/-   ##
==========================================
- Coverage   86.05%   86.02%   -0.03%     
==========================================
  Files         156      158       +2     
  Lines      103384   104070     +686     
  Branches   103384   104070     +686     
==========================================
+ Hits        88964    89530     +566     
- Misses      11905    11994      +89     
- Partials     2515     2546      +31     
Flag Coverage Δ
tests 86.02% <78.72%> (-0.03%) ⬇️

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.


/// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add extension traits in downstream crates too?

Copy link
Contributor

@tnull tnull Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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'?
  2. Either way, couldn't we still make PaginatedKVStore an extension trait?
  3. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

👍

@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from ebb2e5d to 95b82f8 Compare January 26, 2026 20:45
@tnull tnull removed their request for review January 27, 2026 08:30
@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from ffdf5c9 to 77d2cac Compare January 28, 2026 15:57
@benthecarman
Copy link
Contributor Author

Updated with @TheBlueMatt's suggestions, now only uses last_key as the page token and leaves the time/ordering up to the implementation

@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from 0105148 to ac4e907 Compare January 28, 2026 19:07
@benthecarman
Copy link
Contributor Author

implemented the paginated kv store for the file system store

/// Returns a paginated list of keys that are stored under the given `secondary_namespace` in
/// `primary_namespace`, ordered from most recently created to least recently created.
///
/// Implementations must return keys in reverse creation order (newest first). How creation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this might have been discussed above, but could you reiterate why we do creation time, not last update time? The latter seems like the obvious contender to me when we'd like to list payments, in particular, so it seems this might require that we go through all entries and sort them in-memory again for the most common usecase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with using updated at time is that it can be prone to race conditions, added to the docs

/// A vector of keys, ordered from most recently created to least recently created.
pub keys: Vec<String>,

/// The last key in this page, which can be passed to the next call to continue pagination.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While using the last key specifically matches what we do in VSS, requiring the page token to be the last key is a stricter requirement than what the VSS interface guarantees. Could we maybe just make this an opaque next_page_token field without any special semantics? Then the implementation could still use the last key if it works for it, but we wouldn't require specific semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay switched to something more generic

// Get file creation time, falling back to modified time if unavailable.
let metadata = entry.metadata()?;
let created_time = metadata
.created()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in most filesystems the created time is actually gonna be the last-update time because we overwrite-on-update. Thus, I don't think its possible to implement the pagination for the filesystem store (without including additional metadata in the filenames).

Sorry I had earlier assumed this would work but I dont think so.

Comment on lines 943 to 944
assert_eq!(response.keys[0], "key_e");
assert_eq!(response.keys[4], "key_a");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can assert is_sorted

@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from f80bba2 to 2a24e39 Compare February 2, 2026 17:25
@benthecarman
Copy link
Contributor Author

Used claude to implement the files system store v2 we talked about, lmk what you think

@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from f9e26c8 to bb08cb7 Compare February 2, 2026 17:57
&self, primary_namespace: &str, secondary_namespace: &str, key: &str,
) -> Option<String> {
let index = self.key_index.lock().unwrap();
index
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hmm, right, if we prefix we can't just open....Hmmm, I wonder if we can't just modify the file modification times when writing? If we bump the lightning-persist MSRV to 1.75 its available in std - https://doc.rust-lang.org/std/fs/struct.File.html#method.set_times and on my modern linux (btrfs plus CephFS) and macOS the timestamp modification survives a move just fine:

We can't rely on birth/creation times since linux doesn't even let us modify those, but we can backdate modify times arbitrarily. Would mean an extra stat call to the existing file when writing and an extra futimens call before the file fsync but the second call should be ~free and the first at least not wildly expensive.

$ touch test.tmp
$ touch -t 202001010000 test.tmp
$ sync test.tmp
$ stat test.tmp
  File: test.tmp
  Size: 0         	Blocks: 0          IO Block: 4096   regular empty file
Device: 0,27	Inode: 270657054   Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    matt)   Gid: ( 1000/    matt)
Access: 2020-01-01 00:00:00.000000000 +0000
Modify: 2020-01-01 00:00:00.000000000 +0000
Change: 2026-02-02 19:12:19.259194154 +0000
 Birth: 2026-02-02 19:11:39.045261083 +0000
$ mv test.tmp test
$ sync test
$ stat test
  File: test
  Size: 0         	Blocks: 0          IO Block: 4096   regular empty file
Device: 0,27	Inode: 270657054   Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    matt)   Gid: ( 1000/    matt)
Access: 2020-01-01 00:00:00.000000000 +0000
Modify: 2020-01-01 00:00:00.000000000 +0000
Change: 2026-02-02 19:12:25.831453775 +0000
 Birth: 2026-02-02 19:11:39.045261083 +0000

Copy link
Contributor Author

@benthecarman benthecarman Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this, seems like it works


/// A [`KVStore`] and [`KVStoreSync`] implementation that writes to and reads from the file system.
///
/// This is version 2 of the filesystem store which provides:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, do we really want to go there? We had that discussion several times, but rather than doing this (AFAIU) non-backwards compatible v2 of FilesystemStore, wouldn't this finally be the time where we stop treating the file system as a fully supported backend? It seems now is a good time to deprecate the FilesystemStore? I also doubt there is any LDK Node user running it in production (why would they?), so we should be able to just drop support there when we make the switch to require PaginatedKVStore.

Additionally, deprecating FilesystemStore would also make sense if we moved forward with @joostjager 's batch-write work, as then we could finally make use of basic ACID guarantees for KVStore backends.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2 on dropping FilesystemStore

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this finally be the time where we stop treating the file system as a fully supported backend? It seems now is a good time to deprecate the FilesystemStore? I also doubt there is any LDK Node user running it in production (why would they?), so we should be able to just drop support there when we make the switch to require PaginatedKVStore.

One of our largest users uses it. I don't think we can drop it (its also a perfectly good way to store stuff!).

Additionally, deprecating FilesystemStore would also make sense if we moved forward with @joostjager 's batch-write work, as then we could finally make use of basic ACID guarantees for KVStore backends.

Atomic batch writes is not a feature provided by all KV stores, so I'm skeptical of this. Also, FilesystemStore is perfectly ACID compliant...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of our largest users uses it. I don't think we can drop it (its also a perfectly good way to store stuff!).

That's not accurate, they run a fork. I also doubt they would ever make the switch to a non-backwards compatible v2?

Atomic batch writes is not a feature provided by all KV stores

I think besides the FilesystemStore (for which you could even come up with some hacky workarounds), all implementations that I am aware of would support them? But the batch write discussion is slightly orthogonal, just a point I wanted to mention here.

FilesystemStore is perfectly ACID compliant...

Is it? Are we sure that holds, even across all different OSes, filesystems and platforms? So, for instance, there are rollbacks if something goes wrong to ensure consistency in the face of failures, e.g., in the batch write case, but also for other operations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not accurate, they run a fork. I also doubt they would ever make the switch to a non-backwards compatible v2?

Oh? That's news to me, or at least the only changes I was aware of from them were the list bugs, which are fixed by the migration to a new directory structure (the primary reason for it!), though I assumed they moved back to upstream once we "fixed" it. I imagine they'd be happy to migrate to something that fixes the one bug they ran into.

I think besides the FilesystemStore (for which you could even come up with some hacky workarounds), all implementations that I am aware of would support them?

Our implementations (just SQLite and Postgres-backing-VSS) certainly do, but not all KV stores generally do - tons of KV Stores that aren't whole-hog SQL databases don't support atomic batches. Maybe we should decide that we don't want to support them, but it would be nice to.

Is it? Are we sure that holds, even across all different OSes, filesystems and platforms? So, for instance, there are rollbacks if something goes wrong to ensure consistency in the face of failures, e.g., in the batch write case, but also for other operations?

Batch writes are harder (obviously), but I think we're pretty confident in our implementation, yea? I mean ultimately ~every DB out there that is ACID-compliant just writes to a filesystem, so its definitely a pretty well-supported thing :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh? That's news to me, or at least the only changes I was aware of from them were the list bugs, which are fixed by the migration to a new directory structure (the primary reason for it!), though I assumed they moved back to upstream once we "fixed" it. I imagine they'd be happy to migrate to something that fixes the one bug they ran into.

AFAIR, they also introduced custom timeout/retry logic to account for the store living on a remote filesystem.

Our implementations (just SQLite and Postgres-backing-VSS) certainly do, but not all KV stores generally do - tons of KV Stores that aren't whole-hog SQL databases don't support atomic batches. Maybe we should decide that we don't want to support them, but it would be nice to.

IIRC, we did the research when designing the VSS interface (which supports batched writes at the API level) and found that most modern blob stores do support writing multiple keys at the same time. What would be a specific contender for a backend we want to be able to support that doesn't allow for write batching / multi-key writes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR, they also introduced custom timeout/retry logic to account for the store living on a remote filesystem.

Ah, this is news to my. I understood the filesystem handled that in some way.

IIRC, we did the research when designing the VSS interface (which supports batched writes at the API level) and found that most modern blob stores do support writing multiple keys at the same time.

Ah, interesting. I was going on my recollection of the start of that discussion being Gursharan indicating that this is not a common feature. If it is, then cool!

///
/// [`KVStore`]: lightning::util::persist::KVStore
pub struct FilesystemStoreV2 {
inner: Arc<FilesystemStoreV2Inner>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're not using a weird file name, ISTM we could DRY a lot of the logic here with fs_store.rs. They should now work identically except that V2 auto-updates the modification times (i guess we could make V1 do that too?) and the path is different for empty namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@benthecarman benthecarman force-pushed the paginated-kv-store branch 3 times, most recently from e7f043e to b55f58d Compare February 3, 2026 21:07
}
}

if !path.is_file() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original FilesystemStore::list has a retry loop (LIST_DIR_CONSISTENCY_RETRIES = 10) when metadata() returns NotFound due to a file disappearing mid-iteration. In yesterday's dev call this was mentioned as undesirable and even a reason to move to V2?

But this doesn't really address it. It just avoids the error path by using is_file() instead of metadata(). The underlying race is still there, we just silently skip disappeared files instead of retrying.

If that's the correct behavior (which I think it is, since read_dir has no snapshot semantics anyway?), shouldn't the existing FilesystemStore also be simplified to match?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could actually be turned into an error - with the new directory structure, the whole point is we should never encounter non-files at the leaves of our dir structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure? We previously made similar assumptions with v1 and made our checks stricter, only to have users run into issues as they had things in their filesystem lying around we didn't anticipate, which is why we then relaxed the checks again then. As different tools might interfere with the filesystem structure, and users might also be inclined to manually interfere with stuff on their hard drive, I'm not sure we should become stricter than previously here, esp. given the precedent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never encounter non-files at the leaves of our dir structure.

but can't this also be hit on a race condition? read_dir returns a file that is then removed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We previously made similar assumptions with v1 and made our checks stricter, only to have users run into issues as they had things in their filesystem lying around we didn't anticipate, which is why we then relaxed the checks again then.

This is news to me, is there a link somewhere where we had a user report of this? Its definitely possible in v1 that someone does something like maps a namespace to a symlink to store it elsewhere but in v2 within the two layers of folders it seems much less likely someone tries to shove a directory or some other file?

but can't this also be hit on a race condition? read_dir returns a file that is then removed.

Correct, we can definitely encounter deleted objects, but in those cases we should treat it as if it were a file (cause it should be!) whereas if we know its a directory we can fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is news to me, is there a link somewhere where we had a user report of this?

It was reported on Discord, couldn't find the post anymore when I looked. Introduced in d69a5ee fixed as part of 9d32931 (after the former commit we'd error out if there was an unexpected file with extension in the tree).

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like fs_store.rs can be DRYd a lot more here - ideally both fs_store.rs and fs_store_v2.rs only contain the top-level struct and impl KVStore* for * and then delegate to the fs_store_common.rs logic. The only difference should basically be one bool to the write method to decide whether to do the timestamp manipulation and one bool to the list method to decide whether to fail if we find directories vs retry in an outer loop.

}
}

if !path.is_file() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could actually be turned into an error - with the new directory structure, the whole point is we should never encounter non-files at the leaves of our dir structure.

/// ```
///
/// [`FilesystemStore`]: crate::fs_store::FilesystemStore
pub fn migrate_v1_to_v2<S: MigratableKVStore>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we have this? How is this different from https://docs.rs/lightning/latest/lightning/util/persist/fn.migrate_kv_store_data.html ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah didn't know that existed, will remove

@benthecarman
Copy link
Contributor Author

I feel like fs_store.rs can be DRYd a lot more here - ideally both fs_store.rs and fs_store_v2.rs only contain the top-level struct and impl KVStore* for * and then delegate to the fs_store_common.rs logic. The only difference should basically be one bool to the write method to decide whether to do the timestamp manipulation and one bool to the list method to decide whether to fail if we find directories vs retry in an outer loop.

done, simplified a lot more


impl FilesystemStoreV2 {
/// Constructs a new [`FilesystemStoreV2`].
pub fn new(data_dir: PathBuf) -> std::io::Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a check here that aborts if the user would point this to a v1 directory directly, to ensure we don't break any pre-existing store if they are not aware they need to migrate? Maybe check for the channel manager key in the root dir and use that as an indicator?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, could even fail if there's any file in the top-level dir

@benthecarman benthecarman force-pushed the paginated-kv-store branch 2 times, most recently from d4b28e2 to ddbde39 Compare February 10, 2026 21:00
benthecarman and others added 3 commits February 10, 2026 15:11
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.
Ahead of adding the FilesystemStoreV2 we move some common utilies into a
shared file.
Implements PaginatedKVStore traits with timestamp-prefixed filenames for
newest-first pagination and [empty] directory markers for consistent
namespace hierarchy. Includes v1 to v2 migration utility.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants