Skip to content

Stop keeping all DataStore entries in-memory, add pagination - #1024

Draft
tnull wants to merge 9 commits into
lightningdevkit:mainfrom
tnull:2026-08-data-store-cache-policy
Draft

Stop keeping all DataStore entries in-memory, add pagination#1024
tnull wants to merge 9 commits into
lightningdevkit:mainfrom
tnull:2026-08-data-store-cache-policy

Conversation

@tnull

@tnull tnull commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Alternative/prefactor to #959.

Fixes #998.

Draft for now until concept ack.

tnull added 9 commits August 6, 2026 12:49
The `WalletEvent::TxReplaced` handler read the payment from the store
twice, once inside a `debug_assert!` and once for real, so the
assertion and the value actually used could in principle disagree.
Reuse a single lookup instead.

Co-Authored-By: HAL 9000
`DataStore::get`, `contains_key` and `list_filter` were synchronous
and infallible because every entry of a namespace is held in memory,
so a lookup could never fail or block. That assumption goes away once
a store may keep only a subset of its entries in memory and has to
read through to the `KVStore` on a miss.

Turn the readers into async methods and let `get` and `contains_key`
report an error, so that a failed store read is never mistaken for
"no such object". Behavior is unchanged: every reader still answers
from memory and always returns `Ok`.

`Node::payment` consequently returns a `Result`. Async and fallible
are introduced together on purpose, so that adding the read-through
paths later does not have to churn the same call sites twice.

Co-Authored-By: HAL 9000
Mutations persist to the `KVStore` first and only then update the
in-memory state, so that a failed write leaves memory untouched. The
readers, however, did not wait on the mutation lock, so during that
window they could hand out an object the store had already moved
past. The in-code comments documented this as a known caveat.

Now that the readers are async they can wait, so turn the mutation
lock into a read-write lock: mutations take the write guard across
both steps, readers take the read guard and therefore never observe
the intermediate state.

This also becomes load-bearing once entries may be read back from the
store on a cache miss, because a reader that repopulates memory from
a value it read before a concurrent write would otherwise leave
memory durably disagreeing with the store.

Readers now block for the duration of an in-flight write, which for a
remote backend is one network round trip.

Co-Authored-By: HAL 9000
`DataStore` held every object of its namespace in memory for the
lifetime of the node. That is fine for the pending-payment store,
which drops entries as payments resolve, but the payment store grows
without bound, so memory use and startup time grow with a node's
history.

Give each store a caching policy, either keeping all entries as
before, or keeping only a bounded number of least recently used ones
and reading the rest back from the store on demand. Both existing
stores keep all their entries, so nothing changes yet.

The policy is a type parameter rather than a plain value so that
`list_filter`, which can only answer correctly while everything is in
memory, is unavailable on a bounded store. Reaching for a full scan
where it would silently return a subset is a compile error.

A bounded store also has to read through on its write paths, not just
on reads: merging, updating or removing against a cache miss would
otherwise overwrite an evicted object with a partial one, drop an
update as if the object were unknown, or leave a removed object in
the store forever. A miss is only evidence of absence when the cache
holds everything.

Co-Authored-By: HAL 9000
Paginated listing hands the storage backend a token supplied by the
caller, which the backend rejects if it is malformed. Reporting that
as `PersistenceFailed` would be misleading, as nothing failed to
persist, and would give a bindings user who round-trips a token
through their own storage no way to tell a bad token from a broken
store.

Co-Authored-By: HAL 9000
Tests reach for the payment history in a great many places, all of
them spelling out how it is retrieved. Route them through a helper
trait instead, so that they state what they want and the retrieval
lives in one place.

Pure refactor: the helper currently just forwards to the existing
listing API.

Co-Authored-By: HAL 9000
Returning the entire payment history in one call requires holding it
all in memory, which is exactly what a node with a long history
cannot afford, and it gives an app no way to show recent payments
without loading every old one.

Return one page at a time instead, ordered from most recently created
to least recently created, and drop the unpaginated variants.

The ordering and the page tokens are the storage backend's own: we
hand its opaque token straight back to it and never derive an order
of ours. That keeps tokens valid across restarts and independent of
what we happen to hold in memory, and it means a store that caches
only a subset of its namespace can still list all of it, reading back
whatever it does not hold.

Listing deliberately neither waits for in-flight writes across its
reads nor disturbs the cache. Blocking every writer for the duration
of a round trip to a remote backend because something asked for a
page would be a poor trade, and letting a sweep of the whole
namespace count as use would evict the very entries a node works with
most.

Co-Authored-By: HAL 9000
The payment history grows for the lifetime of a node, and holding all
of it in memory was the reason `Node::list_payments` had to hand back
everything at once. Now that the store can read entries back on
demand and listing goes through the storage backend, the payment
store no longer has to.

Keep the most recently used payments in memory and read the rest back
as they are needed. At roughly 400 to 500 bytes per cached payment,
1000 of them bound this at well under a megabyte, regardless of how
long a node has been running.

Also stop reading the payment history at startup, which would
otherwise mean fetching a node's entire history from the storage
backend only to immediately drop all but the newest entries. The
cache now starts empty and fills as payments are used.

One consequence worth noting: a payment that fails to deserialize no
longer fails the build, as we no longer read them all up front. It
surfaces when that payment is accessed instead.

Co-Authored-By: HAL 9000
Seeding a store meant reading its entire namespace, which for a
bounded cache means fetching a node's whole payment history at
startup only to drop all but the newest entries. The previous commit
sidestepped that by not seeding the payment store at all, leaving it
cold and no longer catching unreadable payment data at build time.

Give the reader a bound instead, and seed the payment store with the
newest 50 payments. That matches the storage backends' page size, so
warming the cache costs a single page listing and one batch of reads,
and the first page of `Node::list_payments` is answered without going
to the store.

Take the keys from the paginated listing rather than `KVStore::list`,
which is documented to return them in arbitrary order and would
therefore make "the newest 50" meaningless. Objects now come back in
the store's own creation order, newest first, where before they came
back in whatever order the reads happened to finish.

Note the cache treats the objects it is seeded with as increasingly
recently used, so a newest-first read has to be reversed before
seeding, or the newest entries would be the first ones evicted.

Co-Authored-By: HAL 9000
@ldk-reviews-bot

ldk-reviews-bot commented Aug 6, 2026

Copy link
Copy Markdown

👋 Hi! This PR is now in draft status.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tnull
tnull marked this pull request as draft August 6, 2026 16:07

@benthecarman benthecarman 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.

concept ack, this approach looks good to me

Comment thread CHANGELOG.md
from the configured storage backend, so a token stays valid across restarts. This replaces
the previous unpaginated `Node::list_payments`, and `Node::list_payments_with_filter` has
been removed; filter the returned pages instead.
- `Node::payment` now returns a `Result`, as retrieving a payment may fail.

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.

also async now

Comment thread src/data_store.rs
/// Note also that a page may hold fewer objects than the backend's page size, because objects
/// removed between listing the keys and reading them are skipped. Iterate until
/// `next_page_token` is `None` rather than until a short page.
pub(crate) async fn list_page(

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.

claude:

A single page can mix stale cache hits with fresh store reads — src/data_store.rs list_page

The read guard is dropped after the cache peek, before read_missing runs. Objects served from the cache reflect the state at peek time; objects read from the store reflect a possibly-later state. The doc comment covers concurrent creation and removal but not this. One sentence would close it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop keeping the full payment history in memory

3 participants