Decide on close-broadcasting commitment txn based on channel state

In a previous commit, we removed the ability for users to pick
whether we will broadcast a commitment transaction on channel
closure. However, that doesn't mean that there is no value in never
broadcasting commitment transactions on channel closure. Rather, we
use it to avoid broadcasting transactions which we know cannot
confirm if the channel's funding transaction was not broadcasted.

Here we make this relationship more formal by splitting the
force-closure handling logic in `Channel` into the existing
`ChannelContext::force_shutdown` as well as a new
`ChannelContext::abandon_unfunded_chan`.
`ChannelContext::force_shutdown` is the only public method, but it
delegates to `abandon_unfunded_chan` based on the channel's state.

This has the nice side effect of avoiding commitment transaction
broadcasting when a batch open fails to get past the funding stage.

Author: TheBlueMatt

lightning/src/events/mod.rs

@@ -327,7 +327,9 @@ pub enum ClosureReason {
 		/// Whether or not the latest transaction was broadcasted when the channel was force
 		/// closed.
 		///
-		/// TODO: Update docs on when this will happen!
+		/// This will be set to `Some(true)` for any channels closed after their funding
+		/// transaction was (or might have been) broadcasted, and `Some(false)` for any channels
+		/// closed prior to their funding transaction being broadcasted.
 		///
 		/// This will be `None` for objects generated or written by LDK 0.0.123 and
 		/// earlier.

lightning/src/ln/channel.rs

@@ -1775,11 +1775,9 @@ where
 		}
 	}
 
-	pub fn force_shutdown(
-		&mut self, should_broadcast: bool, closure_reason: ClosureReason,
-	) -> ShutdownResult {
+	pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult {
 		let (funding, context) = self.funding_and_context_mut();
-		context.force_shutdown(funding, should_broadcast, closure_reason)
+		context.force_shutdown(funding, closure_reason)
 	}
 
 	#[rustfmt::skip]
@@ -5340,20 +5338,19 @@ where
 		self.unbroadcasted_funding_txid(funding).filter(|_| self.is_batch_funding())
 	}
 
-	/// Gets the latest commitment transaction and any dependent transactions for relay (forcing
-	/// shutdown of this channel - no more calls into this Channel may be made afterwards except
-	/// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
-	/// Also returns the list of payment_hashes for channels which we can safely fail backwards
-	/// immediately (others we will have to allow to time out).
-	pub fn force_shutdown(
-		&mut self, funding: &FundingScope, should_broadcast: bool, closure_reason: ClosureReason,
+	/// Shuts down this Channel (no more calls into this Channel may be made afterwards except
+	/// those explicitly stated to be alowed after shutdown, e.g. some simple getters).
+	fn force_shutdown(
+		&mut self, funding: &FundingScope, mut closure_reason: ClosureReason,
 	) -> ShutdownResult {
 		// Note that we MUST only generate a monitor update that indicates force-closure - we're
 		// called during initialization prior to the chain_monitor in the encompassing ChannelManager
 		// being fully configured in some cases. Thus, its likely any monitor events we generate will
 		// be delayed in being processed! See the docs for `ChannelManagerReadArgs` for more.
 		assert!(!matches!(self.channel_state, ChannelState::ShutdownComplete));
 
+		let broadcast = self.is_funding_broadcast();
+
 		// We go ahead and "free" any holding cell HTLCs or HTLCs we haven't yet committed to and
 		// return them to fail the payment.
 		let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
@@ -5384,7 +5381,7 @@ where
 				let update = ChannelMonitorUpdate {
 					update_id: self.latest_monitor_update_id,
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed {
-						should_broadcast,
+						should_broadcast: broadcast,
 					}],
 					channel_id: Some(self.channel_id()),
 				};
@@ -5398,6 +5395,12 @@ where
 		let unbroadcasted_batch_funding_txid = self.unbroadcasted_batch_funding_txid(funding);
 		let unbroadcasted_funding_tx = self.unbroadcasted_funding(funding);
 
+		if let ClosureReason::HolderForceClosed { ref mut broadcasted_latest_txn, .. } =
+			&mut closure_reason
+		{
+			*broadcasted_latest_txn = Some(broadcast);
+		}
+
 		self.channel_state = ChannelState::ShutdownComplete;
 		self.update_time_counter += 1;
 		ShutdownResult {
@@ -6047,10 +6050,8 @@ where
 		&self.context
 	}
 
-	pub fn force_shutdown(
-		&mut self, closure_reason: ClosureReason, should_broadcast: bool,
-	) -> ShutdownResult {
-		self.context.force_shutdown(&self.funding, should_broadcast, closure_reason)
+	pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult {
+		self.context.force_shutdown(&self.funding, closure_reason)
 	}
 
 	#[rustfmt::skip]
@@ -8124,7 +8125,7 @@ where
 						(closing_signed, signed_tx, shutdown_result)
 					}
 					Err(err) => {
-						let shutdown = self.context.force_shutdown(&self.funding, true, ClosureReason::ProcessingError {err: err.to_string()});
+						let shutdown = self.context.force_shutdown(&self.funding, ClosureReason::ProcessingError {err: err.to_string()});
 						(None, None, Some(shutdown))
 					}
 				}
@@ -11200,6 +11201,10 @@ impl<SP: Deref> OutboundV1Channel<SP>
 where
 	SP::Target: SignerProvider,
 {
+	pub fn abandon_unfunded_chan(&mut self, closure_reason: ClosureReason) -> ShutdownResult {
+		self.context.force_shutdown(&self.funding, closure_reason)
+	}
+
 	#[allow(dead_code)] // TODO(dual_funding): Remove once opending V2 channels is enabled.
 	#[rustfmt::skip]
 	pub fn new<ES: Deref, F: Deref, L: Deref>(

lightning/src/ln/channelmanager.rs

@@ -3245,7 +3245,7 @@ macro_rules! convert_channel_err {
 	($self: ident, $peer_state: expr, $err: expr, $funded_channel: expr, $channel_id: expr, FUNDED_CHANNEL) => { {
 		let mut do_close = |reason| {
 			(
-				$funded_channel.force_shutdown(reason, true),
+				$funded_channel.force_shutdown(reason),
 				$self.get_channel_update_for_broadcast(&$funded_channel).ok(),
 			)
 		};
@@ -3255,7 +3255,7 @@ macro_rules! convert_channel_err {
 		convert_channel_err!($self, $peer_state, $err, $funded_channel, do_close, locked_close, $channel_id, _internal)
 	} };
 	($self: ident, $peer_state: expr, $err: expr, $channel: expr, $channel_id: expr, UNFUNDED_CHANNEL) => { {
-		let mut do_close = |reason| { ($channel.force_shutdown(true, reason), None) };
+		let mut do_close = |reason| { ($channel.force_shutdown(reason), None) };
 		let locked_close = |_, chan: &mut Channel<_>| { locked_close_channel!($self, chan.context(), UNFUNDED); };
 		convert_channel_err!($self, $peer_state, $err, $channel, do_close, locked_close, $channel_id, _internal)
 	} };
@@ -4435,6 +4435,8 @@ where
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 		log_debug!(self.logger,
 			"Force-closing channel, The error message sent to the peer : {}", error_message);
+		// No matter what value for `broadcast_latest_txn` we set here, `Channel` will override it
+		// and set the appropriate value.
 		let reason = ClosureReason::HolderForceClosed {
 			broadcasted_latest_txn: Some(true),
 			message: error_message,
@@ -5550,12 +5552,12 @@ where
 		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 		let peer_state = &mut *peer_state_lock;
 
-		macro_rules! close_chan { ($err: expr, $api_err: expr, $chan: expr) => { {
+		macro_rules! abandon_chan { ($err: expr, $api_err: expr, $chan: expr) => { {
 			let counterparty;
 			let err = if let ChannelError::Close((msg, reason)) = $err {
 				let channel_id = $chan.context.channel_id();
 				counterparty = $chan.context.get_counterparty_node_id();
-				let shutdown_res = $chan.context.force_shutdown(&$chan.funding, false, reason);
+				let shutdown_res = $chan.abandon_unfunded_chan(reason);
 				MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, shutdown_res, None)
 			} else { unreachable!(); };
 
@@ -5575,7 +5577,7 @@ where
 					Err(err) => {
 						let chan_err = ChannelError::close(err.to_owned());
 						let api_err = APIError::APIMisuseError { err: err.to_owned() };
-						return close_chan!(chan_err, api_err, chan);
+						return abandon_chan!(chan_err, api_err, chan);
 					},
 				}
 
@@ -5585,7 +5587,7 @@ where
 					Ok(funding_msg) => (chan, funding_msg),
 					Err((mut chan, chan_err)) => {
 						let api_err = APIError::ChannelUnavailable { err: "Signer refused to sign the initial commitment transaction".to_owned() };
-						return close_chan!(chan_err, api_err, chan);
+						return abandon_chan!(chan_err, api_err, chan);
 					}
 				}
 			},
@@ -5614,7 +5616,7 @@ where
 				let chan_err = ChannelError::close(err.to_owned());
 				let api_err = APIError::APIMisuseError { err: err.to_owned() };
 				chan.unset_funding_info();
-				return close_chan!(chan_err, api_err, chan);
+				return abandon_chan!(chan_err, api_err, chan);
 			},
 			hash_map::Entry::Vacant(e) => {
 				if let Some(msg) = msg_opt {
@@ -14632,7 +14634,7 @@ where
 						log_error!(logger, " The ChannelMonitor for channel {} is at counterparty commitment transaction number {} but the ChannelManager is at counterparty commitment transaction number {}.",
 							&channel.context.channel_id(), monitor.get_cur_counterparty_commitment_number(), channel.get_cur_counterparty_commitment_transaction_number());
 					}
-					let mut shutdown_result = channel.context.force_shutdown(&channel.funding, true, ClosureReason::OutdatedChannelManager);
+					let mut shutdown_result = channel.force_shutdown(ClosureReason::OutdatedChannelManager);
 					if shutdown_result.unbroadcasted_batch_funding_txid.is_some() {
 						return Err(DecodeError::InvalidValue);
 					}
@@ -14705,7 +14707,6 @@ where
 				// If we were persisted and shut down while the initial ChannelMonitor persistence
 				// was in-progress, we never broadcasted the funding transaction and can still
 				// safely discard the channel.
-				let _ = channel.context.force_shutdown(&channel.funding, false, ClosureReason::DisconnectedPeer);
 				channel_closures.push_back((events::Event::ChannelClosed {
 					channel_id: channel.context.channel_id(),
 					user_channel_id: channel.context.get_user_id(),

lightning/src/ln/functional_tests.rs

@@ -11418,14 +11418,10 @@ pub fn test_close_in_funding_batch() {
 		_ => panic!("Unexpected message."),
 	}
 
-	// We broadcast the commitment transaction as part of the force-close.
-	{
-		let broadcasted_txs = nodes[0].tx_broadcaster.txn_broadcast();
-		assert_eq!(broadcasted_txs.len(), 1);
-		assert!(broadcasted_txs[0].compute_txid() != tx.compute_txid());
-		assert_eq!(broadcasted_txs[0].input.len(), 1);
-		assert_eq!(broadcasted_txs[0].input[0].previous_output.txid, tx.compute_txid());
-	}
+	// Because the funding was never broadcasted, we should never bother to broadcast the
+	// commitment transactions either.
+	let broadcasted_txs = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(broadcasted_txs.len(), 0);
 
 	// All channels in the batch should close immediately.
 	check_closed_events(
@@ -11524,15 +11520,10 @@ pub fn test_batch_funding_close_after_funding_signed() {
 		_ => panic!("Unexpected message."),
 	}
 
-	// TODO: We shouldn't broadcast any commitment transaction here as we have not yet broadcasted
-	// the funding transaction.
-	{
-		let broadcasted_txs = nodes[0].tx_broadcaster.txn_broadcast();
-		assert_eq!(broadcasted_txs.len(), 2);
-		assert!(broadcasted_txs[0].compute_txid() != tx.compute_txid());
-		assert_eq!(broadcasted_txs[0].input.len(), 1);
-		assert_eq!(broadcasted_txs[0].input[0].previous_output.txid, tx.compute_txid());
-	}
+	// Because the funding was never broadcasted, we should never bother to broadcast the
+	// commitment transactions either.
+	let broadcasted_txs = nodes[0].tx_broadcaster.txn_broadcast();
+	assert_eq!(broadcasted_txs.len(), 0);
 
 	// All channels in the batch should close immediately.
 	check_closed_events(
@@ -11559,7 +11550,8 @@ pub fn test_batch_funding_close_after_funding_signed() {
 	assert!(nodes[0].node.list_channels().is_empty());
 }
 
-fn do_test_funding_and_commitment_tx_confirm_same_block(confirm_remote_commitment: bool) {
+#[xtest(feature = "_externalize_tests")]
+pub fn test_funding_and_commitment_tx_confirm_same_block() {
 	// Tests that a node will forget the channel (when it only requires 1 confirmation) if the
 	// funding and commitment transaction confirm in the same block.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -11573,6 +11565,9 @@ fn do_test_funding_and_commitment_tx_confirm_same_block(confirm_remote_commitmen
 	);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_b_id = nodes[1].node.get_our_node_id();
+
 	let funding_tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 1_000_000, 0);
 	let chan_id = ChannelId::v1_from_funding_outpoint(chain::transaction::OutPoint {
 		txid: funding_tx.compute_txid(),
@@ -11582,55 +11577,30 @@ fn do_test_funding_and_commitment_tx_confirm_same_block(confirm_remote_commitmen
 	assert_eq!(nodes[0].node.list_channels().len(), 1);
 	assert_eq!(nodes[1].node.list_channels().len(), 1);
 
-	let (closing_node, other_node) =
-		if confirm_remote_commitment { (&nodes[1], &nodes[0]) } else { (&nodes[0], &nodes[1]) };
-	let closing_node_id = closing_node.node.get_our_node_id();
-	let other_node_id = other_node.node.get_our_node_id();
-
-	let message = "Channel force-closed".to_owned();
-	closing_node
-		.node
-		.force_close_broadcasting_latest_txn(&chan_id, &other_node_id, message.clone())
-		.unwrap();
-	let mut msg_events = closing_node.node.get_and_clear_pending_msg_events();
-	assert_eq!(msg_events.len(), 1);
-	match msg_events.pop().unwrap() {
-		MessageSendEvent::HandleError {
-			action: msgs::ErrorAction::SendErrorMessage { .. },
-			..
-		} => {},
-		_ => panic!("Unexpected event"),
-	}
-	check_added_monitors(closing_node, 1);
-	let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message };
-	check_closed_event(closing_node, 1, reason, false, &[other_node_id], 1_000_000);
-
 	let commitment_tx = {
-		let mut txn = closing_node.tx_broadcaster.txn_broadcast();
+		let mon = get_monitor!(nodes[0], chan_id);
+		let mut txn = mon.unsafe_get_latest_holder_commitment_txn(&nodes[0].logger);
 		assert_eq!(txn.len(), 1);
-		let commitment_tx = txn.pop().unwrap();
-		check_spends!(commitment_tx, funding_tx);
-		commitment_tx
+		txn.pop().unwrap()
 	};
 
 	mine_transactions(&nodes[0], &[&funding_tx, &commitment_tx]);
 	mine_transactions(&nodes[1], &[&funding_tx, &commitment_tx]);
 
-	check_closed_broadcast(other_node, 1, true);
-	check_added_monitors(other_node, 1);
+	check_closed_broadcast(&nodes[0], 1, true);
+	check_added_monitors(&nodes[0], 1);
+	let reason = ClosureReason::CommitmentTxConfirmed;
+	check_closed_event(&nodes[0], 1, reason, false, &[node_b_id], 1_000_000);
+
+	check_closed_broadcast(&nodes[1], 1, true);
+	check_added_monitors(&nodes[1], 1);
 	let reason = ClosureReason::CommitmentTxConfirmed;
-	check_closed_event(other_node, 1, reason, false, &[closing_node_id], 1_000_000);
+	check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], 1_000_000);
 
 	assert!(nodes[0].node.list_channels().is_empty());
 	assert!(nodes[1].node.list_channels().is_empty());
 }
 
-#[xtest(feature = "_externalize_tests")]
-pub fn test_funding_and_commitment_tx_confirm_same_block() {
-	do_test_funding_and_commitment_tx_confirm_same_block(false);
-	do_test_funding_and_commitment_tx_confirm_same_block(true);
-}
-
 #[xtest(feature = "_externalize_tests")]
 pub fn test_accept_inbound_channel_errors_queued() {
 	// For manually accepted inbound channels, tests that a close error is correctly handled