Add a paginated version of `list_payments`

[tnull]

We should eventually add a paginated version of list_payments to avoid overloading the caller in the face of many payments. This will be epecially crucial for a LDK Node 'server' deployment which we intend to add more support for generally in the future.

[theivess]

Hey, I would like to work on this issue, is this up for grabs?

[tnull]

Hey, I would like to work on this issue, is this up for grabs?

Sure, however, I think we should do #57 first. I.e., base the work on #270 and add a timestamp/last_updated field by which we can sort list_payments. Then, we can add a paginated version in a much more reasonable manner. What do you think?

[johncantrell97]

Do you think the PaymentStore should be changed to not keep all payments in memory?

I was thinking if you were going to support pagination you'd change the store to just read from the store anytime it needed a particular payment and then the paginated list payments would only need to load the keys into memory and then only read payments for the specific page being requested.

With that model, in order to support pagination by time/recent payments you'd also need a separate index in the kvstore that stores a mapping with timestamp as key to the payment id.

[tnull]

Do you think the PaymentStore should be changed to not keep all payments in memory?

Especially for the use cases we're mainly focussing right now (self-custodial wallets, (mobile) apps, light server use) the sent/received payments are not a lot of data, so for the time being it's probably fine to keep in memory. We will likely add optional invoice/offer storage however (cf. #51), as invoice/offer data might be a bit heavier (even though still not that much data). If/when we move the focus to heavier server use we might want to consider not keeping everything in memory, but we'll see when we get there (i.e., we have a user that really requires it).

I was thinking if you were going to support pagination you'd change the store to just read from the store anytime it needed a particular payment and then the paginated list payments would only need to load the keys into memory and then only read payments for the specific page being requested.

With that model, in order to support pagination by time/recent payments you'd also need a separate index in the kvstore that stores a mapping with timestamp as key to the payment id.

Yeah, maybe splitting up storage keys might become necessary at some point, however, note that our KVStore interface currently doesn't allow us to make multi-key reads/writes, so keeping any secondary keys in a consistent state in the face of crashes/persistence failures will be non-trivial and we really should only bother if we need it.

[johncantrell97]

Do you think the PaymentStore should be changed to not keep all payments in memory?

Especially for the use cases we're mainly focussing right now (self-custodial wallets, (mobile) apps, light server use) the sent/received payments are not a lot of data, so for the time being it's probably fine to keep in memory. We will likely add optional invoice/offer storage however (cf. #51), as invoice/offer data might be a bit heavier (even though still not that much data). If/when we move the focus to heavier server use we might want to consider not keeping everything in memory, but we'll see when we get there (i.e., we have a user that really requires it).

Makes sense, does seem like the data is pretty small even for thousands of payments. Can always do it later as you say if needed.

I was thinking if you were going to support pagination you'd change the store to just read from the store anytime it needed a particular payment and then the paginated list payments would only need to load the keys into memory and then only read payments for the specific page being requested.
With that model, in order to support pagination by time/recent payments you'd also need a separate index in the kvstore that stores a mapping with timestamp as key to the payment id.

Yeah, maybe splitting up storage keys might become necessary at some point, however, note that our KVStore interface currently doesn't allow us to make multi-key reads/writes, so keeping any secondary keys in a consistent state in the face of crashes/persistence failures will be non-trivial and we really should only bother if we need it.

yeah, I think this is only useful if you don't keep all payments in memory. I'm not sure if you actually have much of a consistency problem. I was thinking it would literally just be an index that mapped from timestamp to PaymentId. You'd never have to update the index because the timestamp and payment_id are static. I guess the only time you'd care about consistency issue is on the initial creation.

anyway, I think it's only relevant if you're trying to not load all payments into memory.

[Anyitechs]

Hey, I would like to work on this issue, is this up for grabs?

Sure, however, I think we should do #57 first. I.e., base the work on #270 and add a timestamp/last_updated field by which we can sort list_payments. Then, we can add a paginated version in a much more reasonable manner. What do you think?

Is this still open? I guess this can be worked on now that #57 is already in, yea?

[tnull]

Is this still open? I guess this can be worked on now that #57 is already in, yea?

Acually, it has already been implemented in LDK Server, and we're looking to upstream the PaginatedKVStore trait to LDK Node eventually.

So not sure if there is a actionable task here right now, it mostly depends on how we think about the upstreaming process though (cc @G8XSU )