Remove unnecessary funding tx clone & remote tx_sigs received check

We actually don't need to check if the counterparty had already sent
their `tx_signatures` in `InteractiveTxSigningSession::received_tx_signatures`.

Further, we can get rid of the clone of `funding_tx_opt` in
`FundedChannel::tx_signatures` when setting the `ChannelContext::funding_transaction`
as we don't actually need to propagate it through to
`ChannelManager::internal_tx_complete` as we can use
`ChannelContext::unbroadcasted_funding()` which clones the
`ChannelContext::funding_transaction` anyway.

Author: dunxen

lightning/src/ln/channel.rs

@@ -2188,6 +2188,7 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
 					context: self.context,
 					interactive_tx_signing_session: Some(signing_session),
 					holder_commitment_point,
+					is_v2_established: true,
 				};
 				Ok((funded_chan, commitment_signed, funding_ready_for_sig_event))
 			},
@@ -4542,6 +4543,9 @@ pub(super) struct FundedChannel<SP: Deref> where SP::Target: SignerProvider {
 	pub context: ChannelContext<SP>,
 	pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,
 	holder_commitment_point: HolderCommitmentPoint,
+	/// Indicates whether this funded channel had been established with V2 channel
+	/// establishment (i.e. is a dual-funded channel).
+	is_v2_established: bool,
 }
 
 #[cfg(any(test, fuzzing))]
@@ -5996,10 +6000,10 @@ impl<SP: Deref> FundedChannel<SP> where
 		}
 	}
 
-	pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError>
+	pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<Option<msgs::TxSignatures>, ChannelError>
 		where L::Target: Logger
 	{
-		if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) {
+		if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) {
 			return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned()));
 		}
 
@@ -6033,25 +6037,23 @@ impl<SP: Deref> FundedChannel<SP> where
 				// for spending. Doesn't seem to be anything in rust-bitcoin.
 			}
 
-			let (tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone())
+			let (holder_tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone())
 				.map_err(|_| ChannelError::Warn("Witness count did not match contributed input count".to_string()))?;
+			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);
+			}
 			if funding_tx_opt.is_some() {
+				// We have a persisted channel monitor and and a finalized funding transaction, so we can move
+				// the channel state forward, set the funding transaction and reset the signing session fields.
+				self.context.funding_transaction = funding_tx_opt;
+				self.context.next_funding_txid = None;
+				self.interactive_tx_signing_session = None;
 				self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
 			}
-			self.context.funding_transaction = funding_tx_opt.clone();
-
-			self.context.next_funding_txid = None;
-
-			// Clear out the signing session
-			self.interactive_tx_signing_session = None;
-
-			if tx_signatures_opt.is_some() && self.context.channel_state.is_monitor_update_in_progress() {
-				log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
-				self.context.monitor_pending_tx_signatures = tx_signatures_opt;
-				return Ok((None, None));
-			}
 
-			Ok((tx_signatures_opt, funding_tx_opt))
+			Ok(holder_tx_signatures_opt)
 		} else {
 			Err(ChannelError::Close((
 				"Unexpected tx_signatures. No funding transaction awaiting signatures".to_string(),
@@ -6254,12 +6256,12 @@ impl<SP: Deref> FundedChannel<SP> where
 		assert!(self.context.channel_state.is_monitor_update_in_progress());
 		self.context.channel_state.clear_monitor_update_in_progress();
 
-		// If we're past (or at) the AwaitingChannelReady stage on an outbound channel, try to
-		// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
+		// If we're past (or at) the AwaitingChannelReady stage on an outbound (or V2-established) channel,
+		// try to (re-)broadcast the funding transaction as we may have declined to broadcast it when we
 		// first received the funding_signed.
 		let mut funding_broadcastable = None;
 		if let Some(funding_transaction) = &self.context.funding_transaction {
-			if self.context.is_outbound() &&
+			if (self.context.is_outbound() || self.is_v2_established()) &&
 				(matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(flags) if !flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) ||
 				matches!(self.context.channel_state, ChannelState::ChannelReady(_)))
 			{
@@ -8551,6 +8553,10 @@ impl<SP: Deref> FundedChannel<SP> where
 			})
 			.chain(self.context.pending_outbound_htlcs.iter().map(|htlc| (&htlc.source, &htlc.payment_hash)))
 	}
+
+	pub fn is_v2_established(&self) -> bool {
+		self.is_v2_established
+	}
 }
 
 /// A not-yet-funded outbound (from holder) channel using V1 channel establishment.
@@ -8814,6 +8820,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 		let mut channel = FundedChannel {
 			context: self.context,
 			interactive_tx_signing_session: None,
+			is_v2_established: false,
 			holder_commitment_point,
 		};
 
@@ -9079,6 +9086,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 		let mut channel = FundedChannel {
 			context: self.context,
 			interactive_tx_signing_session: None,
+			is_v2_established: false,
 			holder_commitment_point,
 		};
 		let need_channel_ready = channel.check_get_channel_ready(0, logger).is_some()
@@ -9887,7 +9895,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 			let mut _val: u64 = Readable::read(reader)?;
 		}
 
-		let channel_id = Readable::read(reader)?;
+		let channel_id: ChannelId = Readable::read(reader)?;
 		let channel_state = ChannelState::from_u32(Readable::read(reader)?).map_err(|_| DecodeError::InvalidValue)?;
 		let channel_value_satoshis = Readable::read(reader)?;
 
@@ -10323,6 +10331,10 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 				}
 			},
 		};
+		let is_v2_established = channel_id.is_v2_channel_id(
+			&channel_parameters.holder_pubkeys.revocation_basepoint,
+			&channel_parameters.counterparty_parameters.as_ref()
+				.expect("Persisted channel must have counterparty parameters").pubkeys.revocation_basepoint);
 
 		Ok(FundedChannel {
 			context: ChannelContext {
@@ -10460,6 +10472,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 				next_funding_txid: None,
 			},
 			interactive_tx_signing_session: None,
+			is_v2_established,
 			holder_commitment_point,
 		})
 	}

lightning/src/ln/channelmanager.rs

@@ -8414,14 +8414,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				match chan_entry.get_mut().as_funded_mut() {
 					Some(chan) => {
 						let logger = WithChannelContext::from(&self.logger, &chan.context, None);
-						let (tx_signatures_opt, funding_tx_opt) = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry);
+						let tx_signatures_opt = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry);
 						if let Some(tx_signatures) = tx_signatures_opt {
 							peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures {
 								node_id: *counterparty_node_id,
 								msg: tx_signatures,
 							});
 						}
-						if let Some(ref funding_tx) = funding_tx_opt {
+						if let Some(ref funding_tx) = chan.context.unbroadcasted_funding() {
 							self.tx_broadcaster.broadcast_transactions(&[funding_tx]);
 							{
 								let mut pending_events = self.pending_events.lock().unwrap();

lightning/src/ln/dual_funding_tests.rs

@@ -23,13 +23,13 @@ use {
 	crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint},
 	crate::ln::functional_test_utils::*,
 	crate::ln::msgs::ChannelMessageHandler,
-	crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete},
+	crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete, TxSignatures},
 	crate::ln::types::ChannelId,
 	crate::prelude::*,
 	crate::sign::{ChannelSigner as _, P2WPKH_WITNESS_WEIGHT},
 	crate::util::ser::TransactionU16LenLimited,
 	crate::util::test_utils,
-	bitcoin::Weight,
+	bitcoin::{Weight, Witness},
 };
 
 #[cfg(dual_funding)]
@@ -41,9 +41,7 @@ struct V2ChannelEstablishmentTestSession {
 #[cfg(dual_funding)]
 // TODO(dual_funding): Use real node and API for creating V2 channels as initiator when available,
 // instead of manually constructing messages.
-fn do_test_v2_channel_establishment(
-	session: V2ChannelEstablishmentTestSession, test_async_persist: bool,
-) {
+fn do_test_v2_channel_establishment(session: V2ChannelEstablishmentTestSession) {
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -210,37 +208,23 @@ fn do_test_v2_channel_establishment(
 		partial_signature_with_nonce: None,
 	};
 
-	if test_async_persist {
-		chanmon_cfgs[1]
-			.persister
-			.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress);
-	}
+	chanmon_cfgs[1].persister.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress);
 
 	// Handle the initial commitment_signed exchange. Order is not important here.
 	nodes[1]
 		.node
 		.handle_commitment_signed(nodes[0].node.get_our_node_id(), &msg_commitment_signed_from_0);
 	check_added_monitors(&nodes[1], 1);
 
-	if test_async_persist {
-		let events = nodes[1].node.get_and_clear_pending_events();
-		assert!(events.is_empty());
+	// The funding transaction should not have been broadcast before persisting initial monitor has
+	// been completed.
+	assert_eq!(nodes[1].tx_broadcaster.txn_broadcast().len(), 0);
+	assert_eq!(nodes[1].node.get_and_clear_pending_events().len(), 0);
 
-		chanmon_cfgs[1]
-			.persister
-			.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::Completed);
-		let (latest_update, _) = *nodes[1]
-			.chain_monitor
-			.latest_monitor_update_id
-			.lock()
-			.unwrap()
-			.get(&channel_id)
-			.unwrap();
-		nodes[1]
-			.chain_monitor
-			.chain_monitor
-			.force_channel_monitor_updated(channel_id, latest_update);
-	}
+	// Complete the persistence of the monitor.
+	let events = nodes[1].node.get_and_clear_pending_events();
+	assert!(events.is_empty());
+	nodes[1].chain_monitor.complete_sole_pending_chan_update(&channel_id);
 
 	let events = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -256,19 +240,30 @@ fn do_test_v2_channel_establishment(
 	);
 
 	assert_eq!(tx_signatures_msg.channel_id, channel_id);
+
+	let mut witness = Witness::new();
+	witness.push([0x0]);
+	// Receive tx_signatures from channel initiator.
+	nodes[1].node.handle_tx_signatures(
+		nodes[0].node.get_our_node_id(),
+		&TxSignatures {
+			channel_id,
+			tx_hash: funding_outpoint.unwrap().txid,
+			witnesses: vec![witness],
+			shared_input_signature: None,
+		},
+	);
+
+	// For an inbound channel V2 channel the transaction should be broadcast once receiving a
+	// tx_signature and applying local tx_signatures:
+	let broadcasted_txs = nodes[1].tx_broadcaster.txn_broadcast();
+	assert_eq!(broadcasted_txs.len(), 1);
 }
 
 #[test]
 #[cfg(dual_funding)]
 fn test_v2_channel_establishment() {
-	// Only initiator contributes, no persist pending
-	do_test_v2_channel_establishment(
-		V2ChannelEstablishmentTestSession { initiator_input_value_satoshis: 100_000 },
-		false,
-	);
-	// Only initiator contributes, persist pending
-	do_test_v2_channel_establishment(
-		V2ChannelEstablishmentTestSession { initiator_input_value_satoshis: 100_000 },
-		true,
-	);
+	do_test_v2_channel_establishment(V2ChannelEstablishmentTestSession {
+		initiator_input_value_satoshis: 100_000,
+	});
 }

lightning/src/ln/interactivetxs.rs

@@ -316,14 +316,18 @@ impl InteractiveTxSigningSession {
 
 	/// Handles a `tx_signatures` message received from the counterparty.
 	///
+	/// If the holder is required to send their `tx_signatures` message and these signatures have
+	/// already been provided to the signing session, then this return value will be `Some`, otherwise
+	/// None.
+	///
+	/// If the holder has already provided their `tx_signatures` to the signing session, a funding
+	/// transaction will be finalized and returned as Some, otherwise None.
+	///
 	/// Returns an error if the witness count does not equal the counterparty's input count in the
 	/// unsigned transaction.
 	pub fn received_tx_signatures(
 		&mut self, tx_signatures: TxSignatures,
 	) -> Result<(Option<TxSignatures>, Option<Transaction>), ()> {
-		if self.counterparty_sent_tx_signatures {
-			return Ok((None, None));
-		};
 		if self.remote_inputs_count() != tx_signatures.witnesses.len() {
 			return Err(());
 		}
@@ -336,13 +340,16 @@ impl InteractiveTxSigningSession {
 			None
 		};
 
-		let funding_tx = if self.holder_tx_signatures.is_some() {
+		// Check if the holder has provided its signatures and if so,
+		// return the finalized funding transaction.
+		let funding_tx_opt = if self.holder_tx_signatures.is_some() {
 			Some(self.finalize_funding_tx())
 		} else {
+			// This means we're still waiting for the holder to provide their signatures.
 			None
 		};
 
-		Ok((holder_tx_signatures, funding_tx))
+		Ok((holder_tx_signatures, funding_tx_opt))
 	}
 
 	/// Provides the holder witnesses for the unsigned transaction.

lightning/src/ln/types.rs

@@ -101,6 +101,11 @@ impl ChannelId {
 	pub fn temporary_v2_from_revocation_basepoint(our_revocation_basepoint: &RevocationBasepoint) -> Self {
 		Self(Sha256::hash(&[[0u8; 33], our_revocation_basepoint.0.serialize()].concat()).to_byte_array())
 	}
+
+	/// Indicates whether this is a V2 channel ID for the given local and remote revocation basepoints.
+	pub fn is_v2_channel_id(&self, ours: &RevocationBasepoint, theirs: &RevocationBasepoint) -> bool {
+		*self == Self::v2_from_revocation_basepoints(ours, theirs)
+	}
 }
 
 impl Writeable for ChannelId {
@@ -211,4 +216,16 @@ mod tests {
 
 		assert_eq!(ChannelId::v2_from_revocation_basepoints(&ours, &theirs), expected_id);
 	}
+
+	#[test]
+	fn test_is_v2_channel_id() {
+		let ours = RevocationBasepoint(PublicKey::from_slice(&<Vec<u8>>::from_hex("0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c").unwrap()[..]).unwrap());
+		let theirs = RevocationBasepoint(PublicKey::from_slice(&<Vec<u8>>::from_hex("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap());
+
+		let channel_id = ChannelId::v2_from_revocation_basepoints(&ours, &theirs);
+		assert!(channel_id.is_v2_channel_id(&ours, &theirs));
+
+		let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
+		assert!(!channel_id.is_v2_channel_id(&ours, &theirs))
+	}
 }