Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Add PaginatedKVStore support to VssStore by benthecarman · Pull Request #864 · lightningdevkit/ldk-node · GitHub
Skip to content

Add PaginatedKVStore support to VssStore - #864

Merged
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss
Jun 18, 2026
Merged

Add PaginatedKVStore support to VssStore#864
tnull merged 4 commits into
lightningdevkit:mainfrom
benthecarman:paginated-vss

Conversation

@benthecarman

@benthecarmanbenthecarman commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken.

At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.

After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node

@ldk-reviews-bot

ldk-reviews-bot commented Apr 1, 2026

Copy link
Copy Markdown

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

@tnulltnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.

I'll let @tankyleo take a first look here and on the corresponding vss-server PR.

@ldk-reviews-bot

Copy link
Copy Markdown

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

@ldk-reviews-bot

Copy link
Copy Markdown

🔔 2nd 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.

Comment threadsrc/io/vss_store.rs Outdated
Comment on lines +725 to +727
// VSS uses empty string to signal the last page
let next_page_token =
response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.

Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.

A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed in lightningdevkit/vss-server#96 to do this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.

I think we're 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.

Comment threadsrc/io/vss_store.rs
Comment on lines +1178 to +1181
match response.next_page_token {
Some(token) => page_token = Some(token),
None => break,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Update to handle empty string

@benthecarman

Copy link
Copy Markdown
ContributorAuthor

Okay updated to be compliant with the AIP 158 stuff

@tankyleo
tankyleo self-requested a review April 9, 2026 15:51
Comment threadsrc/io/vss_store.rs Outdated
PaginatedKVStoreSync::list_paginated(&store, ns, sub, page_token).unwrap();
all_keys.extend(response.keys);
match response.next_page_token {
Some(token) if !token.as_str().is_empty() => page_token = Some(token),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds to me here we should break only if the page token field is None, and not if the token is Some(""), otherwise, some details of the VSS API are leaking past the PaginatedKVStore API ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah true, good catch

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs Outdated

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@tnull
tnull self-requested a review April 13, 2026 12:54
Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs
Ok(keys)
}

async fn list_paginated_internal(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems this is duplicating a lot of the logic of list_all_keys. Rather than doing that, can we

  1. move the logic to a new method called list_keys or similar that takes the paget oken
  2. move the while page_token != Some("".to_string()) loop to list_internal, calling list_keys
  3. hence have both list_internal and list_internal_paginated reuse the same list_keys code to avoid duplication

ghostApr 14, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done, also fixed a potential issue where if the VSS server returned None for the page token we could go into an infinite loop.

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good I think, but I do wonder if we now need protocol-level versioning for VSS, see lightningdevkit/vss-server#96 (review).

Comment threadsrc/io/vss_store.rs Outdated
}
Ok(keys)

Ok((keys, response.next_page_token))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

list_keys returns response.next_page_token raw, so both callers independently handle the Some("") case. Would be cleaner to normalize it in list_keys itself. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@tnull

ghost commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

@benthecarman Any update here?

@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 3e5fc71 to a8ce0c7CompareMay 4, 2026 01:24

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, I think we should wait to land this until we have the protocol versioning in VSS Server, so we can require it here (and error out if not met) before attempting to use pagination and fail at runtime.

@ldk-reviews-bot

ghost commented May 6, 2026

Copy link
Copy Markdown

🔔 1st Reminder

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

@ldk-reviews-bot

ghost commented May 9, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

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

@ldk-reviews-bot

ghost commented Jun 1, 2026

Copy link
Copy Markdown

🔔 12th Reminder

Hey @Camillarhi! 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
tnull removed the request for review from CamillarhiJune 2, 2026 08:20
@benthecarman

ghost commented Jun 11, 2026

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated vss-client to the 0.6

ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly looks good, just some minor comments.

Please also include an entry in the compatibility section of the pending CHANGELOG.md to note that users must upgrade VSS server before upgrading LDK Node.

Comment threadCargo.toml
native-tls = { version = "0.2", default-features = false, optional = true }
postgres-native-tls = { version = "0.5", default-features = false, features = ["runtime"], optional = true }
vss-client = { package = "vss-client-ng", version = "0.5" }
vss-client = { package = "vss-client-ng", version = "0.6" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this version introduced new error types (namely version mismatch, but can't recall if there are others), we'll need to update our retry policy to not retry if we hit a version mismatch, as it's unrecoverable.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed. Looks like it would have failed before but now has better and more explicit error message

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, not what I meant: we need to update

fn retry_policy() -> CustomRetryPolicy{

above to skip_retry_on_error for VSSVersionMismatchError.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, added

Comment threadsrc/io/vss_store.rs
Comment threadsrc/io/vss_store.rs
@benthecarman
benthecarmanforce-pushed the paginated-vss branch 2 times, most recently from 608f607 to f51bf20CompareJune 12, 2026 19:22
@benthecarman
benthecarman requested a review from tnullJune 12, 2026 19:23
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 15, 2026

Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull@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.

benthecarmanand others added 3 commits June 15, 2026 14:22
Switches vss-client-ng to the crates.io 0.6 release.
Generated with OpenAI Codex.
Move repeated VssStore construction logic into a shared
build_vss_store() helper and have existing tests use it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the single-page VSS listing logic into a list_keys method
that accepts page_token and page_size parameters. list_internal
now drives the pagination loop itself, calling list_keys per page.
This prepares for PaginatedKVStore support which will reuse
list_keys for single-page queries.
This also fixes a potential issue where if the VSS server returned None
for the page token we could enter into an infinite loop.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

1 similar comment
@ldk-reviews-bot

ghost commented Jun 17, 2026

Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull@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.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM a few nits/questions

Comment threadsrc/io/vss_store.rs Outdated
let mut keys = vec![];
secondary_namespace: &str, page_token: Option<String>, page_size: Option<i32>,
) -> io::Result<(Vec<String>, Option<String>)> {
let key_prefix = self.build_obfuscated_prefix(primary_namespace, secondary_namespace);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We used to do this encryption once for the entire list, but now we do this once for every page. Encryption with chacha20 is fast so should be fine?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

moved outside to be safe

Comment threadsrc/io/vss_store.rs Outdated
Comment threadsrc/io/vss_store.rs

const PAGE_SIZE: i32 = 50;

let vss_page_token = page_token.map(|t| t.to_string());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought we could do a into_inner instead of allocating a new string to copy a string, but realized this is gated behind the upstream API so should be fine for now.

@tnull
tnull merged commit fb7738f into lightningdevkit:mainJun 18, 2026
@tnulltnull mentioned this pull request Jun 18, 2026
@tnull

ghost commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Oh, seems cargo fmt didn't pass on this branch. Now fixed in #942. I miss CI.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@benthecarman@ldk-reviews-bot@tnull@Camillarhi@tankyleo