Persistent monitor events for HTLC forward fails

We are in the process of moving the resolution of all HTLCs from the Channel to
the ChannelMonitor, via persistent MonitorEvents. This will simplify how we
reconstruct/reconcile the ChannelManager's pending HTLC set on startup and
avoid needing to persist pending HTLCs explicitly in the manager, as we can
just replay pending monitor events to reconstruct the set instead.

In recent commits, we started generating persistent monitor events whenever an
HTLC is failed off-chain. We also made those monitor events the sole driver of
outbound payment failures, failing those HTLCs when the monitor event is
processed. This replaced the previous behavior of failing when we initially
receive update_fail_htlc from our peer.

In this commit, we continue that work by making persistent monitor events the
sole drivers of *forwarded* payment failures. When persistent_monitor_events is
enabled, we will skip existing failure paths for forwarded HTLCs, and instead
(a) fail them when the monitor event is processed, and (b) similar to what we
do for claims, the monitor event will not be acked and will continue to be
re-provided on startup until the HTLC is durably removed on the inbound edge.

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -1289,7 +1289,7 @@ pub(crate) struct ShutdownResult {
 	/// A list of dropped outbound HTLCs that can safely be failed backwards immediately.
 	pub(crate) dropped_outbound_htlcs: Vec<(HTLCSource, PaymentHash, PublicKey, ChannelId)>,
 	/// A list of inbound HTLC ids whose forwarded monitor-event ack condition is complete because
-	/// the HTLC was already claimed when the channel closed.
+	/// the HTLC was already locally removed when the channel closed.
 	pub(crate) htlcs_to_ack: Vec<u64>,
 	/// An unbroadcasted batch funding transaction id. The closure of this channel should be
 	/// propagated to the remainder of the batch.
@@ -6441,8 +6441,9 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 						self.channel_id,
 					));
 				},
-				HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => htlcs_to_ack.push(htlc_id),
-				_ => {},
+				HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. }
+				| HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. }
+				| HTLCUpdateAwaitingACK::FailMalformedHTLC { htlc_id, .. } => htlcs_to_ack.push(htlc_id),
 			}
 		}
 
@@ -6491,9 +6492,7 @@ impl<SP: SignerProvider> ChannelContext<SP> {
 
 		// See `ShutdownResult::htlcs_to_ack`.
 		for htlc in self.pending_inbound_htlcs.iter() {
-			if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill { .. }) =
-				&htlc.state
-			{
+			if let InboundHTLCState::LocalRemoved(_) = &htlc.state {
 				htlcs_to_ack.push(htlc.htlc_id);
 			}
 		}
@@ -9255,8 +9254,8 @@ where
 					log_trace!(logger, " ...removing inbound LocalRemoved {}", &htlc.payment_hash);
 					if let &InboundHTLCRemovalReason::Fulfill { .. } = reason {
 						value_to_self_msat_diff += htlc.amount_msat as i64;
-						htlc_ids_to_ack.push(htlc.htlc_id);
 					}
+					htlc_ids_to_ack.push(htlc.htlc_id);
 					*expecting_peer_commitment_signed = true;
 					false
 				} else {

lightning/src/ln/channelmanager.rs

@@ -1633,9 +1633,9 @@ pub(crate) enum MonitorUpdateCompletionAction {
 	/// via [`ChannelManager::handle_monitor_event_htlc_ack`]. Only generated when
 	/// [`ChannelManager::persistent_monitor_events`] is enabled.
 	///
-	/// If the inbound edge is open, this action fires when an inbound edge RAA update removing an
-	/// HTLC completes. If closed, it fires when the inbound edge has durably persisted the preimage
-	/// monitor update.
+	/// If the inbound edge is open, this action fires when an inbound edge RAA update removing the
+	/// HTLC completes. If the inbound edge is closed and we're claiming, we'll also use this variant
+	/// to block acking the monitor event until the inbound edge has durably persisted the preimage.
 	///
 	/// For claims, we could theoretically ack the outbound edge event once the preimage is durably
 	/// persisted in the inbound edge monitor, but if we stop persisting the holding cell in the
@@ -8267,11 +8267,20 @@ impl<
 						continue;
 					}
 				},
-				HTLCForwardInfo::FailHTLC { .. } | HTLCForwardInfo::FailMalformedHTLC { .. } => {
-					// Channel went away before we could fail it. This implies
-					// the channel is now on chain and our counterparty is
-					// trying to broadcast the HTLC-Timeout, but that's their
-					// problem, not ours.
+				HTLCForwardInfo::FailHTLC { htlc_id, upstream_channel_id, .. }
+				| HTLCForwardInfo::FailMalformedHTLC { htlc_id, upstream_channel_id, .. } => {
+					// Channel went away before we could fail it. This implies the channel is now
+					// on chain and our counterparty is trying to broadcast the HTLC-Timeout, but
+					// that's their problem, not ours.
+
+					// If `persistent_monitor_events` is enabled, there may be a monitor event generated by
+					// the outbound edge resolving this HTLC. That needs to be acked now or it will dangle
+					// forever.
+					if let Some(upstream_channel_id) = upstream_channel_id {
+						if self.persistent_monitor_events {
+							self.handle_monitor_event_htlc_ack(htlc_id, upstream_channel_id);
+						}
+					}
 				},
 			}
 		}
@@ -8547,6 +8556,11 @@ impl<
 					} else {
 						panic!("Stated return value requirements in queue_fail_{{malformed_}}htlc() were not met");
 					}
+					if self.persistent_monitor_events && htlc_not_found {
+						// The HTLC was already fully removed from the channel, so no future RAA
+						// will fire the usual ack path. Ack the outbound monitor event directly.
+						self.handle_monitor_event_htlc_ack(htlc_id, forward_chan_id);
+					}
 					// fail-backs are best-effort, we probably already have one
 					// pending, and if not that's OK, if not, the channel is on
 					// the chain and sending the HTLC-Timeout is their problem.
@@ -10791,9 +10805,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		self.forward_htlcs(htlc_forwards);
 		self.finalize_claims(finalized_claimed_htlcs);
 		for failure in failed_htlcs {
-			if self.persistent_monitor_events
-				&& matches!(failure.0, HTLCSource::OutboundRoute { .. })
-			{
+			if self.persistent_monitor_events {
 				// The MonitorEvent::HTLCEvent generated when the previous counterparty commitment
 				// is pruned will drive the failure instead.
 				continue;
@@ -14199,20 +14211,20 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 							Some(channel_id),
 							Some(htlc_update.payment_hash),
 						);
+						if self.persistent_monitor_events {
+							self.register_pending_forward_monitor_event_acks(
+								htlc_update.payment_hash,
+								htlc_update.source.previous_hop_data(),
+								monitor_event_source,
+							);
+						}
 						match htlc_update.resolution {
 							OutboundHTLCResolution::Claimed { preimage, skimmed_fee_msat } => {
 								log_trace!(
 									logger,
 									"Claiming HTLC with preimage {} from our monitor",
 									preimage
 								);
-								if self.persistent_monitor_events {
-									self.register_pending_forward_monitor_event_acks(
-										htlc_update.payment_hash,
-										htlc_update.source.previous_hop_data(),
-										monitor_event_source,
-									);
-								}
 								// Claim the funds from the previous hop, if there is one. In the future we can
 								// store attribution data in the `ChannelMonitor` and provide it here.
 								self.claim_funds_internal(
@@ -14235,35 +14247,41 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 									self.channel_is_closed(&channel_id, &counterparty_node_id);
 								let we_are_sender =
 									matches!(htlc_update.source, HTLCSource::OutboundRoute { .. });
-								if from_onchain | we_are_sender {
-									log_trace!(logger, "Failing HTLC from our monitor");
-									let failure_type = htlc_update
-										.source
-										.failure_type(counterparty_node_id, channel_id);
-
-									let completion_update =
-										if self.persistent_monitor_events && we_are_sender {
-											EventCompletionAction::AckMonitorEvent {
-												event_id: monitor_event_source,
-											}
-										} else {
-											EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate {
-												counterparty_node_id,
-												channel_funding_outpoint: funding_outpoint,
-												channel_id,
-												htlc_id: SentHTLCId::from_source(&htlc_update.source),
-											})
-										};
-
-									self.fail_htlc_backwards_internal(
-										&htlc_update.source,
-										&htlc_update.payment_hash,
-										&reason,
-										failure_type,
-										Some(completion_update),
-									);
-								}
-								if !we_are_sender {
+								log_trace!(logger, "Failing HTLC from our monitor");
+								let failure_type = htlc_update
+									.source
+									.failure_type(counterparty_node_id, channel_id);
+
+								let completion_update = if self.persistent_monitor_events {
+									if we_are_sender {
+										// Ack the monitor event once PaymentFailed is processed by the user
+										Some(EventCompletionAction::AckMonitorEvent {
+											event_id: monitor_event_source,
+										})
+									} else {
+										// We'll ack the monitor event when it's removed from the inbound edge on RAA
+										None
+									}
+								} else {
+									if from_onchain {
+										Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate {
+											counterparty_node_id,
+											channel_funding_outpoint: funding_outpoint,
+											channel_id,
+											htlc_id: SentHTLCId::from_source(&htlc_update.source),
+										}))
+									} else {
+										None
+									}
+								};
+								self.fail_htlc_backwards_internal(
+									&htlc_update.source,
+									&htlc_update.payment_hash,
+									&reason,
+									failure_type,
+									completion_update,
+								);
+								if !self.persistent_monitor_events {
 									self.chain_monitor.ack_monitor_event(monitor_event_source);
 								}
 							},
@@ -21327,19 +21345,22 @@ impl<
 				ev_action,
 			);
 		}
-		for ((_, hash), htlcs) in already_forwarded_htlcs.into_iter() {
-			for (htlc, _) in htlcs {
-				let channel_id = htlc.channel_id;
-				let node_id = htlc.counterparty_node_id;
-				let source = HTLCSource::PreviousHopData(htlc);
-				let failure_reason = LocalHTLCFailureReason::TemporaryChannelFailure;
-				let failure_data = channel_manager.get_htlc_inbound_temp_fail_data(failure_reason);
-				let reason = HTLCFailReason::reason(failure_reason, failure_data);
-				let receiver = HTLCHandlingFailureType::Forward { node_id, channel_id };
-				// The event completion action is only relevant for HTLCs that originate from our node, not
-				// forwarded HTLCs.
-				channel_manager
-					.fail_htlc_backwards_internal(&source, &hash, &reason, receiver, None);
+		if !channel_manager.persistent_monitor_events {
+			for ((_, hash), htlcs) in already_forwarded_htlcs.into_iter() {
+				for (htlc, _) in htlcs {
+					let channel_id = htlc.channel_id;
+					let node_id = htlc.counterparty_node_id;
+					let source = HTLCSource::PreviousHopData(htlc);
+					let failure_reason = LocalHTLCFailureReason::TemporaryChannelFailure;
+					let failure_data =
+						channel_manager.get_htlc_inbound_temp_fail_data(failure_reason);
+					let reason = HTLCFailReason::reason(failure_reason, failure_data);
+					let receiver = HTLCHandlingFailureType::Forward { node_id, channel_id };
+					// The event completion action is only relevant for HTLCs that originate from our node, not
+					// forwarded HTLCs.
+					channel_manager
+						.fail_htlc_backwards_internal(&source, &hash, &reason, receiver, None);
+				}
 			}
 		}
 

lightning/src/ln/functional_test_utils.rs

@@ -806,9 +806,9 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 				let unacked_monitor_events = monitor.drain_unacked_monitor_events();
 				if !unacked_monitor_events.is_empty() {
 					panic!(
-						"Node {} channel {channel_id:?} had {} unacked monitor events at drop: {unacked_monitor_events:#?}",
-						unacked_monitor_events.len(),
-						self.logger.id
+						"{} had {} unacked monitor events on channel {channel_id:?} at drop: {unacked_monitor_events:#?}",
+						self.logger.id,
+						unacked_monitor_events.len()
 					);
 				}
 			}

lightning/src/ln/functional_tests.rs

@@ -1821,6 +1821,13 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
 	let commitment_tx = get_local_commitment_txn!(nodes[1], chan_1.2);
 	check_spends!(commitment_tx[0], chan_1.3);
 
+	// Close the channel from each node's perspective, by mining the commitment on each of their
+	// chains
+	mine_transaction(&nodes[1], &commitment_tx[0]);
+	check_closed_broadcast(&nodes[1], 1, true);
+	check_added_monitors(&nodes[1], 1);
+	check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, &[node_a_id], 100000);
+
 	mine_transaction(&nodes[0], &commitment_tx[0]);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV + MIN_CLTV_EXPIRY_DELTA as u32); // Confirm blocks until the HTLC expires
 
@@ -5681,7 +5688,7 @@ pub fn test_update_fulfill_htlc_bolt2_after_malformed_htlc_message_must_forward_
 	assert_eq!(events_4.len(), 1);
 
 	//Confirm that handlinge the update_malformed_htlc message produces an update_fail_htlc message to be forwarded back along the route
-	match events_4[0] {
+	let ba_fail = match events_4[0] {
 		MessageSendEvent::UpdateHTLCs {
 			updates:
 				msgs::CommitmentUpdate {
@@ -5690,7 +5697,7 @@ pub fn test_update_fulfill_htlc_bolt2_after_malformed_htlc_message_must_forward_
 					ref update_fail_htlcs,
 					ref update_fail_malformed_htlcs,
 					ref update_fee,
-					..
+					ref commitment_signed,
 				},
 			..
 		} => {
@@ -5699,11 +5706,18 @@ pub fn test_update_fulfill_htlc_bolt2_after_malformed_htlc_message_must_forward_
 			assert_eq!(update_fail_htlcs.len(), 1);
 			assert!(update_fail_malformed_htlcs.is_empty());
 			assert!(update_fee.is_none());
+			(update_fail_htlcs[0].clone(), commitment_signed.clone())
 		},
 		_ => panic!("Unexpected event"),
 	};
 
 	check_added_monitors(&nodes[1], 1);
+
+	// Push the fail back to A so the A-B inbound RAA completes and acks the outbound-edge
+	// monitor event on B-C.
+	nodes[0].node.handle_update_fail_htlc(node_b_id, &ba_fail.0);
+	do_commitment_signed_dance(&nodes[0], &nodes[1], &ba_fail.1, false, true);
+	expect_payment_failed!(nodes[0], our_payment_hash, false);
 }
 
 #[xtest(feature = "_externalize_tests")]

lightning/src/ln/payment_tests.rs

@@ -5090,6 +5090,19 @@ fn do_test_payment_metadata_consistency(do_reload: bool, do_modify: bool) {
 		let fail_type =
 			HTLCHandlingFailureType::Forward { node_id: Some(node_d_id), channel_id: cd_chan_id };
 		expect_htlc_failure_conditions(events, &[fail_type]);
+
+		expect_and_process_pending_htlcs(&nodes[2], false);
+		check_added_monitors(&nodes[2], 1);
+		let cs_fail_a = get_htlc_update_msgs(&nodes[2], &node_a_id);
+		nodes[0].node.handle_update_fail_htlc(node_c_id, &cs_fail_a.update_fail_htlcs[0]);
+		do_commitment_signed_dance(&nodes[0], &nodes[2], &cs_fail_a.commitment_signed, false, true);
+		// The other MPP part is still in flight at D, so only PaymentPathFailed fires here.
+		expect_payment_failed_conditions(
+			&nodes[0],
+			payment_hash,
+			true,
+			PaymentFailedConditions::new().mpp_parts_remain(),
+		);
 	} else {
 		expect_and_process_pending_htlcs(&nodes[3], false);
 		expect_payment_claimable!(nodes[3], payment_hash, payment_secret, amt_msat);

lightning/src/ln/reload_tests.rs

@@ -1544,7 +1544,12 @@ fn removed_payment_no_manager_persistence() {
 		_ => panic!("Unexpected event"),
 	}
 
-	expect_payment_failed!(nodes[0], payment_hash, false);
+	// Whether nodes[0] sees a permanent failure depends on nodes[1]'s mode: with persistent
+	// monitor events nodes[1] re-provides the recipient's `IncorrectPaymentDetails` rejection via
+	// a monitor event; otherwise it proactively fails the forwarded HTLC back with
+	// `ChannelClosed` (non-permanent).
+	let payment_failed_permanently = nodes[1].node.test_persistent_monitor_events_enabled();
+	expect_payment_failed!(nodes[0], payment_hash, payment_failed_permanently);
 }
 
 #[test]
@@ -2455,8 +2460,10 @@ fn test_reload_node_without_preimage_fails_htlc() {
 	reconnect_args.pending_cell_htlc_fails = (0, 1);
 	reconnect_nodes(reconnect_args);
 
-	// nodes[0] should now have received the failure and generate PaymentFailed.
-	expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new());
+	// nodes[0] should now have received the failure and generate PaymentFailed. If persistent
+	// monitor events are enabled, nodes[1] will remember that the recipient rejected the payment.
+	let payment_failed_permanently = nodes[1].node.test_persistent_monitor_events_enabled();
+	expect_payment_failed_conditions(&nodes[0], payment_hash, payment_failed_permanently, PaymentFailedConditions::new());
 }
 
 #[test]