Introduce FundingTransactionReadyForSignatures event

The `FundingTransactionReadyForSignatures` event requests witnesses
from the client for their contributed inputs to an interactively
constructed transaction.

The client calls `ChannelManager::funding_transaction_signed` to provide
the witnesses to LDK.

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

@@ -1692,6 +1692,52 @@ pub enum Event {
 		/// [`ChannelManager::send_static_invoice`]: crate::ln::channelmanager::ChannelManager::send_static_invoice
 		reply_path: Responder,
 	},
+	/// Indicates that a channel funding transaction constructed interactively is ready to be
+	/// signed. This event will only be triggered if at least one input was contributed.
+	///
+	/// The transaction contains all inputs and outputs provided by both parties including the
+	/// channel's funding output and a change output if applicable.
+	///
+	/// No part of the transaction should be changed before signing as the content of the transaction
+	/// has already been negotiated with the counterparty.
+	///
+	/// Each signature MUST use the `SIGHASH_ALL` flag to avoid invalidation of the initial commitment and
+	/// hence possible loss of funds.
+	///
+	/// After signing, call [`ChannelManager::funding_transaction_signed`] with the (partially) signed
+	/// funding transaction.
+	///
+	/// Generated in [`ChannelManager`] message handling.
+	///
+	/// # Failure Behavior and Persistence
+	/// This event will eventually be replayed after failures-to-handle (i.e., the event handler
+	/// returning `Err(ReplayEvent ())`), but will only be regenerated as needed after restarts.
+	///
+	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+	/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed
+	FundingTransactionReadyForSigning {
+		/// The `channel_id` of the channel which you'll need to pass back into
+		/// [`ChannelManager::funding_transaction_signed`].
+		///
+		/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed
+		channel_id: ChannelId,
+		/// The counterparty's `node_id`, which you'll need to pass back into
+		/// [`ChannelManager::funding_transaction_signed`].
+		///
+		/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed
+		counterparty_node_id: PublicKey,
+		/// The `user_channel_id` value passed in for outbound channels, or for inbound channels if
+		/// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise
+		/// `user_channel_id` will be randomized for inbound channels.
+		///
+		/// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels
+		user_channel_id: u128,
+		/// The unsigned transaction to be signed and passed back to
+		/// [`ChannelManager::funding_transaction_signed`].
+		///
+		/// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed
+		unsigned_transaction: Transaction,
+	},
 }
 
 impl Writeable for Event {
@@ -2134,6 +2180,11 @@ impl Writeable for Event {
 				47u8.write(writer)?;
 				// Never write StaticInvoiceRequested events as buffered onion messages aren't serialized.
 			},
+			&Event::FundingTransactionReadyForSigning { .. } => {
+				49u8.write(writer)?;
+				// 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
 			// data via `write_tlv_fields`.
@@ -2716,6 +2767,8 @@ impl MaybeReadable for Event {
 			// Note that we do not write a length-prefixed TLV for StaticInvoiceRequested events.
 			#[cfg(async_payments)]
 			47u8 => Ok(None),
+			// Note that we do not write a length-prefixed TLV for FundingTransactionReadyForSigning events.
+			49u8 => Ok(None),
 			// Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
 			// Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt
 			// reads.

lightning/src/ln/channel.rs

@@ -14,7 +14,7 @@ use bitcoin::constants::ChainHash;
 use bitcoin::script::{Builder, Script, ScriptBuf, WScriptHash};
 use bitcoin::sighash::EcdsaSighashType;
 use bitcoin::transaction::{Transaction, TxIn, TxOut};
-use bitcoin::Weight;
+use bitcoin::{Weight, Witness};
 
 use bitcoin::hash_types::{BlockHash, Txid};
 use bitcoin::hashes::sha256::Hash as Sha256;
@@ -24,9 +24,9 @@ use bitcoin::hashes::Hash;
 use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE;
 use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1};
 use bitcoin::secp256k1::{PublicKey, SecretKey};
-use bitcoin::{secp256k1, sighash};
 #[cfg(splicing)]
-use bitcoin::{Sequence, Witness};
+use bitcoin::Sequence;
+use bitcoin::{secp256k1, sighash};
 
 use crate::chain::chaininterface::{
 	fee_for_weight, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator,
@@ -2181,6 +2181,11 @@ impl FundingScope {
 		self.channel_transaction_parameters.is_outbound_from_holder
 	}
 
+	/// Returns a reference to the funding_transaction
+	pub fn get_funding_transaction(&self) -> &Option<Transaction> {
+		&self.funding_transaction
+	}
+
 	/// Returns the funding_txo we either got from our peer, or were given by
 	/// get_funding_created.
 	pub fn get_funding_txo(&self) -> Option<OutPoint> {
@@ -2549,7 +2554,6 @@ where
 	monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
 	monitor_pending_finalized_fulfills: Vec<(HTLCSource, Option<AttributionData>)>,
 	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
@@ -3269,7 +3273,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,
@@ -3508,7 +3511,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,
@@ -5544,16 +5546,6 @@ where
 		};
 
 		let funding_ready_for_sig_event = if signing_session.local_inputs_count() == 0 {
-			if signing_session.provide_holder_witnesses(self.channel_id, Vec::new()).is_err() {
-				debug_assert!(
-					false,
-					"Zero inputs were provided & zero witnesses were provided, but a count mismatch was somehow found",
-				);
-				return Err(msgs::TxAbort {
-					channel_id: self.channel_id(),
-					data: "V2 channel rejected due to sender error".to_owned().into_bytes(),
-				});
-			}
 			None
 		} else {
 			// TODO(dual_funding): Send event for signing if we've contributed funds.
@@ -6978,15 +6970,7 @@ where
 		log_info!(logger, "Received initial commitment_signed from peer for channel {}", &self.context.channel_id());
 
 		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);
-		}
-
+		self.interactive_tx_signing_session.as_mut().expect("signing session should be present").received_commitment_signed();
 		Ok(channel_monitor)
 	}
 
@@ -7072,13 +7056,11 @@ where
 			channel_id: Some(self.context.channel_id()),
 		};
 
-		let tx_signatures = self
-			.interactive_tx_signing_session
+		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))
 	}
@@ -7988,10 +7970,39 @@ where
 		}
 	}
 
+	pub fn funding_transaction_signed(
+		&mut self, witnesses: Vec<Witness>,
+	) -> Result<Option<msgs::TxSignatures>, APIError> {
+		let (funding_tx_opt, tx_signatures_opt) = self
+			.interactive_tx_signing_session
+			.as_mut()
+			.ok_or_else(|| APIError::APIMisuseError {
+				err: format!(
+					"Channel {} not expecting funding signatures",
+					self.context.channel_id
+				),
+			})
+			.and_then(|signing_session| {
+				signing_session
+					.provide_holder_witnesses(self.context.channel_id, witnesses)
+					.map_err(|err| APIError::APIMisuseError { err })
+			})?;
+
+		if tx_signatures_opt.is_some() {
+			self.context.channel_state.set_our_tx_signatures_ready();
+		}
+
+		if funding_tx_opt.is_some() {
+			self.funding.funding_transaction = funding_tx_opt;
+			self.context.channel_state =
+				ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
+		}
+
+		Ok(tx_signatures_opt)
+	}
+
 	#[rustfmt::skip]
-	pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<(Option<Transaction>, Option<msgs::TxSignatures>), ChannelError>
-		where L::Target: Logger
-	{
+	pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<Transaction>, Option<msgs::TxSignatures>), ChannelError> {
 		if !self.context.channel_state.is_interactive_signing()
 			|| self.context.channel_state.is_their_tx_signatures_sent()
 		{
@@ -8014,23 +8025,12 @@ where
 				return Err(ChannelError::Close((msg.to_owned(), reason)));
 			}
 
-			if msg.witnesses.len() != signing_session.remote_inputs_count() {
-				return Err(ChannelError::Warn(
-					"Witness count did not match contributed input count".to_string()
-				));
-			}
-
 			for witness in &msg.witnesses {
 				if witness.is_empty() {
 					let msg = "Unexpected empty witness in tx_signatures received";
 					let reason = ClosureReason::ProcessingError { err: msg.to_owned() };
 					return Err(ChannelError::Close((msg.to_owned(), reason)));
 				}
-
-				// TODO(dual_funding): Check all sigs are SIGHASH_ALL.
-
-				// TODO(dual_funding): I don't see how we're going to be able to ensure witness-standardness
-				// for spending. Doesn't seem to be anything in rust-bitcoin.
 			}
 
 			let (holder_tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone())
@@ -8040,24 +8040,16 @@ where
 			self.context.channel_state.set_their_tx_signatures_sent();
 
 			if funding_tx_opt.is_some() {
+				// TODO(splicing): Transition back to `ChannelReady` and not `AwaitingChannelReady`
 				// We have a finalized funding transaction, so we can set the funding transaction.
 				self.funding.funding_transaction = funding_tx_opt.clone();
+				self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
 			}
 
-			// Note that `holder_tx_signatures_opt` will be `None` if we sent `tx_signatures` first, so this
-			// 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;
-				return Ok((None, None));
-			}
-
-			if holder_tx_signatures_opt.is_some() {
-				self.context.channel_state.set_our_tx_signatures_ready();
+			if holder_tx_signatures_opt.is_none() {
+				return Ok((funding_tx_opt, None));
 			}
 
-			self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
 			Ok((funding_tx_opt, holder_tx_signatures_opt))
 		} else {
 			let msg = "Unexpected tx_signatures. No funding transaction awaiting signatures";
@@ -8309,25 +8301,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, tx_signatures: None
 			};
 		}
 
@@ -8357,7 +8338,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, tx_signatures: None
 		}
 	}
 
@@ -8834,7 +8815,6 @@ 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:
@@ -8843,14 +8823,8 @@ where
 							// 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();
-								}
+							if session.holder_tx_signatures().is_none() {
+								log_debug!(logger, "Waiting for funding transaction signatures to be provided");
 								None
 							} else {
 								// If `holder_tx_signatures` is `None` here, the `tx_signatures` message will be sent
@@ -13956,7 +13930,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,