Review comments

Author: wvanlint

lightning/src/ln/channel.rs

@@ -1194,7 +1194,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 
 	// Checks whether we should emit a `ChannelPending` event.
 	pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool {
-		self.is_funding_initiated() && !self.channel_pending_event_emitted
+		self.is_funding_broadcast() && !self.channel_pending_event_emitted
 	}
 
 	// Returns whether we already emitted a `ChannelPending` event.
@@ -1255,7 +1255,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 
 	/// Returns true if funding_signed was sent/received and the
 	/// funding transaction has been broadcast if necessary.
-	pub fn is_funding_initiated(&self) -> bool {
+	pub fn is_funding_broadcast(&self) -> bool {
 		self.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 &&
 		self.channel_state & ChannelState::WaitingForBatch as u32 == 0
 	}
@@ -2676,8 +2676,12 @@ impl<SP: Deref> Channel<SP> where
 
 		let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
 
-		// If the WaitingForBatch flag is set, we can receive their channel_ready, but our
-		// channel_ready shouldn't have been sent and we shouldn't move to ChannelReady.
+		// Our channel_ready shouldn't have been sent if we are waiting for other channels in the
+		// batch, but we can receive channel_ready messages.
+		debug_assert!(
+			non_shutdown_state & ChannelState::OurChannelReady as u32 == 0 ||
+			non_shutdown_state & ChannelState::WaitingForBatch as u32 == 0
+		);
 		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) {
@@ -9108,7 +9112,7 @@ mod tests {
 
 		// 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(
+		let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(
 			&feeest,
 			&&keys_provider,
 			&&keys_provider,
@@ -9124,7 +9128,7 @@ mod tests {
 
 		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(
+		let mut node_b_chan = InboundV1Channel::<&TestKeysInterface>::new(
 			&feeest,
 			&&keys_provider,
 			&&keys_provider,

lightning/src/ln/channelmanager.rs

@@ -1198,15 +1198,15 @@ where
 	/// `PersistenceNotifierGuard::notify_on_drop(..)` and pass the lock to it, to ensure the
 	/// Notifier the lock contains sends out a notification when the lock is released.
 	total_consistency_lock: RwLock<()>,
-	/// Tracks the progress of channels going through batch v1 channel establishment by whether
-	/// funding_signed was received and the monitor has been persisted.
+	/// Tracks the progress of channels going through batch funding by whether funding_signed was
+	/// received and the monitor has been persisted.
 	///
 	/// This information does not need to be persisted as funding nodes can forget
 	/// unfunded channels upon disconnection.
-	v1_funding_batch_states: FairRwLock<HashMap<Txid, Mutex<HashMap<([u8;32], PublicKey), bool>>>>,
+	funding_batch_states: FairRwLock<HashMap<Txid, Mutex<HashMap<([u8;32], PublicKey), bool>>>>,
 	/// Remaining channels in a funding batch need to be closed when one channel closes.
 	/// These batches are maintained here to be periodically processed to simplify locking behavior.
-	v1_funding_batches_to_be_closed: Mutex<Vec<Txid>>,
+	funding_batches_to_be_closed: Mutex<Vec<Txid>>,
 
 	background_events_processed_since_startup: AtomicBool,
 
@@ -1818,10 +1818,10 @@ macro_rules! update_maps_on_chan_removal {
 		short_to_chan_info.remove(&$channel_context.outbound_scid_alias());
 		// If the channel was part of a batch funding transaction, all channels in that
 		// batch are affected.
-		let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
+		let funding_batch_states = $self.funding_batch_states.read().unwrap();
 		$channel_context.unbroadcasted_funding_txid().map(|txid| {
-			if v1_funding_batch_states.contains_key(&txid) {
-				$self.v1_funding_batches_to_be_closed.lock().unwrap().push(txid);
+			if funding_batch_states.contains_key(&txid) {
+				$self.funding_batches_to_be_closed.lock().unwrap().push(txid);
 			}
 		})
 	}}
@@ -1972,9 +1972,9 @@ macro_rules! handle_monitor_update_completion {
 		// should be updated as we have received funding_signed and persisted the monitor.
 		let mut completed_batch = None;
 		{
-			let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
+			let funding_batch_states = $self.funding_batch_states.read().unwrap();
 			let batch_state_key_value = $chan.context.unbroadcasted_funding_txid()
-					.and_then(|txid| v1_funding_batch_states.get_key_value(&txid));
+					.and_then(|txid| funding_batch_states.get_key_value(&txid));
 			if let Some((txid, batch_state)) = batch_state_key_value {
 				let mut batch_state = batch_state.lock().unwrap();
 				batch_state.insert(
@@ -2024,7 +2024,7 @@ macro_rules! handle_monitor_update_completion {
 		// When all channels in a batched funding transaction have become ready, it is not necessary
 		// to track the progress of the batch anymore and the state of the channels can be updated.
 		if let Some(txid) = completed_batch {
-			let other_channel_ids = $self.v1_funding_batch_states.write().unwrap()
+			let other_channel_ids = $self.funding_batch_states.write().unwrap()
 					.remove(&txid)
 					.map(|batch_state| batch_state.into_inner().unwrap().into_iter().map(|(k, _)| k))
 					.into_iter().flatten()
@@ -2259,8 +2259,8 @@ where
 			total_consistency_lock: RwLock::new(()),
 			background_events_processed_since_startup: AtomicBool::new(false),
 			persistence_notifier: Notifier::new(),
-			v1_funding_batch_states: FairRwLock::new(HashMap::new()),
-			v1_funding_batches_to_be_closed: Mutex::new(Vec::new()),
+			funding_batch_states: FairRwLock::new(HashMap::new()),
+			funding_batches_to_be_closed: Mutex::new(Vec::new()),
 
 			entropy_source,
 			node_signer,
@@ -3664,7 +3664,7 @@ where
 		}
 
 		let is_batch_funding = temporary_channels.len() > 1;
-		let v1_funding_batch_state = RefCell::new(HashMap::new());
+		let funding_batch_state = RefCell::new(HashMap::new());
 		for (temporary_channel_id, counterparty_node_id) in temporary_channels {
 			result = result.and_then(|_| self.funding_transaction_generated_intern(
 				temporary_channel_id,
@@ -3689,7 +3689,7 @@ where
 						});
 					}
 					let outpoint = OutPoint { txid: tx.txid(), index: output_index.unwrap() };
-					v1_funding_batch_state.borrow_mut().insert((outpoint.to_channel_id(), (*counterparty_node_id).clone()), false);
+					funding_batch_state.borrow_mut().insert((outpoint.to_channel_id(), (*counterparty_node_id).clone()), false);
 					Ok(outpoint)
 				})
 			);
@@ -3707,7 +3707,7 @@ where
 							self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
 						});
 			}
-			for (channel_id, counterparty_node_id) in v1_funding_batch_state.borrow().keys() {
+			for (channel_id, counterparty_node_id) in funding_batch_state.borrow().keys() {
 				per_peer_state.get(counterparty_node_id)
 						.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
 						.and_then(|mut peer_state| peer_state.channel_by_id.remove(channel_id))
@@ -3718,9 +3718,9 @@ where
 			}
 		} else if is_batch_funding {
 			// Initialize the state of the batch.
-			self.v1_funding_batch_states.write().unwrap().insert(
+			self.funding_batch_states.write().unwrap().insert(
 				funding_transaction.txid(),
-				Mutex::new(v1_funding_batch_state.into_inner()),
+				Mutex::new(funding_batch_state.into_inner()),
 			);
 		}
 		result
@@ -4784,9 +4784,9 @@ where
 			// Close remaining channels in funding batches when one channel closes.
 			let mut affected_channels = Vec::new();
 			{
-				let mut v1_funding_batch_states = self.v1_funding_batch_states.write().unwrap();
-				for txid in self.v1_funding_batches_to_be_closed.lock().unwrap().drain(..) {
-					affected_channels.extend(v1_funding_batch_states
+				let mut funding_batch_states = self.funding_batch_states.write().unwrap();
+				for txid in self.funding_batches_to_be_closed.lock().unwrap().drain(..) {
+					affected_channels.extend(funding_batch_states
 							.remove(&txid)
 							.map(|state| state.into_inner().unwrap().into_iter().map(|(k, _)| k))
 							.into_iter().flatten()
@@ -5850,7 +5850,7 @@ where
 		match peer_state.channel_by_id.entry(msg.channel_id) {
 			hash_map::Entry::Occupied(mut chan) => {
 				let is_batch_funding = chan.get().context.unbroadcasted_funding_txid()
-						.map(|txid| self.v1_funding_batch_states.read().unwrap().contains_key(&txid))
+						.map(|txid| self.funding_batch_states.read().unwrap().contains_key(&txid))
 						.unwrap_or(false);
 				let monitor = try_chan_entry!(self,
 					chan.get_mut().funding_signed(&msg, best_block, &self.signer_provider, is_batch_funding, &self.logger), chan);
@@ -7552,20 +7552,24 @@ where
 				let peer_state = &mut *peer_state_lock;
 
 				peer_state.channel_by_id.retain(|_, chan| {
-					if !chan.context.is_funding_initiated() {
+					if !chan.context.is_funding_broadcast() {
+						update_maps_on_chan_removal!(self, &chan.context);
+						self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
 						// It is possible to have persisted the monitor upon funding_signed
 						// but not have broadcast the transaction, especially for batch funding.
 						// The monitor should be moved to the correct state.
 						self.finish_force_close_channel(chan.context.force_shutdown(false));
+						false
 					} else {
 						chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger);
+						if chan.is_shutdown() {
+							update_maps_on_chan_removal!(self, &chan.context);
+							self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
+							false
+						} else {
+							true
+						}
 					}
-					if chan.is_shutdown() {
-						update_maps_on_chan_removal!(self, &chan.context);
-						self.issue_channel_close_events(&chan.context, ClosureReason::DisconnectedPeer);
-						return false;
-					}
-					true
 				});
 				peer_state.inbound_v1_channel_by_id.retain(|_, chan| {
 					update_maps_on_chan_removal!(self, &chan.context);
@@ -8370,7 +8374,7 @@ where
 				}
 				number_of_channels += peer_state.channel_by_id.len();
 				for (_, channel) in peer_state.channel_by_id.iter() {
-					if !channel.context.is_funding_initiated() {
+					if !channel.context.is_funding_broadcast() {
 						unfunded_channels += 1;
 					}
 				}
@@ -8382,7 +8386,7 @@ where
 				let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 				let peer_state = &mut *peer_state_lock;
 				for (_, channel) in peer_state.channel_by_id.iter() {
-					if channel.context.is_funding_initiated() {
+					if channel.context.is_funding_broadcast() {
 						channel.write(writer)?;
 					}
 				}
@@ -8829,7 +8833,7 @@ where
 					if let Some(short_channel_id) = channel.context.get_short_channel_id() {
 						short_to_chan_info.insert(short_channel_id, (channel.context.get_counterparty_node_id(), channel.context.channel_id()));
 					}
-					if channel.context.is_funding_initiated() {
+					if channel.context.is_funding_broadcast() {
 						id_to_peer.insert(channel.context.channel_id(), channel.context.get_counterparty_node_id());
 					}
 					match peer_channels.entry(channel.context.get_counterparty_node_id()) {
@@ -9526,8 +9530,8 @@ where
 			total_consistency_lock: RwLock::new(()),
 			background_events_processed_since_startup: AtomicBool::new(false),
 			persistence_notifier: Notifier::new(),
-			v1_funding_batch_states: FairRwLock::new(HashMap::new()),
-			v1_funding_batches_to_be_closed: Mutex::new(Vec::new()),
+			funding_batch_states: FairRwLock::new(HashMap::new()),
+			funding_batches_to_be_closed: Mutex::new(Vec::new()),
 
 			entropy_source: args.entropy_source,
 			node_signer: args.node_signer,

lightning/src/ln/functional_test_utils.rs

@@ -1447,12 +1447,12 @@ pub fn check_closed_event(node: &Node, events_count: usize, expected_reason: Clo
 	let events = node.node.get_and_clear_pending_events();
 	assert_eq!(events.len(), events_count, "{:?}", events);
 	let mut issues_discard_funding = false;
-	for (idx, event) in events.into_iter().enumerate() {
+	for event in events {
 		match event {
-			Event::ChannelClosed { ref reason, counterparty_node_id, 
+			Event::ChannelClosed { ref reason, counterparty_node_id,
 				channel_capacity_sats, .. } => {
 				assert_eq!(*reason, expected_reason);
-				assert_eq!(counterparty_node_id.unwrap(), expected_counterparty_node_ids[idx]);
+				assert!(expected_counterparty_node_ids.iter().any(|id| id == &counterparty_node_id.unwrap()));
 				assert_eq!(channel_capacity_sats.unwrap(), expected_channel_capacity);
 			},
 			Event::DiscardFunding { .. } => {
@@ -1473,7 +1473,7 @@ macro_rules! check_closed_event {
 		check_closed_event!($node, $events, $reason, false, $counterparty_node_ids, $channel_capacity);
 	};
 	($node: expr, $events: expr, $reason: expr, $is_check_discard_funding: expr, $counterparty_node_ids: expr, $channel_capacity: expr) => {
-		$crate::ln::functional_test_utils::check_closed_event(&$node, $events, $reason, 
+		$crate::ln::functional_test_utils::check_closed_event(&$node, $events, $reason,
 			$is_check_discard_funding, &$counterparty_node_ids, $channel_capacity);
 	}
 }