Prevent redundant PaymentClaimeds for closed channels

Previously, if we had an inbound payment on a closed channel that we started
claiming but did not end up removing from the commitment tx, we would generate
a PaymentClaimed event for the payment on every restart until the
channelmonitor was archived.

The past few commits have laid the groundwork to get rid of this redundancy --
here we now generate an event completion action for when the PaymentClaimed is
processed by the user, at which point a monitor update will be released that
tells the channel monitor that the user has processed the PaymentClaimed event.
The monitor tracks this internally such that the pending claim will no longer
be returned to the ChannelManager when reconstructing the set of mid-claim
inbound payments on startup, and thus no longer generate the redundant event.

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -3270,6 +3270,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	pub(crate) fn get_stored_preimages(
 		&self,
+	) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
+		let inner = self.inner.lock().unwrap();
+		inner
+			.payment_preimages
+			.iter()
+			.filter(|(hash, _)| !inner.inbound_payments_claimed.contains(*hash))
+			.map(|(hash, value)| (*hash, value.clone()))
+			.collect()
+	}
+
+	/// Used in tests to verify preimage propagation.
+	#[cfg(test)]
+	pub(crate) fn test_get_all_stored_preimages(
+		&self,
 	) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> {
 		self.inner.lock().unwrap().payment_preimages.clone()
 	}
@@ -4304,6 +4318,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					self.htlcs_resolved_to_user.insert(*htlc);
 				},
 				ChannelMonitorUpdateStep::InboundPaymentClaimed { payment_hash } => {
+					log_trace!(logger, "Inbound payment {} claimed", payment_hash);
+					self.inbound_payments_claimed.insert(*payment_hash);
 				},
 			}
 		}

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -4598,6 +4598,7 @@ fn test_claim_to_closed_channel_blocks_claimed_event() {
 	// available.
 	nodes[1].chain_monitor.complete_sole_pending_chan_update(&chan_a.2);
 	expect_payment_claimed!(nodes[1], payment_hash, 1_000_000);
+	check_added_monitors(&nodes[1], 1);
 }
 
 #[test]

lightning/src/ln/channelmanager.rs

@@ -10004,11 +10004,34 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						let action = if let Some((outpoint, counterparty_node_id, channel_id)) =
 							durable_preimage_channel
 						{
-							Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
-								channel_funding_outpoint: Some(outpoint),
-								counterparty_node_id,
-								channel_id,
-							})
+							let per_peer_state = self.per_peer_state.read().unwrap();
+							let is_channel_closed = per_peer_state
+								.get(&counterparty_node_id)
+								.map(|peer_state_mutex| {
+									let peer_state = peer_state_mutex.lock().unwrap();
+									!peer_state.channel_by_id.contains_key(&channel_id)
+								})
+								.unwrap_or(true);
+							// For open channels, we use ReleaseRAAChannelMonitorUpdate to maintain the blocking
+							// behavior (RAA updates are blocked until the PaymentClaimed event is handled).
+							// For closed channels, we use InboundPaymentClaimedChannelMonitorUpdate to persist
+							// that the PaymentClaimed event has been handled, preventing regeneration on restart.
+							if is_channel_closed {
+								Some(EventCompletionAction::InboundPaymentClaimedChannelMonitorUpdate(
+										InboundPaymentClaimedUpdate {
+											channel_funding_outpoint: outpoint,
+											counterparty_node_id,
+											channel_id,
+											payment_hash,
+										},
+								))
+							} else {
+								Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
+									channel_funding_outpoint: Some(outpoint),
+									counterparty_node_id,
+									channel_id,
+								})
+							}
 						} else {
 							None
 						};
@@ -14840,7 +14863,23 @@ impl<
 						update_step,
 					);
 				},
-				EventCompletionAction::InboundPaymentClaimedChannelMonitorUpdate(_) => {},
+				EventCompletionAction::InboundPaymentClaimedChannelMonitorUpdate(
+					InboundPaymentClaimedUpdate {
+						counterparty_node_id,
+						channel_funding_outpoint,
+						channel_id,
+						payment_hash,
+					},
+				) => {
+					let update_step =
+						ChannelMonitorUpdateStep::InboundPaymentClaimed { payment_hash };
+					self.handle_closed_channel_monitor_update_for_event(
+						counterparty_node_id,
+						channel_funding_outpoint,
+						channel_id,
+						update_step,
+					);
+				},
 			}
 		}
 	}

lightning/src/ln/monitor_tests.rs

@@ -225,6 +225,9 @@ fn archive_fully_resolved_monitors() {
 	nodes[1].node.claim_funds(payment_preimage);
 	check_added_monitors(&nodes[1], 1);
 	expect_payment_claimed!(nodes[1], payment_hash, 10_000_000);
+	// Processing PaymentClaimed on a closed channel generates a monitor update to mark the claim as
+	// resolved to the user.
+	check_added_monitors(&nodes[1], 1);
 	let htlc_claim_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
 	assert_eq!(htlc_claim_tx.len(), 1);
 
@@ -3282,22 +3285,29 @@ fn test_update_replay_panics() {
 	nodes[1].node.claim_funds(payment_preimage_1);
 	check_added_monitors(&nodes[1], 1);
 	expect_payment_claimed!(nodes[1], payment_hash_1, 1_000_000);
+	// Processing PaymentClaimed on a closed channel generates a monitor update to mark the claim as
+	// resolved to the user.
+	check_added_monitors(&nodes[1], 1);
 
 	nodes[1].node.claim_funds(payment_preimage_2);
 	check_added_monitors(&nodes[1], 1);
 	expect_payment_claimed!(nodes[1], payment_hash_2, 1_000_000);
+	check_added_monitors(&nodes[1], 1);
 
 	let mut updates = nodes[1].chain_monitor.monitor_updates.lock().unwrap().get_mut(&chan.2).unwrap().split_off(0);
 
-	// Update `monitor` until there's just one normal updates, an FC update, and a post-FC claim
-	// update pending
-	for update in updates.drain(..updates.len() - 4) {
+	// Update `monitor` until there's just one normal updates, an FC update, a post-FC claim
+	// and InboundPaymentClaimed updates pending.
+	// Updates are: [normal, FC, preimage1, inbound_claimed1, preimage2, inbound_claimed2]
+	for update in updates.drain(..updates.len() - 6) {
 		monitor.update_monitor(&update, &nodes[1].tx_broadcaster, &nodes[1].fee_estimator, &nodes[1].logger).unwrap();
 	}
-	assert_eq!(updates.len(), 4);
+	assert_eq!(updates.len(), 6);
 	assert!(matches!(updates[1].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }));
 	assert!(matches!(updates[2].updates[0], ChannelMonitorUpdateStep::PaymentPreimage { .. }));
-	assert!(matches!(updates[3].updates[0], ChannelMonitorUpdateStep::PaymentPreimage { .. }));
+	assert!(matches!(updates[3].updates[0], ChannelMonitorUpdateStep::InboundPaymentClaimed { .. }));
+	assert!(matches!(updates[4].updates[0], ChannelMonitorUpdateStep::PaymentPreimage { .. }));
+	assert!(matches!(updates[5].updates[0], ChannelMonitorUpdateStep::InboundPaymentClaimed { .. }));
 
 	// Ensure applying the force-close update skipping the last normal update fails
 	let poisoned_monitor = monitor.clone();
@@ -3384,11 +3394,13 @@ fn test_claim_event_never_handled() {
 	let chan_0_monitor_serialized = get_monitor!(nodes[1], chan.2).encode();
 	let mons = &[&chan_0_monitor_serialized[..]];
 	reload_node!(nodes[1], &init_node_ser, mons, persister, new_chain_mon, nodes_1_reload);
+	check_added_monitors(&nodes[1], 0);
 
 	expect_payment_claimed!(nodes[1], payment_hash_a, 1_000_000);
-	// The reload logic spuriously generates a redundant payment preimage-containing
-	// `ChannelMonitorUpdate`.
-	check_added_monitors(&nodes[1], 2);
+	// The reload logic spuriously generates 2 redundant payment preimage-containing
+	// `ChannelMonitorUpdate`s, plus we get a monitor update once the PaymentClaimed event is
+	// processed.
+	check_added_monitors(&nodes[1], 3);
 }
 
 fn do_test_lost_preimage_monitor_events(on_counterparty_tx: bool, p2a_anchor: bool) {
@@ -3863,6 +3875,7 @@ fn test_ladder_preimage_htlc_claims() {
 	check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, &[node_id_0], 1_000_000);
 
 	nodes[1].node.claim_funds(payment_preimage1);
+	check_added_monitors(&nodes[1], 1);
 	expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000);
 	check_added_monitors(&nodes[1], 1);
 
@@ -3884,6 +3897,7 @@ fn test_ladder_preimage_htlc_claims() {
 	check_added_monitors(&nodes[0], 1);
 
 	nodes[1].node.claim_funds(payment_preimage2);
+	check_added_monitors(&nodes[1], 1);
 	expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000);
 	check_added_monitors(&nodes[1], 1);
 

lightning/src/ln/reload_tests.rs

@@ -853,16 +853,20 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool, double_rest
 	if persist_both_monitors {
 		if let Event::ChannelClosed { reason: ClosureReason::OutdatedChannelManager, .. } = events[2] { } else { panic!(); }
 		if let Event::PaymentClaimed { amount_msat: 15_000_000, .. } = events[3] { } else { panic!(); }
-		check_added_monitors(&nodes[3], 4);
+		// 4 monitors for preimage updates + 1 for InboundPaymentClaimed marking the payment as
+		// claimed in the closed channel's monitor.
+		check_added_monitors(&nodes[3], 5);
 	} else {
 		if let Event::PaymentClaimed { amount_msat: 15_000_000, .. } = events[2] { } else { panic!(); }
+		// Only one channel closed; the durable_preimage_channel is the live one, so no extra
+		// InboundPaymentClaimed update is generated.
 		check_added_monitors(&nodes[3], 3);
 	}
 
 	// Now that we've processed background events, the preimage should have been copied into the
 	// non-persisted monitor:
-	assert!(get_monitor!(nodes[3], chan_id_persisted).get_stored_preimages().contains_key(&payment_hash));
-	assert!(get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash));
+	assert!(get_monitor!(nodes[3], chan_id_persisted).test_get_all_stored_preimages().contains_key(&payment_hash));
+	assert!(get_monitor!(nodes[3], chan_id_not_persisted).test_get_all_stored_preimages().contains_key(&payment_hash));
 
 	// On restart, we should also get a duplicate PaymentClaimed event as we persisted the
 	// ChannelManager prior to handling the original one.