Paginate by creation time instead of key order by benthecarman · Pull Request #96 · lightningdevkit/vss-server

benthecarman · 2026-04-02T01:55:50Z

Clients often need to fetch recent payments or entries without iterating over the entire keyspace. Ordering by created_at lets callers retrieve the newest records first and stop early, which is not possible with lexicographic key ordering.

The page token now encodes (created_at, key) so the cursor remains unique even when multiple rows share the same timestamp. A composite index on (user_token, store_id, created_at, key) keeps the query efficient, and a migration back-fills any NULL created_at values and adds the NOT NULL constraint.

ldk-reviews-bot · 2026-04-02T01:55:53Z

👋 Thanks for assigning @tankyleo 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.

ldk-reviews-bot · 2026-04-04T01:56:25Z

🔔 1st Reminder

Hey @tankyleo! 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.

tankyleo

thanks !

rust/impls/src/migrations.rs

rust/impls/src/postgres_store.rs

rust/api/src/kv_store_tests.rs

tankyleo

We'll want to update types.rs to require this ordering too

rust/impls/src/postgres_store.rs

benthecarman · 2026-04-04T20:12:06Z

responded to review

proto/vss.proto

rust/api/src/kv_store_tests.rs

tankyleo · 2026-04-06T18:29:42Z

proto/vss.proto

 message ListKeyVersionsResponse {

-  // Fetched keys and versions.
+  // Fetched keys and versions, ordered by creation time (oldest first).


To confirm here, the server is free to choose any ordering they would like in case two keys have the same timestamp.

We have the server sort by key after timestamp. I'll add that here

Yes, but I don't this is a requirement on the VSS API right ? Ie clients don't care what ordering is picked beyond newest first.

I think it's better to me more explicit than less. We have to define a tiebreaker anyways so may as well put it in the docs

Let me see this defines an API constraint right, in addition to just documentation ? For sure I see the use for documentation here, but I don't want clients to start relying on this behavior.

rust/impls/src/postgres_store.rs

tankyleo · 2026-04-06T18:44:12Z

please request review when you are ready so i don't miss this PR

tnull · 2026-04-08T07:36:48Z

proto/vss.proto

  // Use this value to query for next-page of paginated `ListKeyVersions` operation, by specifying
  // this value as the `page_token` in the next request.
  //
-  // If `next_page_token` is empty (""), then the "last page" of results has been processed and


No, I think we were intentionally following protobuf's/Google's best practices here. https://google.aip.dev/158 states:

Response messages for collections should define a string next_page_token field, providing the user with a page token that may be used to retrieve the next page.

The field containing pagination results should be the first field in the message and have a field number of 1. It should be a repeated field containing a list of resources constituting a single page of results.

If the end of the collection has been reached, the next_page_token field must be empty. This is the only way to communicate "end-of-collection" to users.

If the end of the collection has not been reached (or if the API can not determine in time), the API must provide a next_page_token.

IMO would be good to revert and include this context in the docs.

Okay updated

ldk-reviews-bot · 2026-04-09T03:36:27Z

🔔 1st Reminder

Hey @tankyleo! 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.

tankyleo

Rushed some comments out, will be back after lunch.

rust/api/src/kv_store_tests.rs

tankyleo · 2026-04-09T19:42:38Z

rust/api/src/kv_store_tests.rs

+		}
+
+		let first_page = ctx.list(None, Some(page_size), None).await?;
+		assert_eq!(first_page.key_versions.len(), page_size as usize);


Given the VSS API, the page could have length 0 here. As long as the page token is not empty, the client would be expected to make another request with the new page token.

I'm confused what you're saying here. This is the first page we are requesting, the only way we'd a length of 0 would be if we had no items.

Just that I think a VSS-server is within bounds if it returns a response with an empty list, but a non-empty token. The client would be expected to continue asking for pages.

I'm mostly going from this line in the docs of ListKeyVersionRequest:

/// `page_size` is used by clients to specify the maximum number of results that can be returned by /// the server. /// The server may further constrain the maximum number of results returned in a single page. /// If the `page_size` is 0 or not set, the server will decide the number of results to be returned. #[prost(int32, optional, tag = "3")] pub page_size: ::core::option::Option<i32>,

TLDR can't assume the page you get back is the same length as the page size in your ListKeyVersionsRequest

TLDR can't assume the page you get back is the same length as the page size in your ListKeyVersionsRequest

This is true.

if it returns a response with an empty list, but a non-empty token.

But there is no reason to respond with empty list and non-empty pagination-token. (I don't think this should happen, can add a kvstore test/assert for it if doesn't exist already.)

rust/impls/src/postgres_store.rs

tankyleo · 2026-04-09T20:52:19Z

Done with this pass here, just needed to add the comment about the VSS server API constraint

G8XSU · 2026-04-09T21:06:07Z

Adding some historical context, VSS was purposefully built to be storage engine agnostic. The VSS protocol/API/data-model doesn't enforce a specific underlying storage database.

We can totally revisit this requirement and say VSS only works with postgres-like relational databases. But extending pagination to order by creation_time limits the capability to use any modern pure KV store as an underlying database engine like DynamoDB or CosmosDB.

I understand the need to efficiently paginate through large result sets. A couple of alternatives that preserve storage engine agnosticism:

Clients could use ordered key IDs like UUIDv7, which gives you time-based ordering for free
via lexicographic sort on the key itself.
Clients could scope their list_key_versions calls to known key prefixes to keep result sets
manageable.
Cursor-based pagination on the key (lexicographic ordering) works on every KV store without
requiring timestamps.

benthecarman · 2026-04-09T22:05:05Z

Fixed @tankyleo comments about tests

tankyleo · 2026-04-09T22:13:53Z

@G8XSU Thank you for the feedback, I'll be considering the tradeoffs over the next few days.

Was wondering do you have an email where I can reach you ? Feel free to ping me at "hello at leonash dot net" I was wondering if we could get the vss-client crate at https://crates.io/crates/vss-client from you.

benthecarman · 2026-04-09T22:16:22Z

@G8XSU my perspective

Clients could use ordered key IDs like UUIDv7, which gives you time-based ordering for free
via lexicographic sort on the key itself.

in theory, yes, but ldk-node isn't using this and migrating would be a huge lift

Clients could scope their list_key_versions calls to known key prefixes to keep result sets
manageable.

ldk-node uses txid/payment_hash as key so we can't really use prefixes

Cursor-based pagination on the key (lexicographic ordering) works on every KV store without
requiring timestamps.

Yes, but then the pagination isn't really useful anymore. You just end up getting a random set of payments rather an actual ordered list.

We can totally revisit this requirement and say VSS only works with postgres-like relational databases. But extending pagination to order by creation_time limits the capability to use any modern pure KV store as an underlying database engine like DynamoDB or CosmosDB.

imo postgres scales really well and you're scaling past something like postgres then it's likely you aren't using the off the shelf solution anymore and will have a whole team to manage things like stuff. And being able to sort by something useful rather than just lexicographical is worth it

rust/api/src/types.rs

Clients often need to fetch recent payments or entries without iterating over the entire keyspace. Ordering by created_at lets callers retrieve the newest records first and stop early, which is not possible with lexicographic key ordering. The page token now encodes (created_at, key) so the cursor remains unique even when multiple rows share the same timestamp. A composite index on (user_token, store_id, created_at, key) keeps the query efficient, and a migration back-fills any NULL created_at values and adds the NOT NULL constraint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

G8XSU · 2026-04-10T00:56:42Z

in theory, yes, but ldk-node isn't using this and migrating would be a huge lift

Yes I understand, but vss wasn't built with just keeping ldk-node payment-store in mind. Maybe it is not the right for payment-store currently.

But tying vss interface to be relational db specific is a 1-way door decision, and such interface change hasn't been made to either ldk-node or ldk iiuc.

Yes, but then the pagination isn't really useful anymore. You just end up getting a random set of payments rather an actual ordered list.

Hmm.. maybe, but why are we only thinking about payment store? Payment store requires PaginatedKvStore interface and on top of that it needs creation time based sorting. Imo the right solution would still be changing payment-storage to use something like uuidv7 so that it can work with any key based paginated store efficiently.
Instead of compensating for that in vss.

imo postgres scales really well and you're scaling past something like postgres then it's likely you aren't using the off the shelf solution anymore and will have a whole team to manage things like stuff. And being able to sort by something useful rather than just lexicographical is worth it

It is not about scaling at all but operational flexibility, can we get the change to sort by creation_time through ldk-storage interface? If not, then the similar reasons apply here as well i.e. clients want to use the storage that they want to use.

it's likely you aren't using the off the shelf solution anymore

It's not about this server code in repo but interoperable protocol and api model.
In the process, we have lost any chance of inter-operable vss protocol with hosted 3p providers, otherwise everyone must use relational db, they can't cost arbitrage or manage their operations in their own way.

I understand it is limiting to some extent but I just wanted to flag the reason for having a conservative api/protocol approach here.

benthecarman requested a review from tankyleo April 2, 2026 01:55

benthecarman force-pushed the creation-pagination branch from defad39 to cb70f98 Compare April 2, 2026 01:58

benthecarman self-assigned this Apr 2, 2026

benthecarman added this to Weekly Goals Apr 2, 2026

benthecarman mentioned this pull request Apr 3, 2026

Add 60 min timeout to ldk-node-integration CI #97

Merged

tankyleo reviewed Apr 4, 2026

View reviewed changes

rust/impls/src/migrations.rs Show resolved Hide resolved

rust/impls/src/migrations.rs Show resolved Hide resolved

rust/impls/src/postgres_store.rs Outdated Show resolved Hide resolved

rust/impls/src/postgres_store.rs Show resolved Hide resolved

tankyleo reviewed Apr 4, 2026

View reviewed changes

rust/api/src/kv_store_tests.rs Outdated Show resolved Hide resolved

rust/api/src/kv_store_tests.rs Show resolved Hide resolved

tankyleo reviewed Apr 4, 2026

View reviewed changes

rust/impls/src/postgres_store.rs Outdated Show resolved Hide resolved

benthecarman force-pushed the creation-pagination branch 2 times, most recently from dc205c9 to cd32024 Compare April 4, 2026 18:18

tankyleo reviewed Apr 6, 2026

View reviewed changes

benthecarman requested a review from tankyleo April 7, 2026 03:35

benthecarman force-pushed the creation-pagination branch from 6622747 to c02dcda Compare April 8, 2026 02:16

benthecarman mentioned this pull request Apr 8, 2026

Add PaginatedKVStore support to VssStore lightningdevkit/ldk-node#864

Open

tnull reviewed Apr 8, 2026

View reviewed changes

benthecarman force-pushed the creation-pagination branch from c02dcda to 34d4241 Compare April 8, 2026 19:46

tankyleo reviewed Apr 9, 2026

View reviewed changes

benthecarman force-pushed the creation-pagination branch from 34d4241 to e17c676 Compare April 9, 2026 22:00

benthecarman requested a review from tankyleo April 9, 2026 22:01

tankyleo reviewed Apr 9, 2026

View reviewed changes

rust/api/src/types.rs Show resolved Hide resolved

benthecarman force-pushed the creation-pagination branch from e17c676 to 36811fc Compare April 9, 2026 23:19

Conversation

benthecarman commented Apr 2, 2026

Uh oh!

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

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 4, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benthecarman commented Apr 4, 2026

Uh oh!

Uh oh!

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!

tankyleo commented Apr 6, 2026 • 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!

ldk-reviews-bot commented Apr 9, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

tankyleo commented Apr 9, 2026

Uh oh!

G8XSU commented Apr 9, 2026

Uh oh!

benthecarman commented Apr 9, 2026

Uh oh!

tankyleo commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman commented Apr 9, 2026

Uh oh!

Uh oh!

G8XSU commented Apr 10, 2026

Uh oh!

Reviewers

ldk-reviews-bot commented Apr 2, 2026 •

edited

Loading

tankyleo commented Apr 6, 2026 •

edited

Loading

tankyleo commented Apr 9, 2026 •

edited

Loading