Follow-ups to PR 3137

ci/ci-tests.sh

@@ -132,3 +132,5 @@ RUSTFLAGS="--cfg=taproot" cargo test --verbose --color always -p lightning
 RUSTFLAGS="--cfg=splicing" cargo test --verbose --color always -p lightning
 [ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
 RUSTFLAGS="--cfg=async_payments" cargo test --verbose --color always -p lightning
+[ "$CI_MINIMIZE_DISK_USAGE" != "" ] && cargo clean
+RUSTFLAGS="--cfg=dual_funding" cargo test --verbose --color always -p lightning

lightning/src/ln/channelmanager.rs

@@ -3207,7 +3207,7 @@ macro_rules! handle_monitor_update_completion {
 			&mut $peer_state.pending_msg_events, $chan, updates.raa,
 			updates.commitment_update, updates.order, updates.accepted_htlcs, updates.pending_update_adds,
 			updates.funding_broadcastable, updates.channel_ready,
-			updates.announcement_sigs, updates.tx_signatures);
+			updates.announcement_sigs, updates.tx_signatures, None);
 		if let Some(upd) = channel_update {
 			$peer_state.pending_msg_events.push(upd);
 		}
@@ -7470,10 +7470,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		pending_forwards: Vec<(PendingHTLCInfo, u64)>, pending_update_adds: Vec<msgs::UpdateAddHTLC>,
 		funding_broadcastable: Option<Transaction>,
 		channel_ready: Option<msgs::ChannelReady>, announcement_sigs: Option<msgs::AnnouncementSignatures>,
-		tx_signatures: Option<msgs::TxSignatures>
+		tx_signatures: Option<msgs::TxSignatures>, tx_abort: Option<msgs::TxAbort>,
 	) -> (Option<(u64, Option<PublicKey>, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec<msgs::UpdateAddHTLC>)>) {
 		let logger = WithChannelContext::from(&self.logger, &channel.context, None);
-		log_trace!(logger, "Handling channel resumption for channel {} with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures",
+		log_trace!(logger, "Handling channel resumption for channel {} with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures, {} tx_abort",
 			&channel.context.channel_id(),
 			if raa.is_some() { "an" } else { "no" },
 			if commitment_update.is_some() { "a" } else { "no" },
@@ -7482,6 +7482,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 			if channel_ready.is_some() { "sending" } else { "without" },
 			if announcement_sigs.is_some() { "sending" } else { "without" },
 			if tx_signatures.is_some() { "sending" } else { "without" },
+			if tx_abort.is_some() { "sending" } else { "without" },
 		);
 
 		let counterparty_node_id = channel.context.get_counterparty_node_id();
@@ -7515,6 +7516,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 				msg,
 			});
 		}
+		if let Some(msg) = tx_abort {
+			pending_msg_events.push(events::MessageSendEvent::SendTxAbort {
+				node_id: counterparty_node_id,
+				msg,
+			});
+		}
 
 		macro_rules! handle_cs { () => {
 			if let Some(update) = commitment_update {
@@ -8362,14 +8369,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();
@@ -8789,14 +8796,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		let peer_state = &mut *peer_state_lock;
 		match peer_state.channel_by_id.entry(msg.channel_id) {
 			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 funding_txo = chan.context.get_funding_txo();
-
-					if chan.interactive_tx_signing_session.is_some() {
-						let monitor = try_channel_entry!(
-							self, peer_state, chan.commitment_signed_initial_v2(msg, best_block, &self.signer_provider, &&logger),
-							chan_entry);
+				let chan = chan_entry.get_mut();
+				let logger = WithChannelContext::from(&self.logger, &chan.context(), None);
+				let funding_txo = chan.context().get_funding_txo();
+				let (monitor_opt, monitor_update_opt) = try_channel_entry!(
+					self, peer_state, chan.commitment_signed(msg, best_block, &self.signer_provider, &&logger),
+					chan_entry);
+
+				if let Some(chan) = chan.as_funded_mut() {
+					if let Some(monitor) = monitor_opt {
 						let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor);
 						if let Ok(persist_state) = monitor_res {
 							handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state,
@@ -8811,19 +8819,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 								)
 							)), chan_entry)
 						}
-					} else {
-						let monitor_update_opt = try_channel_entry!(
-							self, peer_state, chan.commitment_signed(msg, &&logger), chan_entry);
-						if let Some(monitor_update) = monitor_update_opt {
-							handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock,
-								peer_state, per_peer_state, chan);
-						}
+					} else if let Some(monitor_update) = monitor_update_opt {
+						handle_new_monitor_update!(self, funding_txo.unwrap(), monitor_update, peer_state_lock,
+							peer_state, per_peer_state, chan);
 					}
-					Ok(())
-				} else {
-					return try_channel_entry!(self, peer_state, Err(ChannelError::close(
-						"Got a commitment_signed message for an unfunded channel!".into())), chan_entry);
 				}
+				Ok(())
 			},
 			hash_map::Entry::Vacant(_) => Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 		}
@@ -9192,7 +9193,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						let need_lnd_workaround = chan.context.workaround_lnd_bug_4006.take();
 						let (htlc_forwards, decode_update_add_htlcs) = self.handle_channel_resumption(
 							&mut peer_state.pending_msg_events, chan, responses.raa, responses.commitment_update, responses.order,
-							Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs, None);
+							Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs,
+							responses.tx_signatures, responses.tx_abort);
 						debug_assert!(htlc_forwards.is_none());
 						debug_assert!(decode_update_add_htlcs.is_none());
 						if let Some(upd) = channel_update {

lightning/src/ln/dual_funding_tests.rs

@@ -23,7 +23,7 @@ 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},
@@ -41,9 +41,9 @@ 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) {
+	use bitcoin::Witness;
+
 	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,34 +210,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 (outpoint, 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(outpoint, 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);
@@ -253,19 +242,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

@@ -289,16 +289,36 @@ impl ConstructedTransaction {
 /// https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#sharing-funding-signatures-tx_signatures
 #[derive(Debug, Clone, PartialEq)]
 pub(crate) struct InteractiveTxSigningSession {
-	pub unsigned_tx: ConstructedTransaction,
+	unsigned_tx: ConstructedTransaction,
+	counterparty_sent_tx_signatures: bool,
 	holder_sends_tx_signatures_first: bool,
-	received_commitment_signed: bool,
+	has_received_commitment_signed: bool,
 	holder_tx_signatures: Option<TxSignatures>,
-	counterparty_sent_tx_signatures: bool,
 }
 
 impl InteractiveTxSigningSession {
+	pub fn unsigned_tx(&self) -> &ConstructedTransaction {
+		&self.unsigned_tx
+	}
+
+	pub fn counterparty_sent_tx_signatures(&self) -> bool {
+		self.counterparty_sent_tx_signatures
+	}
+
+	pub fn holder_sends_tx_signatures_first(&self) -> bool {
+		self.holder_sends_tx_signatures_first
+	}
+
+	pub fn has_received_commitment_signed(&self) -> bool {
+		self.has_received_commitment_signed
+	}
+
+	pub fn holder_tx_signatures(&self) -> &Option<TxSignatures> {
+		&self.holder_tx_signatures
+	}
+
 	pub fn received_commitment_signed(&mut self) -> Option<TxSignatures> {
-		self.received_commitment_signed = true;
+		self.has_received_commitment_signed = true;
 		if self.holder_sends_tx_signatures_first {
 			self.holder_tx_signatures.clone()
 		} else {
@@ -307,7 +327,7 @@ impl InteractiveTxSigningSession {
 	}
 
 	pub fn get_tx_signatures(&self) -> Option<TxSignatures> {
-		if self.received_commitment_signed {
+		if self.has_received_commitment_signed {
 			self.holder_tx_signatures.clone()
 		} else {
 			None
@@ -316,14 +336,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 +360,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.
@@ -351,7 +378,7 @@ impl InteractiveTxSigningSession {
 	/// unsigned transaction.
 	pub fn provide_holder_witnesses(
 		&mut self, channel_id: ChannelId, witnesses: Vec<Witness>,
-	) -> Result<Option<TxSignatures>, ()> {
+	) -> Result<(), ()> {
 		if self.local_inputs_count() != witnesses.len() {
 			return Err(());
 		}
@@ -363,13 +390,8 @@ impl InteractiveTxSigningSession {
 			witnesses: witnesses.into_iter().collect(),
 			shared_input_signature: None,
 		});
-		if self.received_commitment_signed
-			&& (self.holder_sends_tx_signatures_first || self.counterparty_sent_tx_signatures)
-		{
-			Ok(self.holder_tx_signatures.clone())
-		} else {
-			Ok(None)
-		}
+
+		Ok(())
 	}
 
 	pub fn remote_inputs_count(&self) -> usize {
@@ -986,7 +1008,7 @@ macro_rules! define_state_transitions {
 				let signing_session = InteractiveTxSigningSession {
 					holder_sends_tx_signatures_first: tx.holder_sends_tx_signatures_first,
 					unsigned_tx: tx,
-					received_commitment_signed: false,
+					has_received_commitment_signed: false,
 					holder_tx_signatures: None,
 					counterparty_sent_tx_signatures: false,
 				};

lightning/src/ln/msgs.rs

@@ -822,6 +822,18 @@ pub struct ChannelReestablish {
 	/// The sender's per-commitment point for their current commitment transaction
 	pub my_current_per_commitment_point: PublicKey,
 	/// The next funding transaction ID
+	///
+	/// Allows peers to finalize the signing steps of an interactive transaction construction, or
+	/// safely abort that transaction if it was not signed by one of the peers, who has thus already
+	/// removed it from its state.
+	///
+	/// If we've sent `commtiment_signed` for an interactively constructed transaction
+	/// during a signing session, but have not received `tx_signatures` we MUST set `next_funding_txid`
+	/// to the txid of that interactive transaction, else we MUST NOT set it.
+	///
+	/// See the spec for further details on this:
+	///   * `channel_reestablish`-sending node: https:///github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L2466-L2470
+	///   * `channel_reestablish`-receiving node: https:///github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L2520-L2531
 	pub next_funding_txid: Option<Txid>,
 }