Reclaim forwarded-payment replay markers instead of leaking them

[ajaysehwal]

Summary

ForwardingStore::record_forward writes a permanent ForwardedPaymentReplayMarker for every forwarded HTLC, used to guard against LDK replaying an event whose side effects we already recorded. Nothing ever removed these markers -- the existing aggregation pass reclaims detail records but never touches the marker store, so it grows without bound for the life of the node.

This is worse in the default Stats tracking mode: no detail record is ever written there, so aggregation has nothing to key cleanup off. Worse still, run_forwarded_payment_aggregation's background loop exits for good the first time it observes an empty details store under Stats mode -- which is immediately, since that mode never populates one -- so the reclamation task stops running entirely after its first pass on a routing node's very first startup, in the default config.

Any peer with a channel to the node can trigger growth for the cost of their own routing fee (circular routing refunds it to the attacker). The write also happens synchronously inside the LDK event handler, so on a remote KV backend the accumulating read cost adds to the same hot path already shown to head-of-line block the event queue.

Fix

• Add a forwarded_at_timestamp field to the marker (TLV-optional, defaults to 0 on read, so pre-existing leaked markers are swept on the first pass after upgrading).
• Add prune_expired_replay_markers, run every aggregation cycle independent of retention_secs, so Stats mode reclaims markers too. A marker is only removed once it's past a fixed one-bucket-wide age cutoff and has no corresponding detail record left --- age alone isn't sufficient: the aggregation pass defers an entire bucket (every sibling detail in it, not just the record missing a marker) whenever any one detail in that bucket is still missing its own marker. An age-only version of this fix passed every existing test but silently corrupted exactly that scenario; caught it with a dedicated regression test before landing this version.
• Skip the per-marker detail lookup when the details store is empty (one read instead of one per marker) -- the common case in Stats. mode, where a detail can never exist to check for.
• Fix the background loop's early-exit check to also require the marker store to be empty, not just the details store, so it doesn't stop reclaiming markers while details happen to be empty.

Testing

Six new tests, covering: markers leaking across both tracking modes, the exact bucket-width age cutoff, the sibling-bucket corruption case above, and the background loop no longer exiting while markers remain.

For both substantive changes (the sibling-bucket guard, and the details-empty fast path), I also manually reverted just that piece, confirmed the corresponding test fails with the predicted symptom, and then restored it and confirmed green -- not just written, verified to actually catch the regression it's meant to catch.

cargo fmt --all -- --check, cargo clippy --lib -- -D clippy::unwrap_used (the repo's CI lint), and the full cargo test --lib suite (200/200) all pass.

[ldk-reviews-bot]

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

[benthecarman]

Thanks! Approach looks right, two structural things claude and I discussed on how to do this best:

1. Don't prune on the startup. The aggregation task runs before the background processor replays any events that were handled but not persisted before a crash, so after a long outage this deletes the markers right before they're needed. Waiting one bucket width of uptime before the first prune is enough.

2. In Detailed mode this does a contains_key on the details store for every marker older than an hour, every hour, which is nearly all of them for the whole retention period. Remove the marker in the aggregation pass right after it removes the detail instead. The sweep then only has to handle Stats mode and crash orphans, and the per-marker lookup can stay as is since it rarely fires.

Nits: the doc comment on run_forwarded_payment_aggregation describes a version that never existed on main. Forwarding tracking hasn't shipped yet, so forwarded_at_timestamp can just be required rather than defaulting to 0 for old markers.

[ajaysehwal]

Thanks! Approach looks right, two structural things claude and I discussed on how to do this best:

1. Don't prune on the startup. The aggregation task runs before the background processor replays any events that were handled but not persisted before a crash, so after a long outage this deletes the markers right before they're needed. Waiting one bucket width of uptime before the first prune is enough.
2. In Detailed mode this does a contains_key on the details store for every marker older than an hour, every hour, which is nearly all of them for the whole retention period. Remove the marker in the aggregation pass right after it removes the detail instead. The sweep then only has to handle Stats mode and crash orphans, and the per-marker lookup can stay as is since it rarely fires.

Nits: the doc comment on run_forwarded_payment_aggregation describes a version that never existed on main. Forwarding tracking hasn't shipped yet, so forwarded_at_timestamp can just be required rather than defaulting to 0 for old markers.

Thanks for the review — both fixed in the latest commit: eb251c4

• Don't prune on startup: the sweep now waits one bucket width of uptime before running, so it can't delete a marker a crash-replay still needs. Aggregation still runs immediately, since it only removes a marker alongside a detail it just confirmed is gone.
• Pair marker removal with detail removal: aggregation now removes the marker right after its detail in the same pass (detail first, so a failure between the two leaves a harmless orphan the sweep can catch, not a dangerous one).

Also fixed the stale doc comment and made forwarded_at_timestamp required.

Separately, juliusjulyp caught that the old early-exit logic could stop the loop forever on an idle/fresh node. Removed that logic entirely — the loop now just runs for the node's lifetime, which is negligible cost when idle.

Covered by updated/new tests. Let me know if anything still looks off.

[benthecarman]

Thanks, close now.

One major thing from claude, run_forwarded_payment_aggregation needs to cover the paired removal too. In Detailed mode, the startup aggregation pass after a long outage deletes both the detail and its marker, and the background processor then replays the unpersisted events against an empty marker store. The doc comment says this path carries no such risk, but it does. Skipping marker removal on the startup pass (leave them for the sweep) would fix it.

[ajaysehwal]

@benthecarman All three are fixed now:

• Paired removal on startup — the startup aggregation pass now runs with marker removal skipped entirely; it still aggregates and removes details normally, but leaves every marker for the delayed, age-gated sweep to reclaim once it's actually safe. Added a Detailed-mode regression test (startup_pass_does_not_remove_a_paired_marker_in_detailed_mode) that reproduces the exact scenario you described — a detail+marker pair old enough to aggregate on the very first startup pass — and confirmed it fails without the fix and passes with it.
• Shortened the comment — used your wording verbatim.
• Dropped the pub(crate) — you're right, prune_stale_replay_markers is only ever called from prune_stale_replay_markers_and_log in the same file, so it's just a private method now.

While I was in there, I also swapped the remove_markers: bool parameter threaded through the aggregation functions for a small MarkerRemoval::{Retain, Remove} enum, since the bare true/false at call sites (especially in tests) wasn't self-explanatory.