Skip to content

Prevent redundant PaymentClaimed events for closed channels #4467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
valentinewallace wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Prevent redundant PaymentClaimed events for closed channels #4467

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae6edc5
Helper for closed channel post-event monitor updates
valentinewallace Mar 5, 2026
a4b5c7e
Add ChannelMonitorUpdateStep::InboundPaymentClaimed
valentinewallace Mar 6, 2026
e744298
ChannelMonitor: track user-processed payment claims
valentinewallace Mar 6, 2026
f761051
Add EventCompletionAction::InboundPaymentClaimedChannelMonitorUpdate
valentinewallace Mar 6, 2026
8271ebd
Prevent redundant PaymentClaimeds for closed channels
valentinewallace Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,17 @@ pub(crate) enum ChannelMonitorUpdateStep {
ReleasePaymentComplete {
htlc: SentHTLCId,
},
/// When an [`Event::PaymentClaimed`] is processed by the user, we need to track that so we don't
/// keep regenerating the event redundantly on startup.
///
/// This will remove the HTLC from [`ChannelMonitor::get_stored_preimages`].
///
/// Note that this is only generated for closed channels -- if the channel is open, the inbound
/// payment is pruned automatically when the HTLC is no longer present in any unrevoked
/// commitment transaction.
InboundPaymentClaimed {
payment_hash: PaymentHash,
},
}

impl ChannelMonitorUpdateStep {
Expand All @@ -723,6 +734,7 @@ impl ChannelMonitorUpdateStep {
ChannelMonitorUpdateStep::RenegotiatedFunding { .. } => "RenegotiatedFunding",
ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } => "RenegotiatedFundingLocked",
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete",
ChannelMonitorUpdateStep::InboundPaymentClaimed { .. } => "InboundPaymentClaimed",
}
}
}
Expand Down Expand Up @@ -769,6 +781,9 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
(3, htlc_data, required),
(5, claimed_htlcs, required_vec),
},
(9, InboundPaymentClaimed) => {
(1, payment_hash, required),
},
(10, RenegotiatedFunding) => {
(1, channel_parameters, (required: ReadableArgs, None)),
(3, holder_commitment_tx, required),
Expand Down Expand Up @@ -1342,6 +1357,10 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
/// this and we'll store the set of fully resolved payments here.
htlcs_resolved_to_user: HashSet<SentHTLCId>,

/// The set of inbound payments for which the user has processed an [`Event::PaymentClaimed`].
/// This is used to avoid regenerating the event redundantly on restart for closed channels.
inbound_payments_claimed: HashSet<PaymentHash>,

/// The set of `SpendableOutput` events which we have already passed upstream to be claimed.
/// These are tracked explicitly to ensure that we don't generate the same events redundantly
/// if users duplicatively confirm old transactions. Specifically for transactions claiming a
Expand Down Expand Up @@ -1755,6 +1774,7 @@ pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
(34, channel_monitor.alternative_funding_confirmed, option),
(35, channel_monitor.is_manual_broadcast, required),
(37, channel_monitor.funding_seen_onchain, required),
(39, channel_monitor.inbound_payments_claimed, required),
});

Ok(())
Expand Down Expand Up @@ -1954,6 +1974,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
confirmed_commitment_tx_counterparty_output: None,
htlcs_resolved_on_chain: Vec::new(),
htlcs_resolved_to_user: new_hash_set(),
inbound_payments_claimed: new_hash_set(),
spendable_txids_confirmed: Vec::new(),

best_block,
Expand Down Expand Up @@ -4150,6 +4171,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
assert_eq!(updates.updates.len(), 1);
match updates.updates[0] {
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
ChannelMonitorUpdateStep::InboundPaymentClaimed { .. } => {},
ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
// We should have already seen a `ChannelForceClosed` update if we're trying to
// provide a preimage at this point.
Expand Down Expand Up @@ -4281,6 +4303,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
log_trace!(logger, "HTLC {htlc:?} permanently and fully resolved");
self.htlcs_resolved_to_user.insert(*htlc);
},
ChannelMonitorUpdateStep::InboundPaymentClaimed { payment_hash } => {
},
}
}

Expand Down Expand Up @@ -4313,6 +4337,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
ChannelMonitorUpdateStep::InboundPaymentClaimed { .. } => {},
}
}

Expand Down Expand Up @@ -6504,6 +6529,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
let mut funding_spend_confirmed = None;
let mut htlcs_resolved_on_chain = Some(Vec::new());
let mut htlcs_resolved_to_user = Some(new_hash_set());
let mut inbound_payments_claimed = Some(new_hash_set());
let mut funding_spend_seen = Some(false);
let mut counterparty_node_id = None;
let mut confirmed_commitment_tx_counterparty_output = None;
Expand Down Expand Up @@ -6543,6 +6569,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
(34, alternative_funding_confirmed, option),
(35, is_manual_broadcast, (default_value, false)),
(37, funding_seen_onchain, (default_value, true)),
(39, inbound_payments_claimed, option),
Copy link
Copy Markdown
Contributor

@carlaKC carlaKC Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that write as required but read as option is intentional to be forwards compatible for old monitors?

});
// Note that `payment_preimages_with_info` was added (and is always written) in LDK 0.1, so
// we can use it to determine if this monitor was last written by LDK 0.1 or later.
Expand Down Expand Up @@ -6708,6 +6735,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
confirmed_commitment_tx_counterparty_output,
htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
htlcs_resolved_to_user: htlcs_resolved_to_user.unwrap(),
inbound_payments_claimed: inbound_payments_claimed.unwrap(),
spendable_txids_confirmed: spendable_txids_confirmed.unwrap(),

best_block,
Expand Down
102 changes: 57 additions & 45 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14809,56 +14809,68 @@ impl<
htlc_id,
},
) => {
let per_peer_state = self.per_peer_state.read().unwrap();
let mut peer_state_lock = per_peer_state
.get(&counterparty_node_id)
.map(|state| state.lock().unwrap())
.expect("Channels originating a payment resolution must have peer state");
let peer_state = &mut *peer_state_lock;
let update_id = peer_state
.closed_channel_monitor_update_ids
.get_mut(&channel_id)
.expect("Channels originating a payment resolution must have a monitor");
// Note that for channels closed pre-0.1, the latest update_id is `u64::MAX`.
*update_id = update_id.saturating_add(1);

let update = ChannelMonitorUpdate {
update_id: *update_id,
channel_id: Some(channel_id),
updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete {
htlc: htlc_id,
}],
};

let during_startup =
!self.background_events_processed_since_startup.load(Ordering::Acquire);
if during_startup {
let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
counterparty_node_id,
funding_txo: channel_funding_outpoint,
channel_id,
update,
};
self.pending_background_events.lock().unwrap().push(event);
} else {
if let Some(actions) = self.handle_post_close_monitor_update(
&mut peer_state.in_flight_monitor_updates,
&mut peer_state.monitor_update_blocked_actions,
channel_funding_outpoint,
update,
counterparty_node_id,
channel_id,
) {
mem::drop(peer_state_lock);
mem::drop(per_peer_state);
self.handle_monitor_update_completion_actions(actions);
}
}
let update_step =
ChannelMonitorUpdateStep::ReleasePaymentComplete { htlc: htlc_id };
self.handle_closed_channel_monitor_update_for_event(
counterparty_node_id,
channel_funding_outpoint,
channel_id,
update_step,
);
},
}
}
}

/// Helper for handling closed-channel monitor updates triggered by [`EventCompletionAction`]s.
fn handle_closed_channel_monitor_update_for_event(
&self, counterparty_node_id: PublicKey, funding_outpoint: OutPoint, channel_id: ChannelId,
update_step: ChannelMonitorUpdateStep,
) {
let per_peer_state = self.per_peer_state.read().unwrap();
let mut peer_state_lock = per_peer_state
.get(&counterparty_node_id)
.map(|state| state.lock().unwrap())
.expect("Channels originating a payment resolution must have peer state");
let peer_state = &mut *peer_state_lock;
let update_id = peer_state
.closed_channel_monitor_update_ids
.get_mut(&channel_id)
.expect("Channels originating a payment resolution must have a monitor");
// Note that for channels closed pre-0.1, the latest update_id is `u64::MAX`.
*update_id = update_id.saturating_add(1);

let update = ChannelMonitorUpdate {
update_id: *update_id,
channel_id: Some(channel_id),
updates: vec![update_step],
};

let during_startup =
!self.background_events_processed_since_startup.load(Ordering::Acquire);
if during_startup {
let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
counterparty_node_id,
funding_txo: funding_outpoint,
channel_id,
update,
};
self.pending_background_events.lock().unwrap().push(event);
} else {
if let Some(actions) = self.handle_post_close_monitor_update(
&mut peer_state.in_flight_monitor_updates,
&mut peer_state.monitor_update_blocked_actions,
funding_outpoint,
update,
counterparty_node_id,
channel_id,
) {
mem::drop(peer_state_lock);
mem::drop(per_peer_state);
self.handle_monitor_update_completion_actions(actions);
}
}
}
/// Processes any events asynchronously in the order they were generated since the last call
/// using the given event handler.
///
Expand Down