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

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

Correctly handle lost MonitorEvents #3984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fe043c
Add a new `ChannelMoniorUpdateStep::ReleasePaymentComplete`
TheBlueMatt Jul 30, 2025
3df23c2
Prepare to provide new `ReleasePaymentComplete` monitor updates
TheBlueMatt Aug 2, 2025
533562c
Generate new `ReleasePaymentComplete` monitor updates
TheBlueMatt Aug 2, 2025
a6cb31e
Stop re-hydrating pending payments once they are fully resolved
TheBlueMatt Aug 2, 2025
a791a4d
Properly provide `PaymentPathSuccessful` event for replay claims
TheBlueMatt Aug 1, 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
141 changes: 45 additions & 96 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

},
}

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,12 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> {
/// spending CSV for revocable outputs).
htlcs_resolved_on_chain: Vec<IrrevocablyResolvedHTLC>,

/// When a payment is fully resolved by the user processing a PaymentSent or PaymentFailed
/// event, we are informed by the ChannelManager (if the payment was resolved by an on-chain
/// transaction) of this so that we can stop telling the ChannelManager about the payment in
Comment on lines +1272 to +1273
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rephrase so there's not the big parenthetical in the middle?

/// the future. We 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 +1678,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 +1897,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 +3029,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 +3041,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 +3101,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 +3178,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 +4031,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 +4159,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 Down Expand Up @@ -4248,6 +4193,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
// talking to our peer.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is stale now

ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => {},
}
}

Expand Down Expand Up @@ -6324,6 +6270,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 +6304,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 +6464,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