Update with review from @wpualino.

- Defer creating outbound_scid_alias for a manually accepted channel until the channel is actually accepted. The obviates the need to haul it around in the InboundChannelRequest struct and to clean it up when the channel is rejected.
- Move the InboundChannelRequest struct to channelmanager.rs since it's really just an internal thing to be used there.

Author: waterson

lightning/src/ln/channel.rs

@@ -6025,18 +6025,6 @@ pub(super) struct InboundV1Channel<Signer: ChannelSigner> {
 	pub unfunded_context: UnfundedChannelContext,
 }
 
-/// A not-yet-accepted inbound (from counterparty) channel. Once
-/// accepted, the parameters will be used to construct a channel.
-pub(super) struct InboundChannelRequest {
-	/// The counterparty node id that initiated the channel open.
-	pub counterparty_node_id: PublicKey,
-	/// The original OpenChannel message.
-	pub open_channel_msg: msgs::OpenChannel,
-	/// The outbound SCID alias assigned to the channel when we received
-	/// the OpenChannel request.
-	pub outbound_scid_alias: u64,
-}
-
 impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
 	/// Creates a new channel from a remote sides' request for one.
 	/// Assumes chain_hash has already been checked and corresponds with what we expect!

lightning/src/ln/channelmanager.rs

@@ -40,7 +40,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa
 // Since this struct is returned in `list_channels` methods, expose it here in case users want to
 // construct one themselves.
 use crate::ln::{inbound_payment, PaymentHash, PaymentPreimage, PaymentSecret};
-use crate::ln::channel::{Channel, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundChannelRequest, InboundV1Channel};
+use crate::ln::channel::{Channel, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel};
 use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 #[cfg(any(feature = "_test_utils", test))]
 use crate::ln::features::Bolt11InvoiceFeatures;
@@ -704,10 +704,20 @@ impl <Signer: ChannelSigner> PeerState<Signer> {
 	fn has_channel(&self, channel_id: &[u8; 32]) -> bool {
 		self.channel_by_id.contains_key(channel_id) ||
 			self.outbound_v1_channel_by_id.contains_key(channel_id) ||
-			self.inbound_v1_channel_by_id.contains_key(channel_id)
+			self.inbound_v1_channel_by_id.contains_key(channel_id) ||
+			self.inbound_channel_request_by_id.contains_key(channel_id)
 	}
 }
 
+/// A not-yet-accepted inbound (from counterparty) channel. Once
+/// accepted, the parameters will be used to construct a channel.
+pub(super) struct InboundChannelRequest {
+	/// The counterparty node id that initiated the channel open.
+	pub counterparty_node_id: PublicKey,
+	/// The original OpenChannel message.
+	pub open_channel_msg: msgs::OpenChannel,
+}
+
 /// Stores a PaymentSecret and any other data we may need to validate an inbound payment is
 /// actually ours and not some duplicate HTLC sent to us by a node along the route.
 ///
@@ -2572,9 +2582,6 @@ where
 				// N.B. that we don't send any channel close event here: we
 				// don't have a user_channel_id, and we never sent any opening
 				// events anyway.
-				self.id_to_peer.lock().unwrap().remove(channel_id);
-				self.outbound_scid_aliases.lock().unwrap().remove(&chan.outbound_scid_alias);
-				self.short_to_chan_info.write().unwrap().remove(&chan.outbound_scid_alias);
 				(None, chan.counterparty_node_id)
 			} else {
 				return Err(APIError::ChannelUnavailable{ err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*channel_id), peer_node_id) });
@@ -5205,15 +5212,17 @@ where
 		let peer_state = &mut *peer_state_lock;
 		let is_only_peer_channel = peer_state.total_channel_count() == 1;
 
-		// Find (and remove) the channel in the unaccepted table. If it's
-		// not there, something weird is happening and return an error.
+		// Find (and remove) the channel in the unaccepted table. If it's not there, something weird is
+		// happening and return an error. N.B. that we create channel with an outbound SCID of zero so
+		// that we can delay allocating the SCID until after we're sure that the checks below will
+		// succeed.
 		let mut channel = match peer_state.inbound_channel_request_by_id.remove(temporary_channel_id) {
 			Some(unaccepted_channel) => {
 				let best_block_height = self.best_block.read().unwrap().height();
 				InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
 					counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features,
           &unaccepted_channel.open_channel_msg, user_channel_id, &self.default_configuration, best_block_height,
-					&self.logger, unaccepted_channel.outbound_scid_alias).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
+					&self.logger, /*outbound_scid_alias=*/0).map_err(|e| APIError::ChannelUnavailable { err: e.to_string() })
 			}
 			_ => Err(APIError::APIMisuseError { err: "No such channel awaiting to be accepted.".to_owned() })
 		}?;
@@ -5245,6 +5254,10 @@ where
 			}
 		}
 
+		// Now that we know we have a channel, assign an outbound SCID alias.
+		let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
+		channel.context.set_outbound_scid_alias(outbound_scid_alias);
+		
 		peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
 			node_id: channel.context.get_counterparty_node_id(),
 			msg: channel.accept_inbound_channel(user_channel_id),
@@ -5310,8 +5323,6 @@ where
 
 		let mut random_bytes = [0u8; 16];
 		random_bytes.copy_from_slice(&self.entropy_source.get_secure_random_bytes()[..16]);
-		let user_channel_id = u128::from_be_bytes(random_bytes);
-		let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
 
 		// Get the number of peers with channels, but without funded ones. We don't care too much
 		// about peers that never open a channel, so we filter by peers that have at least one
@@ -5350,7 +5361,6 @@ where
 		let channel_id = msg.temporary_channel_id;
 		let channel_exists = peer_state.has_channel(&channel_id);
 		if channel_exists {
-			self.outbound_scid_aliases.lock().unwrap().remove(&outbound_scid_alias);
 			return Err(MsgHandleErrInternal::send_err_msg_no_close("temporary_channel_id collision for the same peer!".to_owned(), msg.temporary_channel_id.clone()));
 		}
 
@@ -5367,12 +5377,13 @@ where
 			peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest {
 				counterparty_node_id: *counterparty_node_id,
 				open_channel_msg: msg.clone(),
-				outbound_scid_alias,
 			});
 			return Ok(());
 		}
 
 		// Otherwise create the channel right now.
+		let user_channel_id = u128::from_be_bytes(random_bytes);
+		let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
 		let mut channel = match InboundV1Channel::new(&self.fee_estimator, &self.entropy_source, &self.signer_provider,
 			counterparty_node_id.clone(), &self.channel_type_features(), &peer_state.latest_features, msg, user_channel_id,
 			&self.default_configuration, best_block_height, &self.logger, outbound_scid_alias)