Persistent monitor events for onchain outbound claims

Currently, the resolution of HTLCs (and decisions on when HTLCs can be
forwarded) is the responsibility of Channel objects (a part of ChannelManager)
until the channel is closed, and then the ChannelMonitor thereafter. This leads
to some complexity around race conditions for HTLCs right around channel
closure. Additionally, there is lots of complexity reconstructing the state of
all HTLCs in the ChannelManager deserialization/loading logic.

Instead, we want to do all resolution in ChannelMonitors (in response to
ChannelMonitorUpdates) and pass them back to ChannelManager in the form of
MonitorEvents (similar to how HTLCs are resolved after channels are closed). In
order to have reliable resolution, we'll need to keep MonitorEvents around in
the ChannelMonitor until the ChannelManager has finished processing them. This
will simplify things - on restart instead of examining the set of HTLCs in
monitors we can simply replay all the pending MonitorEvents.

In recent work, we added support for keeping monitor events around until they
are explicitly acked by the ChannelManager, but would always ack monitor events
immediately, which preserved the previous behavior and didn't break any tests.

Here we start acking monitor events for on-chain HTLC claims when the user
processes the PaymentSent event, if the persistent_monitor_events feature is
enabled. This allows us to stop issuing ReleasePaymentComplete monitor updates
for onchain payment claims, because the purpose of that behavior is to ensure
we won't keep repeatedly issuing PaymentSent events, and the monitor event and
acking it on PaymentSent processing now serves that purpose instead.

Author: valentinewallace

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3954,7 +3954,7 @@ fn do_test_durable_preimages_on_closed_channel(
 
 	mine_transactions(&nodes[0], &[&as_closing_tx[0], bs_preimage_tx]);
 	check_closed_broadcast(&nodes[0], 1, false);
-	expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+	expect_payment_sent!(&nodes[0], payment_preimage);
 
 	if close_chans_before_reload && !hold_post_reload_mon_update {
 		// For close_chans_before_reload with hold=false, the deferred completions

lightning/src/ln/channelmanager.rs

@@ -10013,7 +10013,18 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 					"We don't support claim_htlc claims during startup - monitors may not be available yet");
 				debug_assert_eq!(next_channel_counterparty_node_id, path.hops[0].pubkey);
 
-				let mut ev_completion_action = if from_onchain {
+				let mut ev_completion_action = if self.persistent_monitor_events {
+					// If persistent monitor events is enabled:
+					// * If the channel is on-chain, we don't need to use ReleasePaymentComplete like we do
+					//   below because we will stop hearing about this payment after the relevant monitor event
+					//   is acked
+					// * If the channel is open, we don't need to block the preimage-removing monitor update
+					//   like we do below because we will keep hearing about the preimage until we explicitly
+					//   ack the monitor event for this payment
+					monitor_event_id.map(|event_id| EventCompletionAction::AckMonitorEvent {
+						event_id: MonitorEventSource { channel_id: next_channel_id, event_id },
+					})
+				} else if from_onchain {
 					let release = PaymentCompleteUpdate {
 						counterparty_node_id: next_channel_counterparty_node_id,
 						channel_funding_outpoint: next_channel_outpoint,
@@ -10022,17 +10033,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 					};
 					Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release))
 				} else {
-					if self.persistent_monitor_events {
-						monitor_event_id.map(|event_id| EventCompletionAction::AckMonitorEvent {
-							event_id: MonitorEventSource { channel_id: next_channel_id, event_id },
-						})
-					} else {
-						Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
-							channel_funding_outpoint: Some(next_channel_outpoint),
-							channel_id: next_channel_id,
-							counterparty_node_id: path.hops[0].pubkey,
-						})
-					}
+					Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
+						channel_funding_outpoint: Some(next_channel_outpoint),
+						channel_id: next_channel_id,
+						counterparty_node_id: path.hops[0].pubkey,
+					})
 				};
 				let logger = WithContext::for_payment(
 					&self.logger,
@@ -13640,7 +13645,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 									Some(event_id),
 								);
 							}
-							if from_onchain | !we_are_sender {
+							if !we_are_sender {
 								self.chain_monitor.ack_monitor_event(monitor_event_source);
 							}
 						} else {
@@ -19730,6 +19735,12 @@ impl<
 								..
 							} => {
 								if let Some(preimage) = preimage_opt {
+									if persistent_monitor_events {
+										// If persistent_monitor_events is enabled, then the monitor will keep
+										// providing us with a monitor event for this claim until the ChannelManager
+										// explicitly marks it as resolved, no need to explicitly handle it here.
+										continue;
+									}
 									let pending_events = Mutex::new(pending_events_read);
 									let update = PaymentCompleteUpdate {
 										counterparty_node_id: monitor.get_counterparty_node_id(),

lightning/src/ln/functional_tests.rs

@@ -1629,7 +1629,10 @@ pub fn test_htlc_on_chain_success() {
 	check_closed_broadcast(&nodes[0], 1, true);
 	check_added_monitors(&nodes[0], 1);
 	let events = nodes[0].node.get_and_clear_pending_events();
-	check_added_monitors(&nodes[0], 2);
+	check_added_monitors(
+		&nodes[0],
+		if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 2 },
+	);
 	assert_eq!(events.len(), 5);
 	let mut first_claimed = false;
 	for event in events {

lightning/src/ln/monitor_tests.rs

@@ -274,7 +274,7 @@ fn archive_fully_resolved_monitors() {
 
 	// Finally, we process the pending `MonitorEvent` from nodes[0], allowing the `ChannelMonitor`
 	// to be archived `ARCHIVAL_DELAY_BLOCKS` blocks later.
-	expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+	expect_payment_sent!(&nodes[0], payment_preimage);
 	nodes[0].chain_monitor.chain_monitor.archive_fully_resolved_channel_monitors();
 	assert_eq!(nodes[0].chain_monitor.chain_monitor.list_monitors().len(), 1);
 	connect_blocks(&nodes[0], ARCHIVAL_DELAY_BLOCKS - 1);
@@ -747,9 +747,9 @@ fn do_test_claim_value_force_close(keyed_anchors: bool, p2a_anchor: bool, prev_c
 	mine_transaction(&nodes[0], &b_broadcast_txn[0]);
 	if prev_commitment_tx {
 		expect_payment_path_successful!(nodes[0]);
-		check_added_monitors(&nodes[0], 1);
+		check_added_monitors(&nodes[0], if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 1 });
 	} else {
-		expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+		expect_payment_sent!(&nodes[0], payment_preimage);
 	}
 	assert_eq!(sorted_vec(vec![sent_htlc_balance.clone(), sent_htlc_timeout_balance.clone()]),
 		sorted_vec(nodes[0].chain_monitor.chain_monitor.get_monitor(chan_id).unwrap().get_claimable_balances()));
@@ -1015,7 +1015,7 @@ fn do_test_balances_on_local_commitment_htlcs(keyed_anchors: bool, p2a_anchor: b
 	// Now confirm nodes[1]'s HTLC claim, giving nodes[0] the preimage. Note that the "maybe
 	// claimable" balance remains until we see ANTI_REORG_DELAY blocks.
 	mine_transaction(&nodes[0], &bs_htlc_claim_txn[0]);
-	expect_payment_sent(&nodes[0], payment_preimage_2, None, true, true);
+	expect_payment_sent!(&nodes[0], payment_preimage_2);
 	assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
 			amount_satoshis: 1_000_000 - 10_000 - 20_000 - commitment_tx_fee - anchor_outputs_value,
 			confirmation_height: node_a_commitment_claimable,
@@ -2097,7 +2097,7 @@ fn do_test_revoked_counterparty_aggregated_claims(keyed_anchors: bool, p2a_ancho
 		as_revoked_txn[1].clone()
 	};
 	mine_transaction(&nodes[1], &htlc_success_claim);
-	expect_payment_sent(&nodes[1], claimed_payment_preimage, None, true, true);
+	expect_payment_sent!(&nodes[1], claimed_payment_preimage);
 
 	let mut claim_txn_2 = nodes[1].tx_broadcaster.txn_broadcast();
 	// Once B sees the HTLC-Success transaction it splits its claim transaction into two, though in
@@ -3547,9 +3547,13 @@ fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool, p2a_anchor: bo
 	// After the background events are processed in `get_and_clear_pending_events`, above, node B
 	// will create the requisite `ChannelMontiorUpdate` for claiming the forwarded payment back.
 	// The HTLC, however, is added to the holding cell for replay after the peer connects, below.
-	// It will also apply a `ChannelMonitorUpdate` to let the `ChannelMonitor` know that the
-	// payment can now be forgotten as the `PaymentSent` event was handled.
-	check_added_monitors(&nodes[1], 2);
+	if nodes[1].node.test_persistent_monitor_events_enabled() {
+		check_added_monitors(&nodes[1], 1);
+	} else {
+		// It will also apply a `ChannelMonitorUpdate` to let the `ChannelMonitor` know that the
+		// payment can now be forgotten as the `PaymentSent` event was handled.
+		check_added_monitors(&nodes[1], 2);
+	}
 
 	nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id());
 
@@ -3943,8 +3947,7 @@ fn test_ladder_preimage_htlc_claims() {
 	connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
 	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
 
-	expect_payment_sent(&nodes[0], payment_preimage1, None, true, false);
-	check_added_monitors(&nodes[0], 1);
+	expect_payment_sent!(&nodes[0], payment_preimage1);
 
 	nodes[1].node.claim_funds(payment_preimage2);
 	expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000);
@@ -3966,6 +3969,5 @@ fn test_ladder_preimage_htlc_claims() {
 	connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
 	connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
 
-	expect_payment_sent(&nodes[0], payment_preimage2, None, true, false);
-	check_added_monitors(&nodes[0], 1);
+	expect_payment_sent!(&nodes[0], payment_preimage2);
 }

lightning/src/ln/payment_tests.rs

@@ -957,7 +957,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
 		assert_eq!(txn[0].compute_txid(), as_commitment_tx.compute_txid());
 	}
 	mine_transaction(&nodes[0], &bs_htlc_claim_txn);
-	expect_payment_sent(&nodes[0], payment_preimage_1, None, true, true);
+	expect_payment_sent!(&nodes[0], payment_preimage_1);
 	connect_blocks(&nodes[0], TEST_FINAL_CLTV * 4 + 20);
 	let (first_htlc_timeout_tx, second_htlc_timeout_tx) = {
 		let mut txn = nodes[0].tx_broadcaster.unique_txn_broadcast();
@@ -1368,7 +1368,7 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(
 		let conditions = PaymentFailedConditions::new().from_mon_update();
 		expect_payment_failed_conditions(&nodes[0], payment_hash, false, conditions);
 	} else {
-		expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+		expect_payment_sent!(&nodes[0], payment_preimage);
 	}
 	// Note that if we persist the monitor before processing the events, above, we'll always get
 	// them replayed on restart no matter what
@@ -1406,18 +1406,22 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(
 		} else {
 			expect_payment_sent(&nodes[0], payment_preimage, None, true, false);
 		}
-		if persist_manager_post_event {
-			// After reload, the ChannelManager identified the failed payment and queued up the
-			// PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we
-			// already did that) and corresponding ChannelMonitorUpdate to mark the payment
-			// handled, but while processing the pending `MonitorEvent`s (which were not processed
-			// before the monitor was persisted) we will end up with a duplicate
-			// ChannelMonitorUpdate.
-			check_added_monitors(&nodes[0], 2);
+		if !nodes[0].node.test_persistent_monitor_events_enabled() {
+			if persist_manager_post_event {
+				// After reload, the ChannelManager identified the failed payment and queued up the
+				// PaymentSent (or not, if `persist_manager_post_event` resulted in us detecting we
+				// already did that) and corresponding ChannelMonitorUpdate to mark the payment
+				// handled, but while processing the pending `MonitorEvent`s (which were not processed
+				// before the monitor was persisted) we will end up with a duplicate
+				// ChannelMonitorUpdate.
+				check_added_monitors(&nodes[0], 2);
+			} else {
+				// ...unless we got the PaymentSent event, in which case we have de-duplication logic
+				// preventing a redundant monitor event.
+				check_added_monitors(&nodes[0], 1);
+			}
 		} else {
-			// ...unless we got the PaymentSent event, in which case we have de-duplication logic
-			// preventing a redundant monitor event.
-			check_added_monitors(&nodes[0], 1);
+			check_added_monitors(&nodes[0], 0);
 		}
 	}
 
@@ -4198,10 +4202,14 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
 	check_added_monitors(&nodes[0], 0);
 	nodes[0].node.test_process_background_events();
 	check_added_monitors(&nodes[0], 1);
-	// Then once we process the PaymentSent event we'll apply a monitor update to remove the
-	// pending payment from being re-hydrated on the next startup.
 	let 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], 0);
+	} else {
+		// Once we process the PaymentSent event we'll apply a monitor update to remove the
+		// pending payment from being re-hydrated on the next startup.
+		check_added_monitors(&nodes[0], 1);
+	}
 	assert_eq!(events.len(), 3, "{events:?}");
 	if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[0] {
 	} else {

lightning/src/ln/reorg_tests.rs

@@ -254,7 +254,7 @@ fn test_counterparty_revoked_reorg() {
 
 	// Connect the HTLC claim transaction for HTLC 3
 	mine_transaction(&nodes[1], &unrevoked_local_txn[2]);
-	expect_payment_sent(&nodes[1], payment_preimage_3, None, true, true);
+	expect_payment_sent!(&nodes[1], payment_preimage_3);
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 
 	// Connect blocks to confirm the unrevoked commitment transaction
@@ -1228,7 +1228,10 @@ fn do_test_split_htlc_expiry_tracking(use_third_htlc: bool, reorg_out: bool, p2a
 
 	check_added_monitors(&nodes[0], 0);
 	let sent_events = nodes[0].node.get_and_clear_pending_events();
-	check_added_monitors(&nodes[0], 2);
+	check_added_monitors(
+		&nodes[0],
+		if nodes[0].node.test_persistent_monitor_events_enabled() { 0 } else { 2 },
+	);
 	assert_eq!(sent_events.len(), 4, "{sent_events:?}");
 	let mut found_expected_events = [false, false, false, false];
 	for event in sent_events {

lightning/src/ln/splicing_tests.rs

@@ -1984,7 +1984,12 @@ fn do_test_splice_commitment_broadcast(splice_status: SpliceStatus, claim_htlcs:
 			2
 		);
 	}
-	check_added_monitors(&nodes[0], 2); // Two `ReleasePaymentComplete` monitor updates
+	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);
 
 	// 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