Persistent monitor events for onchain outbound 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
off-chain 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 *on*chain outbound payment failures. When
persistent_monitor_events is enabled, we will skip existing failure paths for
on-chain outbound 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 resulting
PaymentFailed event is processed by the user.

Forward HTLC failure paths are unchanged and will be migrated separately in
upcoming work.

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -3371,7 +3371,25 @@ 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);
-						if !us.htlcs_resolved_to_user.contains(&htlc_id) {
+						let skip_htlc = if us.persistent_events_enabled {
+							let htlc_resolved_on_chain = match htlc.transaction_output_index {
+								Some(idx) => us
+									.htlcs_resolved_on_chain
+									.iter()
+									.any(|r| r.commitment_tx_output_idx == Some(idx)),
+								None => {
+									// Dust HTLCs are implicitly resolved when the commitment reaches
+									// ANTI_REORG_DELAY
+									us.confirmed_funding_spend_past_anti_reorg().is_some()
+								},
+							};
+							// If the HTLC was resolved on-chain and we don't have a pending monitor event for
+							// it, that implies the event was acked due to the HTLC being resolved to the user.
+							htlc_resolved_on_chain && !us.has_pending_event_for_htlc(source)
+						} else {
+							us.htlcs_resolved_to_user.contains(&htlc_id)
+						};
+						if !skip_htlc {
 							let preimage_opt =
 								us.counterparty_fulfilled_htlcs.get(&htlc_id).cloned();
 							res.insert((**source).clone(), (htlc.clone(), preimage_opt));
@@ -7601,7 +7619,10 @@ mod tests {
 			assert!(events.iter().any(|e| matches!(e, Event::PaymentPathFailed { .. })));
 			assert!(events.iter().any(|e| matches!(e, Event::PaymentFailed { .. })));
 			assert!(events.iter().any(|e| matches!(e, Event::ChannelClosed { .. })));
-			check_added_monitors(&nodes[1], 2);
+			// If persistent monitor events are enabled, there won't be a ReleasePaymentComplete monitor
+			// update.
+			let persistent_mon_evs = nodes[1].node.test_persistent_monitor_events_enabled();
+			check_added_monitors(&nodes[1], if persistent_mon_evs { 1 } else { 2 });
 		} else {
 			check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, &[nodes[0].node.get_our_node_id()], 100000);
 			check_added_monitors(&nodes[1], 1);

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -5814,8 +5814,10 @@ fn do_test_late_counterparty_commitment_update_after_funding_spend(fully_confirm
 		false,
 		PaymentFailedConditions::new(),
 	);
-	// The payment failure generates a ReleasePaymentComplete monitor update.
-	check_added_monitors(&nodes[0], 1);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		// The payment failure generates a ReleasePaymentComplete monitor update.
+		check_added_monitors(&nodes[0], 1);
+	}
 }
 
 #[test]
@@ -5916,7 +5918,9 @@ fn do_test_late_counterparty_commitment_update_after_holder_commitment_spend(dus
 		false,
 		PaymentFailedConditions::new(),
 	);
-	check_added_monitors(&nodes[0], 1);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[0], 1); // ReleasePaymentComplete monitor update
+	}
 
 	// Verify HTLC X was NOT failed (no payment failure event for it at this point).
 	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
@@ -5940,7 +5944,9 @@ fn do_test_late_counterparty_commitment_update_after_holder_commitment_spend(dus
 		false,
 		PaymentFailedConditions::new(),
 	);
-	check_added_monitors(&nodes[0], 1);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[0], 1); // ReleasePaymentComplete monitor update
+	}
 }
 
 #[test]

lightning/src/ln/channelmanager.rs

@@ -14221,20 +14221,19 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 										.source
 										.failure_type(counterparty_node_id, channel_id);
 
-									let completion_update = if self.persistent_monitor_events
-										&& we_are_sender && !from_onchain
-									{
-										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),
-										})
-									};
+									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,
@@ -14244,7 +14243,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 										Some(completion_update),
 									);
 								}
-								if from_onchain | !we_are_sender {
+								if !we_are_sender {
 									self.chain_monitor.ack_monitor_event(monitor_event_source);
 								}
 							},
@@ -21274,6 +21273,13 @@ impl<
 				htlc_source;
 			let failure_type = source.failure_type(counterparty_id, channel_id);
 			let reason = HTLCFailReason::from_failure_code(failure_reason);
+			if channel_manager.persistent_monitor_events
+				&& matches!(source, HTLCSource::OutboundRoute { .. })
+			{
+				// The persistent monitor event generated for the failed HTLC will drive the failure
+				// instead.
+				continue;
+			}
 			channel_manager.fail_htlc_backwards_internal(
 				&source,
 				&hash,

lightning/src/ln/functional_test_utils.rs

@@ -3501,8 +3501,8 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
 		check_added_monitors(node, 0);
 	}
 	let events = node.node.get_and_clear_pending_events();
-	if conditions.from_mon_update {
-		check_added_monitors(node, 1);
+	if conditions.from_mon_update && !node.node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(node, 1); // ReleasePaymentComplete update
 	}
 	expect_payment_failed_conditions_event(
 		events,

lightning/src/ln/functional_tests.rs

@@ -2069,7 +2069,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(
 	check_added_monitors(&nodes[1], 0);
 	let events = nodes[1].node.get_and_clear_pending_events();
 	if deliver_bs_raa {
-		check_added_monitors(&nodes[1], 2);
+		let persistent_mon_evs = nodes[1].node.test_persistent_monitor_events_enabled();
+		check_added_monitors(&nodes[1], if persistent_mon_evs { 1 } else { 2 });
 	} else {
 		check_added_monitors(&nodes[1], 1);
 	}
@@ -5895,7 +5896,9 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) {
 	connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
 	check_added_monitors(&nodes[0], 0);
 	let events = nodes[0].node.get_and_clear_pending_events();
-	check_added_monitors(&nodes[0], 2);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[0], 2); // ReleasePaymentComplete updates
+	}
 	// Only 2 PaymentPathFailed events should show up, over-dust HTLC has to be failed by timeout tx
 	assert_eq!(events.len(), 4);
 	let mut first_failed = false;

lightning/src/ln/monitor_tests.rs

@@ -1622,7 +1622,9 @@ fn do_test_revoked_counterparty_commitment_balances(keyed_anchors: bool, p2a_anc
 
 	check_added_monitors(&nodes[1], 0);
 	let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events();
-	check_added_monitors(&nodes[1], 2);
+	if !nodes[1].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[1], 2); // ReleasePaymentComplete monitor updates
+	}
 	expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(),
 		missing_htlc_payment_hash, false, PaymentFailedConditions::new());
 	expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(),
@@ -1633,7 +1635,9 @@ fn do_test_revoked_counterparty_commitment_balances(keyed_anchors: bool, p2a_anc
 		test_spendable_output(&nodes[1], &claim_txn[0], false);
 		check_added_monitors(&nodes[1], 0);
 		let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events();
-		check_added_monitors(&nodes[1], 2);
+		if !nodes[1].node.test_persistent_monitor_events_enabled() {
+			check_added_monitors(&nodes[1], 2); // ReleasePaymentComplete monitor updates
+		}
 		expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(),
 			live_payment_hash, false, PaymentFailedConditions::new());
 		expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(),
@@ -1648,7 +1652,9 @@ fn do_test_revoked_counterparty_commitment_balances(keyed_anchors: bool, p2a_anc
 		test_spendable_output(&nodes[1], &claim_txn[0], false);
 		check_added_monitors(&nodes[1], 0);
 		let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events();
-		check_added_monitors(&nodes[1], 2);
+		if !nodes[1].node.test_persistent_monitor_events_enabled() {
+			check_added_monitors(&nodes[1], 2); // ReleasePaymentComplete monitor updates
+		}
 		expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(),
 			live_payment_hash, false, PaymentFailedConditions::new());
 		expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(),

lightning/src/ln/payment_tests.rs

@@ -1790,7 +1790,9 @@ fn onchain_failed_probe_yields_event() {
 
 	check_added_monitors(&nodes[0], 0);
 	let mut events = nodes[0].node.get_and_clear_pending_events();
-	check_added_monitors(&nodes[0], 1);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[0], 1);
+	}
 	assert_eq!(events.len(), 2);
 	let mut found_probe_failed = false;
 	for event in events.drain(..) {

lightning/src/ln/reorg_tests.rs

@@ -1323,7 +1323,9 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool, p2a
 		if use_third_htlc {
 			check_added_monitors(&nodes[0], 0);
 			let failed_events = nodes[0].node.get_and_clear_pending_events();
-			check_added_monitors(&nodes[0], 1);
+			if !nodes[0].node.test_persistent_monitor_events_enabled() {
+				check_added_monitors(&nodes[0], 1);
+			}
 			assert_eq!(failed_events.len(), 2);
 			let mut found_expected_events = [false, false];
 			for event in failed_events {

lightning/src/ln/splicing_tests.rs

@@ -2266,12 +2266,9 @@ fn do_test_splice_commitment_broadcast(splice_status: SpliceStatus, claim_htlcs:
 			2
 		);
 	}
-	let expected_mons = if nodes[0].node.test_persistent_monitor_events_enabled() && claim_htlcs {
-		0
-	} else {
-		2 // Two `ReleasePaymentComplete` monitor updates
-	};
-	check_added_monitors(&nodes[0], expected_mons);
+	if !nodes[0].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[0], 2); //`ReleasePaymentComplete` monitor updates
+	}
 
 	// When the splice never confirms and we see a commitment transaction broadcast and confirm for
 	// the current funding instead, we should expect to see an `Event::DiscardFunding` for the