Clear Existing HRMP Channel Request When Force Opening

runtime/parachains/src/hrmp.rs

@@ -591,7 +591,7 @@ pub mod pallet {
 		/// Chain's configured limits.
 		///
 		/// Expected use is when one of the `ParaId`s involved in the channel is governed by the
-		/// Relay Chain, e.g. a common good parachain.
+		/// Relay Chain, e.g. a system parachain.
 		#[pallet::call_index(7)]
 		#[pallet::weight(<T as Config>::WeightInfo::force_open_hrmp_channel())]
 		pub fn force_open_hrmp_channel(
@@ -602,6 +602,17 @@ pub mod pallet {
 			max_message_size: u32,
 		) -> DispatchResult {
 			ensure_root(origin)?;
+
+			// Guard against a common footgun where someone makes a channel request to a system
+			// parachain and then makes a proposal to open the channel via governance, which fails
+			// because `init_open_channel` fails if there is an existing request. This check will
+			// clear an existing request such that `init_open_channel` should otherwise succeed.
+			let channel_id = HrmpChannelId { sender, recipient };
+			if HrmpOpenChannelRequests::<T>::get(&channel_id).is_some() {
+				Self::cancel_open_request(sender, channel_id)?;
+			}
+
+			// Now we proceed with normal init/accept.
 			Self::init_open_channel(sender, recipient, max_capacity, max_message_size)?;
 			Self::accept_open_channel(recipient, sender)?;
 			Self::deposit_event(Event::HrmpChannelForceOpened(

runtime/parachains/src/hrmp/benchmarking.rs

@@ -301,6 +301,7 @@ frame_benchmarking::benchmarks! {
 
 	force_open_hrmp_channel {
 		let sender_id: ParaId = 1u32.into();
+		let sender_origin: crate::Origin = 1u32.into();
 		let recipient_id: ParaId = 2u32.into();
 
 		// make sure para is registered, and has enough balance.
@@ -315,9 +316,18 @@ frame_benchmarking::benchmarks! {
 		let capacity = Configuration::<T>::config().hrmp_channel_max_capacity;
 		let message_size = Configuration::<T>::config().hrmp_channel_max_message_size;
 
-		// make sure this channel doesn't exist
+		// this will consume more weight if a channel _request_ already exists, because it will need
+		// to clear the request.
+		assert_ok!(Hrmp::<T>::hrmp_init_open_channel(
+			sender_origin.into(),
+			recipient_id,
+			capacity,
+			message_size
+		));
 		let channel_id = HrmpChannelId { sender: sender_id, recipient: recipient_id };
-		assert!(HrmpOpenChannelRequests::<T>::get(&channel_id).is_none());
+		assert!(HrmpOpenChannelRequests::<T>::get(&channel_id).is_some());
+
+		// but the _channel_ should not exist
 		assert!(HrmpChannels::<T>::get(&channel_id).is_none());
 	}: _(frame_system::Origin::<T>::Root, sender_id, recipient_id, capacity, message_size)
 	verify {

runtime/parachains/src/hrmp/tests.rs

@@ -203,6 +203,50 @@ fn force_open_channel_works() {
 	});
 }
 
+#[test]
+fn force_open_channel_works_with_existing_request() {
+	let para_a = 1.into();
+	let para_a_origin: crate::Origin = 1.into();
+	let para_b = 3.into();
+
+	new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| {
+		// We need both A & B to be registered and live parachains.
+		register_parachain(para_a);
+		register_parachain(para_b);
+
+		// Request a channel from `a` to `b`.
+		run_to_block(3, Some(vec![2, 3]));
+		Hrmp::hrmp_init_open_channel(para_a_origin.into(), para_b, 2, 8).unwrap();
+		Hrmp::assert_storage_consistency_exhaustive();
+		assert!(System::events().iter().any(|record| record.event ==
+			MockEvent::Hrmp(Event::OpenChannelRequested(para_a, para_b, 2, 8))));
+
+		run_to_block(5, Some(vec![4, 5]));
+		// the request exists, but no channel.
+		assert!(HrmpOpenChannelRequests::<Test>::get(&HrmpChannelId {
+			sender: para_a,
+			recipient: para_b
+		})
+		.is_some());
+		assert!(!channel_exists(para_a, para_b));
+		// now force open it.
+		Hrmp::force_open_hrmp_channel(RuntimeOrigin::root(), para_a, para_b, 2, 8).unwrap();
+		Hrmp::assert_storage_consistency_exhaustive();
+		assert!(System::events().iter().any(|record| record.event ==
+			MockEvent::Hrmp(Event::HrmpChannelForceOpened(para_a, para_b, 2, 8))));
+
+		// Advance to a block 6, but without session change. That means that the channel has
+		// not been created yet.
+		run_to_block(6, None);
+		assert!(!channel_exists(para_a, para_b));
+		Hrmp::assert_storage_consistency_exhaustive();
+
+		// Now let the session change happen and thus open the channel.
+		run_to_block(8, Some(vec![8]));
+		assert!(channel_exists(para_a, para_b));
+	});
+}
+
 #[test]
 fn close_channel_works() {
 	let para_a = 5.into();