Properly provide `PaymentPathSuccessful` event for replay claims
What changed, and why it matters
This change fixes a bookkeeping quirk in the Lightning Dev Kit. Previously, when a payment was completed via an on-chain claim discovered during startup, the software deliberately kept the payment record around forever and skipped sending a 'this payment path succeeded' event, to avoid repeatedly replaying the claim every time the node restarted. Now that the software can mark such claims as completed inside the channel monitor, it can safely send the missing success event and clean up the old payment state. This is a correctness and cleanup improvement, not a vulnerability that an attacker can exploit.
Treat as a normal bugfix/correctness patch. No urgent security action required. Users relying on PaymentPathSuccessful events for accounting or UX during on-chain claim replays at startup will now receive them correctly.
Security signals we found
Behavioral fix in payment state cleanup after on-chain HTLC claim replay
Previously pending payment could remain forever, now properly completed
No cryptographic, network, or trust-boundary weakness introduced
Test expectations updated to reflect new event emission
Evidence from the diff
The commit changes one argument in channelmanager.rs’s claim_htlc call from from_onchain = false to from_onchain = true for replayed on-chain HTLC claims discovered at startup. Previously the code intentionally left pending outbound payments in limbo forever to prevent regenerating PaymentPathSuccessful/PaymentSent events on every restart. With newer ChannelMonitor logic that marks claims completed and suppresses replay, the conservative workaround is no longer needed. Tests are updated to expect the additional PaymentPathSuccessful event and adjusted monitor counts.
Changed components
lightning/src/ln/channelmanager.rslightning/src/ln/monitor_tests.rslightning/src/ln/payment_tests.rsInspect captured patch +23 / −18
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 2bf2db5..d378f44 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -16714,21 +16714,13 @@ where
let mut compl_action = Some(
EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update)
);
- // Note that we set `from_onchain` to "false" here,
- // deliberately keeping the pending payment around forever.
- // Given it should only occur when we have a channel we're
- // force-closing for being stale that's okay.
- // The alternative would be to wipe the state when claiming,
- // generating a `PaymentPathSuccessful` event but regenerating
- // it and the `PaymentSent` on every restart until the
- // `ChannelMonitor` is removed.
pending_outbounds.claim_htlc(
payment_id,
preimage,
bolt12_invoice,
session_priv,
path,
- false,
+ true,
&mut compl_action,
&pending_events,
&&logger,
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index 03c9e2b..0a508f2 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -3450,12 +3450,15 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool) {
check_added_monitors(&nodes[1], 0);
let preimage_events = nodes[1].node.get_and_clear_pending_events();
- assert_eq!(preimage_events.len(), 2, "{preimage_events:?}");
+ assert_eq!(preimage_events.len(), 3, "{preimage_events:?}");
for ev in preimage_events {
match ev {
Event::PaymentSent { payment_hash, .. } => {
assert_eq!(payment_hash, hash_b);
},
+ Event::PaymentPathSuccessful { payment_hash, .. } => {
+ assert_eq!(payment_hash, Some(hash_b));
+ },
Event::PaymentForwarded { claim_from_onchain_tx, .. } => {
assert!(claim_from_onchain_tx);
},
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index f93eb3f..1a23179 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -1379,13 +1379,19 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(
} else {
expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
}
- // After reload, the ChannelManager identified the failed payment and queued up the
- // PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we
- // already did that) and corresponding ChannelMonitorUpdate to mark the payment
- // handled, but while processing the pending `MonitorEvent`s (which were not processed
- // before the monitor was persisted) we will end up with a duplicate
- // ChannelMonitorUpdate.
- check_added_monitors(&nodes[0], 2);
+ if persist_manager_post_event {
+ // After reload, the ChannelManager identified the failed payment and queued up the
+ // PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we
+ // already did that) and corresponding ChannelMonitorUpdate to mark the payment
+ // handled, but while processing the pending `MonitorEvent`s (which were not processed
+ // before the monitor was persisted) we will end up with a duplicate
+ // ChannelMonitorUpdate.
+ check_added_monitors(&nodes[0], 2);
+ } else {
+ // ...unless we got the PaymentSent event, in which case we have de-duplication logic
+ // preventing a redundant monitor event.
+ check_added_monitors(&nodes[0], 1);
+ }
}
// Note that if we re-connect the block which exposed nodes[0] to the payment preimage (but
@@ -4130,7 +4136,7 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
// pending payment from being re-hydrated on the next startup.
let events = nodes[0].node.get_and_clear_pending_events();
check_added_monitors(&nodes[0], 1);
- assert_eq!(events.len(), 2);
+ assert_eq!(events.len(), 3, "{events:?}");
if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] {
} else {
panic!();
@@ -4140,6 +4146,10 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
} else {
panic!();
}
+ if let Event::PaymentPathSuccessful { .. } = events[2] {
+ } else {
+ panic!();
+ }
// Note that we don't get a PaymentPathSuccessful here as we leave the HTLC pending to avoid
// the double-claim that would otherwise appear at the end of this test.
nodes[0].node.timer_tick_occurred();
Why this scored 21/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.