Reclaim forwarded-payment replay markers instead of leaking them - #1107
Open
ajaysehwal wants to merge 1 commit into
Open
ajaysehwal wants to merge 1 commit into
ajaysehwal wants to merge 1 commit into
Conversation
|
I've assigned @tnull as a reviewer! |
ajaysehwal
force-pushed
the
fix/forwarding-replay-marker-leak
branch
from
September 18, 2026 15:59
8b94cde to
1839828
Compare
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.
Summary
ForwardingStore::record_forwardwrites a permanentForwardedPaymentReplayMarkerfor 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
Statstracking 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 underStatsmode -- 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
forwarded_at_timestampfield to the marker (TLV-optional, defaults to0on read, so pre-existing leaked markers are swept on the first pass after upgrading).prune_expired_replay_markers, run every aggregation cycle independent ofretention_secs, soStatsmode 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.Stats.mode, where a detail can never exist to check for.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 fullcargo test --libsuite (200/200) all pass.