Stop keeping all DataStore entries in-memory, add pagination - #1024
Draft
tnull wants to merge 9 commits into
Draft
Stop keeping all DataStore entries in-memory, add pagination#1024tnull wants to merge 9 commits into
DataStore entries in-memory, add pagination#1024tnull wants to merge 9 commits into
Conversation
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
|
👋 Hi! This PR is now in draft status. |
benthecarman
reviewed
Aug 6, 2026
benthecarman
left a comment
Contributor
There was a problem hiding this comment.
concept ack, this approach looks good to me
benthecarman
reviewed
Aug 6, 2026
| 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. |
| /// 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( |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative/prefactor to #959.
Fixes #998.
Draft for now until concept ack.