Limit return value of coop close methods to possible states

Our coop close methods `Channel::maybe_propose_closing_signed` and
`Channel::closing_signed` may return a `Transaction` to broadcast
as well as a `ShutdownResult` to provide the post-shutdown
handling fields. However, it only does either both of them or
neither - we only and always broadcast when we're done closing.

Here we tweak the return values to match the possible states,
combining the two fields in the return value into a single
`Option`.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -8819,10 +8819,7 @@ where
 
 	pub fn maybe_propose_closing_signed<F: Deref, L: Deref>(
 		&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
-	) -> Result<
-		(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>),
-		ChannelError,
-	>
+	) -> Result<(Option<msgs::ClosingSigned>, Option<(Transaction, ShutdownResult)>), ChannelError>
 	where
 		F::Target: FeeEstimator,
 		L::Target: Logger,
@@ -8832,20 +8829,20 @@ where
 		// initiate `closing_signed` negotiation until we're clear of all pending messages. Note
 		// that closing_negotiation_ready checks this case (as well as a few others).
 		if self.context.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
-			return Ok((None, None, None));
+			return Ok((None, None));
 		}
 
 		if !self.funding.is_outbound() {
 			if let Some(msg) = &self.context.pending_counterparty_closing_signed.take() {
 				return self.closing_signed(fee_estimator, &msg, logger);
 			}
-			return Ok((None, None, None));
+			return Ok((None, None));
 		}
 
 		// If we're waiting on a counterparty `commitment_signed` to clear some updates from our
 		// local commitment transaction, we can't yet initiate `closing_signed` negotiation.
 		if self.context.expecting_peer_commitment_signed {
-			return Ok((None, None, None));
+			return Ok((None, None));
 		}
 
 		let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
@@ -8864,7 +8861,7 @@ where
 			our_max_fee,
 			logger,
 		);
-		Ok((closing_signed, None, None))
+		Ok((closing_signed, None))
 	}
 
 	fn mark_response_received(&mut self) {
@@ -9127,10 +9124,7 @@ where
 	pub fn closing_signed<F: Deref, L: Deref>(
 		&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::ClosingSigned,
 		logger: &L,
-	) -> Result<
-		(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>),
-		ChannelError,
-	>
+	) -> Result<(Option<msgs::ClosingSigned>, Option<(Transaction, ShutdownResult)>), ChannelError>
 	where
 		F::Target: FeeEstimator,
 		L::Target: Logger,
@@ -9170,7 +9164,7 @@ where
 
 		if self.context.channel_state.is_monitor_update_in_progress() {
 			self.context.pending_counterparty_closing_signed = Some(msg.clone());
-			return Ok((None, None, None));
+			return Ok((None, None));
 		}
 
 		let funding_redeemscript = self.funding.get_funding_redeemscript();
@@ -9224,7 +9218,7 @@ where
 					self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
 				self.context.channel_state = ChannelState::ShutdownComplete;
 				self.context.update_time_counter += 1;
-				return Ok((None, Some(tx), Some(shutdown_result)));
+				return Ok((None, Some((tx, shutdown_result))));
 			}
 		}
 
@@ -9247,26 +9241,25 @@ where
 					our_max_fee,
 					logger,
 				);
-				let (signed_tx, shutdown_result) = if $new_fee == msg.fee_satoshis {
-					let shutdown_result =
-						closing_signed.as_ref().map(|_| self.shutdown_result_coop_close());
-					if closing_signed.is_some() {
-						self.context.channel_state = ChannelState::ShutdownComplete;
-					}
+				let signed_tx_shutdown = if $new_fee == msg.fee_satoshis {
 					self.context.update_time_counter += 1;
 					self.context.last_received_closing_sig = Some(msg.signature.clone());
-					let tx = closing_signed.as_ref().map(|ClosingSigned { signature, .. }| {
-						self.build_signed_closing_transaction(
+					if let Some(ClosingSigned { signature, .. }) = &closing_signed {
+						let shutdown_result = self.shutdown_result_coop_close();
+						self.context.channel_state = ChannelState::ShutdownComplete;
+						let tx = self.build_signed_closing_transaction(
 							&closing_tx,
 							&msg.signature,
 							signature,
-						)
-					});
-					(tx, shutdown_result)
+						);
+						Some((tx, shutdown_result))
+					} else {
+						None
+					}
 				} else {
-					(None, None)
+					None
 				};
-				return Ok((closing_signed, signed_tx, shutdown_result))
+				return Ok((closing_signed, signed_tx_shutdown))
 			};
 		}
 

lightning/src/ln/channelmanager.rs

@@ -9766,26 +9766,26 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				hash_map::Entry::Occupied(mut chan_entry) => {
 					if let Some(chan) = chan_entry.get_mut().as_funded_mut() {
 						let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-						let (closing_signed, tx, shutdown_result) = try_channel_entry!(self, peer_state, chan.closing_signed(&self.fee_estimator, &msg, &&logger), chan_entry);
-						debug_assert_eq!(shutdown_result.is_some(), chan.is_shutdown());
+						let res = chan.closing_signed(&self.fee_estimator, &msg, &&logger);
+						let (closing_signed, tx_shutdown_result) =
+							try_channel_entry!(self, peer_state, res, chan_entry);
+						debug_assert_eq!(tx_shutdown_result.is_some(), chan.is_shutdown());
 						if let Some(msg) = closing_signed {
 							peer_state.pending_msg_events.push(MessageSendEvent::SendClosingSigned {
 								node_id: counterparty_node_id.clone(),
 								msg,
 							});
 						}
-						if let Some(mut close_res) = shutdown_result {
+						if let Some((tx, mut close_res)) = tx_shutdown_result {
 							// We're done with this channel, we've got a signed closing transaction and
 							// will send the closing_signed back to the remote peer upon return. This
 							// 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)!
-							debug_assert!(tx.is_some());
 							locked_close_channel!(self, peer_state, chan, close_res, FUNDED);
-							(tx, Some(chan_entry.remove()), Some(close_res))
+							(Some(tx), Some(chan_entry.remove()), Some(close_res))
 						} else {
-							debug_assert!(tx.is_none());
-							(tx, None, None)
+							(None, None, None)
 						}
 					} else {
 						return try_channel_entry!(self, peer_state, Err(ChannelError::close(
@@ -11136,19 +11136,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						Some(funded_chan) => {
 							let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None);
 							match funded_chan.maybe_propose_closing_signed(&self.fee_estimator, &&logger) {
-								Ok((msg_opt, tx_opt, shutdown_result_opt)) => {
+								Ok((msg_opt, tx_shutdown_result_opt)) => {
 									if let Some(msg) = msg_opt {
 										has_update = true;
 										pending_msg_events.push(MessageSendEvent::SendClosingSigned {
 											node_id: funded_chan.context.get_counterparty_node_id(), msg,
 										});
 									}
-									debug_assert_eq!(shutdown_result_opt.is_some(), funded_chan.is_shutdown());
-									if let Some(mut shutdown_result) = shutdown_result_opt {
-										locked_close_channel!(self, peer_state, funded_chan, shutdown_result, FUNDED);
-										shutdown_results.push(shutdown_result);
-									}
-									if let Some(tx) = tx_opt {
+									debug_assert_eq!(tx_shutdown_result_opt.is_some(), funded_chan.is_shutdown());
+									if let Some((tx, mut shutdown_res)) = tx_shutdown_result_opt {
+										locked_close_channel!(self, peer_state, funded_chan, shutdown_res, FUNDED);
+										shutdown_results.push(shutdown_res);
 										// We're done with this channel. We got a closing_signed and sent back
 										// a closing_signed with a closing transaction to broadcast.
 										if let Ok(update) = self.get_channel_update_for_broadcast(&funded_chan) {