Add storage for forwarded payments by benthecarman · Pull Request #772 · lightningdevkit/ldk-node

benthecarman · 2026-01-27T17:33:31Z

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.

ldk-reviews-bot · 2026-01-27T17:33:34Z

👋 Thanks for assigning @tnull 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.

tnull

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?

benthecarman · 2026-01-28T14:56:40Z

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

TheBlueMatt

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?

tnull · 2026-02-05T08:22:11Z

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.

benthecarman · 2026-02-05T14:56:04Z

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?

TheBlueMatt · 2026-02-05T15:46:16Z

I suspect they might even want individual values for each payment forwarded rather than aggregated numbers.

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?

TheBlueMatt · 2026-02-05T15:46:44Z

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?

I think its totally fair to store the last N hours of individual forwards, yea! After that question is format.

benthecarman · 2026-02-06T01:26:16Z

Made it so the Detailed mode now only stores for a configured time period and will aggregate them into channel pair stats

tnull · 2026-06-11T10:52:55Z

It seems this should be unblocked by now, but needs a considerable rebase?

benthecarman · 2026-07-01T00:21:32Z

Rebased and updated for pagination.

However probably worth holding off on until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4766 is fixed

tnull · 2026-07-01T10:41:53Z

Rebased and updated for pagination.

However probably worth holding off on until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/issues/4766 is fixed

Ugh, okay, that means that the fix needs to happen for 0.3 still then.

tnull

Needs a rebase, some comments.

tnull · 2026-07-21T08:40:33Z

+		}]);
+	}
+
+	// LDK reports the incoming and outgoing HTLCs, but not which incoming HTLC funded each


It seems like here we're trying to re-establish an API contract that LDK doesn't provide, and it seems error prone. Can we avoid this? If we think this matching is important we might need to change upstream first?

There isn't a mapping of the sats for when we receive them to when we send them, they are all in a single batch. I did FIFO because it's a simple way to split the amounts while keeping the totals correct. I don't know if this is something we really can upstream in ldk because there really is no "pairs" its just one batch of htlcs coming in and going out.

To be honest together with #772 (comment) this seems like a blocker for this PR. If the upstream API doesn't provide sufficient data for us to do the level of detailed aggregation we want to do, and also doesn't allow us to aggregate safely without overcounting on restarts, we probably either a) start with a version that exposes fewer details or b) first change the upstream API altogether so it provides all the data we need to do this correctly and safely.

Changed back to only handle prev/next_hltc len = 1 events again. LDK doesn't forward trampoline payments so we can handle these once we do

tnull · 2026-07-21T08:44:48Z

+			let channel_pair_stats_store = Arc::clone(&self.channel_pair_forwarding_stats_store);
+			let logger = Arc::clone(&self.logger);
+			self.runtime.spawn_cancellable_background_task(async move {
+				run_forwarded_payment_aggregation(


I'm not sure aggregating stats needs its own background task? I mean they are relatively cheap, but there's still some overhead. Do we think doing it on-demand would be very costly?

Or maybe, if we want to do it in the background, doing some prefactoring to have some of these tasks merged into a single 'maintenance task' could make sense (as we have a few by now).

I'd rather not do it on-demand, that always feels flaky and confusing to me, especially when handling potential storage errors.

I think a maintenance task could make sense as a follow up

tnull · 2026-07-21T08:46:32Z

+						}
+					}
+
+					for inbound_stats in inbound_stats_by_channel.into_values() {


Codex:

[P1] Forwarding accounting is not idempotent across event replay.
The handler performs additive channel-stat updates and creates detailed records with fresh random IDs before all later persistence—including the user event queue write—has succeeded. If a later write fails, ReplayEvent causes the same forward to be processed again, doubling counts,
amounts, and fees. LDK also explicitly permits duplicate PaymentForwarded events when the fee is unknown. See /home/tnull/worktrees/ldk-node/pr-772-latest-20260721/src/event.rs:1769, /home/tnull/worktrees/ldk-node/pr-772-latest-20260721/src/payment/store.rs:1419, and LDK’s event contract
(https://github.com/lightningdevkit/rust-lightning/blob/f680fd37d8bea69383e1b18730423f347e6a8341/lightning/src/events/mod.rs#L1489-L1514). A stable idempotency marker or atomic persistence strategy is needed.

True, though unclear how to tackle without an idempotency marker / event ID.

yeah i dont really know a way around this unless we add the payment hash to the forwarded event

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.

tnull

Needs a rebase. As mentioned below, I'm not sure if we can move forward with this before we changed the upstream APIs. If we need it right away in LDK server, we probably need to start with a simplified version that doesn't lean on the very brittle re-derivation of channel pairs, for which we simply don't have sufficient information available.

Also here are some comments from Claude:

Findings

Bugs / must-fix

Leftover doc comment, src/io/mod.rs:50 — /// The immutable forwarded-payment bucket size will be persisted under this key. sits stacked on top of the NODE_METRICS doc comment. It's a remnant of the removed bucket-size persistence and should be deleted.

Event-replay double counting (known, still unresolved) — the handler at src/event.rs:1736–1830 persists inbound stats, then outbound stats, then detail records, then enqueues the user event; any later failure returns ReplayEvent, and the replay re-applies the already-persisted increments
(and inserts duplicate detail records under fresh random IDs). The PR discussion acknowledges this. Counters can't be made idempotent without an LDK-side event ID, but the detail records could be: derive the ID deterministically from event contents (e.g., hash of HTLC set + amounts + fees)
so replays overwrite rather than duplicate. Worth at least a doc caveat that stats are at-least-once.

Design concerns

Channel-pair buckets grow without bound — detail records are pruned, but the per-(pair, hour) ChannelPairForwardingStats buckets are never compacted or expired, and all of them are loaded into memory at every startup (read_all_objects in builder.rs) and held in the DataStore HashMap
forever. An LSP forwarding across many pairs accrues up to ~8,760 buckets per active pair per year. This partially defeats the bounded-storage goal that motivated the aggregation design. Suggest a coarser second-tier compaction (daily/weekly) or a documented retention policy.

Pagination reads disk while everything is already in memory — DataStore::list_page (src/data_store.rs) does a paginated KVStore list plus one KVStore read per key, yet the same objects all sit in the in-memory map. For VSS that's a network round-trip per record for data already in RAM.
Either serve pages from memory, or (better long-term, given point 3) stop retaining these datasets fully in memory — but the current halfway state has the costs of both.

Startup blocks on aggregation, src/lib.rs:362 — start() runs aggregate_expired_forwarded_payments synchronously via block_on, and each detail removal is an individually awaited KVStore op. A Detailed-mode node that was offline for a while, on a remote store, pays that as startup latency.
Since the periodic task exists anyway, consider making it run one pass immediately when spawned instead of only at the next bucket boundary, and dropping the blocking pass from start().

Semantics / documentation

Fee attribution is asymmetric and undocumented — fees are credited to the inbound channel's ChannelForwardingStats only (src/event.rs:1758–1766), while outbound entries hardcode total_fee_earned_msat: Some(0) even when the event's fee is unknown (inbound uses total_fee_earned_msat.map(|_|

and correctly poisons to None). Defensible as an attribution convention, but the field doc ("Total forwarding fees earned through this channel") doesn't say it, and users summing outbound channels will conclude those channels never earn fees. Similarly, onchain_claims_count goes once to
the first outbound channel while the detail/pair records set claim_from_onchain_tx on every pair of the forward — the two stores won't reconcile. Both conventions should be stated on the field docs.

Clock-skew edge case — timestamps come from SystemTime::now(); a backwards NTP step can insert a detail record into a bucket already committed via the contains_key marker (src/payment/store.rs:1646), and the next pass silently deletes it without counting it. Low severity, but worth a
comment. Also inconsistent: event.rs uses .expect(...) on duration_since(UNIX_EPOCH) while store.rs uses unwrap_or(0).

list_channel_pair_stats_in_range can scan the whole store — keys sort by channel pair, not time, so a sparse time range walks every page (each a blocking KVStore round-trip) before returning. The per-call behavior is documented, but callers on the FFI thread may be surprised; worth a doc
note about cost.

tnull · 2026-07-27T12:33:17Z

+		}]);
+	}
+
+	// LDK reports the incoming and outgoing HTLCs, but not which incoming HTLC funded each


To be honest together with #772 (comment) this seems like a blocker for this PR. If the upstream API doesn't provide sufficient data for us to do the level of detailed aggregation we want to do, and also doesn't allow us to aggregate safely without overcounting on restarts, we probably either a) start with a version that exposes fewer details or b) first change the upstream API altogether so it provides all the data we need to do this correctly and safely.

tnull · 2026-08-19T06:18:56Z

Please rebase now that #1024 landed.

tnull

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

tnull · 2026-08-19T12:47:29Z

 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.


Shouldn't all forwarded-payment-store related objects be stored under the same primary namespace, and be discerned by secondary?

tnull · 2026-08-19T12:51:24Z

+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> {


I don't understand why we need this? The idea of DataStore being generic is exactly to avoid adding specialized stores for every instance?

Removed, goal was to not cache all these and just write to store

tnull · 2026-08-19T13:00:01Z

+						}
+					}
+
+					for inbound_stats in inbound_stats_by_channel.into_values() {


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.

benthecarman · 2026-08-21T02:33:10Z

Addressed most review comments, however, can hold off until https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4912 is merged

tnull

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?

tnull · 2026-08-21T09:21:09Z

+	/// Returns whether this store contains no objects.
+	pub(crate) async fn is_empty(&self) -> Result<bool, Error> {
+		let mut page_token = None;
+		loop {


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?

tnull · 2026-08-21T09:30:16Z

+	}
+
+	/// 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>(


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.

tnull · 2026-08-21T10:37:37Z

Needs a rebase now.

Use the latest LDK 0.3 revision so forwarding events include stable HTLC identifiers. Update the payment-instructions pin to keep its LDK types compatible. This change was made with assistance from OpenAI Codex.

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

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

benthecarman requested a review from tnull January 27, 2026 17:33

benthecarman force-pushed the save-fwd-payment branch from 1feb7ae to ba15c40 Compare January 27, 2026 17:40

tnull reviewed Jan 28, 2026

View reviewed changes

Comment thread src/io/utils.rs Outdated

Comment thread src/io/utils.rs Outdated

Comment thread src/payment/store.rs Outdated

benthecarman force-pushed the save-fwd-payment branch from ba15c40 to 20877bd Compare January 28, 2026 14:53

tnull reviewed Jan 29, 2026

View reviewed changes

Comment thread src/lib.rs Outdated

benthecarman self-assigned this Jan 29, 2026

benthecarman added this to Weekly Goals Jan 29, 2026

benthecarman force-pushed the save-fwd-payment branch 3 times, most recently from 970a395 to c3f7714 Compare February 4, 2026 21:48

TheBlueMatt reviewed Feb 4, 2026

View reviewed changes

benthecarman force-pushed the save-fwd-payment branch 2 times, most recently from 19c5b89 to cbb235e Compare February 6, 2026 01:26

benthecarman force-pushed the save-fwd-payment branch from cbb235e to ad559cc Compare February 6, 2026 01:39

benthecarman mentioned this pull request Feb 6, 2026

Beta Release lightningdevkit/ldk-server#121

Open

25 tasks

benthecarman mentioned this pull request Feb 23, 2026

Update LDK dependency for splicing, http, and wallet_utils #794

Merged

benthecarman mentioned this pull request Mar 20, 2026

Add timestamp to ForwardedPayment lightningdevkit/ldk-server#162

Open

tnull added this to the 0.8 milestone Apr 20, 2026

benthecarman force-pushed the save-fwd-payment branch from ad559cc to d8a4671 Compare July 1, 2026 00:20

benthecarman force-pushed the save-fwd-payment branch from d8a4671 to a831210 Compare July 1, 2026 01:47

tnull self-requested a review July 14, 2026 10:11

benthecarman force-pushed the save-fwd-payment branch from 81249d0 to 6319183 Compare July 16, 2026 18:15

tnull reviewed Jul 21, 2026

View reviewed changes

benthecarman force-pushed the save-fwd-payment branch from 6319183 to 4a2b945 Compare July 22, 2026 01:33

benthecarman requested a review from tnull July 27, 2026 01:31

tnull reviewed Jul 27, 2026

View reviewed changes

benthecarman force-pushed the save-fwd-payment branch 2 times, most recently from 12ba5f9 to f031041 Compare July 27, 2026 23:15

benthecarman mentioned this pull request Jul 28, 2026

Expose Postgres storage backend lightningdevkit/ldk-server#243

Open

tnull mentioned this pull request Jul 28, 2026

Add paginated payment listing API #959

Closed

benthecarman force-pushed the save-fwd-payment branch from f031041 to 975082c Compare August 19, 2026 09:51

benthecarman requested a review from tnull August 19, 2026 09:51

tnull reviewed Aug 19, 2026

View reviewed changes

benthecarman force-pushed the save-fwd-payment branch from 975082c to 64444a0 Compare August 21, 2026 02:31

tnull reviewed Aug 21, 2026

View reviewed changes

Comment thread src/builder.rs Outdated

Comment thread src/event.rs Outdated

benthecarman force-pushed the save-fwd-payment branch from 64444a0 to 765a62c Compare August 21, 2026 09:07

benthecarman requested a review from tnull August 21, 2026 09:11

tnull reviewed Aug 21, 2026

View reviewed changes

benthecarman force-pushed the save-fwd-payment branch from 765a62c to 35b2212 Compare August 21, 2026 11:15

benthecarman added 3 commits August 21, 2026 15:17

Bump LDK 0.3 dependencies

1176850

Use the latest LDK 0.3 revision so forwarding events include stable HTLC identifiers. Update the payment-instructions pin to keep its LDK types compatible. This change was made with assistance from OpenAI Codex.

Add uncached data store policy

767cbcc

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

benthecarman force-pushed the save-fwd-payment branch from 35b2212 to 449c831 Compare August 21, 2026 20:17

benthecarman force-pushed the save-fwd-payment branch from 449c831 to 2397afd Compare August 21, 2026 21:01

Uh oh!

Conversation

benthecarman commented Jan 27, 2026

Uh oh!

ldk-reviews-bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benthecarman commented Jan 28, 2026

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Feb 5, 2026

Uh oh!

benthecarman commented Feb 5, 2026

Uh oh!

TheBlueMatt commented Feb 5, 2026

Uh oh!

TheBlueMatt commented Feb 5, 2026

Uh oh!

benthecarman commented Feb 6, 2026

Uh oh!

tnull commented Jun 11, 2026

Uh oh!

benthecarman commented Jul 1, 2026

Uh oh!

tnull commented Jul 1, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Aug 19, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ldk-reviews-bot commented Jan 27, 2026 •

edited

Loading

tnull Jul 21, 2026 •

edited

Loading

tnull Aug 21, 2026 •

edited

Loading