Drop the (broken) ability to disable broadcast when FC'ing a chan

`ChannelManager` has two public methods to force-close a (single)
channel - `force_close_broadcasting_latest_txn` and
`force_close_without_broadcasting_txn`. The second exists to allow
a user who has restored a stale backup to (sort of) recover their
funds by starting, force-closing channels without broadcasting
their latest state, and only then reconnecting to peers (which
would otherwise cause a panic due to the stale state).

To my knowledge, no one has ever implemented this (fairly
half-assed) recovery flow, and more importantly its rather
substantially broken. `ChannelMonitor` has never tracked any
concept of whether a channel is stale, and if we connect blocks
towards an HTLC time-out it will immediately broadcast the stale
state anyway.

Finally, we really shouldn't encourage users to try to "recover"
from a stale state by running an otherwise-operational node on the
other end. Rather, we should formalize such a recovery flow by
having a `ChainMonitor` variant that takes a list of
`ChannelMonitor`s and claims available funds, dropping the
`ChannelManager` entirely.

While work continues towards that goal, having long-broken
functionality in `ChannelManager` is also not acceptable, so here
we simply remove the "without broadcasting" force-closure version.

Fixes #2875

Author: TheBlueMatt

lightning/src/events/mod.rs

@@ -327,15 +327,10 @@ pub enum ClosureReason {
 		/// Whether or not the latest transaction was broadcasted when the channel was force
 		/// closed.
 		///
-		/// Channels closed using [`ChannelManager::force_close_broadcasting_latest_txn`] will have
-		/// this field set to true, whereas channels closed using [`ChannelManager::force_close_without_broadcasting_txn`]
-		/// or force-closed prior to being funded will have this field set to false.
+		/// TODO: Update docs on when this will happen!
 		///
 		/// This will be `None` for objects generated or written by LDK 0.0.123 and
 		/// earlier.
-		///
-		/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn.
-		/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn.
 		broadcasted_latest_txn: Option<bool>,
 		/// The error message provided to [`ChannelManager::force_close_broadcasting_latest_txn`] or
 		/// [`ChannelManager::force_close_all_channels_broadcasting_latest_txn`].
@@ -1484,7 +1479,7 @@ pub enum Event {
 	///
 	/// To accept the request (and in the case of a dual-funded channel, not contribute funds),
 	/// call [`ChannelManager::accept_inbound_channel`].
-	/// To reject the request, call [`ChannelManager::force_close_without_broadcasting_txn`].
+	/// To reject the request, call [`ChannelManager::force_close_broadcasting_latest_txn`].
 	/// Note that a ['ChannelClosed`] event will _not_ be triggered if the channel is rejected.
 	///
 	/// The event is only triggered when a new open channel request is received and the
@@ -1495,27 +1490,27 @@ pub enum Event {
 	/// returning `Err(ReplayEvent ())`) and won't be persisted across restarts.
 	///
 	/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
-	/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+	/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
 	/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
 	OpenChannelRequest {
 		/// The temporary channel ID of the channel requested to be opened.
 		///
 		/// When responding to the request, the `temporary_channel_id` should be passed
 		/// back to the ChannelManager through [`ChannelManager::accept_inbound_channel`] to accept,
-		/// or through [`ChannelManager::force_close_without_broadcasting_txn`] to reject.
+		/// or through [`ChannelManager::force_close_broadcasting_latest_txn`] to reject.
 		///
 		/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
-		/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+		/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
 		temporary_channel_id: ChannelId,
 		/// The node_id of the counterparty requesting to open the channel.
 		///
 		/// When responding to the request, the `counterparty_node_id` should be passed
 		/// back to the `ChannelManager` through [`ChannelManager::accept_inbound_channel`] to
-		/// accept the request, or through [`ChannelManager::force_close_without_broadcasting_txn`] to reject the
-		/// request.
+		/// accept the request, or through [`ChannelManager::force_close_broadcasting_latest_txn`]
+		/// to reject the request.
 		///
 		/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel
-		/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+		/// [`ChannelManager::force_close_broadcasting_latest_txn`]: crate::ln::channelmanager::ChannelManager::force_close_broadcasting_latest_txn
 		counterparty_node_id: PublicKey,
 		/// The channel value of the requested channel.
 		funding_satoshis: u64,

lightning/src/ln/channel.rs

@@ -8047,11 +8047,6 @@ where
 
 	/// May panic if some calls other than message-handling calls (which will all Err immediately)
 	/// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-	///
-	/// Some links printed in log lines are included here to check them during build (when run with
-	/// `cargo doc --document-private-items`):
-	/// [`super::channelmanager::ChannelManager::force_close_without_broadcasting_txn`] and
-	/// [`super::channelmanager::ChannelManager::force_close_all_channels_without_broadcasting_txn`].
 	#[rustfmt::skip]
 	pub fn channel_reestablish<L: Deref, NS: Deref>(
 		&mut self, msg: &msgs::ChannelReestablish, logger: &L, node_signer: &NS,
@@ -8089,18 +8084,17 @@ where
 			if msg.next_remote_commitment_number > our_commitment_transaction {
 				macro_rules! log_and_panic {
 					($err_msg: expr) => {
-						log_error!(logger, $err_msg, &self.context.channel_id, log_pubkey!(self.context.counterparty_node_id));
-						panic!($err_msg, &self.context.channel_id, log_pubkey!(self.context.counterparty_node_id));
+						log_error!(logger, $err_msg);
+						panic!($err_msg);
 					}
 				}
 				log_and_panic!("We have fallen behind - we have received proof that if we broadcast our counterparty is going to claim all our funds.\n\
 					This implies you have restarted with lost ChannelMonitor and ChannelManager state, the first of which is a violation of the LDK chain::Watch requirements.\n\
 					More specifically, this means you have a bug in your implementation that can cause loss of funds, or you are running with an old backup, which is unsafe.\n\
-					If you have restored from an old backup and wish to force-close channels and return to operation, you should start up, call\n\
-					ChannelManager::force_close_without_broadcasting_txn on channel {} with counterparty {} or\n\
-					ChannelManager::force_close_all_channels_without_broadcasting_txn, then reconnect to peer(s).\n\
-					Note that due to a long-standing bug in lnd you may have to reach out to peers running lnd-based nodes to ask them to manually force-close channels\n\
-					See https://github.com/lightningdevkit/rust-lightning/issues/1565 for more info.");
+					If you have restored from an old backup and wish to claim any available funds, you should restart with\n\
+					an empty ChannelManager and no ChannelMonitors, reconnect to peer(s), ensure they've force-closed all of your\n\
+					previous channels and that the closure transaction(s) have confirmed on-chain,\n\
+					then restart with an empty ChannelManager and the latest ChannelMonitors that you do have.");
 			}
 		}
 

lightning/src/ln/channelmanager.rs

@@ -940,6 +940,16 @@ impl MsgHandleErrInternal {
 		Self { err, closes_channel: false, shutdown_finish: None }
 	}
 
+	fn dont_send_error_message(&mut self) {
+		match &mut self.err.action {
+			msgs::ErrorAction::DisconnectPeer { msg } => *msg = None,
+			msgs::ErrorAction::SendErrorMessage { msg: _ } => {
+				self.err.action = msgs::ErrorAction::IgnoreError;
+			},
+			_ => {},
+		}
+	}
+
 	fn closes_channel(&self) -> bool {
 		self.closes_channel
 	}
@@ -1990,7 +2000,7 @@ where
 ///     match event {
 ///         Event::OpenChannelRequest { temporary_channel_id, counterparty_node_id, ..  } => {
 ///             if !is_trusted(counterparty_node_id) {
-///                 match channel_manager.force_close_without_broadcasting_txn(
+///                 match channel_manager.force_close_broadcasting_latest_txn(
 ///                     &temporary_channel_id, &counterparty_node_id, error_message.to_string()
 ///                 ) {
 ///                     Ok(()) => println!("Rejecting channel {}", temporary_channel_id),
@@ -4350,85 +4360,66 @@ where
 	/// `peer_msg` should be set when we receive a message from a peer, but not set when the
 	/// user closes, which will be re-exposed as the `ChannelClosed` reason.
 	#[rustfmt::skip]
-	fn force_close_channel_with_peer(&self, channel_id: &ChannelId, peer_node_id: &PublicKey, peer_msg: Option<&String>, broadcast: bool)
+	fn force_close_channel_with_peer(&self, channel_id: &ChannelId, peer_node_id: &PublicKey, reason: ClosureReason)
 	-> Result<(), APIError> {
 		let per_peer_state = self.per_peer_state.read().unwrap();
 		let peer_state_mutex = per_peer_state.get(peer_node_id)
 			.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", peer_node_id) })?;
-		let update_opt = {
-			let mut peer_state = peer_state_mutex.lock().unwrap();
-			let closure_reason = if let Some(peer_msg) = peer_msg {
-				ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(peer_msg.to_string()) }
-			} else {
-				ClosureReason::HolderForceClosed {
-					broadcasted_latest_txn: Some(broadcast),
-					message: "Channel force-closed".to_owned(), // TODO
-				}
-			};
-			let logger = WithContext::from(&self.logger, Some(*peer_node_id), Some(*channel_id), None);
-			if let hash_map::Entry::Occupied(mut chan_entry) = peer_state.channel_by_id.entry(channel_id.clone()) {
-				log_error!(logger, "Force-closing channel {}", channel_id);
-				let (mut shutdown_res, update_opt) = match chan_entry.get_mut().as_funded_mut() {
-					Some(chan) => {
-						(
-							chan.context.force_shutdown(&chan.funding, broadcast, closure_reason),
-							self.get_channel_update_for_broadcast(&chan).ok(),
-						)
-					},
-					None => {
-						// Unfunded channel has no update
-						(chan_entry.get_mut().force_shutdown(false, closure_reason), None)
-					},
-				};
-				remove_channel_entry!(self, peer_state, chan_entry, shutdown_res);
-				mem::drop(peer_state);
-				mem::drop(per_peer_state);
-				self.finish_close_channel(shutdown_res);
-				update_opt
-			} else if peer_state.inbound_channel_request_by_id.remove(channel_id).is_some() {
-				log_error!(logger, "Force-closing channel {}", &channel_id);
-				// N.B. that we don't send any channel close event here: we
-				// don't have a user_channel_id, and we never sent any opening
-				// events anyway.
-				None
-			} else {
-				return Err(APIError::ChannelUnavailable{ err: format!("Channel with id {} not found for the passed counterparty node_id {}", channel_id, peer_node_id) });
-			}
+		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
+		let peer_state = &mut *peer_state_lock;
+		let logger = WithContext::from(&self.logger, Some(*peer_node_id), Some(*channel_id), None);
+
+		let is_from_counterparty = matches!(reason, ClosureReason::CounterpartyForceClosed { .. });
+		let message = match &reason {
+			ClosureReason::HolderForceClosed { message, .. } => message.clone(),
+			_ => reason.to_string(),
 		};
-		if let Some(update) = update_opt {
-			// If we have some Channel Update to broadcast, we cache it and broadcast it later.
-			let mut pending_broadcast_messages = self.pending_broadcast_messages.lock().unwrap();
-			pending_broadcast_messages.push(MessageSendEvent::BroadcastChannelUpdate {
-				msg: update
-			});
+
+		if let Some(mut chan) = peer_state.channel_by_id.remove(channel_id) {
+			log_error!(logger, "Force-closing channel {}", channel_id);
+			let err = ChannelError::Close((message, reason));
+			let (_, mut e) = convert_channel_err!(self, peer_state, err, chan, channel_id);
+			mem::drop(peer_state_lock);
+			mem::drop(per_peer_state);
+			if is_from_counterparty {
+				// If the peer is the one who asked us to force-close, don't reply with a fresh
+				// error message.
+				e.dont_send_error_message();
+			}
+			let _ = handle_error!(self, Err::<(), _>(e), *peer_node_id);
+			Ok(())
+		} else if peer_state.inbound_channel_request_by_id.remove(channel_id).is_some() {
+			log_error!(logger, "Force-closing inbound channel request {}", &channel_id);
+			if !is_from_counterparty {
+				peer_state.pending_msg_events.push(
+					MessageSendEvent::HandleError {
+						node_id: *peer_node_id,
+						action: msgs::ErrorAction::SendErrorMessage {
+							msg: msgs::ErrorMessage { channel_id: *channel_id, data: message }
+						},
+					}
+				);
+			}
+			// N.B. that we don't send any channel close event here: we
+			// don't have a user_channel_id, and we never sent any opening
+			// events anyway.
+			Ok(())
+		} else {
+			Err(APIError::ChannelUnavailable{ err: format!("Channel with id {} not found for the passed counterparty node_id {}", channel_id, peer_node_id) })
 		}
-		Ok(())
 	}
 
 	#[rustfmt::skip]
-	fn force_close_sending_error(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, broadcast: bool, error_message: String)
+	fn force_close_sending_error(&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, error_message: String)
 	-> Result<(), APIError> {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 		log_debug!(self.logger,
 			"Force-closing channel, The error message sent to the peer : {}", error_message);
-		match self.force_close_channel_with_peer(channel_id, &counterparty_node_id, None, broadcast) {
-			Ok(()) => {
-				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 = peer_state_mutex.lock().unwrap();
-					peer_state.pending_msg_events.push(
-						MessageSendEvent::HandleError {
-							node_id: *counterparty_node_id,
-							action: msgs::ErrorAction::SendErrorMessage {
-								msg: msgs::ErrorMessage { channel_id: *channel_id, data: error_message }
-							},
-						}
-					);
-				}
-				Ok(())
-			},
-			Err(e) => Err(e)
-		}
+		let reason = ClosureReason::HolderForceClosed {
+			broadcasted_latest_txn: Some(true),
+			message: error_message,
+		};
+		self.force_close_channel_with_peer(channel_id, &counterparty_node_id, reason)
 	}
 
 	/// Force closes a channel, immediately broadcasting the latest local transaction(s),
@@ -4442,23 +4433,7 @@ where
 	pub fn force_close_broadcasting_latest_txn(
 		&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, error_message: String,
 	) -> Result<(), APIError> {
-		self.force_close_sending_error(channel_id, counterparty_node_id, true, error_message)
-	}
-
-	/// Force closes a channel, rejecting new HTLCs on the given channel but skips broadcasting
-	/// the latest local transaction(s).
-	///
-	/// The provided `error_message` is sent to connected peers for closing channels and should
-	/// be a human-readable description of what went wrong.
-	///
-	/// Fails if `channel_id` is unknown to the manager, or if the
-	/// `counterparty_node_id` isn't the counterparty of the corresponding channel.
-	/// You can always broadcast the latest local transaction(s) via
-	/// [`ChannelMonitor::broadcast_latest_holder_commitment_txn`].
-	pub fn force_close_without_broadcasting_txn(
-		&self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, error_message: String,
-	) -> Result<(), APIError> {
-		self.force_close_sending_error(channel_id, counterparty_node_id, false, error_message)
+		self.force_close_sending_error(channel_id, counterparty_node_id, error_message)
 	}
 
 	/// Force close all channels, immediately broadcasting the latest local commitment transaction
@@ -4476,21 +4451,6 @@ where
 		}
 	}
 
-	/// Force close all channels rejecting new HTLCs on each but without broadcasting the latest
-	/// local transaction(s).
-	///
-	/// The provided `error_message` is sent to connected peers for closing channels and
-	/// should be a human-readable description of what went wrong.
-	pub fn force_close_all_channels_without_broadcasting_txn(&self, error_message: String) {
-		for chan in self.list_channels() {
-			let _ = self.force_close_without_broadcasting_txn(
-				&chan.channel_id,
-				&chan.counterparty.node_id,
-				error_message.clone(),
-			);
-		}
-	}
-
 	/// Initiate a splice, to change the channel capacity of an existing funded channel.
 	/// After completion of splicing, the funding transaction will be replaced by a new one, spending the old funding transaction,
 	/// with optional extra inputs (splice-in) and/or extra outputs (splice-out or change).
@@ -12993,6 +12953,9 @@ where
 
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 
+		let peer_msg = UntrustedString(msg.data.clone());
+		let reason = ClosureReason::CounterpartyForceClosed { peer_msg };
+
 		if msg.channel_id.is_zero() {
 			let channel_ids: Vec<ChannelId> = {
 				let per_peer_state = self.per_peer_state.read().unwrap();
@@ -13007,7 +12970,7 @@ where
 			};
 			for channel_id in channel_ids {
 				// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
-				let _ = self.force_close_channel_with_peer(&channel_id, &counterparty_node_id, Some(&msg.data), true);
+				let _ = self.force_close_channel_with_peer(&channel_id, &counterparty_node_id, reason.clone());
 			}
 		} else {
 			{
@@ -13043,7 +13006,7 @@ where
 			}
 
 			// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
-			let _ = self.force_close_channel_with_peer(&msg.channel_id, &counterparty_node_id, Some(&msg.data), true);
+			let _ = self.force_close_channel_with_peer(&msg.channel_id, &counterparty_node_id, reason);
 		}
 	}
 
@@ -16346,9 +16309,9 @@ mod tests {
 
 		let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
 
-		nodes[0].node.force_close_channel_with_peer(&chan.2, &nodes[1].node.get_our_node_id(), None, true).unwrap();
-		check_added_monitors!(nodes[0], 1);
 		let message = "Channel force-closed".to_owned();
+		nodes[0].node.force_close_broadcasting_latest_txn(&chan.2, &nodes[1].node.get_our_node_id(), message.clone()).unwrap();
+		check_added_monitors!(nodes[0], 1);
 		let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
 		check_closed_event!(nodes[0], 1, reason, [nodes[1].node.get_our_node_id()], 100000);
 
@@ -16552,8 +16515,6 @@ mod tests {
 
 		check_unkown_peer_error(nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &unkown_public_key, error_message.to_string()), unkown_public_key);
 
-		check_unkown_peer_error(nodes[0].node.force_close_without_broadcasting_txn(&channel_id, &unkown_public_key, error_message.to_string()), unkown_public_key);
-
 		check_unkown_peer_error(nodes[0].node.forward_intercepted_htlc(intercept_id, &channel_id, unkown_public_key, 1_000_000), unkown_public_key);
 
 		check_unkown_peer_error(nodes[0].node.update_channel_config(&unkown_public_key, &[channel_id], &ChannelConfig::default()), unkown_public_key);
@@ -16584,8 +16545,6 @@ mod tests {
 
 		check_channel_unavailable_error(nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &counterparty_node_id, error_message.to_string()), channel_id, counterparty_node_id);
 
-		check_channel_unavailable_error(nodes[0].node.force_close_without_broadcasting_txn(&channel_id, &counterparty_node_id, error_message.to_string()), channel_id, counterparty_node_id);
-
 		check_channel_unavailable_error(nodes[0].node.forward_intercepted_htlc(InterceptId([0; 32]), &channel_id, counterparty_node_id, 1_000_000), channel_id, counterparty_node_id);
 
 		check_channel_unavailable_error(nodes[0].node.update_channel_config(&counterparty_node_id, &[channel_id], &ChannelConfig::default()), channel_id, counterparty_node_id);

lightning/src/ln/functional_test_utils.rs

@@ -1722,6 +1722,7 @@ pub fn check_closed_broadcast(node: &Node, num_channels: usize, with_error_msg:
 		dummy_connected = true;
 	}
 	let msg_events = node.node.get_and_clear_pending_msg_events();
+eprintln!("{:?}", msg_events);
 	assert_eq!(msg_events.len(), if with_error_msg { num_channels * 2 } else { num_channels });
 	if dummy_connected {
 		disconnect_dummy_node(&node);