Review comments

Author: wvanlint

lightning/src/ln/channel.rs

@@ -793,6 +793,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 
 	pub(crate) channel_transaction_parameters: ChannelTransactionParameters,
 	funding_transaction: Option<Transaction>,
+	funding_txid: Option<Txid>,
 
 	counterparty_cur_commitment_point: Option<PublicKey>,
 	counterparty_prev_commitment_point: Option<PublicKey>,
@@ -1926,16 +1927,28 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 		res
 	}
 
-	/// Returns transaction if there is pending funding transaction that is yet to broadcast
-	pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
+	fn if_unbroadcasted_funding<F, O>(&self, f: F) -> Option<O>
+		where F: Fn() -> Option<O> {
 		if self.channel_state & ChannelState::FundingCreated as u32 != 0 ||
 				self.channel_state & ChannelState::WaitingForBatch as u32 != 0 {
-			self.funding_transaction.clone()
+			f()
 		} else {
 			None
 		}
 	}
 
+	/// Returns the transaction if there is a pending funding transaction that is yet to be
+	/// broadcast.
+	pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
+		self.if_unbroadcasted_funding(|| self.funding_transaction.clone())
+	}
+
+	/// Returns the transaction ID if there is a pending funding transaction that is yet to be
+	/// broadcast.
+	pub fn unbroadcasted_funding_txid(&self) -> Option<Txid> {
+		self.if_unbroadcasted_funding(|| self.funding_txid.clone())
+	}
+
 	/// Gets the latest commitment transaction and any dependent transactions for relay (forcing
 	/// shutdown of this channel - no more calls into this Channel may be made afterwards except
 	/// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
@@ -2603,7 +2616,7 @@ impl<SP: Deref> Channel<SP> where
 
 		let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
 
-		if non_shutdown_state == ChannelState::FundingSent as u32 {
+		if non_shutdown_state & !(ChannelState::WaitingForBatch as u32) == ChannelState::FundingSent as u32 {
 			self.context.channel_state |= ChannelState::TheirChannelReady as u32;
 		} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurChannelReady as u32) {
 			self.context.channel_state = ChannelState::ChannelReady as u32 | (self.context.channel_state & MULTI_STATE_FLAGS);
@@ -3693,6 +3706,7 @@ impl<SP: Deref> Channel<SP> where
 		// first received the funding_signed.
 		let mut funding_broadcastable =
 			if self.context.is_outbound() && self.context.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 && self.context.channel_state & ChannelState::WaitingForBatch as u32 == 0 {
+				self.context.funding_txid.take();
 				self.context.funding_transaction.take()
 			} else { None };
 		// That said, if the funding transaction is already confirmed (ie we're active with a
@@ -4594,7 +4608,7 @@ impl<SP: Deref> Channel<SP> where
 	pub fn is_awaiting_initial_mon_persist(&self) -> bool {
 		if !self.is_awaiting_monitor_update() { return false; }
 		if self.context.channel_state &
-			!(ChannelState::TheirChannelReady as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)
+			!(ChannelState::TheirChannelReady as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32 | ChannelState::WaitingForBatch as u32)
 				== ChannelState::FundingSent as u32 {
 			// If we're not a 0conf channel, we'll be waiting on a monitor update with only
 			// FundingSent set, though our peer could have sent their channel_ready.
@@ -4674,6 +4688,8 @@ impl<SP: Deref> Channel<SP> where
 			return None;
 		}
 
+		// Note that we don't include ChannelState::WaitingForBatch as we don't want to send
+		// channel_ready until the entire batch is ready.
 		let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
 		let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
 			self.context.channel_state |= ChannelState::OurChannelReady as u32;
@@ -5747,6 +5763,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 					channel_type_features: channel_type.clone()
 				},
 				funding_transaction: None,
+				funding_txid: None,
 
 				counterparty_cur_commitment_point: None,
 				counterparty_prev_commitment_point: None,
@@ -5840,6 +5857,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 		self.context.channel_state = ChannelState::FundingCreated as u32;
 		self.context.channel_id = funding_txo.to_channel_id();
 		self.context.funding_transaction = Some(funding_transaction);
+		self.context.funding_txid = self.context.funding_transaction.as_ref().map(|tx| tx.txid());
 
 		let channel = Channel {
 			context: self.context,
@@ -6389,6 +6407,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 					channel_type_features: channel_type.clone()
 				},
 				funding_transaction: None,
+				funding_txid: None,
 
 				counterparty_cur_commitment_point: Some(msg.first_per_commitment_point),
 				counterparty_prev_commitment_point: None,
@@ -7226,7 +7245,8 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 		};
 
 		let mut channel_parameters: ChannelTransactionParameters = Readable::read(reader)?;
-		let funding_transaction = Readable::read(reader)?;
+		let funding_transaction: Option<Transaction> = Readable::read(reader)?;
+		let funding_txid = funding_transaction.as_ref().map(|tx| tx.txid());
 
 		let counterparty_cur_commitment_point = Readable::read(reader)?;
 
@@ -7469,6 +7489,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 
 				channel_transaction_parameters: channel_parameters,
 				funding_transaction,
+				funding_txid,
 
 				counterparty_cur_commitment_point,
 				counterparty_prev_commitment_point,
@@ -7522,7 +7543,7 @@ mod tests {
 	use crate::ln::PaymentHash;
 	use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
 	use crate::ln::channel::InitFeatures;
-	use crate::ln::channel::{Channel, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, commit_tx_fee_msat};
+	use crate::ln::channel::{Channel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, commit_tx_fee_msat};
 	use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
 	use crate::ln::features::ChannelTypeFeatures;
 	use crate::ln::msgs::{ChannelUpdate, DecodeError, UnsignedChannelUpdate, MAX_VALUE_MSAT};
@@ -8996,4 +9017,148 @@ mod tests {
 		);
 		assert!(res.is_err());
 	}
+
+	#[test]
+	fn test_waiting_for_batch() {
+		let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
+		let logger = test_utils::TestLogger::new();
+		let secp_ctx = Secp256k1::new();
+		let seed = [42; 32];
+		let network = Network::Testnet;
+		let best_block = BestBlock::from_network(network);
+		let chain_hash = genesis_block(network).header.block_hash();
+		let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+
+		let mut config = UserConfig::default();
+		// Set trust_own_funding_0conf while ensuring we don't send channel_ready for a
+		// channel in a batch before all channels are ready.
+		config.channel_handshake_limits.trust_own_funding_0conf = true;
+
+		// Create a channel from node a to node b that will be part of batch funding.
+		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+		let mut node_a_chan = OutboundV1Channel::<EnforcingSigner>::new(
+			&feeest,
+			&&keys_provider,
+			&&keys_provider,
+			node_b_node_id,
+			&channelmanager::provided_init_features(&config),
+			10000000,
+			100000,
+			42,
+			&config,
+			0,
+			42,
+		).unwrap();
+
+		let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
+		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
+		let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(
+			&feeest,
+			&&keys_provider,
+			&&keys_provider,
+			node_b_node_id,
+			&channelmanager::provided_channel_type_features(&config),
+			&channelmanager::provided_init_features(&config),
+			&open_channel_msg,
+			7,
+			&config,
+			0,
+			&&logger,
+			42,
+		).unwrap();
+
+		// Allow node b to send a 0conf channel_ready.
+		node_b_chan.set_0conf();
+
+		let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
+		node_a_chan.accept_channel(
+			&accept_channel_msg,
+			&config.channel_handshake_limits,
+			&channelmanager::provided_init_features(&config),
+		).unwrap();
+
+		// Fund the channel with a batch funding transaction.
+		let output_script = node_a_chan.context.get_funding_redeemscript();
+		let tx = Transaction {
+			version: 1,
+			lock_time: PackedLockTime::ZERO,
+			input: Vec::new(),
+			output: vec![
+				TxOut {
+					value: 10000000, script_pubkey: output_script.clone(),
+				},
+				TxOut {
+					value: 10000000, script_pubkey: Builder::new().into_script(),
+				},
+			]};
+		let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
+		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(
+			tx.clone(),
+			funding_outpoint,
+			&&logger,
+		).map_err(|_| ()).unwrap();
+		let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(
+			&funding_created_msg,
+			best_block,
+			&&keys_provider,
+			&&logger,
+		).map_err(|_| ()).unwrap();
+		let node_b_updates = node_b_chan.monitor_updating_restored(
+			&&logger,
+			&&keys_provider,
+			chain_hash,
+			&config,
+			0,
+		);
+
+		// Receive funding_signed, but the channel will be configured to hold sending channel_ready and
+		// broadcasting the funding transaction until the batch is ready.
+		let _ = node_a_chan.funding_signed(
+			&funding_signed_msg,
+			best_block,
+			&&keys_provider,
+			true,
+			&&logger,
+		).unwrap();
+		let node_a_updates = node_a_chan.monitor_updating_restored(
+			&&logger,
+			&&keys_provider,
+			chain_hash,
+			&config,
+			0,
+		);
+		// Our channel_ready shouldn't be sent yet, even with trust_own_funding_0conf set,
+		// as the funding transaction depends on all channels in the batch becoming ready.
+		assert!(node_a_updates.channel_ready.is_none());
+		assert!(node_a_updates.funding_broadcastable.is_none());
+		assert_eq!(
+			node_a_chan.context.channel_state,
+			ChannelState::FundingSent as u32 |
+			ChannelState::WaitingForBatch as u32,
+		);
+
+		// It is possible to receive a 0conf channel_ready from the remote node.
+		node_a_chan.channel_ready(
+			&node_b_updates.channel_ready.unwrap(),
+			&&keys_provider,
+			chain_hash,
+			&config,
+			&best_block,
+			&&logger,
+		).unwrap();
+		assert_eq!(
+			node_a_chan.context.channel_state,
+			ChannelState::FundingSent as u32 |
+			ChannelState::WaitingForBatch as u32 |
+			ChannelState::TheirChannelReady as u32,
+		);
+
+		// Clear the ChannelState::WaitingForBatch only when called by ChannelManager.
+		node_a_chan.set_batch_ready();
+		assert_eq!(
+			node_a_chan.context.channel_state,
+			ChannelState::FundingSent as u32 |
+			ChannelState::TheirChannelReady as u32,
+		);
+	}
 }