Skip to content

Reclaim forwarded-payment replay markers instead of leaking them - #1107

Open
ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix/forwarding-replay-marker-leak
Open

ajaysehwal wants to merge 1 commit into
lightningdevkit:mainfrom
ajaysehwal:fix/forwarding-replay-marker-leak

Conversation

@ajaysehwal

Copy link
Copy Markdown

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

ldk-reviews-bot commented Sep 18, 2026

Copy link
Copy Markdown

I've assigned @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.

@ajaysehwal
ajaysehwal force-pushed the fix/forwarding-replay-marker-leak branch from 8b94cde to 1839828 Compare September 18, 2026 15:59
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.

2 participants