f - Ensure consistency guarantees with by_id map

Author: ViktorTigerstrom

lightning/src/ln/channelmanager.rs

@@ -748,6 +748,18 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	/// Outbound SCID aliases are added here once the channel is available for normal use, with
 	/// SCIDs being added once the funding transaction is confirmed at the channel's required
 	/// confirmation depth.
+	///
+	/// Consistancy guarantees with the `channel_state::by_id` map, guarantees that any
+	/// `channel_id`s stored, are guaranteed to be existing in the `channel_state::by_id` map.
+	/// Therefore when adding or removing a channel, the channel must be added to the
+	/// `channel_state::by_id` map before to being added to `short_to_chan_info`, as well as
+	/// be removed from the `short_to_chan_info` prior to being removed from the
+	/// `channel_state::by_id` map. Alternatively, if the add/remove order can't be upheld, the
+	/// `short_to_chan_info` lock must be held while the channel is added to both
+	/// `channel_state::by_id` and `short_to_chan_info`, or removed from the respective maps.
+	///
+	/// If also holding `channel_state` lock, must lock `channel_state` prior to
+	/// `short_to_chan_info`.
 	#[cfg(any(test, feature = "_test_utils"))]
 	pub(super) short_to_chan_info: FairRwLock<HashMap<u64, (PublicKey, [u8; 32])>>,
 	#[cfg(not(any(test, feature = "_test_utils")))]
@@ -1335,9 +1347,9 @@ macro_rules! try_chan_entry {
 macro_rules! remove_channel {
 	($self: expr, $channel_state: expr, $entry: expr) => {
 		{
-			let channel = $entry.remove_entry().1;
-			update_maps_on_chan_removal!($self, channel);
-			channel
+			update_maps_on_chan_removal!($self, $entry.get());
+			let removed_channel = $entry.remove_entry().1;
+			removed_channel
 		}
 	}
 }
@@ -4639,17 +4651,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 		let mut channel_state_lock = self.channel_state.lock().unwrap();
 		let channel_state = &mut *channel_state_lock;
-		match channel_state.by_id.entry(funding_msg.channel_id) {
+		let channel_id = funding_msg.channel_id;
+		match channel_state.by_id.entry(channel_id) {
 			hash_map::Entry::Occupied(_) => {
-				return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), funding_msg.channel_id))
+				return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), channel_id))
 			},
 			hash_map::Entry::Vacant(e) => {
 				let mut id_to_peer = self.id_to_peer.lock().unwrap();
 				match id_to_peer.entry(chan.channel_id()) {
 					hash_map::Entry::Occupied(_) => {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close(
 							"The funding_created message had the same funding_txid as an existing channel - funding is not possible".to_owned(),
-							funding_msg.channel_id))
+							channel_id))
 					},
 					hash_map::Entry::Vacant(i_e) => {
 						i_e.insert(chan.get_counterparty_node_id());
@@ -4659,12 +4672,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					node_id: counterparty_node_id.clone(),
 					msg: funding_msg,
 				});
-				if let Some(msg) = channel_ready {
-					send_channel_ready!(self, channel_state.pending_msg_events, chan, msg);
-				}
 				e.insert(chan);
 			}
 		}
+
+		if let Some(msg) = channel_ready {
+			// Call send_channel_ready after the channel has been added with the real chan_id to
+			// the by_id map, to ensure that short_to_chan_info is populated after by_id.
+			if let Some(chan) = channel_state.by_id.get(&channel_id){
+				send_channel_ready!(self, channel_state.pending_msg_events, chan, msg);
+			}
+		}
 		Ok(())
 	}