f avoid excess map lookups & lockorder assertions

Author: dunxen

lightning/src/ln/channelmanager.rs

@@ -698,18 +698,6 @@ impl <Signer: ChannelSigner> PeerState<Signer> {
 			self.outbound_v1_channel_by_id.contains_key(channel_id) ||
 			self.inbound_v1_channel_by_id.contains_key(channel_id)
 	}
-
-	/// Returns a bool indicating whether the given `channel_id` matches a channel we have with this
-	/// peer that is in one of our pending (unfunded) channel maps.
-	///
-	/// NOTE: Although V1 established channels will always have a `temporary_channel_id` if they're
-	///       in `(outbound/inbound)_v1_channel_by_id`, we use the more general `channel_id` as V2
-	///       established channels will have a fixed `channel_id` already after the `accept_channel2`
-	///       message is sent/received.
-	fn has_pending_channel(&self, channel_id: &[u8; 32]) -> bool {
-		self.outbound_v1_channel_by_id.contains_key(channel_id) ||
-			self.inbound_v1_channel_by_id.contains_key(channel_id)
-	}
 }
 
 /// Stores a PaymentSecret and any other data we may need to validate an inbound payment is
@@ -2392,56 +2380,58 @@ where
 
 		let mut failed_htlcs: Vec<(HTLCSource, PaymentHash)>;
 		let result: Result<(), _> = loop {
-			let per_peer_state = self.per_peer_state.read().unwrap();
+			{
+				let per_peer_state = self.per_peer_state.read().unwrap();
 
-			let peer_state_mutex = per_peer_state.get(counterparty_node_id)
-				.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) })?;
+				let peer_state_mutex = per_peer_state.get(counterparty_node_id)
+					.ok_or_else(|| APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {}", counterparty_node_id) })?;
 
-			let mut peer_state_lock = peer_state_mutex.lock().unwrap();
-			let peer_state = &mut *peer_state_lock;
+				let mut peer_state_lock = peer_state_mutex.lock().unwrap();
+				let peer_state = &mut *peer_state_lock;
 
-			if peer_state.has_pending_channel(&channel_id) {
-				// If the channel was still in an unfunded channel map, then we force-close the channel, ignoring
-				// any channel-not-found errors.
-				let _ = self.force_close_channel_with_peer(&channel_id, counterparty_node_id, None, false);
-				return Ok(());
-			}
+				match peer_state.channel_by_id.entry(channel_id.clone()) {
+					hash_map::Entry::Occupied(mut chan_entry) => {
+						let funding_txo_opt = chan_entry.get().context.get_funding_txo();
+						let their_features = &peer_state.latest_features;
+						let (shutdown_msg, mut monitor_update_opt, htlcs) = chan_entry.get_mut()
+							.get_shutdown(&self.signer_provider, their_features, target_feerate_sats_per_1000_weight, override_shutdown_script)?;
+						failed_htlcs = htlcs;
 
-			match peer_state.channel_by_id.entry(channel_id.clone()) {
-				hash_map::Entry::Occupied(mut chan_entry) => {
-					let funding_txo_opt = chan_entry.get().context.get_funding_txo();
-					let their_features = &peer_state.latest_features;
-					let (shutdown_msg, mut monitor_update_opt, htlcs) = chan_entry.get_mut()
-						.get_shutdown(&self.signer_provider, their_features, target_feerate_sats_per_1000_weight, override_shutdown_script)?;
-					failed_htlcs = htlcs;
-
-					// We can send the `shutdown` message before updating the `ChannelMonitor`
-					// here as we don't need the monitor update to complete until we send a
-					// `shutdown_signed`, which we'll delay if we're pending a monitor update.
-					peer_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
-						node_id: *counterparty_node_id,
-						msg: shutdown_msg,
-					});
+						// We can send the `shutdown` message before updating the `ChannelMonitor`
+						// here as we don't need the monitor update to complete until we send a
+						// `shutdown_signed`, which we'll delay if we're pending a monitor update.
+						peer_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
+							node_id: *counterparty_node_id,
+							msg: shutdown_msg,
+						});
 
-					// Update the monitor with the shutdown script if necessary.
-					if let Some(monitor_update) = monitor_update_opt.take() {
-						break handle_new_monitor_update!(self, funding_txo_opt.unwrap(), monitor_update,
-							peer_state_lock, peer_state, per_peer_state, chan_entry).map(|_| ());
-					}
+						// Update the monitor with the shutdown script if necessary.
+						if let Some(monitor_update) = monitor_update_opt.take() {
+							break handle_new_monitor_update!(self, funding_txo_opt.unwrap(), monitor_update,
+								peer_state_lock, peer_state, per_peer_state, chan_entry).map(|_| ());
+						}
 
-					if chan_entry.get().is_shutdown() {
-						let channel = remove_channel!(self, chan_entry);
-						if let Ok(channel_update) = self.get_channel_update_for_broadcast(&channel) {
-							peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-								msg: channel_update
-							});
+						if chan_entry.get().is_shutdown() {
+							let channel = remove_channel!(self, chan_entry);
+							if let Ok(channel_update) = self.get_channel_update_for_broadcast(&channel) {
+								peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+									msg: channel_update
+								});
+							}
+							self.issue_channel_close_events(&channel.context, ClosureReason::HolderForceClosed);
 						}
-						self.issue_channel_close_events(&channel.context, ClosureReason::HolderForceClosed);
-					}
-					break Ok(());
-				},
-				hash_map::Entry::Vacant(_) => return Err(APIError::ChannelUnavailable{err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*channel_id), counterparty_node_id) })
+						break Ok(());
+					},
+					hash_map::Entry::Vacant(_) => (),
+				}
 			}
+			// If we reach this point, it means that the channel_id either refers to an unfunded channel or
+			// it does not exists for this peer. Either way, we can attempt to force-close it.
+			//
+			// An appropriate error will be returned forn non-existence of the channel if that's the case.
+			return self.force_close_channel_with_peer(&channel_id, counterparty_node_id, None, false).map(|_| ())
+			// TODO(dunxen): This is still not ideal as we're doing some extra lookups.
+			// Fix this with https://github.com/lightningdevkit/rust-lightning/issues/2422
 		};
 
 		for htlc_source in failed_htlcs.drain(..) {