Incorporate feedback from dunxen's review.

- Rename the `UnacceptedInboundV1Channel` struct to `InboundChannelRequest`, as it may be used for both V1 and V2 channels.
- Similarlly, rename the `unaccepted_inbound_v1_channel_by_id` table to `inbound_channel_request_by_id`
- Verify that _no_ channel close event got generated when a request was rejected.

Author: waterson

lightning/src/ln/channel.rs

@@ -6027,7 +6027,7 @@ pub(super) struct InboundV1Channel<Signer: ChannelSigner> {
 /// A not-yet-accepted inbound (from counterparty) channel using V1
 /// channel establishment. Once accepted, the parameters will be used
 /// to construct a channel.
-pub(super) struct UnacceptedInboundV1Channel {
+pub(super) struct InboundChannelRequest {
 	/// The counterparty node id that initiated the channel open.
 	pub counterparty_node_id: PublicKey,
 	/// The original OpenChannel message.

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, UnacceptedInboundV1Channel};
+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;
@@ -638,7 +638,7 @@ pub(super) struct PeerState<Signer: ChannelSigner> {
 	/// `temporary_channel_id` -> `UnacceptedInboundV1Channel`.
 	///
 	/// Holds all unaccepted inbound V1 channels where the peer is the counterparty.
-	pub(super) unaccepted_inbound_v1_channel_by_id: HashMap<[u8; 32], UnacceptedInboundV1Channel>,
+	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
@@ -694,7 +694,7 @@ impl <Signer: ChannelSigner> PeerState<Signer> {
 		self.channel_by_id.len() +
 			self.outbound_v1_channel_by_id.len() +
 			self.inbound_v1_channel_by_id.len() +
-			self.unaccepted_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.
@@ -2564,7 +2564,7 @@ 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.unaccepted_inbound_v1_channel_by_id.remove(channel_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
@@ -5190,7 +5190,7 @@ where
 
 		// 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.unaccepted_inbound_v1_channel_by_id.remove(temporary_channel_id) {
+		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,
@@ -5279,7 +5279,7 @@ where
 				num_unfunded_channels += 1;
 			}
 		}
-		num_unfunded_channels + peer.unaccepted_inbound_v1_channel_by_id.len()
+		num_unfunded_channels + peer.inbound_channel_request_by_id.len()
 	}
 
 	fn internal_open_channel(&self, counterparty_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
@@ -5347,7 +5347,7 @@ where
 				push_msat: msg.push_msat,
 				channel_type: msg.channel_type.clone().unwrap(),
 			}, None));
-			peer_state.unaccepted_inbound_v1_channel_by_id.insert(channel_id, UnacceptedInboundV1Channel {
+			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: outbound_scid_alias,
@@ -7293,7 +7293,7 @@ where
 						channel_by_id: HashMap::new(),
 						outbound_v1_channel_by_id: HashMap::new(),
 						inbound_v1_channel_by_id: HashMap::new(),
-						unaccepted_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(),
@@ -8488,7 +8488,7 @@ where
 				channel_by_id,
 				outbound_v1_channel_by_id: HashMap::new(),
 				inbound_v1_channel_by_id: HashMap::new(),
-				unaccepted_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(),
@@ -10210,8 +10210,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());
 
-		// Since we never created a InboundV1Channel object, we cannot generate close event.
-		//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

@@ -7871,8 +7871,8 @@ fn test_manually_reject_inbound_channel_request() {
 		_ => panic!("Unexpected event"),
 	}
 
-	// If the channel was never opened, we cannot generate a closed 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!