Add storage for forwarded payments - #772
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
1feb7ae to
ba15c40
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks! I do wonder if we should really enable storing all forwards by default, or rather make this optional.
Also more generally I wonder if users really expect us to store all forwarded payments forever, or if we should only keep the last X entries in the store? Also, with general-purpose HTLC interception coming up, maybe storing forwards might even be something we entirely want to leave entirely to the user after all?
What do you think?
ba15c40 to
20877bd
Compare
|
Addressed comments. I think this makes sense to include in ldk-node and not just leaving it up to the user. If we are focusing on LSPs this will be an essential feature, especially if we want to add accounting tools down the line. I think it can make sense to disable this and/or add a function to prune the storage for it. Maybe just an option that tracks totals per channel rather than individual htlcs |
970a395 to
c3f7714
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
IMO storing granular forwarding information long-term is dangerous. Instead, can we store information that allows for easier compaction? eg total forwarding on a per-X basis between channel pairs?
Agree it's not great to store everything forever. However, it's also hard to guess which values users are interested in. For example, one metric they def. want to see is fee revenue, and I suspect they might even want individual values for each payment forwarded rather than aggregated numbers. |
|
I started working on a version where you set an interval, say 12 hours, and we store individual events and after the time has lapsed we combine them into a single entry. Does that sound good? |
Yea, I kinda wonder what kind of aggregate stats they might want. Pairwise totals and counts over discrete time horizons suffices for things like "average forwarded per payment over this channel pair", but median is of course trickier. Maybe that's okay? |
I think its totally fair to store the last N hours of individual forwards, yea! After that question is format. |
19c5b89 to
cbb235e
Compare
|
Made it so the |
cbb235e to
ad559cc
Compare
|
It seems this should be unblocked by now, but needs a considerable rebase? |
ad559cc to
d8a4671
Compare
|
Rebased and updated for pagination. However probably worth holding off on until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4766 is fixed |
d8a4671 to
a831210
Compare
Ugh, okay, that means that the fix needs to happen for 0.3 still then. |
12ba5f9 to
f031041
Compare
|
Please rebase now that #1024 landed. |
f031041 to
975082c
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks!
Some comments, I think the two main things remaining is that we still don't have a way to handle replayed events and that we should probably reuse DataStore for this (fwiw, there are a few fixed bugs in DiskStore that we can just avoid when we drop the fork/DRY up the code).
| pub(crate) const PENDING_PAYMENT_INFO_PERSISTENCE_PRIMARY_NAMESPACE: &str = "pending_payments"; | ||
| pub(crate) const PENDING_PAYMENT_INFO_PERSISTENCE_SECONDARY_NAMESPACE: &str = ""; | ||
|
|
||
| /// The forwarded payment information will be persisted under this prefix. |
There was a problem hiding this comment.
Shouldn't all forwarded-payment-store related objects be stored under the same primary namespace, and be discerned by secondary?
| pub(crate) const FORWARDED_PAYMENT_AGGREGATION_BUCKET_SIZE_SECS: u64 = 60 * 60; | ||
|
|
||
| /// A disk-backed store for forwarding data that is too large to keep in memory. | ||
| pub(crate) struct DiskStore<SO: StorableObject> { |
There was a problem hiding this comment.
I don't understand why we need this? The idea of DataStore being generic is exactly to avoid adding specialized stores for every instance?
There was a problem hiding this comment.
Removed, goal was to not cache all these and just write to store
| } | ||
| } | ||
|
|
||
| for inbound_stats in inbound_stats_by_channel.into_values() { |
There was a problem hiding this comment.
Hmm, @TheBlueMatt recently indicated the HTLC ID was added to PaymentForwarded, but it still doesn't seem to be the case. So AFAIU we have no idempotency token available here. Begs the question if we're fine shipping this with a known bug or how we'd want to deal with this.
975082c to
64444a0
Compare
|
Addressed most review comments, however, can hold off until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4912 is merged |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Addressed most review comments, however, can hold off until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4912 is merged
Cool. That should land shortly, though we'll also need a backport PR to land. That said, it seems all fixups now happened in the second commit. Can you split all changes to DataStore out to 1-2 prefactor commits, also to avoid switching to UpdatableObject and back?
64444a0 to
765a62c
Compare
| /// Returns whether this store contains no objects. | ||
| pub(crate) async fn is_empty(&self) -> Result<bool, Error> { | ||
| let mut page_token = None; | ||
| loop { |
There was a problem hiding this comment.
Why do we need this loop here? That seems just like a risk that we eventually could add a bug that loops forever? Whether the store is empty should be a simple O(1) decision based on the first response, no?
| } | ||
|
|
||
| /// Returns all stored objects matching `f` by reading each page from the backing store. | ||
| pub(crate) async fn list_filter_from_store<F: FnMut(&&SO) -> bool>( |
There was a problem hiding this comment.
No, one of the main design goals (i.e., why we introduced the typestate pattern and dropped list_payments_with_filter) of the recent refactor was to disallow any internal callers from making prohibitively expensive listing calls. For a long running node this will be insanely expensive as it always interatively walks the entire uncached store. We can't do this, IMO hence have to drop this and all dependent callers.
|
Needs a rebase now. |
449c831 to
2397afd
Compare
StorableObject required an update representation from every type a store holds. Stores that only read, write, and delete whole objects still had to supply update methods that no caller used. Move merge behavior to UpdatableObject. Keep DataStore available to all storable objects, and restrict only merge operations to updatable objects. AI-assisted-by: OpenAI Codex and Anthropic Fable
Add a cache policy that reads objects from persistent storage without keeping them in memory. Add a key-only empty check for namespaces that can grow without a bounded working set. AI-assisted-by: OpenAI Codex and Anthropic Fable
2397afd to
829887f
Compare
Store unambiguous single-HTLC forwarding events. Aggregate them into per-channel and channel-pair statistics. Use fixed one-hour buckets for detailed records. Keep details out of the payment LRU cache. Use one persistence namespace for forwarding data. Expose analytics through Rust and UniFFI. Keep forwarding persistence and event-recording logic behind one internal store. AI-assisted-by: OpenAI Codex and Anthropic Fable
829887f to
012976d
Compare
4 participants
Routing nodes and LSPs want to track forwarded payments so they can run accounting on fees earned and track profitability across time. We now store these to make it easier to track and allows for future accounting utils in the future.
This shouldn't effect edge user nodes as they should never be forwarding payments.
Implementation is mostly just copied how we currently handle normal payments and adapted for forwarded payments.