From: Matt Corallo <git@bluematt.me>
Date: Sun, 29 Sep 2024 19:30:48 +0000 (+0000)
Subject: Add missing `update_maps_on_chan_removal` call in signer restore
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=9f9d448efbd0bbcefc2acb1a9d56f1c9bdd96289;p=rust-lightning

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.
---

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 5cb92d3cb..5315e6a4e 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 {