Skip to content

Correctly handle lost MonitorEvents #3984

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

Merged
merged 6 commits into from
Sep 18, 2025
Merged

Correctly handle lost MonitorEvents #3984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c49ce57
Add a new `ChannelMoniorUpdateStep::ReleasePaymentComplete`
TheBlueMatt Jul 30, 2025
8b637cc
Prepare to provide new `ReleasePaymentComplete` monitor updates
TheBlueMatt Aug 2, 2025
71a364c
Generate new `ReleasePaymentComplete` monitor updates
TheBlueMatt Aug 2, 2025
226520b
Stop re-hydrating pending payments once they are fully resolved
TheBlueMatt Aug 2, 2025
ba6528f
Properly provide `PaymentPathSuccessful` event for replay claims
TheBlueMatt Aug 1, 2025
77026c9
Remove redundant `startup_replay` arg to `claim_funds_internal`
TheBlueMatt Sep 15, 2025
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
150 changes: 51 additions & 99 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,20 @@ pub(crate) enum ChannelMonitorUpdateStep {
RenegotiatedFundingLocked {
funding_txid: Txid,
},
/// When a payment is finally resolved by the user handling an [`Event::PaymentSent`] or
/// [`Event::PaymentFailed`] event, the `ChannelManager` no longer needs to hear about it on
/// startup (which would cause it to re-hydrate the payment information even though the user
/// already learned about the payment's result).
///
/// This will remove the HTLC from [`ChannelMonitor::get_all_current_outbound_htlcs`] and
/// [`ChannelMonitor::get_onchain_failed_outbound_htlcs`].
///
/// Note that this is only generated for closed channels as this is implicit in the
/// [`Self::CommitmentSecret`] update which clears the payment information from all un-revoked
/// counterparty commitment transactions.
ReleasePaymentComplete {
htlc: SentHTLCId,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/htlc/htlc_id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean isn't it clear from the type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not as clear as it could be at the callsites, IMO. Since this field isn't actually an HTLC. Not a big deal though.

},
}

impl ChannelMonitorUpdateStep {
Expand All @@ -709,6 +723,7 @@ impl ChannelMonitorUpdateStep {
ChannelMonitorUpdateStep::ShutdownScript { .. } => "ShutdownScript",
ChannelMonitorUpdateStep::RenegotiatedFunding { .. } => "RenegotiatedFunding",
ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } => "RenegotiatedFundingLocked",
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete",
}
}
}
Expand Down Expand Up @@ -747,6 +762,9 @@ impl_writeable_tlv_based_enum_upgradable!(ChannelMonitorUpdateStep,
(1, commitment_txs, required_vec),
(3, htlc_data, required),
},
(7, ReleasePaymentComplete) => {
(1, htlc, required),
},
(8, LatestHolderCommitment) => {
(1, commitment_txs, required_vec),
(3, htlc_data, required),
Expand Down Expand Up @@ -1250,6 +1268,14 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
/// spending CSV for revocable outputs).
htlcs_resolved_on_chain: Vec<IrrevocablyResolvedHTLC>,

/// When a payment is resolved through an on-chain transaction, we tell the `ChannelManager`
/// about this via [`ChannelMonitor::get_onchain_failed_outbound_htlcs`] and
/// [`ChannelMonitor::get_all_current_outbound_htlcs`] at startup. We'll keep repeating the
/// same payments until they're eventually fully resolved by the user processing a
/// `PaymentSent` or `PaymentFailed` event, at which point the `ChannelManager` will inform of
/// this and we'll store the set of fully resolved payments here.
htlcs_resolved_to_user: HashSet<SentHTLCId>,

/// 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 @@ -1654,6 +1680,7 @@ pub(crate) fn write_chanmon_internal<Signer: EcdsaChannelSigner, W: Writer>(
(29, channel_monitor.initial_counterparty_commitment_tx, option),
(31, channel_monitor.funding.channel_parameters, required),
(32, channel_monitor.pending_funding, optional_vec),
(33, channel_monitor.htlcs_resolved_to_user, required),
(34, channel_monitor.alternative_funding_confirmed, option),
});

Expand Down Expand Up @@ -1872,6 +1899,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
funding_spend_confirmed: None,
confirmed_commitment_tx_counterparty_output: None,
htlcs_resolved_on_chain: Vec::new(),
htlcs_resolved_to_user: new_hash_set(),
spendable_txids_confirmed: Vec::new(),

best_block,
Expand Down Expand Up @@ -3003,10 +3031,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
/// Gets the set of outbound HTLCs which can be (or have been) resolved by this
/// `ChannelMonitor`. This is used to determine if an HTLC was removed from the channel prior
/// to the `ChannelManager` having been persisted.
///
/// This is similar to [`Self::get_pending_or_resolved_outbound_htlcs`] except it includes
/// HTLCs which were resolved on-chain (i.e. where the final HTLC resolution was done by an
/// event from this `ChannelMonitor`).
pub(crate) fn get_all_current_outbound_htlcs(
&self,
) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
Expand All @@ -3019,8 +3043,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
for &(ref htlc, ref source_option) in latest_outpoints.iter() {
if let &Some(ref source) = source_option {
let htlc_id = SentHTLCId::from_source(source);
let preimage_opt = us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned();
res.insert((**source).clone(), (htlc.clone(), preimage_opt));
if !us.htlcs_resolved_to_user.contains(&htlc_id) {
let preimage_opt =
us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned();
res.insert((**source).clone(), (htlc.clone(), preimage_opt));
}
}
}
}
Expand Down Expand Up @@ -3076,6 +3103,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
} else {
continue;
};
let htlc_id = SentHTLCId::from_source(source);
if us.htlcs_resolved_to_user.contains(&htlc_id) {
continue;
}

let confirmed = $htlc_iter.find(|(_, conf_src)| Some(source) == *conf_src);
if let Some((confirmed_htlc, _)) = confirmed {
let filter = |v: &&IrrevocablyResolvedHTLC| {
Expand Down Expand Up @@ -3148,96 +3180,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
res
}

/// Gets the set of outbound HTLCs which are pending resolution in this channel or which were
/// resolved with a preimage from our counterparty.
///
/// This is used to reconstruct pending outbound payments on restart in the ChannelManager.
///
/// Currently, the preimage is unused, however if it is present in the relevant internal state
/// an HTLC is always included even if it has been resolved.
#[rustfmt::skip]
pub(crate) fn get_pending_or_resolved_outbound_htlcs(&self) -> HashMap<HTLCSource, (HTLCOutputInCommitment, Option<PaymentPreimage>)> {
let us = self.inner.lock().unwrap();
// We're only concerned with the confirmation count of HTLC transactions, and don't
// actually care how many confirmations a commitment transaction may or may not have. Thus,
// we look for either a FundingSpendConfirmation event or a funding_spend_confirmed.
let confirmed_txid = us.funding_spend_confirmed.or_else(|| {
us.onchain_events_awaiting_threshold_conf.iter().find_map(|event| {
if let OnchainEvent::FundingSpendConfirmation { .. } = event.event {
Some(event.txid)
} else { None }
})
});

if confirmed_txid.is_none() {
// If we have not seen a commitment transaction on-chain (ie the channel is not yet
// closed), just get the full set.
mem::drop(us);
return self.get_all_current_outbound_htlcs();
}

let mut res = new_hash_map();
macro_rules! walk_htlcs {
($holder_commitment: expr, $htlc_iter: expr) => {
for (htlc, source) in $htlc_iter {
if us.htlcs_resolved_on_chain.iter().any(|v| v.commitment_tx_output_idx == htlc.transaction_output_index) {
// We should assert that funding_spend_confirmed is_some() here, but we
// have some unit tests which violate HTLC transaction CSVs entirely and
// would fail.
// TODO: Once tests all connect transactions at consensus-valid times, we
// should assert here like we do in `get_claimable_balances`.
} else if htlc.offered == $holder_commitment {
// If the payment was outbound, check if there's an HTLCUpdate
// indicating we have spent this HTLC with a timeout, claiming it back
// and awaiting confirmations on it.
let htlc_update_confd = us.onchain_events_awaiting_threshold_conf.iter().any(|event| {
if let OnchainEvent::HTLCUpdate { commitment_tx_output_idx: Some(commitment_tx_output_idx), .. } = event.event {
// If the HTLC was timed out, we wait for ANTI_REORG_DELAY blocks
// before considering it "no longer pending" - this matches when we
// provide the ChannelManager an HTLC failure event.
Some(commitment_tx_output_idx) == htlc.transaction_output_index &&
us.best_block.height >= event.height + ANTI_REORG_DELAY - 1
} else if let OnchainEvent::HTLCSpendConfirmation { commitment_tx_output_idx, .. } = event.event {
// If the HTLC was fulfilled with a preimage, we consider the HTLC
// immediately non-pending, matching when we provide ChannelManager
// the preimage.
Some(commitment_tx_output_idx) == htlc.transaction_output_index
} else { false }
});
if let Some(source) = source {
let counterparty_resolved_preimage_opt =
us.counterparty_fulfilled_htlcs.get(&SentHTLCId::from_source(source)).cloned();
if !htlc_update_confd || counterparty_resolved_preimage_opt.is_some() {
res.insert(source.clone(), (htlc.clone(), counterparty_resolved_preimage_opt));
}
} else {
panic!("Outbound HTLCs should have a source");
}
}
}
}
}

let commitment_txid = confirmed_txid.unwrap();
let funding_spent = get_confirmed_funding_scope!(us);

if Some(commitment_txid) == funding_spent.current_counterparty_commitment_txid || Some(commitment_txid) == funding_spent.prev_counterparty_commitment_txid {
walk_htlcs!(false, funding_spent.counterparty_claimable_outpoints.get(&commitment_txid).unwrap().iter().filter_map(|(a, b)| {
if let &Some(ref source) = b {
Some((a, Some(&**source)))
} else { None }
}));
} else if commitment_txid == funding_spent.current_holder_commitment_tx.trust().txid() {
walk_htlcs!(true, holder_commitment_htlcs!(us, CURRENT_WITH_SOURCES));
} else if let Some(prev_commitment_tx) = &funding_spent.prev_holder_commitment_tx {
if commitment_txid == prev_commitment_tx.trust().txid() {
walk_htlcs!(true, holder_commitment_htlcs!(us, PREV_WITH_SOURCES).unwrap());
}
}

res
}

pub(crate) fn get_stored_preimages(
&self,
) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
Expand Down Expand Up @@ -4091,6 +4033,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID || self.lockdown_from_offchain {
assert_eq!(updates.updates.len(), 1);
match updates.updates[0] {
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
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 @@ -4218,6 +4161,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
panic!("Attempted to replace shutdown script {} with {}", shutdown_script, scriptpubkey);
}
},
ChannelMonitorUpdateStep::ReleasePaymentComplete { htlc } => {
log_trace!(logger, "HTLC {htlc:?} permanently and fully resolved");
self.htlcs_resolved_to_user.insert(*htlc);
},
}
}

Expand All @@ -4243,11 +4190,13 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
|ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } =>
is_pre_close_update = true,
// After a channel is closed, we don't communicate with our peer about it, so the
// only things we will update is getting a new preimage (from a different channel)
// or being told that the channel is closed. All other updates are generated while
// talking to our peer.
// only things we will update is getting a new preimage (from a different channel),
// being told that the channel is closed, or being told a payment which was
// resolved on-chain has had its resolution communicated to the user. All other
// updates are generated while talking to our peer.
ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
}
}

Expand Down Expand Up @@ -6324,6 +6273,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 funding_spend_seen = Some(false);
let mut counterparty_node_id = None;
let mut confirmed_commitment_tx_counterparty_output = None;
Expand Down Expand Up @@ -6357,6 +6307,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
(29, initial_counterparty_commitment_tx, option),
(31, channel_parameters, (option: ReadableArgs, None)),
(32, pending_funding, optional_vec),
(33, htlcs_resolved_to_user, option),
(34, alternative_funding_confirmed, option),
});
if let Some(payment_preimages_with_info) = payment_preimages_with_info {
Expand Down Expand Up @@ -6516,6 +6467,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP
funding_spend_confirmed,
confirmed_commitment_tx_counterparty_output,
htlcs_resolved_on_chain: htlcs_resolved_on_chain.unwrap(),
htlcs_resolved_to_user: htlcs_resolved_to_user.unwrap(),
spendable_txids_confirmed: spendable_txids_confirmed.unwrap(),

best_block,
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3870,6 +3870,7 @@ fn do_test_durable_preimages_on_closed_channel(
};
nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &node_b_id, err_msg).unwrap();
check_closed_event(&nodes[0], 1, reason, false, &[node_b_id], 100000);
check_added_monitors(&nodes[0], 1);
let as_closing_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
assert_eq!(as_closing_tx.len(), 1);

Expand Down
Loading
Loading