Generate new ReleasePaymentComplete monitor updates

`MonitorEvent`s aren't delivered to the `ChannelManager` in a
durable fasion - if the `ChannelManager` fetches the pending
`MonitorEvent`s, then the `ChannelMonitor` gets persisted (i.e. due
to a block update) then the node crashes, prior to persisting the
`ChannelManager` again, the `MonitorEvent` and its effects on the
`ChannelManger` will be lost. This isn't likely in a sync persist
environment, but in an async one this could be an issue.

Note that this is only an issue for closed channels -
`MonitorEvent`s only inform the `ChannelManager` that a channel is
closed (which the `ChannelManager` will learn on startup or when it
next tries to advance the channel state), that
`ChannelMonitorUpdate` writes completed (which the `ChannelManager`
will detect on startup), or that HTLCs resolved on-chain post
closure. Of the three, only the last is problematic to lose prior
to a reload.

In previous commits we ensured that HTLC resolutions which came to
`ChannelManager` via a `MonitorEvent` were replayed on startup if
the `MonitorEvent` was lost. However, in cases where the
`ChannelManager` was so stale that it didn't have the payment state
for an HTLC at all, we only re-add it in cases where
`ChannelMonitor::get_pending_or_resolved_outbound_htlcs` includes
it.

Because constantly re-adding a payment state and then failing it
would generate lots of noise for users on startup (not to mention
risk of confusing stale payment events for the latest state of a
payment when the `PaymentId` has been reused to retry a payment).
Thus, `get_pending_or_resolved_outbound_htlcs` does not include
state for HTLCs which were resolved on chain with a preimage or
HTLCs which were resolved on chain with a timeout after
`ANTI_REORG_DELAY` confirmations.

This critera matches the critera for generating a `MonitorEvent`,
and works great under the assumption that `MonitorEvent`s are
reliably delivered. However, if they are not, and our
`ChannelManager` is lost or substantially old (or, in a future
where we do not persist `ChannelManager` at all), we will not end
up seeing payment resolution events for an HTLC.

Instead, we really want to tell our `ChannelMonitor`s when the
resolution of an HTLC is complete. Note that we don't particularly
care about non-payment HTLCs, as there is no re-hydration of state
to do there - `ChannelManager` load ignores forwarded HTLCs coming
back from `get_pending_or_resolved_outbound_htlcs` as there's
nothing to do - we always attempt to replay the success/failure and
figure out if it mattered based on whether there was still an HTLC
to claim/fail.

Here we begin generating the new
`ChannelMonitorUpdateStep::ReleasePaymentComplete` updates,
updating functional tests for the new `ChannelMonitorUpdate`s where
required.

Backport of 71a364c

Conflicts resolved in:
 * lightning/src/ln/chanmon_update_fail_tests.rs
 * lightning/src/ln/channelmanager.rs
 * lightning/src/ln/functional_test_utils.rs
 * lightning/src/ln/functional_tests.rs
 * lightning/src/ln/monitor_tests.rs
 * lightning/src/ln/outbound_payment.rs
 * lightning/src/ln/payment_tests.rs

Note that unlike the original commit, on this branch we do not fail
to deserialize a `ChannelMonitor` if the `counterparty_node_id` is
`None` (implying it has not seen a `ChannelMonitorUpdate` since
LDK 0.0.118). Thus, we skip the new logic in some cases, generating
a warning log instead.

As we assumed that it is now reasonable to require
`counterparty_node_id`s in LDK 0.2, it seems reasonable to skip the
new logic (potentially generating some additional spurious payment
events on restart) now here as well.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -3356,6 +3356,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.

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -3314,6 +3314,7 @@ fn do_test_durable_preimages_on_closed_channel(close_chans_before_reload: bool,
 
 	nodes[0].node.force_close_broadcasting_latest_txn(&chan_id_ab, &nodes[1].node.get_our_node_id(), error_message.to_string()).unwrap();
 	check_closed_event(&nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, false, &[nodes[1].node.get_our_node_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);
 

lightning/src/ln/channelmanager.rs

@@ -1206,6 +1206,12 @@ pub(crate) enum EventCompletionAction {
 		channel_funding_outpoint: Option<OutPoint>,
 		channel_id: ChannelId,
 	},
+
+	/// When a payment's resolution is communicated to the downstream logic via
+	/// [`Event::PaymentSent`] or [`Event::PaymentFailed`] we may want to mark the payment as
+	/// fully-resolved in the [`ChannelMonitor`], which we do via this action.
+	/// Note that this action will be dropped on downgrade to LDK prior to 0.2!
+	ReleasePaymentCompleteChannelMonitorUpdate(PaymentCompleteUpdate),
 }
 impl_writeable_tlv_based_enum!(EventCompletionAction,
 	(0, ReleaseRAAChannelMonitorUpdate) => {
@@ -1218,6 +1224,7 @@ impl_writeable_tlv_based_enum!(EventCompletionAction,
 			ChannelId::v1_from_funding_outpoint(channel_funding_outpoint.unwrap())
 		})),
 	}
+	{1, ReleasePaymentCompleteChannelMonitorUpdate} => (),
 );
 
 /// The source argument which is passed to [`ChannelManager::claim_mpp_part`].
@@ -6907,11 +6914,20 @@ where
 				if let Some(update) = from_monitor_update_completion {
 					// If `fail_htlc` didn't `take` the post-event action, we should go ahead and
 					// complete it here as the failure was duplicative - we've already handled it.
-					// This should mostly only happen on startup, but it is possible to hit it in
-					// rare cases where a MonitorUpdate is replayed after restart because a
-					// ChannelMonitor wasn't persisted after it was applied (even though the
-					// ChannelManager was).
-					// TODO
+					// This can happen in rare cases where a MonitorUpdate is replayed after
+					// restart because a ChannelMonitor wasn't persisted after it was applied (even
+					// though the ChannelManager was).
+					// For such cases, we also check that there's no existing pending event to
+					// complete this action already, which we let finish instead.
+					let action =
+						EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update);
+					let have_action = {
+						let pending_events = self.pending_events.lock().unwrap();
+						pending_events.iter().any(|(_, act)| act.as_ref() == Some(&action))
+					};
+					if !have_action {
+						self.handle_post_event_actions([action]);
+					}
 				}
 			},
 			HTLCSource::PreviousHopData(HTLCPreviousHopData {
@@ -7397,19 +7413,38 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		startup_replay: bool, next_channel_counterparty_node_id: Option<PublicKey>,
 		next_channel_outpoint: OutPoint, next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
 	) {
+		debug_assert_eq!(
+			startup_replay,
+			!self.background_events_processed_since_startup.load(Ordering::Acquire)
+		);
+		let htlc_id = SentHTLCId::from_source(&source);
 		match source {
 			HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
-				debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
+				debug_assert!(!startup_replay,
 					"We don't support claim_htlc claims during startup - monitors may not be available yet");
 				if let Some(pubkey) = next_channel_counterparty_node_id {
 					debug_assert_eq!(pubkey, path.hops[0].pubkey);
 				}
-				let mut ev_completion_action =
+				let mut ev_completion_action = if from_onchain {
+					if let Some(counterparty_node_id) = next_channel_counterparty_node_id {
+						let release = PaymentCompleteUpdate {
+							counterparty_node_id,
+							channel_funding_outpoint: next_channel_outpoint,
+							channel_id: next_channel_id,
+							htlc_id,
+						};
+						Some(EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(release))
+					} else {
+						log_warn!(self.logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart");
+						None
+					}
+				} else {
 					Some(EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 						channel_funding_outpoint: Some(next_channel_outpoint),
 						channel_id: next_channel_id,
 						counterparty_node_id: path.hops[0].pubkey,
-					});
+					})
+				};
 				self.pending_outbound_payments.claim_htlc(
 					payment_id,
 					payment_preimage,
@@ -9535,12 +9570,24 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 							log_trace!(logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 							let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id };
 							let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
+							let completion_update =
+								if let Some(counterparty_node_id) = counterparty_node_id {
+									Some(PaymentCompleteUpdate {
+										counterparty_node_id,
+										channel_funding_outpoint: funding_outpoint,
+										channel_id,
+										htlc_id: SentHTLCId::from_source(&htlc_update.source),
+									})
+								} else {
+									log_warn!(self.logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart");
+									None
+								};
 							self.fail_htlc_backwards_internal(
 								&htlc_update.source,
 								&htlc_update.payment_hash,
 								&reason,
 								receiver,
-								None,
+								completion_update,
 							);
 						}
 					},
@@ -10894,8 +10941,63 @@ where
 					channel_id,
 					counterparty_node_id,
 				} => {
+					let startup_complete =
+						self.background_events_processed_since_startup.load(Ordering::Acquire);
+					debug_assert!(startup_complete);
 					self.handle_monitor_update_release(counterparty_node_id, channel_id, None);
 				},
+				EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(
+					PaymentCompleteUpdate {
+						counterparty_node_id,
+						channel_funding_outpoint,
+						channel_id,
+						htlc_id,
+					},
+				) => {
+					let per_peer_state = self.per_peer_state.read().unwrap();
+					let mut peer_state = per_peer_state
+						.get(&counterparty_node_id)
+						.map(|state| state.lock().unwrap())
+						.expect("Channels originating a payment resolution must have peer state");
+					let update_id = peer_state
+						.closed_channel_monitor_update_ids
+						.get_mut(&channel_id)
+						.expect("Channels originating a payment resolution must have a monitor");
+					*update_id += 1;
+
+					let update = ChannelMonitorUpdate {
+						update_id: *update_id,
+						channel_id: Some(channel_id),
+						counterparty_node_id: Some(counterparty_node_id),
+						updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete {
+							htlc: htlc_id,
+						}],
+					};
+
+					let during_startup =
+						!self.background_events_processed_since_startup.load(Ordering::Acquire);
+					if during_startup {
+						let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+							counterparty_node_id,
+							funding_txo: channel_funding_outpoint,
+							channel_id,
+							update,
+						};
+						self.pending_background_events.lock().unwrap().push(event);
+					} else {
+						handle_new_monitor_update!(
+							self,
+							channel_funding_outpoint,
+							update,
+							peer_state,
+							peer_state,
+							per_peer_state,
+							counterparty_node_id,
+							channel_id,
+							POST_CHANNEL_CLOSE
+						);
+					}
+				},
 			}
 		}
 	}
@@ -13984,6 +14086,7 @@ where
 				if counterparty_opt.is_none() {
 					for (htlc_source, (htlc, preimage_opt)) in monitor.get_all_current_outbound_htlcs() {
 						let logger = WithChannelMonitor::from(&args.logger, monitor, Some(htlc.payment_hash));
+						let htlc_id = SentHTLCId::from_source(&htlc_source);
 						match htlc_source {
 							HTLCSource::PreviousHopData(prev_hop_data) => {
 								let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| {
@@ -14035,6 +14138,21 @@ where
 							HTLCSource::OutboundRoute { payment_id, session_priv, path, .. } => {
 								if let Some(preimage) = preimage_opt {
 									let pending_events = Mutex::new(pending_events_read);
+									let mut compl_action =
+										if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() {
+											let update = PaymentCompleteUpdate {
+												counterparty_node_id,
+												channel_funding_outpoint: monitor.get_funding_txo().0,
+												channel_id: monitor.channel_id(),
+												htlc_id,
+											};
+											Some(
+												EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate(update)
+											)
+										} else {
+											log_warn!(logger, "Missing counterparty node id in monitor when trying to re-claim a payment resolved on chain. This may lead to redundant PaymentSent events on restart");
+											None
+										};
 									// Note that we set `from_onchain` to "false" here,
 									// deliberately keeping the pending payment around forever.
 									// Given it should only occur when we have a channel we're
@@ -14043,13 +14161,6 @@ where
 									// generating a `PaymentPathSuccessful` event but regenerating
 									// it and the `PaymentSent` on every restart until the
 									// `ChannelMonitor` is removed.
-									let mut compl_action = Some(
-										EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
-											channel_funding_outpoint: Some(monitor.get_funding_txo().0),
-											channel_id: monitor.channel_id(),
-											counterparty_node_id: path.hops[0].pubkey,
-										},
-									);
 									pending_outbounds.claim_htlc(
 										payment_id,
 										preimage,
@@ -14060,6 +14171,44 @@ where
 										&pending_events,
 										&&logger,
 									);
+									// If the completion action was not consumed, then there was no
+									// payment to claim, and we need to tell the `ChannelMonitor`
+									// we don't need to hear about the HTLC again, at least as long
+									// as the PaymentSent event isn't still sitting around in our
+									// event queue.
+									let have_action = if compl_action.is_some() {
+										let pending_events = pending_events.lock().unwrap();
+										pending_events.iter().any(|(_, act)| *act == compl_action)
+									} else {
+										false
+									};
+									if !have_action && compl_action.is_some() {
+										if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() {
+											let mut peer_state = per_peer_state
+												.get(&counterparty_node_id)
+												.map(|state| state.lock().unwrap())
+												.expect("Channels originating a preimage must have peer state");
+											let update_id = peer_state
+												.closed_channel_monitor_update_ids
+												.get_mut(&monitor.channel_id())
+												.expect("Channels originating a preimage must have a monitor");
+											*update_id += 1;
+
+											pending_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+												counterparty_node_id: counterparty_node_id,
+												funding_txo: monitor.get_funding_txo().0,
+												channel_id: monitor.channel_id(),
+												update: ChannelMonitorUpdate {
+													update_id: *update_id,
+													channel_id: Some(monitor.channel_id()),
+													updates: vec![ChannelMonitorUpdateStep::ReleasePaymentComplete {
+														htlc: htlc_id,
+													}],
+													counterparty_node_id: Some(counterparty_node_id),
+												},
+											});
+										}
+									}
 									pending_events_read = pending_events.into_inner().unwrap();
 								}
 							},

lightning/src/ln/functional_test_utils.rs

@@ -2332,10 +2332,14 @@ macro_rules! expect_payment_claimed {
 	}
 }
 
-pub fn expect_payment_sent<CM: AChannelManager, H: NodeHolder<CM=CM>>(node: &H,
-	expected_payment_preimage: PaymentPreimage, expected_fee_msat_opt: Option<Option<u64>>,
-	expect_per_path_claims: bool, expect_post_ev_mon_update: bool,
+pub fn expect_payment_sent<CM: AChannelManager, H: NodeHolder<CM=CM>>(
+	node: &H, expected_payment_preimage: PaymentPreimage,
+	expected_fee_msat_opt: Option<Option<u64>>, expect_per_path_claims: bool,
+	expect_post_ev_mon_update: bool,
 ) {
+	if expect_post_ev_mon_update {
+		check_added_monitors(node, 0);
+	}
 	let events = node.node().get_and_clear_pending_events();
 	let expected_payment_hash = PaymentHash(
 		bitcoin::hashes::sha256::Hash::hash(&expected_payment_preimage.0).to_byte_array());
@@ -2535,6 +2539,7 @@ pub struct PaymentFailedConditions<'a> {
 	pub(crate) expected_blamed_scid: Option<u64>,
 	pub(crate) expected_blamed_chan_closed: Option<bool>,
 	pub(crate) expected_mpp_parts_remain: bool,
+	pub(crate) from_mon_update: bool,
 }
 
 impl<'a> PaymentFailedConditions<'a> {
@@ -2544,6 +2549,7 @@ impl<'a> PaymentFailedConditions<'a> {
 			expected_blamed_scid: None,
 			expected_blamed_chan_closed: None,
 			expected_mpp_parts_remain: false,
+			from_mon_update: false,
 		}
 	}
 	pub fn mpp_parts_remain(mut self) -> Self {
@@ -2562,6 +2568,10 @@ impl<'a> PaymentFailedConditions<'a> {
 		self.expected_htlc_error_data = Some((code, data));
 		self
 	}
+	pub fn from_mon_update(mut self) -> Self {
+		self.from_mon_update = true;
+		self
+	}
 }
 
 #[cfg(test)]
@@ -2647,8 +2657,19 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
 	node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool,
 	conditions: PaymentFailedConditions<'e>
 ) {
+	if conditions.from_mon_update {
+		check_added_monitors(node, 0);
+	}
 	let events = node.node.get_and_clear_pending_events();
-	expect_payment_failed_conditions_event(events, expected_payment_hash, expected_payment_failed_permanently, conditions);
+	if conditions.from_mon_update {
+		check_added_monitors(node, 1);
+	}
+	expect_payment_failed_conditions_event(
+		events,
+		expected_payment_hash,
+		expected_payment_failed_permanently,
+		conditions,
+	);
 }
 
 pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId {