From 1feb71375c0b0041c78aaa8dce9ef12ab3a6cd3b Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Sun, 29 Sep 2024 15:22:29 +0000
Subject: [PATCH 1/8] Pass the `peer_state` lock through to
 `update_maps_on_chan_removal`

`update_maps_on_chan_removal` is used to perform `ChannelManager`
state updates when a channel is being removed, prior to dropping
the `peer_state` lock. In a future commit we'll use it to update
fields in the `per_peer_state`, but in order to do so we'll need to
have access to that state in the macro.

Here we get set up for this by passing the per-peer state to
`update_maps_on_chan_removal`, which is sadly a fairly large patch.
---
 lightning/src/ln/channelmanager.rs | 196 ++++++++++++++---------------
 1 file changed, 95 insertions(+), 101 deletions(-)

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index f31c23b5a67..5cb92d3cb9e 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -49,7 +49,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa
 use crate::ln::inbound_payment;
 use crate::ln::types::ChannelId;
 use crate::types::payment::{PaymentHash, PaymentPreimage, PaymentSecret};
-use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext};
+use crate::ln::channel::{self, Channel, ChannelPhase, ChannelError, ChannelUpdateStatus, ShutdownResult, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext};
 use crate::ln::channel_state::ChannelDetails;
 use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 #[cfg(any(feature = "_test_utils", test))]
@@ -2889,7 +2889,7 @@ macro_rules! handle_error {
 }
 
 macro_rules! update_maps_on_chan_removal {
-	($self: expr, $channel_context: expr) => {{
+	($self: expr, $peer_state: expr, $channel_context: expr) => {{
 		if let Some(outpoint) = $channel_context.get_funding_txo() {
 			$self.outpoint_to_peer.lock().unwrap().remove(&outpoint);
 		}
@@ -2912,7 +2912,7 @@ macro_rules! update_maps_on_chan_removal {
 
 /// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
 macro_rules! convert_chan_phase_err {
-	($self: ident, $err: expr, $channel: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => {
+	($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, MANUAL_CHANNEL_UPDATE, $channel_update: expr) => {
 		match $err {
 			ChannelError::Warn(msg) => {
 				(false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), *$channel_id))
@@ -2923,7 +2923,7 @@ macro_rules! convert_chan_phase_err {
 			ChannelError::Close((msg, reason)) => {
 				let logger = WithChannelContext::from(&$self.logger, &$channel.context, None);
 				log_error!(logger, "Closing channel {} due to close-required error: {}", $channel_id, msg);
-				update_maps_on_chan_removal!($self, $channel.context);
+				update_maps_on_chan_removal!($self, $peer_state, $channel.context);
 				let shutdown_res = $channel.context.force_shutdown(true, reason);
 				let err =
 					MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $channel_update);
@@ -2931,42 +2931,42 @@ macro_rules! convert_chan_phase_err {
 			},
 		}
 	};
-	($self: ident, $err: expr, $channel: expr, $channel_id: expr, FUNDED_CHANNEL) => {
-		convert_chan_phase_err!($self, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, { $self.get_channel_update_for_broadcast($channel).ok() })
+	($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, FUNDED_CHANNEL) => {
+		convert_chan_phase_err!($self, $peer_state, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, { $self.get_channel_update_for_broadcast($channel).ok() })
 	};
-	($self: ident, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => {
-		convert_chan_phase_err!($self, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, None)
+	($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => {
+		convert_chan_phase_err!($self, $peer_state, $err, $channel, $channel_id, MANUAL_CHANNEL_UPDATE, None)
 	};
-	($self: ident, $err: expr, $channel_phase: expr, $channel_id: expr) => {
+	($self: ident, $peer_state: expr, $err: expr, $channel_phase: expr, $channel_id: expr) => {
 		match $channel_phase {
 			ChannelPhase::Funded(channel) => {
-				convert_chan_phase_err!($self, $err, channel, $channel_id, FUNDED_CHANNEL)
+				convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, FUNDED_CHANNEL)
 			},
 			ChannelPhase::UnfundedOutboundV1(channel) => {
-				convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL)
+				convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL)
 			},
 			ChannelPhase::UnfundedInboundV1(channel) => {
-				convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL)
+				convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL)
 			},
 			#[cfg(any(dual_funding, splicing))]
 			ChannelPhase::UnfundedOutboundV2(channel) => {
-				convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL)
+				convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL)
 			},
 			#[cfg(any(dual_funding, splicing))]
 			ChannelPhase::UnfundedInboundV2(channel) => {
-				convert_chan_phase_err!($self, $err, channel, $channel_id, UNFUNDED_CHANNEL)
+				convert_chan_phase_err!($self, $peer_state, $err, channel, $channel_id, UNFUNDED_CHANNEL)
 			},
 		}
 	};
 }
 
 macro_rules! break_chan_phase_entry {
-	($self: ident, $res: expr, $entry: expr) => {
+	($self: ident, $peer_state: expr, $res: expr, $entry: expr) => {
 		match $res {
 			Ok(res) => res,
 			Err(e) => {
 				let key = *$entry.key();
-				let (drop, res) = convert_chan_phase_err!($self, e, $entry.get_mut(), &key);
+				let (drop, res) = convert_chan_phase_err!($self, $peer_state, e, $entry.get_mut(), &key);
 				if drop {
 					$entry.remove_entry();
 				}
@@ -2977,12 +2977,12 @@ macro_rules! break_chan_phase_entry {
 }
 
 macro_rules! try_chan_phase_entry {
-	($self: ident, $res: expr, $entry: expr) => {
+	($self: ident, $peer_state: expr, $res: expr, $entry: expr) => {
 		match $res {
 			Ok(res) => res,
 			Err(e) => {
 				let key = *$entry.key();
-				let (drop, res) = convert_chan_phase_err!($self, e, $entry.get_mut(), &key);
+				let (drop, res) = convert_chan_phase_err!($self, $peer_state, e, $entry.get_mut(), &key);
 				if drop {
 					$entry.remove_entry();
 				}
@@ -2993,10 +2993,10 @@ macro_rules! try_chan_phase_entry {
 }
 
 macro_rules! remove_channel_phase {
-	($self: expr, $entry: expr) => {
+	($self: expr, $peer_state: expr, $entry: expr) => {
 		{
 			let channel = $entry.remove_entry().1;
-			update_maps_on_chan_removal!($self, &channel.context());
+			update_maps_on_chan_removal!($self, $peer_state, &channel.context());
 			channel
 		}
 	}
@@ -3674,7 +3674,7 @@ where
 								peer_state_lock, peer_state, per_peer_state, chan);
 						}
 					} else {
-						let mut chan_phase = remove_channel_phase!(self, chan_phase_entry);
+						let mut chan_phase = remove_channel_phase!(self, peer_state, chan_phase_entry);
 						shutdown_result = Some(chan_phase.context_mut().force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }));
 					}
 				},
@@ -3798,7 +3798,7 @@ where
 				if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
 					let mut peer_state = peer_state_mutex.lock().unwrap();
 					if let Some(mut chan) = peer_state.channel_by_id.remove(&channel_id) {
-						update_maps_on_chan_removal!(self, &chan.context());
+						update_maps_on_chan_removal!(self, peer_state, &chan.context());
 						shutdown_results.push(chan.context_mut().force_shutdown(false, ClosureReason::FundingBatchClosure));
 					}
 				}
@@ -3858,7 +3858,7 @@ where
 			let logger = WithContext::from(&self.logger, Some(*peer_node_id), Some(*channel_id), None);
 			if let hash_map::Entry::Occupied(chan_phase_entry) = peer_state.channel_by_id.entry(channel_id.clone()) {
 				log_error!(logger, "Force-closing channel {}", channel_id);
-				let mut chan_phase = remove_channel_phase!(self, chan_phase_entry);
+				let mut chan_phase = remove_channel_phase!(self, peer_state, chan_phase_entry);
 				mem::drop(peer_state);
 				mem::drop(per_peer_state);
 				match chan_phase {
@@ -4364,7 +4364,7 @@ where
 								first_hop_htlc_msat: htlc_msat,
 								payment_id,
 							}, onion_packet, None, &self.fee_estimator, &&logger);
-						match break_chan_phase_entry!(self, send_res, chan_phase_entry) {
+						match break_chan_phase_entry!(self, peer_state, send_res, chan_phase_entry) {
 							Some(monitor_update) => {
 								match handle_new_monitor_update!(self, funding_txo, monitor_update, peer_state_lock, peer_state, per_peer_state, chan) {
 									false => {
@@ -5142,7 +5142,7 @@ where
 						.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
 						.and_then(|mut peer_state| peer_state.channel_by_id.remove(&channel_id).map(|chan| (chan, peer_state)))
 						.map(|(mut chan, mut peer_state)| {
-							update_maps_on_chan_removal!(self, &chan.context());
+							update_maps_on_chan_removal!(self, peer_state, &chan.context());
 							let closure_reason = ClosureReason::ProcessingError { err: e.clone() };
 							shutdown_results.push(chan.context_mut().force_shutdown(false, closure_reason));
 							peer_state.pending_msg_events.push(events::MessageSendEvent::HandleError {
@@ -6276,34 +6276,32 @@ where
 			let mut pending_peers_awaiting_removal = Vec::new();
 			let mut shutdown_channels = Vec::new();
 
-			let mut process_unfunded_channel_tick = |
-				chan_id: &ChannelId,
-				context: &mut ChannelContext<SP>,
-				unfunded_context: &mut UnfundedChannelContext,
-				pending_msg_events: &mut Vec<MessageSendEvent>,
-				counterparty_node_id: PublicKey,
-			| {
-				context.maybe_expire_prev_config();
-				if unfunded_context.should_expire_unfunded_channel() {
-					let logger = WithChannelContext::from(&self.logger, context, None);
-					log_error!(logger,
-						"Force-closing pending channel with ID {} for not establishing in a timely manner", chan_id);
-					update_maps_on_chan_removal!(self, &context);
-					shutdown_channels.push(context.force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }));
-					pending_msg_events.push(MessageSendEvent::HandleError {
-						node_id: counterparty_node_id,
-						action: msgs::ErrorAction::SendErrorMessage {
-							msg: msgs::ErrorMessage {
-								channel_id: *chan_id,
-								data: "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned(),
+			macro_rules! process_unfunded_channel_tick {
+				($peer_state: expr, $chan: expr, $pending_msg_events: expr) => { {
+					let context = &mut $chan.context;
+					context.maybe_expire_prev_config();
+					if $chan.unfunded_context.should_expire_unfunded_channel() {
+						let logger = WithChannelContext::from(&self.logger, context, None);
+						log_error!(logger,
+							"Force-closing pending channel with ID {} for not establishing in a timely manner",
+							context.channel_id());
+						update_maps_on_chan_removal!(self, $peer_state, context);
+						shutdown_channels.push(context.force_shutdown(false, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }));
+						$pending_msg_events.push(MessageSendEvent::HandleError {
+							node_id: context.get_counterparty_node_id(),
+							action: msgs::ErrorAction::SendErrorMessage {
+								msg: msgs::ErrorMessage {
+									channel_id: context.channel_id(),
+									data: "Force-closing pending channel due to timeout awaiting establishment handshake".to_owned(),
+								},
 							},
-						},
-					});
-					false
-				} else {
-					true
-				}
-			};
+						});
+						false
+					} else {
+						true
+					}
+				} }
+			}
 
 			{
 				let per_peer_state = self.per_peer_state.read().unwrap();
@@ -6324,7 +6322,7 @@ where
 								if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
 
 								if let Err(e) = chan.timer_check_closing_negotiation_progress() {
-									let (needs_close, err) = convert_chan_phase_err!(self, e, chan, chan_id, FUNDED_CHANNEL);
+									let (needs_close, err) = convert_chan_phase_err!(self, peer_state, e, chan, chan_id, FUNDED_CHANNEL);
 									handle_errors.push((Err(err), counterparty_node_id));
 									if needs_close { return false; }
 								}
@@ -6389,22 +6387,18 @@ where
 								true
 							},
 							ChannelPhase::UnfundedInboundV1(chan) => {
-								process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context,
-									pending_msg_events, counterparty_node_id)
+								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
 							},
 							ChannelPhase::UnfundedOutboundV1(chan) => {
-								process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context,
-									pending_msg_events, counterparty_node_id)
+								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
 							},
 							#[cfg(any(dual_funding, splicing))]
 							ChannelPhase::UnfundedInboundV2(chan) => {
-								process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context,
-									pending_msg_events, counterparty_node_id)
+								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
 							},
 							#[cfg(any(dual_funding, splicing))]
 							ChannelPhase::UnfundedOutboundV2(chan) => {
-								process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context,
-									pending_msg_events, counterparty_node_id)
+								process_unfunded_channel_tick!(peer_state, chan, pending_msg_events)
 							},
 						}
 					});
@@ -7897,7 +7891,7 @@ where
 				hash_map::Entry::Occupied(mut phase) => {
 					match phase.get_mut() {
 						ChannelPhase::UnfundedOutboundV1(chan) => {
-							try_chan_phase_entry!(self, chan.accept_channel(&msg, &self.default_configuration.channel_handshake_limits, &peer_state.latest_features), phase);
+							try_chan_phase_entry!(self, peer_state, chan.accept_channel(&msg, &self.default_configuration.channel_handshake_limits, &peer_state.latest_features), phase);
 							(chan.context.get_value_satoshis(), chan.context.get_funding_redeemscript().to_p2wsh(), chan.context.get_user_id())
 						},
 						_ => {
@@ -7945,14 +7939,14 @@ where
 							// Really we should be returning the channel_id the peer expects based
 							// on their funding info here, but they're horribly confused anyway, so
 							// there's not a lot we can do to save them.
-							return Err(convert_chan_phase_err!(self, err, &mut ChannelPhase::UnfundedInboundV1(inbound_chan), &msg.temporary_channel_id).1);
+							return Err(convert_chan_phase_err!(self, peer_state, err, &mut ChannelPhase::UnfundedInboundV1(inbound_chan), &msg.temporary_channel_id).1);
 						},
 					}
 				},
 				Some(mut phase) => {
 					let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
 					let err = ChannelError::close(err_msg);
-					return Err(convert_chan_phase_err!(self, err, &mut phase, &msg.temporary_channel_id).1);
+					return Err(convert_chan_phase_err!(self, peer_state, err, &mut phase, &msg.temporary_channel_id).1);
 				},
 				None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id))
 			};
@@ -7968,7 +7962,7 @@ where
 			// on the channel.
 			let err = ChannelError::close($err.to_owned());
 			chan.unset_funding_info(msg.temporary_channel_id);
-			return Err(convert_chan_phase_err!(self, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
+			return Err(convert_chan_phase_err!(self, peer_state, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
 		} } }
 
 		match peer_state.channel_by_id.entry(funded_channel_id) {
@@ -8057,7 +8051,7 @@ where
 								// found an (unreachable) panic when the monitor update contained
 								// within `shutdown_finish` was applied.
 								chan.unset_funding_info(msg.channel_id);
-								return Err(convert_chan_phase_err!(self, e, &mut ChannelPhase::Funded(chan), &msg.channel_id).1);
+								return Err(convert_chan_phase_err!(self, peer_state, e, &mut ChannelPhase::Funded(chan), &msg.channel_id).1);
 							}
 						},
 						Err((chan, e)) => {
@@ -8066,7 +8060,7 @@ where
 							// We've already removed this outbound channel from the map in
 							// `PeerState` above so at this point we just need to clean up any
 							// lingering entries concerning this channel as it is safe to do so.
-							return Err(convert_chan_phase_err!(self, e, &mut ChannelPhase::UnfundedOutboundV1(chan), &msg.channel_id).1);
+							return Err(convert_chan_phase_err!(self, peer_state, e, &mut ChannelPhase::UnfundedOutboundV1(chan), &msg.channel_id).1);
 						}
 					}
 				} else {
@@ -8092,7 +8086,7 @@ where
 			hash_map::Entry::Occupied(mut chan_phase_entry) => {
 				if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 					let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-					let announcement_sigs_opt = try_chan_phase_entry!(self, chan.channel_ready(&msg, &self.node_signer,
+					let announcement_sigs_opt = try_chan_phase_entry!(self, peer_state, chan.channel_ready(&msg, &self.node_signer,
 						self.chain_hash, &self.default_configuration, &self.best_block.read().unwrap(), &&logger), chan_phase_entry);
 					if let Some(announcement_sigs) = announcement_sigs_opt {
 						log_trace!(logger, "Sending announcement_signatures for channel {}", chan.context.channel_id());
@@ -8122,7 +8116,7 @@ where
 
 					Ok(())
 				} else {
-					try_chan_phase_entry!(self, Err(ChannelError::close(
+					try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got a channel_ready message for an unfunded channel!".into())), chan_phase_entry)
 				}
 			},
@@ -8156,7 +8150,7 @@ where
 						}
 
 						let funding_txo_opt = chan.context.get_funding_txo();
-						let (shutdown, monitor_update_opt, htlcs) = try_chan_phase_entry!(self,
+						let (shutdown, monitor_update_opt, htlcs) = try_chan_phase_entry!(self, peer_state,
 							chan.shutdown(&self.signer_provider, &peer_state.latest_features, &msg), chan_phase_entry);
 						dropped_htlcs = htlcs;
 
@@ -8179,7 +8173,7 @@ where
 						let context = phase.context_mut();
 						let logger = WithChannelContext::from(&self.logger, context, None);
 						log_error!(logger, "Immediately closing unfunded channel {} as peer asked to cooperatively shut it down (which is unnecessary)", &msg.channel_id);
-						let mut chan = remove_channel_phase!(self, chan_phase_entry);
+						let mut chan = remove_channel_phase!(self, peer_state, chan_phase_entry);
 						finish_shutdown = Some(chan.context_mut().force_shutdown(false, ClosureReason::CounterpartyCoopClosedUnfundedChannel));
 					},
 					// TODO(dual_funding): Combine this match arm with above.
@@ -8187,7 +8181,7 @@ where
 					ChannelPhase::UnfundedInboundV2(_) | ChannelPhase::UnfundedOutboundV2(_) => {
 						let context = phase.context_mut();
 						log_error!(self.logger, "Immediately closing unfunded channel {} as peer asked to cooperatively shut it down (which is unnecessary)", &msg.channel_id);
-						let mut chan = remove_channel_phase!(self, chan_phase_entry);
+						let mut chan = remove_channel_phase!(self, peer_state, chan_phase_entry);
 						finish_shutdown = Some(chan.context_mut().force_shutdown(false, ClosureReason::CounterpartyCoopClosedUnfundedChannel));
 					},
 				}
@@ -8221,7 +8215,7 @@ where
 				hash_map::Entry::Occupied(mut chan_phase_entry) => {
 					if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 						let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-						let (closing_signed, tx, shutdown_result) = try_chan_phase_entry!(self, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_phase_entry);
+						let (closing_signed, tx, shutdown_result) = try_chan_phase_entry!(self, peer_state, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_phase_entry);
 						debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown());
 						if let Some(msg) = closing_signed {
 							peer_state.pending_msg_events.push(events::MessageSendEvent::SendClosingSigned {
@@ -8235,10 +8229,10 @@ where
 							// also implies there are no pending HTLCs left on the channel, so we can
 							// fully delete it from tracking (the channel monitor is still around to
 							// watch for old state broadcasts)!
-							(tx, Some(remove_channel_phase!(self, chan_phase_entry)), shutdown_result)
+							(tx, Some(remove_channel_phase!(self, peer_state, chan_phase_entry)), shutdown_result)
 						} else { (tx, None, shutdown_result) }
 					} else {
-						return try_chan_phase_entry!(self, Err(ChannelError::close(
+						return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 							"Got a closing_signed message for an unfunded channel!".into())), chan_phase_entry);
 					}
 				},
@@ -8336,9 +8330,9 @@ where
 							}
 						}
 					}
-					try_chan_phase_entry!(self, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry);
+					try_chan_phase_entry!(self, peer_state, chan.update_add_htlc(&msg, pending_forward_info, &self.fee_estimator), chan_phase_entry);
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got an update_add_htlc message for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8362,7 +8356,7 @@ where
 			match peer_state.channel_by_id.entry(msg.channel_id) {
 				hash_map::Entry::Occupied(mut chan_phase_entry) => {
 					if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
-						let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
+						let res = try_chan_phase_entry!(self, peer_state, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 						if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 							let logger = WithChannelContext::from(&self.logger, &chan.context, None);
 							log_trace!(logger,
@@ -8382,7 +8376,7 @@ where
 						next_user_channel_id = chan.context.get_user_id();
 						res
 					} else {
-						return try_chan_phase_entry!(self, Err(ChannelError::close(
+						return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 							"Got an update_fulfill_htlc message for an unfunded channel!".into())), chan_phase_entry);
 					}
 				},
@@ -8411,9 +8405,9 @@ where
 		match peer_state.channel_by_id.entry(msg.channel_id) {
 			hash_map::Entry::Occupied(mut chan_phase_entry) => {
 				if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
-					try_chan_phase_entry!(self, chan.update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan_phase_entry);
+					try_chan_phase_entry!(self, peer_state, chan.update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan_phase_entry);
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got an update_fail_htlc message for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8437,12 +8431,12 @@ where
 			hash_map::Entry::Occupied(mut chan_phase_entry) => {
 				if (msg.failure_code & 0x8000) == 0 {
 					let chan_err = ChannelError::close("Got update_fail_malformed_htlc with BADONION not set".to_owned());
-					try_chan_phase_entry!(self, Err(chan_err), chan_phase_entry);
+					try_chan_phase_entry!(self, peer_state, Err(chan_err), chan_phase_entry);
 				}
 				if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
-					try_chan_phase_entry!(self, chan.update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan_phase_entry);
+					try_chan_phase_entry!(self, peer_state, chan.update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan_phase_entry);
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got an update_fail_malformed_htlc message for an unfunded channel!".into())), chan_phase_entry);
 				}
 				Ok(())
@@ -8465,14 +8459,14 @@ where
 				if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 					let logger = WithChannelContext::from(&self.logger, &chan.context, None);
 					let funding_txo = chan.context.get_funding_txo();
-					let monitor_update_opt = try_chan_phase_entry!(self, chan.commitment_signed(&msg, &&logger), chan_phase_entry);
+					let monitor_update_opt = try_chan_phase_entry!(self, peer_state, chan.commitment_signed(&msg, &&logger), chan_phase_entry);
 					if let Some(monitor_update) = monitor_update_opt {
 						handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock,
 							peer_state, per_peer_state, chan);
 					}
 					Ok(())
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got a commitment_signed message for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8665,7 +8659,7 @@ where
 								&peer_state.actions_blocking_raa_monitor_updates, funding_txo, msg.channel_id,
 								*counterparty_node_id)
 						} else { false };
-						let (htlcs_to_fail, monitor_update_opt) = try_chan_phase_entry!(self,
+						let (htlcs_to_fail, monitor_update_opt) = try_chan_phase_entry!(self, peer_state,
 							chan.revoke_and_ack(&msg, &self.fee_estimator, &&logger, mon_update_blocked), chan_phase_entry);
 						if let Some(monitor_update) = monitor_update_opt {
 							let funding_txo = funding_txo_opt
@@ -8675,7 +8669,7 @@ where
 						}
 						htlcs_to_fail
 					} else {
-						return try_chan_phase_entry!(self, Err(ChannelError::close(
+						return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 							"Got a revoke_and_ack message for an unfunded channel!".into())), chan_phase_entry);
 					}
 				},
@@ -8699,9 +8693,9 @@ where
 			hash_map::Entry::Occupied(mut chan_phase_entry) => {
 				if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 					let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-					try_chan_phase_entry!(self, chan.update_fee(&self.fee_estimator, &msg, &&logger), chan_phase_entry);
+					try_chan_phase_entry!(self, peer_state, chan.update_fee(&self.fee_estimator, &msg, &&logger), chan_phase_entry);
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got an update_fee message for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8727,7 +8721,7 @@ where
 					}
 
 					peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
-						msg: try_chan_phase_entry!(self, chan.announcement_signatures(
+						msg: try_chan_phase_entry!(self, peer_state, chan.announcement_signatures(
 							&self.node_signer, self.chain_hash, self.best_block.read().unwrap().height,
 							msg, &self.default_configuration
 						), chan_phase_entry),
@@ -8736,7 +8730,7 @@ where
 						update_msg: Some(self.get_channel_update_for_broadcast(chan).unwrap()),
 					});
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got an announcement_signatures message for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8780,7 +8774,7 @@ where
 					} else {
 						let logger = WithChannelContext::from(&self.logger, &chan.context, None);
 						log_debug!(logger, "Received channel_update {:?} for channel {}.", msg, chan_id);
-						let did_change = try_chan_phase_entry!(self, chan.channel_update(&msg), chan_phase_entry);
+						let did_change = try_chan_phase_entry!(self, peer_state, chan.channel_update(&msg), chan_phase_entry);
 						// If nothing changed after applying their update, we don't need to bother
 						// persisting.
 						if !did_change {
@@ -8788,7 +8782,7 @@ where
 						}
 					}
 				} else {
-					return try_chan_phase_entry!(self, Err(ChannelError::close(
+					return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 						"Got a channel_update for an unfunded channel!".into())), chan_phase_entry);
 				}
 			},
@@ -8819,7 +8813,7 @@ where
 						// disconnect, so Channel's reestablish will never hand us any holding cell
 						// freed HTLCs to fail backwards. If in the future we no longer drop pending
 						// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
-						let responses = try_chan_phase_entry!(self, chan.channel_reestablish(
+						let responses = try_chan_phase_entry!(self, peer_state, chan.channel_reestablish(
 							msg, &&logger, &self.node_signer, self.chain_hash,
 							&self.default_configuration, &*self.best_block.read().unwrap()), chan_phase_entry);
 						let mut channel_update = None;
@@ -8850,7 +8844,7 @@ where
 						}
 						need_lnd_workaround
 					} else {
-						return try_chan_phase_entry!(self, Err(ChannelError::close(
+						return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close(
 							"Got a channel_reestablish message for an unfunded channel!".into())), chan_phase_entry);
 					}
 				},
@@ -8937,7 +8931,7 @@ where
 								let peer_state = &mut *peer_state_lock;
 								let pending_msg_events = &mut peer_state.pending_msg_events;
 								if let hash_map::Entry::Occupied(chan_phase_entry) = peer_state.channel_by_id.entry(channel_id) {
-									if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, chan_phase_entry) {
+									if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, peer_state, chan_phase_entry) {
 										let reason = if let MonitorEvent::HolderForceClosedWithInfo { reason, .. } = monitor_event {
 											reason
 										} else {
@@ -9186,13 +9180,13 @@ where
 
 										log_info!(logger, "Broadcasting {}", log_tx!(tx));
 										self.tx_broadcaster.broadcast_transactions(&[&tx]);
-										update_maps_on_chan_removal!(self, &chan.context);
+										update_maps_on_chan_removal!(self, peer_state, &chan.context);
 										false
 									} else { true }
 								},
 								Err(e) => {
 									has_update = true;
-									let (close_channel, res) = convert_chan_phase_err!(self, e, chan, channel_id, FUNDED_CHANNEL);
+									let (close_channel, res) = convert_chan_phase_err!(self, peer_state, e, chan, channel_id, FUNDED_CHANNEL);
 									handle_errors.push((chan.context.get_counterparty_node_id(), Err(res)));
 									!close_channel
 								}
@@ -10404,7 +10398,7 @@ where
 									}
 								}
 							} else if let Err(reason) = res {
-								update_maps_on_chan_removal!(self, &channel.context);
+								update_maps_on_chan_removal!(self, peer_state, &channel.context);
 								// It looks like our counterparty went on-chain or funding transaction was
 								// reorged out of the main chain. Close the channel.
 								let reason_message = format!("{}", reason);
@@ -10833,7 +10827,7 @@ where
 						},
 					};
 					// Clean up for removal.
-					update_maps_on_chan_removal!(self, &context);
+					update_maps_on_chan_removal!(self, peer_state, &context);
 					failed_channels.push(context.force_shutdown(false, ClosureReason::DisconnectedPeer));
 					false
 				});

From 9f9d448efbd0bbcefc2acb1a9d56f1c9bdd96289 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Sun, 29 Sep 2024 19:30:48 +0000
Subject: [PATCH 2/8] Add missing `update_maps_on_chan_removal` call in signer
 restore

When a channel is closed, we have to call
`update_maps_on_chan_removal` in the same per-peer-state lock as
the removal of the `ChannelPhase` object. We forgot to do so in
`ChannelManager::signer_unblocked` leaving dangling references to
the channel.

We also take this opportunity to include more context in the
channel-closure log in `ChannelManager::signer_unblocked` and add
documentation to `update_maps_on_chan_removal` and
`finish_close_channel` to hopefully avoid this issue in the future.
---
 lightning/src/ln/channelmanager.rs | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 5cb92d3cb9e..5315e6a4ef6 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2888,6 +2888,13 @@ macro_rules! handle_error {
 	} };
 }
 
+/// When a channel is removed, two things need to happen:
+/// (a) This must be called in the same `per_peer_state` lock as the channel-closing action,
+/// (b) [`ChannelManager::finish_close_channel`] needs to be called without holding any locks
+///     (except [`ChannelManager::total_consistency_lock`].
+///
+/// Note that this step can be skipped if the channel was never opened (through the creation of a
+/// [`ChannelMonitor`]/channel funding transaction) to begin with.
 macro_rules! update_maps_on_chan_removal {
 	($self: expr, $peer_state: expr, $channel_context: expr) => {{
 		if let Some(outpoint) = $channel_context.get_funding_txo() {
@@ -3762,6 +3769,11 @@ where
 		self.close_channel_internal(channel_id, counterparty_node_id, target_feerate_sats_per_1000_weight, shutdown_script)
 	}
 
+	/// When a channel is removed, two things need to happen:
+	/// (a) [`update_maps_on_chan_removal`] must be called in the same `per_peer_state` lock as
+	///     the channel-closing action,
+	/// (b) this needs to be called without holding any locks (except
+	///     [`ChannelManager::total_consistency_lock`].
 	fn finish_close_channel(&self, mut shutdown_res: ShutdownResult) {
 		debug_assert_ne!(self.per_peer_state.held_by_thread(), LockHeldState::HeldByThread);
 		#[cfg(debug_assertions)]
@@ -9124,7 +9136,10 @@ where
 					_ => unblock_chan(chan, &mut peer_state.pending_msg_events),
 				};
 				if let Some(shutdown_result) = shutdown_result {
-					log_trace!(self.logger, "Removing channel after unblocking signer");
+					let context = &chan.context();
+					let logger = WithChannelContext::from(&self.logger, context, None);
+					log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id());
+					update_maps_on_chan_removal!(self, peer_state, context);
 					shutdown_results.push(shutdown_result);
 					false
 				} else {

From b423a33bc7e90234b5b9b45a960c59d1d0d2fdc7 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Sat, 21 Sep 2024 04:23:09 +0000
Subject: [PATCH 3/8] Avoid a `short_to_chan_info` read lock in
 `claim_funds_from_hop`

In 453ed11f80b40f28b6e95a74b1f7ed2cd7f012ad we started tracking the
counterparty's `node_id` in `HTLCPreviousHopData`, however we were
still trying to look it up using `prev_short_channel_id` in
`claim_funds_from_hop`.

Because we now usually have the counterparty's `node_id` directly
accessible, we should skip the `prev_short_channel_id` lookup.

This will also be more important in the next commit where we need
to look up state for our counterparty to generate
`ChannelMonitorUpdate`s whether we have a live channel or not.
---
 lightning/src/ln/channelmanager.rs | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 5315e6a4ef6..aab2ec03164 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6928,11 +6928,10 @@ where
 		&self, prev_hop: HTLCPreviousHopData, payment_preimage: PaymentPreimage,
 		payment_info: Option<PaymentClaimDetails>, completion_action: ComplFunc,
 	) {
-		let counterparty_node_id =
-			match self.short_to_chan_info.read().unwrap().get(&prev_hop.short_channel_id) {
-				Some((cp_id, _dup_chan_id)) => Some(cp_id.clone()),
-				None => None
-			};
+		let counterparty_node_id = prev_hop.counterparty_node_id.or_else(|| {
+			let short_to_chan_info = self.short_to_chan_info.read().unwrap();
+			short_to_chan_info.get(&prev_hop.short_channel_id).map(|(cp_id, _)| *cp_id)
+		});
 
 		let htlc_source = HTLCClaimSource {
 			counterparty_node_id,

From ebf1de5bc79097e03f1099b009098bb380fbc81f Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 4 Oct 2024 17:54:00 +0000
Subject: [PATCH 4/8] Build `per_peer_state` immediately in `ChannelManager`
 deser

Instead of first building a map from peers to a list of channels
then pulling out of that to build the `per_peer_state`, we build
`per_peer_state` immediately and store channels in it immediately.

This avoids an unnecessary map indirection but also gives us
access to the new fields in `per_peer_state` when reading
`Channel`s which we'll need in a coming commit.
---
 lightning/src/ln/channelmanager.rs | 57 +++++++++++++-----------------
 lightning/src/sync/debug_sync.rs   |  4 +++
 lightning/src/sync/nostd_sync.rs   |  4 +++
 3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index aab2ec03164..d464d74f42e 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -12403,11 +12403,23 @@ where
 		let best_block_height: u32 = Readable::read(reader)?;
 		let best_block_hash: BlockHash = Readable::read(reader)?;
 
-		let mut failed_htlcs = Vec::new();
+		let empty_peer_state = || {
+			PeerState {
+				channel_by_id: new_hash_map(),
+				inbound_channel_request_by_id: new_hash_map(),
+				latest_features: InitFeatures::empty(),
+				pending_msg_events: Vec::new(),
+				in_flight_monitor_updates: BTreeMap::new(),
+				monitor_update_blocked_actions: BTreeMap::new(),
+				actions_blocking_raa_monitor_updates: BTreeMap::new(),
+				is_connected: false,
+			}
+		};
 
+		let mut failed_htlcs = Vec::new();
 		let channel_count: u64 = Readable::read(reader)?;
 		let mut funding_txo_set = hash_set_with_capacity(cmp::min(channel_count as usize, 128));
-		let mut funded_peer_channels: HashMap<PublicKey, HashMap<ChannelId, ChannelPhase<SP>>> = hash_map_with_capacity(cmp::min(channel_count as usize, 128));
+		let mut per_peer_state = hash_map_with_capacity(cmp::min(channel_count as usize, MAX_ALLOC_SIZE/mem::size_of::<(PublicKey, Mutex<PeerState<SP>>)>()));
 		let mut outpoint_to_peer = hash_map_with_capacity(cmp::min(channel_count as usize, 128));
 		let mut short_to_chan_info = hash_map_with_capacity(cmp::min(channel_count as usize, 128));
 		let mut channel_closures = VecDeque::new();
@@ -12495,17 +12507,10 @@ where
 					if let Some(funding_txo) = channel.context.get_funding_txo() {
 						outpoint_to_peer.insert(funding_txo, channel.context.get_counterparty_node_id());
 					}
-					match funded_peer_channels.entry(channel.context.get_counterparty_node_id()) {
-						hash_map::Entry::Occupied(mut entry) => {
-							let by_id_map = entry.get_mut();
-							by_id_map.insert(channel.context.channel_id(), ChannelPhase::Funded(channel));
-						},
-						hash_map::Entry::Vacant(entry) => {
-							let mut by_id_map = new_hash_map();
-							by_id_map.insert(channel.context.channel_id(), ChannelPhase::Funded(channel));
-							entry.insert(by_id_map);
-						}
-					}
+					per_peer_state.entry(channel.context.get_counterparty_node_id())
+						.or_insert_with(|| Mutex::new(empty_peer_state()))
+						.get_mut().unwrap()
+						.channel_by_id.insert(channel.context.channel_id(), ChannelPhase::Funded(channel));
 				}
 			} else if channel.is_awaiting_initial_mon_persist() {
 				// If we were persisted and shut down while the initial ChannelMonitor persistence
@@ -12572,27 +12577,13 @@ where
 			claimable_htlcs_list.push((payment_hash, previous_hops));
 		}
 
-		let peer_state_from_chans = |channel_by_id| {
-			PeerState {
-				channel_by_id,
-				inbound_channel_request_by_id: new_hash_map(),
-				latest_features: InitFeatures::empty(),
-				pending_msg_events: Vec::new(),
-				in_flight_monitor_updates: BTreeMap::new(),
-				monitor_update_blocked_actions: BTreeMap::new(),
-				actions_blocking_raa_monitor_updates: BTreeMap::new(),
-				is_connected: false,
-			}
-		};
-
 		let peer_count: u64 = Readable::read(reader)?;
-		let mut per_peer_state = hash_map_with_capacity(cmp::min(peer_count as usize, MAX_ALLOC_SIZE/mem::size_of::<(PublicKey, Mutex<PeerState<SP>>)>()));
 		for _ in 0..peer_count {
-			let peer_pubkey = Readable::read(reader)?;
-			let peer_chans = funded_peer_channels.remove(&peer_pubkey).unwrap_or(new_hash_map());
-			let mut peer_state = peer_state_from_chans(peer_chans);
-			peer_state.latest_features = Readable::read(reader)?;
-			per_peer_state.insert(peer_pubkey, Mutex::new(peer_state));
+			let peer_pubkey: PublicKey = Readable::read(reader)?;
+			let latest_features = Readable::read(reader)?;
+			if let Some(peer_state) = per_peer_state.get_mut(&peer_pubkey) {
+				peer_state.get_mut().unwrap().latest_features = latest_features;
+			}
 		}
 
 		let event_count: u64 = Readable::read(reader)?;
@@ -12804,7 +12795,7 @@ where
 					// still open, we need to replay any monitor updates that are for closed channels,
 					// creating the neccessary peer_state entries as we go.
 					let peer_state_mutex = per_peer_state.entry(counterparty_id).or_insert_with(|| {
-						Mutex::new(peer_state_from_chans(new_hash_map()))
+						Mutex::new(empty_peer_state())
 					});
 					let mut peer_state = peer_state_mutex.lock().unwrap();
 					handle_in_flight_updates!(counterparty_id, chan_in_flight_updates,
diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs
index 776e35e8ce0..183c074701e 100644
--- a/lightning/src/sync/debug_sync.rs
+++ b/lightning/src/sync/debug_sync.rs
@@ -311,6 +311,10 @@ impl<T> Mutex<T> {
 		}
 		res
 	}
+
+	pub fn get_mut<'a>(&'a mut self) -> LockResult<&'a mut T> {
+		self.inner.get_mut().map_err(|_| ())
+	}
 }
 
 impl<'a, T: 'a> LockTestExt<'a> for Mutex<T> {
diff --git a/lightning/src/sync/nostd_sync.rs b/lightning/src/sync/nostd_sync.rs
index 03fa65b69c6..b3963da762e 100644
--- a/lightning/src/sync/nostd_sync.rs
+++ b/lightning/src/sync/nostd_sync.rs
@@ -40,6 +40,10 @@ impl<T> Mutex<T> {
 	pub fn into_inner(self) -> LockResult<T> {
 		Ok(self.inner.into_inner())
 	}
+
+	pub fn get_mut<'a>(&'a mut self) -> LockResult<&'a mut T> {
+		Ok(self.inner.get_mut())
+	}
 }
 
 impl<'a, T: 'a> LockTestExt<'a> for Mutex<T> {

From 6f023f8f0896545fdac3aa30eb9e0003a58897fe Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Wed, 9 Oct 2024 19:05:18 +0000
Subject: [PATCH 5/8] Req the counterparty node id when claiming against a
 closed chan

Currently we store in-flight `ChannelMonitorUpdate`s in the
per-peer structure in `ChannelManager`. This is nice and simple as
we're generally updating it when we're updating other per-peer
data, so we already have the relevant lock(s) and map entries.

Sadly, when we're claiming an HTLC against a closed channel, we
didn't have the `counterparty_node_id` available until it was
added in 0.0.124 (and now we only have it for HTLCs which were
forwarded in 0.0.124). This means we can't look up the per-peer
structure when claiming old HTLCs, making it difficult to track the
new `ChannelMonitorUpdate` as in-flight.

While we could transition the in-flight `ChannelMonitorUpdate`
tracking to a new global map indexed by `OutPoint`, doing so would
result in a major lock which would be highly contended across
channels with different peers.

Instead, as we move towards tracking in-flight
`ChannelMonitorUpdate`s for closed channels we'll keep our existing
storage, leaving only the `counterparty_node_id` issue to contend
with.

Here we simply accept the issue, requiring that
`counterparty_node_id` be available when claiming HTLCs against a
closed channel. On startup, we explicitly check for any forwarded
HTLCs which came from a closed channel where the forward happened
prior to 0.0.124, failing to deserialize, or logging an warning if
the channel is still open (implying things may work out, but panics
may occur if the channel closes prior to HTLC resolution).

While this is a somewhat dissapointing resolution, LDK nodes which
forward HTLCs are generally fairly well-upgraded, so it is not
anticipated to be an issue in practice.
---
 lightning/src/ln/channelmanager.rs         | 181 +++++++++++++++++----
 pending_changelog/matt-no-upgrade-skip.txt |   6 +
 2 files changed, 152 insertions(+), 35 deletions(-)
 create mode 100644 pending_changelog/matt-no-upgrade-skip.txt

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index d464d74f42e..30fea378022 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -40,7 +40,7 @@ use crate::blinded_path::payment::{BlindedPaymentPath, Bolt12OfferContext, Bolt1
 use crate::chain;
 use crate::chain::{Confirm, ChannelMonitorUpdateStatus, Watch, BestBlock};
 use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator};
-use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID};
+use crate::chain::channelmonitor::{Balance, ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::events;
 use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination, PaymentFailureReason, ReplayEvent};
@@ -7082,6 +7082,16 @@ where
 			channel_id: Some(prev_hop.channel_id),
 		};
 
+		if prev_hop.counterparty_node_id.is_none() {
+			let payment_hash: PaymentHash = payment_preimage.into();
+			panic!(
+				"Prior to upgrading to LDK 0.1, all pending HTLCs forwarded by LDK 0.0.123 or before must be resolved. It appears at least the HTLC with payment_hash {} (preimage {}) was not resolved. Please downgrade to LDK 0.0.125 and resolve the HTLC prior to upgrading.",
+				payment_hash,
+				payment_preimage,
+			);
+		}
+		let counterparty_node_id = prev_hop.counterparty_node_id.expect("Checked immediately above");
+
 		if !during_init {
 			// We update the ChannelMonitor on the backward link, after
 			// receiving an `update_fulfill_htlc` from the forward link.
@@ -7119,40 +7129,25 @@ where
 		let (action_opt, raa_blocker_opt) = completion_action(None, false);
 
 		if let Some(raa_blocker) = raa_blocker_opt {
-			let counterparty_node_id = prev_hop.counterparty_node_id.or_else(||
-				// prev_hop.counterparty_node_id is always available for payments received after
-				// LDK 0.0.123, but for those received on 0.0.123 and claimed later, we need to
-				// look up the counterparty in the `action_opt`, if possible.
-				action_opt.as_ref().and_then(|action|
-					if let MonitorUpdateCompletionAction::PaymentClaimed { pending_mpp_claim, .. } = action {
-						pending_mpp_claim.as_ref().map(|(node_id, _, _, _)| *node_id)
-					} else { None }
-				)
-			);
-			if let Some(counterparty_node_id) = counterparty_node_id {
-				// TODO: Avoid always blocking the world for the write lock here.
-				let mut per_peer_state = self.per_peer_state.write().unwrap();
-				let peer_state_mutex = per_peer_state.entry(counterparty_node_id).or_insert_with(||
-					Mutex::new(PeerState {
-						channel_by_id: new_hash_map(),
-						inbound_channel_request_by_id: new_hash_map(),
-						latest_features: InitFeatures::empty(),
-						pending_msg_events: Vec::new(),
-						in_flight_monitor_updates: BTreeMap::new(),
-						monitor_update_blocked_actions: BTreeMap::new(),
-						actions_blocking_raa_monitor_updates: BTreeMap::new(),
-						is_connected: false,
-					}));
-				let mut peer_state = peer_state_mutex.lock().unwrap();
+			// TODO: Avoid always blocking the world for the write lock here.
+			let mut per_peer_state = self.per_peer_state.write().unwrap();
+			let peer_state_mutex = per_peer_state.entry(counterparty_node_id).or_insert_with(||
+				Mutex::new(PeerState {
+					channel_by_id: new_hash_map(),
+					inbound_channel_request_by_id: new_hash_map(),
+					latest_features: InitFeatures::empty(),
+					pending_msg_events: Vec::new(),
+					in_flight_monitor_updates: BTreeMap::new(),
+					monitor_update_blocked_actions: BTreeMap::new(),
+					actions_blocking_raa_monitor_updates: BTreeMap::new(),
+					is_connected: false,
+				}));
+			let mut peer_state = peer_state_mutex.lock().unwrap();
 
-				peer_state.actions_blocking_raa_monitor_updates
-					.entry(prev_hop.channel_id)
-					.or_default()
-					.push(raa_blocker);
-			} else {
-				debug_assert!(false,
-					"RAA ChannelMonitorUpdate blockers are only set with PaymentClaimed completion actions, so we should always have a counterparty node id");
-			}
+			peer_state.actions_blocking_raa_monitor_updates
+				.entry(prev_hop.channel_id)
+				.or_default()
+				.push(raa_blocker);
 		}
 
 		self.handle_monitor_update_completion_actions(action_opt);
@@ -12928,11 +12923,97 @@ where
 				// Whether the downstream channel was closed or not, try to re-apply any payment
 				// preimages from it which may be needed in upstream channels for forwarded
 				// payments.
+				let mut fail_read = false;
 				let outbound_claimed_htlcs_iter = monitor.get_all_current_outbound_htlcs()
 					.into_iter()
 					.filter_map(|(htlc_source, (htlc, preimage_opt))| {
-						if let HTLCSource::PreviousHopData(_) = htlc_source {
+						if let HTLCSource::PreviousHopData(prev_hop) = &htlc_source {
 							if let Some(payment_preimage) = preimage_opt {
+								let inbound_edge_monitor = args.channel_monitors.get(&prev_hop.outpoint);
+								// Note that for channels which have gone to chain,
+								// `get_all_current_outbound_htlcs` is never pruned and always returns
+								// a constant set until the monitor is removed/archived. Thus, we
+								// want to skip replaying claims that have definitely been resolved
+								// on-chain.
+
+								// If the inbound monitor is not present, we assume it was fully
+								// resolved and properly archived, implying this payment had plenty
+								// of time to get claimed and we can safely skip any further
+								// attempts to claim it (they wouldn't succeed anyway as we don't
+								// have a monitor against which to do so).
+								let inbound_edge_monitor = if let Some(monitor) = inbound_edge_monitor {
+									monitor
+								} else {
+									return None;
+								};
+								// Second, if the inbound edge of the payment's monitor has been
+								// fully claimed we've had at least `ANTI_REORG_DELAY` blocks to
+								// get any PaymentForwarded event(s) to the user and assume that
+								// there's no need to try to replay the claim just for that.
+								let inbound_edge_balances = inbound_edge_monitor.get_claimable_balances();
+								if inbound_edge_balances.is_empty() {
+									return None;
+								}
+
+								if prev_hop.counterparty_node_id.is_none() {
+									// We no longer support claiming an HTLC where we don't have
+									// the counterparty_node_id available if the claim has to go to
+									// a closed channel. Its possible we can get away with it if
+									// the channel is not yet closed, but its by no means a
+									// guarantee.
+
+									// Thus, in this case we are a bit more aggressive with our
+									// pruning - if we have no use for the claim (because the
+									// inbound edge of the payment's monitor has already claimed
+									// the HTLC) we skip trying to replay the claim.
+									let htlc_payment_hash: PaymentHash = payment_preimage.into();
+									let balance_could_incl_htlc = |bal| match bal {
+										&Balance::ClaimableOnChannelClose { .. } => {
+											// The channel is still open, assume we can still
+											// claim against it
+											true
+										},
+										&Balance::MaybePreimageClaimableHTLC { payment_hash, .. } => {
+											payment_hash == htlc_payment_hash
+										},
+										_ => false,
+									};
+									let htlc_may_be_in_balances =
+										inbound_edge_balances.iter().any(balance_could_incl_htlc);
+									if !htlc_may_be_in_balances {
+										return None;
+									}
+
+									// First check if we're absolutely going to fail - if we need
+									// to replay this claim to get the preimage into the inbound
+									// edge monitor but the channel is closed (and thus we'll
+									// immediately panic if we call claim_funds_from_hop).
+									if short_to_chan_info.get(&prev_hop.short_channel_id).is_none() {
+										log_error!(args.logger,
+											"We need to replay the HTLC claim for payment_hash {} (preimage {}) but cannot do so as the HTLC was forwarded prior to LDK 0.0.124.\
+											All HTLCs that were forwarded by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1",
+											htlc_payment_hash,
+											payment_preimage,
+										);
+										fail_read = true;
+									}
+
+									// At this point we're confident we need the claim, but the
+									// inbound edge channel is still live. As long as this remains
+									// the case, we can conceivably proceed, but we run some risk
+									// of panicking at runtime. The user ideally should have read
+									// the release notes and we wouldn't be here, but we go ahead
+									// and let things run in the hope that it'll all just work out.
+									log_error!(args.logger,
+										"We need to replay the HTLC claim for payment_hash {} (preimage {}) but don't have all the required information to do so reliably.\
+										As long as the channel for the inbound edge of the forward remains open, this may work okay, but we may panic at runtime!\
+										All HTLCs that were forwarded by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1\
+										Continuing anyway, though panics may occur!",
+										htlc_payment_hash,
+										payment_preimage,
+									);
+								}
+
 								Some((htlc_source, payment_preimage, htlc.amount_msat,
 									// Check if `counterparty_opt.is_none()` to see if the
 									// downstream chan is closed (because we don't have a
@@ -12952,6 +13033,9 @@ where
 				for tuple in outbound_claimed_htlcs_iter {
 					pending_claims_to_replay.push(tuple);
 				}
+				if fail_read {
+					return Err(DecodeError::InvalidValue);
+				}
 			}
 		}
 
@@ -13028,6 +13112,33 @@ where
 			}
 		}
 
+		// Similar to the above cases for forwarded payments, if we have any pending inbound HTLCs
+		// which haven't yet been claimed, we may be missing counterparty_node_id info and would
+		// panic if we attempted to claim them at this point.
+		for (payment_hash, payment) in claimable_payments.iter() {
+			for htlc in payment.htlcs.iter() {
+				if htlc.prev_hop.counterparty_node_id.is_some() {
+					continue;
+				}
+				if short_to_chan_info.get(&htlc.prev_hop.short_channel_id).is_some() {
+					log_error!(args.logger,
+						"We do not have the required information to claim a pending payment with payment hash {} reliably.\
+						As long as the channel for the inbound edge of the forward remains open, this may work okay, but we may panic at runtime!\
+						All HTLCs that were received by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1\
+						Continuing anyway, though panics may occur!",
+						payment_hash,
+					);
+				} else {
+					log_error!(args.logger,
+						"We do not have the required information to claim a pending payment with payment hash {}.\
+						All HTLCs that were received by LDK 0.0.123 and prior must be resolved prior to upgrading to LDK 0.1",
+						payment_hash,
+					);
+					return Err(DecodeError::InvalidValue);
+				}
+			}
+		}
+
 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&args.entropy_source.get_secure_random_bytes());
 
diff --git a/pending_changelog/matt-no-upgrade-skip.txt b/pending_changelog/matt-no-upgrade-skip.txt
new file mode 100644
index 00000000000..f5fcb8c5f25
--- /dev/null
+++ b/pending_changelog/matt-no-upgrade-skip.txt
@@ -0,0 +1,6 @@
+## Backwards Compatibility
+ * Nodes with pending forwarded HTLCs or unclaimed payments cannot be
+   upgraded directly from 0.0.123 or earlier to 0.1. Instead, they must
+   first either resolve all pending HTLCs (including those pending
+   resolution on-chain), or run 0.0.124 and resolve any HTLCs that were
+   originally forwarded or received running 0.0.123 or earlier.

From 3f368909121897d35cfd3dc77e743531c7301ee8 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Sun, 6 Oct 2024 19:54:32 +0000
Subject: [PATCH 6/8] Prefer to use `MonitorUpdateRegeneratedOnStartup` where
 possible

In the next commit we'll drop the magic `u64::MAX`
`ChannelMonitorUpdate::update_id` value used when we don't know the
`ChannelMonitor`'s `latest_update_id` (i.e. when the channel is
closed). In order to do so, we will store further information about
`ChannelMonitor`s in the per-peer structure, keyed by the
counterparty's node ID, which will be used when applying
`ChannelMonitorUpdate`s to closed channels.

By taking advantage of the change in the previous commit, that
information is now reliably available when we generate the
`ChannelMonitorUpdate` (when claiming HTLCs), but in order to
ensure it is available when applying the `ChannelMonitorUpdate` we
need to use `BackgroundEvent::MonitorUpdateRegeneratedOnStartup`
instead of
`BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup` where
possible.

Here we do this, leaving `ClosedMonitorUpdateRegeneratedOnStartup`
only used to ensure very old channels (created in 0.0.118 or
earlier) which are not in the `ChannelManager` are force-closed on
startup.
---
 lightning/src/ln/channelmanager.rs | 63 +++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 30fea378022..c9ffc73a3f2 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -973,9 +973,9 @@ impl ClaimablePayments {
 #[derive(Debug)]
 enum BackgroundEvent {
 	/// Handle a ChannelMonitorUpdate which closes the channel or for an already-closed channel.
-	/// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as the
-	/// maybe-non-closing variant needs a public key to handle channel resumption, whereas if the
-	/// channel has been force-closed we do not need the counterparty node_id.
+	/// This is only separated from [`Self::MonitorUpdateRegeneratedOnStartup`] as for truly
+	/// ancient [`ChannelMonitor`]s that haven't seen an update since LDK 0.0.118 we may not have
+	/// the counterparty node ID available.
 	///
 	/// Note that any such events are lost on shutdown, so in general they must be updates which
 	/// are regenerated on startup.
@@ -7109,17 +7109,15 @@ where
 			// If we're running during init we cannot update a monitor directly - they probably
 			// haven't actually been loaded yet. Instead, push the monitor update as a background
 			// event.
-			// Note that while it's safe to use `ClosedMonitorUpdateRegeneratedOnStartup` here (the
-			// channel is already closed) we need to ultimately handle the monitor update
-			// completion action only after we've completed the monitor update. This is the only
-			// way to guarantee this update *will* be regenerated on startup (otherwise if this was
-			// from a forwarded HTLC the downstream preimage may be deleted before we claim
-			// upstream). Thus, we need to transition to some new `BackgroundEvent` type which will
-			// complete the monitor update completion action from `completion_action`.
-			self.pending_background_events.lock().unwrap().push(
-				BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((
-					prev_hop.funding_txo, prev_hop.channel_id, preimage_update,
-				)));
+			// TODO: Track this update as pending and only complete the completion action when it
+			// finishes.
+			let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+				counterparty_node_id,
+				funding_txo: prev_hop.funding_txo,
+				channel_id: prev_hop.channel_id,
+				update: preimage_update,
+			};
+			self.pending_background_events.lock().unwrap().push(event);
 		}
 		// Note that we do process the completion action here. This totally could be a
 		// duplicate claim, but we have no way of knowing without interrogating the
@@ -7212,22 +7210,35 @@ where
 								// There should be a `BackgroundEvent` pending...
 								assert!(background_events.iter().any(|ev| {
 									match ev {
-										// to apply a monitor update that blocked the claiming channel,
 										BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
 											funding_txo, update, ..
 										} => {
 											if *funding_txo == claiming_chan_funding_outpoint {
+												// to apply a monitor update that blocked the claiming channel,
 												assert!(update.updates.iter().any(|upd|
 													if let ChannelMonitorUpdateStep::PaymentPreimage {
 														payment_preimage: update_preimage, ..
 													} = upd {
 														payment_preimage == *update_preimage
-													} else { false }
+													} else {
+														false
+													}
+												), "{:?}", update);
+												true
+											} else if *funding_txo == next_channel_outpoint {
+												// or the channel we'd unblock is already closed,
+												assert!(update.updates.iter().any(|upd|
+													if let ChannelMonitorUpdateStep::ChannelForceClosed { .. } = upd {
+														true
+													} else {
+														false
+													}
 												), "{:?}", update);
 												true
 											} else { false }
 										},
-										// or the channel we'd unblock is already closed,
+										// or the channel we'd unblock is already closed (for an
+										// old channel),
 										BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(
 											(funding_txo, _channel_id, monitor_update)
 										) => {
@@ -12543,7 +12554,23 @@ where
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }],
 					channel_id: Some(monitor.channel_id()),
 				};
-				close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update)));
+				if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() {
+					let update = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+						counterparty_node_id,
+						funding_txo: *funding_txo,
+						channel_id,
+						update: monitor_update,
+					};
+					close_background_events.push(update);
+				} else {
+					// This is a fairly old `ChannelMonitor` that hasn't seen an update to its
+					// off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain
+					// `ChannelMonitorUpdate` will set the counterparty ID).
+					// Thus, we assume that it has no pending HTLCs and we will not need to
+					// generate a `ChannelMonitorUpdate` for it aside from this
+					// `ChannelForceClosed` one.
+					close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update)));
+				}
 			}
 		}
 

From c99d3d785dd78f594837d283636b82222c3b9ef1 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Sun, 6 Oct 2024 19:58:29 +0000
Subject: [PATCH 7/8] Stop using a constant for monitor `update_id`s after
 closure

Because `ChannelManager` doesn't have a corresponding `Channel`
after the channels are closed, we'd always used an `update_id` of
`u64::MAX` for any `ChannelMonitorUpdate`s we need to build after
the channel is closed.

This completely breaks the abstraction of `update_id`s and leaks
into persistence logic - because we might have more than one
`ChannelMonitorUpdate` with the same (`u64::MAX`) value, suddenly
instead of being able to safely use `update_id` as IDs, the
`MonitorUpdatingPersister` has to have special logic to handle
this.

Worse, because we don't have a unique ID with which to refer to
post-close `ChannelMonitorUpdate`s we cannot track when they
complete async persistence. This means we cannot properly support
async persist for forwarded payments where the inbound edge has hit
the chain prior to the preimage coming to us.

Here we rectify this by using consistent `update_id`s even after a
channel has closed. In order to do so we have to keep some state
for all channels for which the `ChannelMonitor` has not been
archived (after which point we can be confident we will not need to
update them). While this violates our long-standing policy of
having no state at all in `ChannelManager`s for closed channels,
its only a `(ChannelId, u64)` pair per channel, so shouldn't be
problematic for any of our users (as they already store a whole
honkin `ChannelMonitor` for these channels anyway).

While limited changes are made to the connection-count-limiting
logic, reviewers should carefully analyze the interactions the new
map created here has with that logic.
---
 lightning-persister/src/test_utils.rs |   3 +-
 lightning/src/chain/channelmonitor.rs |  64 +++++----
 lightning/src/ln/channel.rs           |   4 +-
 lightning/src/ln/channelmanager.rs    | 191 +++++++++++++++++++++-----
 lightning/src/ln/functional_tests.rs  |  11 +-
 lightning/src/ln/reload_tests.rs      |   5 +-
 lightning/src/util/persist.rs         | 168 +++++++---------------
 7 files changed, 256 insertions(+), 190 deletions(-)

diff --git a/lightning-persister/src/test_utils.rs b/lightning-persister/src/test_utils.rs
index 5dd28f98b9a..735870e49f7 100644
--- a/lightning-persister/src/test_utils.rs
+++ b/lightning-persister/src/test_utils.rs
@@ -1,4 +1,3 @@
-use lightning::chain::channelmonitor::CLOSED_CHANNEL_UPDATE_ID;
 use lightning::events::ClosureReason;
 use lightning::ln::functional_test_utils::{
 	connect_block, create_announced_chan_between_nodes, create_chanmon_cfgs, create_dummy_block,
@@ -168,5 +167,5 @@ pub(crate) fn do_test_store<K: KVStore>(store_0: &K, store_1: &K) {
 	check_added_monitors!(nodes[1], 1);
 
 	// Make sure everything is persisted as expected after close.
-	check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID);
+	check_persisted_data!(11);
 }
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 2d76c09f1bb..8653dc8274c 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -89,11 +89,9 @@ pub struct ChannelMonitorUpdate {
 	/// [`ChannelMonitorUpdateStatus::InProgress`] have been applied to all copies of a given
 	/// ChannelMonitor when ChannelManager::channel_monitor_updated is called.
 	///
-	/// The only instances we allow where update_id values are not strictly increasing have a
-	/// special update ID of [`CLOSED_CHANNEL_UPDATE_ID`]. This update ID is used for updates that
-	/// will force close the channel by broadcasting the latest commitment transaction or
-	/// special post-force-close updates, like providing preimages necessary to claim outputs on the
-	/// broadcast commitment transaction. See its docs for more details.
+	/// Note that for [`ChannelMonitorUpdate`]s generated on LDK versions prior to 0.1 after the
+	/// channel was closed, this value may be [`u64::MAX`]. In that case, multiple updates may
+	/// appear with the same ID, and all should be replayed.
 	///
 	/// [`ChannelMonitorUpdateStatus::InProgress`]: super::ChannelMonitorUpdateStatus::InProgress
 	pub update_id: u64,
@@ -104,15 +102,9 @@ pub struct ChannelMonitorUpdate {
 	pub channel_id: Option<ChannelId>,
 }
 
-/// The update ID used for a [`ChannelMonitorUpdate`] that is either:
-///
-///	(1) attempting to force close the channel by broadcasting our latest commitment transaction or
-///	(2) providing a preimage (after the channel has been force closed) from a forward link that
-///		allows us to spend an HTLC output on this channel's (the backward link's) broadcasted
-///		commitment transaction.
-///
-/// No other [`ChannelMonitorUpdate`]s are allowed after force-close.
-pub const CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX;
+/// LDK prior to 0.1 used this constant as the [`ChannelMonitorUpdate::update_id`] for any
+/// [`ChannelMonitorUpdate`]s which were generated after the channel was closed.
+const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = core::u64::MAX;
 
 impl Writeable for ChannelMonitorUpdate {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
@@ -1553,6 +1545,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 
 	/// Gets the update_id from the latest ChannelMonitorUpdate which was applied to this
 	/// ChannelMonitor.
+	///
+	/// Note that for channels closed prior to LDK 0.1, this may return [`u64::MAX`].
 	pub fn get_latest_update_id(&self) -> u64 {
 		self.inner.lock().unwrap().get_latest_update_id()
 	}
@@ -3116,11 +3110,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		F::Target: FeeEstimator,
 		L::Target: Logger,
 	{
-		if self.latest_update_id == CLOSED_CHANNEL_UPDATE_ID && updates.update_id == CLOSED_CHANNEL_UPDATE_ID {
-			log_info!(logger, "Applying post-force-closed update to monitor {} with {} change(s).",
+		if self.latest_update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID && updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID {
+			log_info!(logger, "Applying pre-0.1 post-force-closed update to monitor {} with {} change(s).",
 				log_funding_info!(self), updates.updates.len());
-		} else if updates.update_id == CLOSED_CHANNEL_UPDATE_ID {
-			log_info!(logger, "Applying force close update to monitor {} with {} change(s).",
+		} else if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID {
+			log_info!(logger, "Applying pre-0.1 force close update to monitor {} with {} change(s).",
 				log_funding_info!(self), updates.updates.len());
 		} else {
 			log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} change(s).",
@@ -3143,14 +3137,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		// The `ChannelManager` may also queue redundant `ChannelForceClosed` updates if it still
 		// thinks the channel needs to have its commitment transaction broadcast, so we'll allow
 		// them as well.
-		if updates.update_id == CLOSED_CHANNEL_UPDATE_ID {
+		if updates.update_id == LEGACY_CLOSED_CHANNEL_UPDATE_ID || self.lockdown_from_offchain {
 			assert_eq!(updates.updates.len(), 1);
 			match updates.updates[0] {
 				ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
 				// We should have already seen a `ChannelForceClosed` update if we're trying to
 				// provide a preimage at this point.
 				ChannelMonitorUpdateStep::PaymentPreimage { .. } =>
-					debug_assert_eq!(self.latest_update_id, CLOSED_CHANNEL_UPDATE_ID),
+					debug_assert!(self.lockdown_from_offchain),
 				_ => {
 					log_error!(logger, "Attempted to apply post-force-close ChannelMonitorUpdate of type {}", updates.updates[0].variant_name());
 					panic!("Attempted to apply post-force-close ChannelMonitorUpdate that wasn't providing a payment preimage");
@@ -3230,17 +3224,29 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			self.counterparty_commitment_txs_from_update(updates);
 		}
 
-		// If the updates succeeded and we were in an already closed channel state, then there's no
-		// need to refuse any updates we expect to receive afer seeing a confirmed commitment.
-		if ret.is_ok() && updates.update_id == CLOSED_CHANNEL_UPDATE_ID && self.latest_update_id == updates.update_id {
-			return Ok(());
-		}
-
 		self.latest_update_id = updates.update_id;
 
-		// Refuse updates after we've detected a spend onchain, but only if we haven't processed a
-		// force closed monitor update yet.
-		if ret.is_ok() && self.funding_spend_seen && self.latest_update_id != CLOSED_CHANNEL_UPDATE_ID {
+		// Refuse updates after we've detected a spend onchain (or if the channel was otherwise
+		// closed), but only if the update isn't the kind of update we expect to see after channel
+		// closure.
+		let mut is_pre_close_update = false;
+		for update in updates.updates.iter() {
+			match update {
+				ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { .. }
+					|ChannelMonitorUpdateStep::LatestCounterpartyCommitmentTXInfo { .. }
+					|ChannelMonitorUpdateStep::ShutdownScript { .. }
+					|ChannelMonitorUpdateStep::CommitmentSecret { .. } =>
+						is_pre_close_update = true,
+				// After a channel is closed, we don't communicate with our peer about it, so the
+				// only things we will update is getting a new preimage (from a different channel)
+				// or being told that the channel is closed. All other updates are generated while
+				// talking to our peer.
+				ChannelMonitorUpdateStep::PaymentPreimage { .. } => {},
+				ChannelMonitorUpdateStep::ChannelForceClosed { .. } => {},
+			}
+		}
+
+		if ret.is_ok() && (self.funding_spend_seen || self.lockdown_from_offchain) && is_pre_close_update {
 			log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent");
 			Err(())
 		} else { ret }
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index cd9d5bf8b2c..80aa0b8479c 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -45,7 +45,7 @@ use crate::ln::chan_utils;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::chain::BestBlock;
 use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator};
-use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS, CLOSED_CHANNEL_UPDATE_ID};
+use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::sign::ecdsa::EcdsaChannelSigner;
 use crate::sign::{EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient};
@@ -3656,7 +3656,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 			// monitor update to the user, even if we return one).
 			// See test_duplicate_chan_id and test_pre_lockin_no_chan_closed_update for more.
 			if !self.channel_state.is_pre_funded_state() {
-				self.latest_monitor_update_id = CLOSED_CHANNEL_UPDATE_ID;
+				self.latest_monitor_update_id += 1;
 				Some((self.get_counterparty_node_id(), funding_txo, self.channel_id(), ChannelMonitorUpdate {
 					update_id: self.latest_monitor_update_id,
 					counterparty_node_id: Some(self.counterparty_node_id),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index c9ffc73a3f2..f06a3af5243 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -40,7 +40,7 @@ use crate::blinded_path::payment::{BlindedPaymentPath, Bolt12OfferContext, Bolt1
 use crate::chain;
 use crate::chain::{Confirm, ChannelMonitorUpdateStatus, Watch, BestBlock};
 use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator};
-use crate::chain::channelmonitor::{Balance, ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID};
+use crate::chain::channelmonitor::{Balance, ChannelMonitor, ChannelMonitorUpdate, WithChannelMonitor, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::events;
 use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination, PaymentFailureReason, ReplayEvent};
@@ -1299,6 +1299,13 @@ pub(super) struct PeerState<SP: Deref> where SP::Target: SignerProvider {
 	/// will remove a preimage that needs to be durably in an upstream channel first), we put an
 	/// entry here to note that the channel with the key's ID is blocked on a set of actions.
 	actions_blocking_raa_monitor_updates: BTreeMap<ChannelId, Vec<RAAMonitorUpdateBlockingAction>>,
+	/// The latest [`ChannelMonitor::get_latest_update_id`] value for all closed channels as they
+	/// exist on-disk/in our [`chain::Watch`]. This *ignores* all pending updates not yet applied
+	/// in [`ChannelManager::pending_background_events`].
+	///
+	/// If there are any updates pending in [`Self::in_flight_monitor_updates`] this will contain
+	/// the highest `update_id` of all the pending in-flight updates.
+	closed_channel_monitor_update_ids: BTreeMap<ChannelId, u64>,
 	/// The peer is currently connected (i.e. we've seen a
 	/// [`ChannelMessageHandler::peer_connected`] and no corresponding
 	/// [`ChannelMessageHandler::peer_disconnected`].
@@ -1325,6 +1332,7 @@ impl <SP: Deref> PeerState<SP> where SP::Target: SignerProvider {
 		)
 			&& self.monitor_update_blocked_actions.is_empty()
 			&& self.in_flight_monitor_updates.is_empty()
+			&& self.closed_channel_monitor_update_ids.is_empty()
 	}
 
 	// Returns a count of all channels we have with this peer, including unfunded channels.
@@ -2897,6 +2905,32 @@ macro_rules! handle_error {
 /// [`ChannelMonitor`]/channel funding transaction) to begin with.
 macro_rules! update_maps_on_chan_removal {
 	($self: expr, $peer_state: expr, $channel_context: expr) => {{
+		// If there's a possibility that we need to generate further monitor updates for this
+		// channel, we need to store the last update_id of it. However, we don't want to insert
+		// into the map (which prevents the `PeerState` from being cleaned up) for channels that
+		// never even got confirmations (which would open us up to DoS attacks).
+		let mut update_id = $channel_context.get_latest_monitor_update_id();
+		if $channel_context.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth() == Some(0) || update_id > 1 {
+			// There may be some pending background events which we have to ignore when setting the
+			// latest update ID.
+			for event in $self.pending_background_events.lock().unwrap().iter() {
+				match event {
+					BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, channel_id, update, .. } => {
+						if *channel_id == $channel_context.channel_id() && *counterparty_node_id == $channel_context.get_counterparty_node_id() {
+							update_id = cmp::min(update_id, update.update_id - 1);
+						}
+					},
+					BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(..) => {
+						// This is only generated for very old channels which were already closed
+						// on startup, so it should never be present for a channel that is closing
+						// here.
+					},
+					BackgroundEvent::MonitorUpdatesComplete { .. } => {},
+				}
+			}
+			let chan_id = $channel_context.channel_id();
+			$peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id);
+		}
 		if let Some(outpoint) = $channel_context.get_funding_txo() {
 			$self.outpoint_to_peer.lock().unwrap().remove(&outpoint);
 		}
@@ -3769,6 +3803,64 @@ where
 		self.close_channel_internal(channel_id, counterparty_node_id, target_feerate_sats_per_1000_weight, shutdown_script)
 	}
 
+	/// Ensures any saved latest ID in [`PeerState::closed_channel_monitor_update_ids`] is updated,
+	/// then applies the provided [`ChannelMonitorUpdate`].
+	#[must_use]
+	fn apply_post_close_monitor_update(
+		&self, counterparty_node_id: PublicKey, channel_id: ChannelId, funding_txo: OutPoint,
+		mut monitor_update: ChannelMonitorUpdate,
+	) -> ChannelMonitorUpdateStatus {
+		// Note that there may be some post-close updates which need to be well-ordered with
+		// respect to the `update_id`, so we hold the `closed_channel_monitor_update_ids` lock
+		// here (and also make sure the `monitor_update` we're applying has the right id.
+		let per_peer_state = self.per_peer_state.read().unwrap();
+		let mut peer_state_lock = per_peer_state.get(&counterparty_node_id)
+			.expect("We must always have a peer entry for a peer with which we have channels that have ChannelMonitors")
+			.lock().unwrap();
+		let peer_state = &mut *peer_state_lock;
+		match peer_state.channel_by_id.entry(channel_id) {
+			hash_map::Entry::Occupied(mut chan_phase) => {
+				if let ChannelPhase::Funded(chan) = chan_phase.get_mut() {
+					let in_flight = handle_new_monitor_update!(self, funding_txo,
+						monitor_update, peer_state_lock, peer_state, per_peer_state, chan);
+					return if in_flight { ChannelMonitorUpdateStatus::InProgress } else { ChannelMonitorUpdateStatus::Completed };
+				} else {
+					debug_assert!(false, "We shouldn't have an update for a non-funded channel");
+				}
+			},
+			hash_map::Entry::Vacant(_) => {},
+		}
+		match peer_state.closed_channel_monitor_update_ids.entry(channel_id) {
+			btree_map::Entry::Vacant(entry) => {
+				let is_closing_unupdated_monitor = monitor_update.update_id == 1
+					&& monitor_update.updates.len() == 1
+					&& matches!(&monitor_update.updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. });
+				// If the ChannelMonitorUpdate is closing a channel that never got past initial
+				// funding (to have any commitment updates), we'll skip inserting in
+				// `update_maps_on_chan_removal`, allowing us to avoid keeping around the PeerState
+				// for that peer. In that specific case we expect no entry in the map here. In any
+				// other cases, this is a bug, but in production we go ahead and recover by
+				// inserting the update_id and hoping its right.
+				debug_assert!(is_closing_unupdated_monitor, "Expected closing monitor against an unused channel, got {:?}", monitor_update);
+				if !is_closing_unupdated_monitor {
+					entry.insert(monitor_update.update_id);
+				}
+			},
+			btree_map::Entry::Occupied(entry) => {
+				// If we're running in a threaded environment its possible we generate updates for
+				// a channel that is closing, then apply some preimage update, then go back and
+				// apply the close monitor update here. In order to ensure the updates are still
+				// well-ordered, we have to use the `closed_channel_monitor_update_ids` map to
+				// override the `update_id`, taking care to handle old monitors where the
+				// `latest_update_id` is already `u64::MAX`.
+				let latest_update_id = entry.into_mut();
+				*latest_update_id = latest_update_id.saturating_add(1);
+				monitor_update.update_id = *latest_update_id;
+			}
+		}
+		self.chain_monitor.update_channel(funding_txo, &monitor_update)
+	}
+
 	/// When a channel is removed, two things need to happen:
 	/// (a) [`update_maps_on_chan_removal`] must be called in the same `per_peer_state` lock as
 	///     the channel-closing action,
@@ -3798,7 +3890,7 @@ where
 			// force-closing. The monitor update on the required in-memory copy should broadcast
 			// the latest local state, which is the best we can do anyway. Thus, it is safe to
 			// ignore the result here.
-			let _ = self.chain_monitor.update_channel(funding_txo, &monitor_update);
+			let _ = self.apply_post_close_monitor_update(shutdown_res.counterparty_node_id, shutdown_res.channel_id, funding_txo, monitor_update);
 		}
 		let mut shutdown_results = Vec::new();
 		if let Some(txid) = shutdown_res.unbroadcasted_batch_funding_txid {
@@ -6148,30 +6240,9 @@ where
 					let _ = self.chain_monitor.update_channel(funding_txo, &update);
 				},
 				BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => {
-					let mut updated_chan = false;
-					{
-						let per_peer_state = self.per_peer_state.read().unwrap();
-						if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) {
-							let mut peer_state_lock = peer_state_mutex.lock().unwrap();
-							let peer_state = &mut *peer_state_lock;
-							match peer_state.channel_by_id.entry(channel_id) {
-								hash_map::Entry::Occupied(mut chan_phase) => {
-									if let ChannelPhase::Funded(chan) = chan_phase.get_mut() {
-										updated_chan = true;
-										handle_new_monitor_update!(self, funding_txo, update.clone(),
-											peer_state_lock, peer_state, per_peer_state, chan);
-									} else {
-										debug_assert!(false, "We shouldn't have an update for a non-funded channel");
-									}
-								},
-								hash_map::Entry::Vacant(_) => {},
-							}
-						}
-					}
-					if !updated_chan {
-						// TODO: Track this as in-flight even though the channel is closed.
-						let _ = self.chain_monitor.update_channel(funding_txo, &update);
-					}
+					// The monitor update will be replayed on startup if it doesnt complete, so no
+					// use bothering to care about the monitor update completing.
+					let _ = self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update);
 				},
 				BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, channel_id } => {
 					let per_peer_state = self.per_peer_state.read().unwrap();
@@ -7072,8 +7143,9 @@ where
 				}
 			}
 		}
+
 		let preimage_update = ChannelMonitorUpdate {
-			update_id: CLOSED_CHANNEL_UPDATE_ID,
+			update_id: 0, // apply_post_close_monitor_update will set the right value
 			counterparty_node_id: prev_hop.counterparty_node_id,
 			updates: vec![ChannelMonitorUpdateStep::PaymentPreimage {
 				payment_preimage,
@@ -7095,7 +7167,7 @@ where
 		if !during_init {
 			// We update the ChannelMonitor on the backward link, after
 			// receiving an `update_fulfill_htlc` from the forward link.
-			let update_res = self.chain_monitor.update_channel(prev_hop.funding_txo, &preimage_update);
+			let update_res = self.apply_post_close_monitor_update(counterparty_node_id, prev_hop.channel_id, prev_hop.funding_txo, preimage_update);
 			if update_res != ChannelMonitorUpdateStatus::Completed {
 				// TODO: This needs to be handled somehow - if we receive a monitor update
 				// with a preimage we *must* somehow manage to propagate it to the upstream
@@ -7119,6 +7191,7 @@ where
 			};
 			self.pending_background_events.lock().unwrap().push(event);
 		}
+
 		// Note that we do process the completion action here. This totally could be a
 		// duplicate claim, but we have no way of knowing without interrogating the
 		// `ChannelMonitor` we've provided the above update to. Instead, note that `Event`s are
@@ -7138,6 +7211,7 @@ where
 					in_flight_monitor_updates: BTreeMap::new(),
 					monitor_update_blocked_actions: BTreeMap::new(),
 					actions_blocking_raa_monitor_updates: BTreeMap::new(),
+					closed_channel_monitor_update_ids: BTreeMap::new(),
 					is_connected: false,
 				}));
 			let mut peer_state = peer_state_mutex.lock().unwrap();
@@ -10955,6 +11029,7 @@ where
 							in_flight_monitor_updates: BTreeMap::new(),
 							monitor_update_blocked_actions: BTreeMap::new(),
 							actions_blocking_raa_monitor_updates: BTreeMap::new(),
+							closed_channel_monitor_update_ids: BTreeMap::new(),
 							is_connected: true,
 						}));
 					},
@@ -12418,6 +12493,7 @@ where
 				in_flight_monitor_updates: BTreeMap::new(),
 				monitor_update_blocked_actions: BTreeMap::new(),
 				actions_blocking_raa_monitor_updates: BTreeMap::new(),
+				closed_channel_monitor_update_ids: BTreeMap::new(),
 				is_connected: false,
 			}
 		};
@@ -12467,7 +12543,19 @@ where
 					if shutdown_result.unbroadcasted_batch_funding_txid.is_some() {
 						return Err(DecodeError::InvalidValue);
 					}
-					if let Some((counterparty_node_id, funding_txo, channel_id, update)) = shutdown_result.monitor_update {
+					if let Some((counterparty_node_id, funding_txo, channel_id, mut update)) = shutdown_result.monitor_update {
+						// Our channel information is out of sync with the `ChannelMonitor`, so
+						// force the update to use the `ChannelMonitor`'s update_id for the close
+						// update.
+						let latest_update_id = monitor.get_latest_update_id();
+						update.update_id = latest_update_id.saturating_add(1);
+						per_peer_state.entry(counterparty_node_id)
+							.or_insert_with(|| Mutex::new(empty_peer_state()))
+							.lock().unwrap()
+							.closed_channel_monitor_update_ids.entry(channel_id)
+								.and_modify(|v| *v = cmp::max(latest_update_id, *v))
+								.or_insert(latest_update_id);
+
 						close_background_events.push(BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
 							counterparty_node_id, funding_txo, channel_id, update
 						});
@@ -12548,8 +12636,8 @@ where
 				let channel_id = monitor.channel_id();
 				log_info!(logger, "Queueing monitor update to ensure missing channel {} is force closed",
 					&channel_id);
-				let monitor_update = ChannelMonitorUpdate {
-					update_id: CLOSED_CHANNEL_UPDATE_ID,
+				let mut monitor_update = ChannelMonitorUpdate {
+					update_id: monitor.get_latest_update_id().saturating_add(1),
 					counterparty_node_id: None,
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }],
 					channel_id: Some(monitor.channel_id()),
@@ -12562,6 +12650,13 @@ where
 						update: monitor_update,
 					};
 					close_background_events.push(update);
+
+					per_peer_state.entry(counterparty_node_id)
+						.or_insert_with(|| Mutex::new(empty_peer_state()))
+						.lock().unwrap()
+						.closed_channel_monitor_update_ids.entry(monitor.channel_id())
+							.and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v))
+							.or_insert(monitor.get_latest_update_id());
 				} else {
 					// This is a fairly old `ChannelMonitor` that hasn't seen an update to its
 					// off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain
@@ -12569,6 +12664,7 @@ where
 					// Thus, we assume that it has no pending HTLCs and we will not need to
 					// generate a `ChannelMonitorUpdate` for it aside from this
 					// `ChannelForceClosed` one.
+					monitor_update.update_id = u64::MAX;
 					close_background_events.push(BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((*funding_txo, channel_id, monitor_update)));
 				}
 			}
@@ -14047,15 +14143,15 @@ mod tests {
 		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
-		let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
+		create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 0);
 
 		nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id());
 		nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
+		let chan_id = nodes[0].node.list_channels()[0].channel_id;
 		let error_message = "Channel force-closed";
-		nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id(), error_message.to_string()).unwrap();
-		check_closed_broadcast!(nodes[0], true);
+		nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &nodes[1].node.get_our_node_id(), error_message.to_string()).unwrap();
 		check_added_monitors!(nodes[0], 1);
-		check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 100000);
+		check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, [nodes[1].node.get_our_node_id()], 1_000_000);
 
 		{
 			// Assert that nodes[1] is awaiting removal for nodes[0] once nodes[1] has been
@@ -14074,6 +14170,31 @@ mod tests {
 		}
 	}
 
+	#[test]
+	fn test_drop_peers_when_removing_unfunded_channels() {
+		let chanmon_cfgs = create_chanmon_cfgs(2);
+		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+		exchange_open_accept_chan(&nodes[0], &nodes[1], 1_000_000, 0);
+		let events = nodes[0].node.get_and_clear_pending_events();
+		assert_eq!(events.len(), 1, "Unexpected events {:?}", events);
+		match events[0] {
+			Event::FundingGenerationReady { .. } => {}
+			_ => panic!("Unexpected event {:?}", events),
+		}
+
+		nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id());
+		nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
+		check_closed_event!(nodes[0], 1, ClosureReason::DisconnectedPeer, [nodes[1].node.get_our_node_id()], 1_000_000);
+		check_closed_event!(nodes[1], 1, ClosureReason::DisconnectedPeer, [nodes[0].node.get_our_node_id()], 1_000_000);
+
+		// At this point the state for the peers should have been removed.
+		assert_eq!(nodes[0].node.per_peer_state.read().unwrap().len(), 0);
+		assert_eq!(nodes[1].node.per_peer_state.read().unwrap().len(), 0);
+	}
+
 	#[test]
 	fn bad_inbound_payment_hash() {
 		// Add coverage for checking that a user-provided payment hash matches the payment secret.
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index ee73b30baef..dfef2e09cd2 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -15,7 +15,7 @@ use crate::chain;
 use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::chaininterface::LowerBoundedFeeEstimator;
 use crate::chain::channelmonitor;
-use crate::chain::channelmonitor::{Balance, CLOSED_CHANNEL_UPDATE_ID, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
+use crate::chain::channelmonitor::{Balance, ChannelMonitorUpdateStep, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
 use crate::chain::transaction::OutPoint;
 use crate::sign::{ecdsa::EcdsaChannelSigner, EntropySource, OutputSpender, SignerProvider};
 use crate::events::{Event, FundingInfo, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason};
@@ -11005,7 +11005,8 @@ fn test_close_in_funding_batch() {
 		let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap();
 		let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap();
 		assert_eq!(monitor_updates_1.len(), 1);
-		assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
+		assert_eq!(monitor_updates_1[0].updates.len(), 1);
+		assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }));
 	}
 
 	let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
@@ -11092,10 +11093,12 @@ fn test_batch_funding_close_after_funding_signed() {
 		let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap();
 		let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap();
 		assert_eq!(monitor_updates_1.len(), 1);
-		assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
+		assert_eq!(monitor_updates_1[0].updates.len(), 1);
+		assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }));
 		let monitor_updates_2 = monitor_updates.get(&channel_id_2).unwrap();
 		assert_eq!(monitor_updates_2.len(), 1);
-		assert_eq!(monitor_updates_2[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
+		assert_eq!(monitor_updates_2[0].updates.len(), 1);
+		assert!(matches!(monitor_updates_2[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }));
 	}
 	let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 	match msg_events[0] {
diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs
index c32fca6bd75..d614737a05e 100644
--- a/lightning/src/ln/reload_tests.rs
+++ b/lightning/src/ln/reload_tests.rs
@@ -11,7 +11,7 @@
 
 use crate::chain::{ChannelMonitorUpdateStatus, Watch};
 use crate::chain::chaininterface::LowerBoundedFeeEstimator;
-use crate::chain::channelmonitor::{CLOSED_CHANNEL_UPDATE_ID, ChannelMonitor};
+use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateStep};
 use crate::sign::EntropySource;
 use crate::chain::transaction::OutPoint;
 use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
@@ -1264,7 +1264,8 @@ fn test_reload_partial_funding_batch() {
 		let mut monitor_updates = nodes[0].chain_monitor.monitor_updates.lock().unwrap();
 		let monitor_updates_1 = monitor_updates.get(&channel_id_1).unwrap();
 		assert_eq!(monitor_updates_1.len(), 1);
-		assert_eq!(monitor_updates_1[0].update_id, CLOSED_CHANNEL_UPDATE_ID);
+		assert_eq!(monitor_updates_1[0].updates.len(), 1);
+		assert!(matches!(monitor_updates_1[0].updates[0], ChannelMonitorUpdateStep::ChannelForceClosed { .. }));
 	}
 
 	// The funding transaction should not have been broadcast, but we broadcast the force-close
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index 8a385eda9a6..732b549555e 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -21,9 +21,7 @@ use crate::{io, log_error};
 use crate::chain;
 use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use crate::chain::chainmonitor::Persist;
-use crate::chain::channelmonitor::{
-	ChannelMonitor, ChannelMonitorUpdate, CLOSED_CHANNEL_UPDATE_ID,
-};
+use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate};
 use crate::chain::transaction::OutPoint;
 use crate::ln::channelmanager::AChannelManager;
 use crate::routing::gossip::NetworkGraph;
@@ -728,16 +726,17 @@ where
 	///   - No full monitor is found in [`KVStore`]
 	///   - The number of pending updates exceeds `maximum_pending_updates` as given to [`Self::new`]
 	///   - LDK commands re-persisting the entire monitor through this function, specifically when
-	///	`update` is `None`.
-	///   - The update is at [`CLOSED_CHANNEL_UPDATE_ID`]
+	///	    `update` is `None`.
+	///   - The update is at [`u64::MAX`], indicating an update generated by pre-0.1 LDK.
 	fn update_persisted_channel(
 		&self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>,
 		monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
+		const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;
 		if let Some(update) = update {
-			if update.update_id != CLOSED_CHANNEL_UPDATE_ID
-				&& update.update_id % self.maximum_pending_updates != 0
-			{
+			let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID
+				&& update.update_id % self.maximum_pending_updates != 0;
+			if persist_update {
 				let monitor_name = MonitorName::from(funding_txo);
 				let update_name = UpdateName::from(update.update_id);
 				match self.kv_store.write(
@@ -764,7 +763,7 @@ where
 				// In case of channel-close monitor update, we need to read old monitor before persisting
 				// the new one in order to determine the cleanup range.
 				let maybe_old_monitor = match monitor.get_latest_update_id() {
-					CLOSED_CHANNEL_UPDATE_ID => self.read_monitor(&monitor_name).ok(),
+					LEGACY_CLOSED_CHANNEL_UPDATE_ID => self.read_monitor(&monitor_name).ok(),
 					_ => None,
 				};
 
@@ -772,23 +771,24 @@ where
 				let monitor_update_status = self.persist_new_channel(funding_txo, monitor);
 
 				if let chain::ChannelMonitorUpdateStatus::Completed = monitor_update_status {
-					let cleanup_range =
-						if monitor.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID {
-							// If there is an error while reading old monitor, we skip clean up.
-							maybe_old_monitor.map(|(_, ref old_monitor)| {
-								let start = old_monitor.get_latest_update_id();
-								// We never persist an update with update_id = CLOSED_CHANNEL_UPDATE_ID
-								let end = cmp::min(
-									start.saturating_add(self.maximum_pending_updates),
-									CLOSED_CHANNEL_UPDATE_ID - 1,
-								);
-								(start, end)
-							})
-						} else {
-							let end = monitor.get_latest_update_id();
-							let start = end.saturating_sub(self.maximum_pending_updates);
-							Some((start, end))
-						};
+					let channel_closed_legacy =
+						monitor.get_latest_update_id() == LEGACY_CLOSED_CHANNEL_UPDATE_ID;
+					let cleanup_range = if channel_closed_legacy {
+						// If there is an error while reading old monitor, we skip clean up.
+						maybe_old_monitor.map(|(_, ref old_monitor)| {
+							let start = old_monitor.get_latest_update_id();
+							// We never persist an update with the legacy closed update_id
+							let end = cmp::min(
+								start.saturating_add(self.maximum_pending_updates),
+								LEGACY_CLOSED_CHANNEL_UPDATE_ID - 1,
+							);
+							(start, end)
+						})
+					} else {
+						let end = monitor.get_latest_update_id();
+						let start = end.saturating_sub(self.maximum_pending_updates);
+						Some((start, end))
+					};
 
 					if let Some((start, end)) = cleanup_range {
 						self.cleanup_in_range(monitor_name, start, end);
@@ -1185,24 +1185,19 @@ mod tests {
 					// check that when we read it, we got the right update id
 					assert_eq!(mon.get_latest_update_id(), $expected_update_id);
 
-					// if the CM is at consolidation threshold, ensure no updates are stored.
 					let monitor_name = MonitorName::from(mon.get_funding_txo().0);
-					if mon.get_latest_update_id() % persister_0_max_pending_updates == 0
-						|| mon.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID
-					{
-						assert_eq!(
-							persister_0
-								.kv_store
-								.list(
-									CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-									monitor_name.as_str()
-								)
-								.unwrap()
-								.len(),
-							0,
-							"updates stored when they shouldn't be in persister 0"
-						);
-					}
+					assert_eq!(
+						persister_0
+							.kv_store
+							.list(
+								CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+								monitor_name.as_str()
+							)
+							.unwrap()
+							.len() as u64,
+						mon.get_latest_update_id() % persister_0_max_pending_updates,
+						"Wrong number of updates stored in persister 0",
+					);
 				}
 				persisted_chan_data_1 =
 					persister_1.read_all_channel_monitors_with_updates().unwrap();
@@ -1210,23 +1205,18 @@ mod tests {
 				for (_, mon) in persisted_chan_data_1.iter() {
 					assert_eq!(mon.get_latest_update_id(), $expected_update_id);
 					let monitor_name = MonitorName::from(mon.get_funding_txo().0);
-					// if the CM is at consolidation threshold, ensure no updates are stored.
-					if mon.get_latest_update_id() % persister_1_max_pending_updates == 0
-						|| mon.get_latest_update_id() == CLOSED_CHANNEL_UPDATE_ID
-					{
-						assert_eq!(
-							persister_1
-								.kv_store
-								.list(
-									CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-									monitor_name.as_str()
-								)
-								.unwrap()
-								.len(),
-							0,
-							"updates stored when they shouldn't be in persister 1"
-						);
-					}
+					assert_eq!(
+						persister_1
+							.kv_store
+							.list(
+								CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
+								monitor_name.as_str()
+							)
+							.unwrap()
+							.len() as u64,
+						mon.get_latest_update_id() % persister_1_max_pending_updates,
+						"Wrong number of updates stored in persister 1",
+					);
 				}
 			};
 		}
@@ -1283,28 +1273,8 @@ mod tests {
 		check_added_monitors!(nodes[1], 1);
 
 		// Make sure everything is persisted as expected after close.
-		check_persisted_data!(CLOSED_CHANNEL_UPDATE_ID);
-
-		// Make sure the expected number of stale updates is present.
-		let persisted_chan_data = persister_0.read_all_channel_monitors_with_updates().unwrap();
-		let (_, monitor) = &persisted_chan_data[0];
-		let monitor_name = MonitorName::from(monitor.get_funding_txo().0);
-		// The channel should have 0 updates, as it wrote a full monitor and consolidated.
-		assert_eq!(
-			persister_0
-				.kv_store
-				.list(CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, monitor_name.as_str())
-				.unwrap()
-				.len(),
-			0
-		);
-		assert_eq!(
-			persister_1
-				.kv_store
-				.list(CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE, monitor_name.as_str())
-				.unwrap()
-				.len(),
-			0
+		check_persisted_data!(
+			persister_0_max_pending_updates * 2 * EXPECTED_UPDATES_PER_PAYMENT + 1
 		);
 	}
 
@@ -1452,40 +1422,6 @@ mod tests {
 				UpdateName::from(1).as_str()
 			)
 			.is_err());
-
-		// Force close.
-		let chan_id = nodes[0].node.list_channels()[0].channel_id;
-		let node_id_1 = nodes[1].node.get_our_node_id();
-		let err_msg = "Channel force-closed".to_string();
-		nodes[0].node.force_close_broadcasting_latest_txn(&chan_id, &node_id_1, err_msg).unwrap();
-		let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) };
-		check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 100000);
-		check_closed_broadcast!(nodes[0], true);
-		check_added_monitors!(nodes[0], 1);
-
-		// Write an update near u64::MAX
-		persister_0
-			.kv_store
-			.write(
-				CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-				monitor_name.as_str(),
-				UpdateName::from(u64::MAX - 1).as_str(),
-				&[0u8; 1],
-			)
-			.unwrap();
-
-		// Do the stale update cleanup
-		persister_0.cleanup_stale_updates(false).unwrap();
-
-		// Confirm the stale update is unreadable/gone
-		assert!(persister_0
-			.kv_store
-			.read(
-				CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE,
-				monitor_name.as_str(),
-				UpdateName::from(u64::MAX - 1).as_str()
-			)
-			.is_err());
 	}
 
 	fn persist_fn<P: Deref, ChannelSigner: EcdsaChannelSigner>(_persist: P) -> bool

From 4582b201ea7a2880d452ef06354a953979e75dbd Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Thu, 10 Oct 2024 19:42:16 +0000
Subject: [PATCH 8/8] Avoid startup `PeerState` entries for peers with unfunded
 channels

If a peer creates a channel with us which never reaches the funding
stage (or never gets any commitment updates after creation), we'll
avoid inserting the `update_id` into
`closed_channel_monitor_update_ids` at runtime to avoid keeping a
`PeerState` entry around for no reason. However, on startup we
still create a `ChannelMonitorUpdate` with a `ChannelForceClosed`
update step to ensure the `ChannelMonitor` is locked and shut down.

This is pretty redundant, and results in a bunch of on-startup
`ChannelMonitorUpdate`s for any old but non-archived
`ChannelMonitor`s. Instead, here, we check if a `ChannelMonitor`
already saw a `ChannelForceClosed` update step before we generate
the on-startup `ChannelMonitorUpdate`.

This also allows us to skip the `closed_channel_monitor_update_ids`
insertion as we can be confident we'll never have a
`ChannelMonitorUpdate` for this channel at all.
---
 lightning/src/chain/channelmonitor.rs |  6 ++
 lightning/src/ln/channelmanager.rs    | 86 +++++++--------------------
 lightning/src/ln/monitor_tests.rs     |  4 --
 lightning/src/ln/payment_tests.rs     |  4 --
 4 files changed, 28 insertions(+), 72 deletions(-)

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 8653dc8274c..01698aab6f8 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -1711,6 +1711,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		self.inner.lock().unwrap().get_cur_holder_commitment_number()
 	}
 
+	/// Gets whether we've been notified that this channel is closed by the `ChannelManager` (i.e.
+	/// via a [`ChannelMonitorUpdateStep::ChannelForceClosed`]).
+	pub(crate) fn offchain_closed(&self) -> bool {
+		self.inner.lock().unwrap().lockdown_from_offchain
+	}
+
 	/// Gets the `node_id` of the counterparty for this channel.
 	///
 	/// Will be `None` for channels constructed on LDK versions prior to 0.0.110 and always `Some`
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index f06a3af5243..7cb484d4fac 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -7253,8 +7253,6 @@ where
 				let prev_channel_id = hop_data.channel_id;
 				let prev_user_channel_id = hop_data.user_channel_id;
 				let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
-				#[cfg(debug_assertions)]
-				let claiming_chan_funding_outpoint = hop_data.outpoint;
 				self.claim_funds_from_hop(hop_data, payment_preimage, None,
 					|htlc_claim_value_msat, definitely_duplicate| {
 						let chan_to_release =
@@ -7279,61 +7277,6 @@ where
 							// monitor updates still in flight. In that case, we shouldn't
 							// immediately free, but instead let that monitor update complete
 							// in the background.
-							#[cfg(debug_assertions)] {
-								let background_events = self.pending_background_events.lock().unwrap();
-								// There should be a `BackgroundEvent` pending...
-								assert!(background_events.iter().any(|ev| {
-									match ev {
-										BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
-											funding_txo, update, ..
-										} => {
-											if *funding_txo == claiming_chan_funding_outpoint {
-												// to apply a monitor update that blocked the claiming channel,
-												assert!(update.updates.iter().any(|upd|
-													if let ChannelMonitorUpdateStep::PaymentPreimage {
-														payment_preimage: update_preimage, ..
-													} = upd {
-														payment_preimage == *update_preimage
-													} else {
-														false
-													}
-												), "{:?}", update);
-												true
-											} else if *funding_txo == next_channel_outpoint {
-												// or the channel we'd unblock is already closed,
-												assert!(update.updates.iter().any(|upd|
-													if let ChannelMonitorUpdateStep::ChannelForceClosed { .. } = upd {
-														true
-													} else {
-														false
-													}
-												), "{:?}", update);
-												true
-											} else { false }
-										},
-										// or the channel we'd unblock is already closed (for an
-										// old channel),
-										BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup(
-											(funding_txo, _channel_id, monitor_update)
-										) => {
-											if *funding_txo == next_channel_outpoint {
-												assert_eq!(monitor_update.updates.len(), 1);
-												assert!(matches!(
-													monitor_update.updates[0],
-													ChannelMonitorUpdateStep::ChannelForceClosed { .. }
-												));
-												true
-											} else { false }
-										},
-										// or the monitor update has completed and will unblock
-										// immediately once we get going.
-										BackgroundEvent::MonitorUpdatesComplete {
-											channel_id, ..
-										} =>
-											*channel_id == prev_channel_id,
-									}
-								}), "{:?}", *background_events);
-							}
 							(None, None)
 						} else if definitely_duplicate {
 							if let Some(other_chan) = chan_to_release {
@@ -12632,6 +12575,28 @@ where
 
 		for (funding_txo, monitor) in args.channel_monitors.iter() {
 			if !funding_txo_set.contains(funding_txo) {
+				if let Some(counterparty_node_id) = monitor.get_counterparty_node_id() {
+					// If the ChannelMonitor had any updates, we may need to update it further and
+					// thus track it in `closed_channel_monitor_update_ids`. If the channel never
+					// had any updates at all, there can't be any HTLCs pending which we need to
+					// claim.
+					// Note that a `ChannelMonitor` is created with `update_id` 0 and after we
+					// provide it with a closure update its `update_id` will be at 1.
+					if !monitor.offchain_closed() || monitor.get_latest_update_id() > 1 {
+						per_peer_state.entry(counterparty_node_id)
+							.or_insert_with(|| Mutex::new(empty_peer_state()))
+							.lock().unwrap()
+							.closed_channel_monitor_update_ids.entry(monitor.channel_id())
+								.and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v))
+								.or_insert(monitor.get_latest_update_id());
+					}
+				}
+
+				if monitor.offchain_closed() {
+					// We already appled a ChannelForceClosed update.
+					continue;
+				}
+
 				let logger = WithChannelMonitor::from(&args.logger, monitor, None);
 				let channel_id = monitor.channel_id();
 				log_info!(logger, "Queueing monitor update to ensure missing channel {} is force closed",
@@ -12650,13 +12615,6 @@ where
 						update: monitor_update,
 					};
 					close_background_events.push(update);
-
-					per_peer_state.entry(counterparty_node_id)
-						.or_insert_with(|| Mutex::new(empty_peer_state()))
-						.lock().unwrap()
-						.closed_channel_monitor_update_ids.entry(monitor.channel_id())
-							.and_modify(|v| *v = cmp::max(monitor.get_latest_update_id(), *v))
-							.or_insert(monitor.get_latest_update_id());
 				} else {
 					// This is a fairly old `ChannelMonitor` that hasn't seen an update to its
 					// off-chain state since LDK 0.0.118 (as in LDK 0.0.119 any off-chain
diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs
index 4aad8f569fc..99ad31d3583 100644
--- a/lightning/src/ln/monitor_tests.rs
+++ b/lightning/src/ln/monitor_tests.rs
@@ -2302,9 +2302,6 @@ fn do_test_restored_packages_retry(check_old_monitor_retries_after_upgrade: bool
 
 	// Connecting more blocks should result in the HTLC transactions being rebroadcast.
 	connect_blocks(&nodes[0], crate::chain::package::LOW_FREQUENCY_BUMP_INTERVAL);
-	if check_old_monitor_retries_after_upgrade {
-		check_added_monitors(&nodes[0], 1);
-	}
 	{
 		let txn = nodes[0].tx_broadcaster.txn_broadcast();
 		assert_eq!(txn.len(), 1);
@@ -3014,7 +3011,6 @@ fn do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(confirm_c
 		// If we saw the commitment before our `counterparty_payment_script` was fixed, we'll never
 		// get the spendable output event for the `to_remote` output, so we'll need to get it
 		// manually via `get_spendable_outputs`.
-		check_added_monitors(&nodes[1], 1);
 		let outputs = get_monitor!(nodes[1], chan_id).get_spendable_outputs(&commitment_tx, commitment_tx_conf_height);
 		assert_eq!(outputs.len(), 1);
 		let spend_tx = nodes[1].keys_manager.backing.spend_spendable_outputs(
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 7839b49be77..46a6e302655 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -993,7 +993,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
 
 	nodes[0].node.test_process_background_events();
-	check_added_monitors(&nodes[0], 1);
 
 	let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
 	reconnect_args.send_channel_ready = (true, true);
@@ -1023,7 +1022,6 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	nodes[1].node.peer_disconnected(nodes[0].node.get_our_node_id());
 
 	nodes[0].node.test_process_background_events();
-	check_added_monitors(&nodes[0], 1);
 
 	reconnect_nodes(ReconnectArgs::new(&nodes[0], &nodes[1]));
 
@@ -1162,7 +1160,6 @@ fn do_test_dup_htlc_onchain_doesnt_fail_on_reload(persist_manager_post_event: bo
 	let height = nodes[0].blocks.lock().unwrap().len() as u32 - 1;
 	nodes[0].chain_monitor.chain_monitor.block_connected(&claim_block, height);
 	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
-	check_added_monitors(&nodes[0], 1);
 }
 
 #[test]
@@ -3522,7 +3519,6 @@ fn do_no_missing_sent_on_reload(persist_manager_with_payment: bool, at_midpoint:
 	reload_node!(nodes[0], test_default_channel_config(), &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister_c, chain_monitor_c, nodes_0_deserialized_c);
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert!(events.is_empty());
-	check_added_monitors(&nodes[0], 1);
 }
 
 #[test]