Merge pull request lightningdevkit#3984 from TheBlueMatt/2025-07-mon-event-failures

Correctly handle lost `MonitorEvent`s

Author: wpaulino

lightning/src/chain/channelmonitor.rs

@@ -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,
+	},
 }
 
 impl ChannelMonitorUpdateStep {
@@ -709,6 +723,7 @@ impl ChannelMonitorUpdateStep {
 			ChannelMonitorUpdateStep::ShutdownScript { .. } => "ShutdownScript",
 			ChannelMonitorUpdateStep::RenegotiatedFunding { .. } => "RenegotiatedFunding",
 			ChannelMonitorUpdateStep::RenegotiatedFundingLocked { .. } => "RenegotiatedFundingLocked",
+			ChannelMonitorUpdateStep::ReleasePaymentComplete { .. } => "ReleasePaymentComplete",
 		}
 	}
 }
@@ -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),
@@ -1288,6 +1306,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
@@ -1697,6 +1723,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),
 	});
 
@@ -1915,6 +1942,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,
@@ -3074,10 +3102,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>)> {
@@ -3090,8 +3114,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));
+						}
 					}
 				}
 			}
@@ -3147,6 +3174,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| {
@@ -3219,96 +3251,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>)> {
@@ -4195,6 +4137,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.
@@ -4322,6 +4265,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);
+				},
 			}
 		}
 
@@ -4347,11 +4294,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 { .. } => {},
 			}
 		}
 
@@ -6447,6 +6396,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;
@@ -6480,6 +6430,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),
 		});
 		// Note that `payment_preimages_with_info` was added (and is always written) in LDK 0.1, so
@@ -6642,6 +6593,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,

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -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);