Generate FundingTransactionReadyForSigning on channel resumption

The `handle_channel_resumption` method handles resumption from both
a channel re-establish and a monitor update. When the corresponding
monitor update for the commitment_signed message completes, we will
push the event here.

We can thus only ever provide holder signatures after a monitor update
has completed.

We can also get rid of the reestablish code involved with
`monitor_pending_tx_signatures` and remove that field too.

Author: dunxen

lightning/src/events/mod.rs

@@ -2161,10 +2161,8 @@ impl Writeable for Event {
 			},
 			&Event::FundingTransactionReadyForSigning { .. } => {
 				49u8.write(writer)?;
-				// We never write out FundingTransactionReadyForSigning events as, upon disconnection, peers
-				// drop any V2-established/spliced channels which have not yet exchanged the initial `commitment_signed`.
-				// We only exhange the initial `commitment_signed` after the client calls
-				// `ChannelManager::funding_transaction_signed` and ALWAYS before we send a `tx_signatures`
+				// We never write out FundingTransactionReadyForSigning events as they will be regenerated when
+				// necessary.
 			},
 			// Note that, going forward, all new events must only write data inside of
 			// `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write

lightning/src/ln/channel.rs

@@ -1188,7 +1188,6 @@ pub(super) struct MonitorRestoreUpdates {
 	pub funding_broadcastable: Option<Transaction>,
 	pub channel_ready: Option<msgs::ChannelReady>,
 	pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
-	pub tx_signatures: Option<msgs::TxSignatures>,
 }
 
 /// The return value of `signer_maybe_unblocked`
@@ -1214,7 +1213,6 @@ pub(super) struct ReestablishResponses {
 	pub order: RAACommitmentOrder,
 	pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
 	pub shutdown_msg: Option<msgs::Shutdown>,
-	pub tx_signatures: Option<msgs::TxSignatures>,
 	pub tx_abort: Option<msgs::TxAbort>,
 }
 
@@ -1766,7 +1764,7 @@ where
 
 	pub fn funding_tx_constructed<L: Deref>(
 		&mut self, signing_session: InteractiveTxSigningSession, logger: &L,
-	) -> Result<(msgs::CommitmentSigned, Option<Transaction>), ChannelError>
+	) -> Result<msgs::CommitmentSigned, ChannelError>
 	where
 		L::Target: Logger,
 	{
@@ -1786,7 +1784,7 @@ where
 	#[rustfmt::skip]
 	pub fn commitment_signed<L: Deref>(
 		&mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
-	) -> Result<(Option<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>, Option<ChannelMonitorUpdate>), ChannelError>
+	) -> Result<(Option<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>>, Option<ChannelMonitorUpdate>, Option<Transaction>), ChannelError>
 	where
 		L::Target: Logger
 	{
@@ -1813,7 +1811,7 @@ where
 					pending_splice: None,
 				};
 				let res = funded_channel.commitment_signed_initial_v2(msg, best_block, signer_provider, logger)
-					.map(|monitor| (Some(monitor), None))
+					.map(|(monitor, funding_tx_opt)| (Some(monitor), None, funding_tx_opt))
 					// TODO: Change to `inspect_err` when MSRV is high enough.
 					.map_err(|err| {
 						// We always expect a `ChannelError` close.
@@ -1840,15 +1838,15 @@ where
 				let res = if has_negotiated_pending_splice && !session_received_commitment_signed {
 					funded_channel
 						.splice_initial_commitment_signed(msg, logger)
-						.map(|monitor_update_opt| (None, monitor_update_opt))
+						.map(|monitor_update_opt| (None, monitor_update_opt, None))
 				} else {
 					funded_channel.commitment_signed(msg, logger)
-						.map(|monitor_update_opt| (None, monitor_update_opt))
+						.map(|monitor_update_opt| (None, monitor_update_opt, None))
 				};
 
 				#[cfg(not(splicing))]
 				let res = funded_channel.commitment_signed(msg, logger)
-					.map(|monitor_update_opt| (None, monitor_update_opt));
+					.map(|monitor_update_opt| (None, monitor_update_opt, None));
 
 				self.phase = ChannelPhase::Funded(funded_channel);
 				res
@@ -2315,7 +2313,6 @@ where
 	monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
 	monitor_pending_finalized_fulfills: Vec<HTLCSource>,
 	monitor_pending_update_adds: Vec<msgs::UpdateAddHTLC>,
-	monitor_pending_tx_signatures: Option<msgs::TxSignatures>,
 
 	/// If we went to send a revoke_and_ack but our signer was unable to give us a signature,
 	/// we should retry at some point in the future when the signer indicates it may have a
@@ -2916,7 +2913,7 @@ where
 	#[rustfmt::skip]
 	pub fn funding_tx_constructed<L: Deref>(
 		&mut self, mut signing_session: InteractiveTxSigningSession, logger: &L
-	) -> Result<(msgs::CommitmentSigned, Option<Transaction>), ChannelError>
+	) -> Result<msgs::CommitmentSigned, ChannelError>
 	where
 		L::Target: Logger
 	{
@@ -2954,7 +2951,8 @@ where
 			},
 		};
 
-		let funding_tx_opt = if signing_session.local_inputs_count() == 0 {
+		// Check that we have the expected number of local inputs
+		if signing_session.local_inputs_count() == 0 {
 			debug_assert_eq!(our_funding_satoshis, 0);
 			if signing_session.provide_holder_witnesses(self.context.channel_id, Vec::new()).is_err() {
 				debug_assert!(
@@ -2965,10 +2963,7 @@ where
 				let reason = ClosureReason::ProcessingError { err: msg.to_owned() };
 				return Err(ChannelError::Close((msg.to_owned(), reason)));
 			}
-			None
-		} else {
-			Some(signing_session.unsigned_tx().build_unsigned_tx())
-		};
+		}
 
 		let mut channel_state = ChannelState::FundingNegotiated(FundingNegotiatedFlags::new());
 		channel_state.set_interactive_signing();
@@ -2978,7 +2973,7 @@ where
 		self.interactive_tx_constructor.take();
 		self.interactive_tx_signing_session = Some(signing_session);
 
-		Ok((commitment_signed, funding_tx_opt))
+		Ok(commitment_signed)
 	}
 }
 
@@ -3260,7 +3255,6 @@ where
 			monitor_pending_failures: Vec::new(),
 			monitor_pending_finalized_fulfills: Vec::new(),
 			monitor_pending_update_adds: Vec::new(),
-			monitor_pending_tx_signatures: None,
 
 			signer_pending_revoke_and_ack: false,
 			signer_pending_commitment_update: false,
@@ -3506,7 +3500,6 @@ where
 			monitor_pending_failures: Vec::new(),
 			monitor_pending_finalized_fulfills: Vec::new(),
 			monitor_pending_update_adds: Vec::new(),
-			monitor_pending_tx_signatures: None,
 
 			signer_pending_revoke_and_ack: false,
 			signer_pending_commitment_update: false,
@@ -6628,7 +6621,7 @@ where
 	#[rustfmt::skip]
 	pub fn commitment_signed_initial_v2<L: Deref>(
 		&mut self, msg: &msgs::CommitmentSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
-	) -> Result<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>, ChannelError>
+	) -> Result<(ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>, Option<Transaction>), ChannelError>
 	where L::Target: Logger
 	{
 		if !self.context.channel_state.is_interactive_signing()
@@ -6650,15 +6643,12 @@ where
 
 		self.monitor_updating_paused(false, false, false, Vec::new(), Vec::new(), Vec::new());
 
-		if let Some(tx_signatures) = self.interactive_tx_signing_session.as_mut().and_then(
-			|session| session.received_commitment_signed()
-		) {
-			// We're up first for submitting our tx_signatures, but our monitor has not persisted yet
-			// so they'll be sent as soon as that's done.
-			self.context.monitor_pending_tx_signatures = Some(tx_signatures);
-		}
+		let _ = self.interactive_tx_signing_session.as_mut().and_then(|session| session.received_commitment_signed());
+		// Only build the unsigned transaction for signing if there are any holder inputs to actually sign
+		let funding_tx_opt = self.interactive_tx_signing_session.as_ref().and_then(|session|
+			session.local_inputs_count().gt(&0).then_some(session.unsigned_tx().build_unsigned_tx()));
 
-		Ok(channel_monitor)
+		Ok((channel_monitor, funding_tx_opt))
 	}
 
 	/// Handles an incoming `commitment_signed` message for the first commitment transaction of the
@@ -6739,13 +6729,12 @@ where
 			channel_id: Some(self.context.channel_id()),
 		};
 
-		let tx_signatures = self
+		let _ = self
 			.interactive_tx_signing_session
 			.as_mut()
 			.expect("Signing session must exist for negotiated pending splice")
 			.received_commitment_signed();
 		self.monitor_updating_paused(false, false, false, Vec::new(), Vec::new(), Vec::new());
-		self.context.monitor_pending_tx_signatures = tx_signatures;
 
 		Ok(self.push_ret_blockable_mon_update(monitor_update))
 	}
@@ -7606,8 +7595,10 @@ where
 				.map_err(|err| APIError::APIMisuseError { err })?
 			{
 				if self.is_awaiting_initial_mon_persist() {
-					log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
-					self.context.monitor_pending_tx_signatures = Some(holder_tx_signatures);
+					log_debug!(
+						logger,
+						"Not sending tx_signatures: a monitor update is in progress."
+					);
 					return Ok(None);
 				}
 				return Ok(Some(holder_tx_signatures));
@@ -7684,8 +7675,7 @@ where
 			// case checks if there is a monitor persist in progress when we need to respond with our `tx_signatures`
 			// and sets it as pending.
 			if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() {
-				log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
-				self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt;
+				log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress.");
 				return Ok((None, None));
 			}
 
@@ -7941,25 +7931,14 @@ where
 		mem::swap(&mut finalized_claimed_htlcs, &mut self.context.monitor_pending_finalized_fulfills);
 		let mut pending_update_adds = Vec::new();
 		mem::swap(&mut pending_update_adds, &mut self.context.monitor_pending_update_adds);
-		// For channels established with V2 establishment we won't send a `tx_signatures` when we're in
-		// MonitorUpdateInProgress (and we assume the user will never directly broadcast the funding
-		// transaction and waits for us to do it).
-		let tx_signatures = self.context.monitor_pending_tx_signatures.take();
-		if tx_signatures.is_some() {
-			if self.context.channel_state.is_their_tx_signatures_sent() {
-				self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
-			} else {
-				self.context.channel_state.set_our_tx_signatures_ready();
-			}
-		}
 
 		if self.context.channel_state.is_peer_disconnected() {
 			self.context.monitor_pending_revoke_and_ack = false;
 			self.context.monitor_pending_commitment_signed = false;
 			return MonitorRestoreUpdates {
 				raa: None, commitment_update: None, order: RAACommitmentOrder::RevokeAndACKFirst,
 				accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, pending_update_adds,
-				funding_broadcastable, channel_ready, announcement_sigs, tx_signatures
+				funding_broadcastable, channel_ready, announcement_sigs
 			};
 		}
 
@@ -7989,7 +7968,7 @@ where
 			match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
 		MonitorRestoreUpdates {
 			raa, commitment_update, order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs,
-			pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs, tx_signatures
+			pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs
 		}
 	}
 
@@ -8358,7 +8337,6 @@ where
 					raa: None, commitment_update: None,
 					order: RAACommitmentOrder::CommitmentFirst,
 					shutdown_msg, announcement_sigs,
-					tx_signatures: None,
 					tx_abort: None,
 				});
 			}
@@ -8369,7 +8347,6 @@ where
 				raa: None, commitment_update: None,
 				order: RAACommitmentOrder::CommitmentFirst,
 				shutdown_msg, announcement_sigs,
-				tx_signatures: None,
 				tx_abort: None,
 			});
 		}
@@ -8414,7 +8391,7 @@ where
 			}
 
 			// if next_funding_txid is set:
-			let (commitment_update, tx_signatures, tx_abort) = if let Some(next_funding_txid) = msg.next_funding_txid {
+			let (commitment_update, tx_abort) = if let Some(next_funding_txid) = msg.next_funding_txid {
 				if let Some(session) = &self.interactive_tx_signing_session {
 					// if next_funding_txid matches the latest interactive funding transaction:
 					let our_next_funding_txid = session.unsigned_tx().compute_txid();
@@ -8435,41 +8412,14 @@ where
 								update_fee: None,
 							})
 						} else { None };
-						// TODO(dual_funding): For async signing support we need to hold back `tx_signatures` until the `commitment_signed` is ready.
-						let tx_signatures = if (
-							// if it has not received tx_signatures for that funding transaction AND
-							// if it has already received commitment_signed AND it should sign first, as specified in the tx_signatures requirements:
-							//   MUST send its tx_signatures for that funding transaction.
-							!self.context.channel_state.is_their_tx_signatures_sent() && session.has_received_commitment_signed() && session.holder_sends_tx_signatures_first()
-							// else if it has already received tx_signatures for that funding transaction:
-							//   MUST send its tx_signatures for that funding transaction.
-						) || self.context.channel_state.is_their_tx_signatures_sent() {
-							if self.context.channel_state.is_monitor_update_in_progress() {
-								// The `monitor_pending_tx_signatures` field should have already been set in `commitment_signed_initial_v2`
-								// if we were up first for signing and had a monitor update in progress, but check again just in case.
-								debug_assert!(self.context.monitor_pending_tx_signatures.is_some(), "monitor_pending_tx_signatures should already be set");
-								log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
-								if self.context.monitor_pending_tx_signatures.is_none() {
-									self.context.monitor_pending_tx_signatures = session.holder_tx_signatures().clone();
-								}
-								None
-							} else {
-								// If `holder_tx_signatures` is `None` here, the `tx_signatures` message will be sent
-								// when the holder provides their witnesses as this will queue a `tx_signatures` if the
-								// holder must send one.
-								session.holder_tx_signatures().clone()
-							}
-						} else {
-							None
-						};
 						if !session.has_received_commitment_signed() {
 							self.context.expecting_peer_commitment_signed = true;
 						}
-						(commitment_update, tx_signatures, None)
+						(commitment_update, None)
 					} else {
 						// The `next_funding_txid` does not match the latest interactive funding transaction so we
 						// MUST send tx_abort to let the remote  know that they can forget this funding transaction.
-						(None, None, Some(msgs::TxAbort {
+						(None, Some(msgs::TxAbort {
 							channel_id: self.context.channel_id(),
 							data: format!(
 								"next_funding_txid {} does match our latest interactive funding txid {}",
@@ -8480,22 +8430,21 @@ where
 					// We'll just send a `tx_abort` here if we don't have a signing session for this channel
 					// on reestablish and tell our peer to just forget about it.
 					// Our peer is doing something strange, but it doesn't warrant closing the channel.
-					(None, None, Some(msgs::TxAbort {
+					(None, Some(msgs::TxAbort {
 						channel_id: self.context.channel_id(),
 						data:
 							"No active signing session. The associated funding transaction may have already been broadcast.".as_bytes().to_vec() }))
 				}
 			} else {
 				// Don't send anything related to interactive signing if `next_funding_txid` is not set.
-				(None, None, None)
+				(None, None)
 			};
 
 			Ok(ReestablishResponses {
 				channel_ready, shutdown_msg, announcement_sigs,
 				raa: required_revoke,
 				commitment_update,
 				order: self.context.resend_order.clone(),
-				tx_signatures,
 				tx_abort,
 			})
 		} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 {
@@ -8511,7 +8460,6 @@ where
 					channel_ready, shutdown_msg, announcement_sigs,
 					commitment_update: None, raa: None,
 					order: self.context.resend_order.clone(),
-					tx_signatures: None,
 					tx_abort: None,
 				})
 			} else {
@@ -8535,7 +8483,6 @@ where
 					channel_ready, shutdown_msg, announcement_sigs,
 					raa, commitment_update,
 					order: self.context.resend_order.clone(),
-					tx_signatures: None,
 					tx_abort: None,
 				})
 			}
@@ -13211,7 +13158,6 @@ where
 				monitor_pending_failures,
 				monitor_pending_finalized_fulfills: monitor_pending_finalized_fulfills.unwrap(),
 				monitor_pending_update_adds: monitor_pending_update_adds.unwrap_or_default(),
-				monitor_pending_tx_signatures: None,
 
 				signer_pending_revoke_and_ack: false,
 				signer_pending_commitment_update: false,