Clear Existing HRMP Channel Request When Force Opening (#7389)

* clear existing hrmp channel request when force opening

* return unused weight

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>

* fix

* update weight signature to u32

---------

Co-authored-by: Bastian Köcher <git@kchr.de>

polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs

@@ -272,7 +272,7 @@ impl<T: frame_system::Config> runtime_parachains::hrmp::WeightInfo for WeightInf
 	/// Proof Skipped: Hrmp HrmpIngressChannelsIndex (max_values: None, max_size: None, mode: Measured)
 	/// Storage: Hrmp HrmpAcceptedChannelRequestCount (r:1 w:1)
 	/// Proof Skipped: Hrmp HrmpAcceptedChannelRequestCount (max_values: None, max_size: None, mode: Measured)
-	fn force_open_hrmp_channel() -> Weight {
+	fn force_open_hrmp_channel(_c: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `350`
 		//  Estimated: `6290`

polkadot/runtime/parachains/src/hrmp.rs

@@ -59,7 +59,7 @@ pub trait WeightInfo {
 	fn force_process_hrmp_close(c: u32) -> Weight;
 	fn hrmp_cancel_open_request(c: u32) -> Weight;
 	fn clean_open_channel_requests(c: u32) -> Weight;
-	fn force_open_hrmp_channel() -> Weight;
+	fn force_open_hrmp_channel(c: u32) -> Weight;
 }
 
 /// A weight info that is only suitable for testing.
@@ -90,7 +90,7 @@ impl WeightInfo for TestWeightInfo {
 	fn clean_open_channel_requests(_: u32) -> Weight {
 		Weight::MAX
 	}
-	fn force_open_hrmp_channel() -> Weight {
+	fn force_open_hrmp_channel(_: u32) -> Weight {
 		Weight::MAX
 	}
 }
@@ -591,17 +591,32 @@ 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())]
+		#[pallet::weight(<T as Config>::WeightInfo::force_open_hrmp_channel(1))]
 		pub fn force_open_hrmp_channel(
 			origin: OriginFor<T>,
 			sender: ParaId,
 			recipient: ParaId,
 			max_capacity: u32,
 			max_message_size: u32,
-		) -> DispatchResult {
+		) -> DispatchResultWithPostInfo {
 			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 };
+			let cancel_request: u32 =
+				if let Some(_open_channel) = HrmpOpenChannelRequests::<T>::get(&channel_id) {
+					Self::cancel_open_request(sender, channel_id)?;
+					1
+				} else {
+					0
+				};
+
+			// 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(
@@ -610,7 +625,8 @@ pub mod pallet {
 				max_capacity,
 				max_message_size,
 			));
-			Ok(())
+
+			Ok(Some(<T as Config>::WeightInfo::force_open_hrmp_channel(cancel_request)).into())
 		}
 	}
 }

polkadot/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,34 @@ 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
+		// Weight parameter only accepts `u32`, `0` and `1` used to represent `false` and `true`,
+		// respectively.
+		let c = [0, 1];
 		let channel_id = HrmpChannelId { sender: sender_id, recipient: recipient_id };
-		assert!(HrmpOpenChannelRequests::<T>::get(&channel_id).is_none());
+		for channels_to_close in c {
+			if channels_to_close == 1 {
+				// 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.clone().into(),
+					recipient_id,
+					capacity,
+					message_size
+				));
+				assert!(HrmpOpenChannelRequests::<T>::get(&channel_id).is_some());
+			} else {
+				if HrmpOpenChannelRequests::<T>::get(&channel_id).is_some() {
+					assert_ok!(Hrmp::<T>::hrmp_cancel_open_request(
+						sender_origin.clone().into(),
+						channel_id.clone(),
+						MAX_UNIQUE_CHANNELS,
+					));
+				}
+				assert!(HrmpOpenChannelRequests::<T>::get(&channel_id).is_none());
+			}
+		}
+
+		// 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 {

polkadot/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();

polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs

@@ -282,7 +282,7 @@ impl<T: frame_system::Config> runtime_parachains::hrmp::WeightInfo for WeightInf
 	/// Proof Skipped: Hrmp HrmpIngressChannelsIndex (max_values: None, max_size: None, mode: Measured)
 	/// Storage: Hrmp HrmpAcceptedChannelRequestCount (r:1 w:1)
 	/// Proof Skipped: Hrmp HrmpAcceptedChannelRequestCount (max_values: None, max_size: None, mode: Measured)
-	fn force_open_hrmp_channel() -> Weight {
+	fn force_open_hrmp_channel(_c: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `666`
 		//  Estimated: `6606`

polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs

@@ -279,7 +279,7 @@ impl<T: frame_system::Config> runtime_parachains::hrmp::WeightInfo for WeightInf
 	/// Proof Skipped: Hrmp HrmpIngressChannelsIndex (max_values: None, max_size: None, mode: Measured)
 	/// Storage: Hrmp HrmpAcceptedChannelRequestCount (r:1 w:1)
 	/// Proof Skipped: Hrmp HrmpAcceptedChannelRequestCount (max_values: None, max_size: None, mode: Measured)
-	fn force_open_hrmp_channel() -> Weight {
+	fn force_open_hrmp_channel(_c: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `704`
 		//  Estimated: `6644`

polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs

@@ -272,7 +272,7 @@ impl<T: frame_system::Config> runtime_parachains::hrmp::WeightInfo for WeightInf
 	/// Proof Skipped: Hrmp HrmpIngressChannelsIndex (max_values: None, max_size: None, mode: Measured)
 	/// Storage: Hrmp HrmpAcceptedChannelRequestCount (r:1 w:1)
 	/// Proof Skipped: Hrmp HrmpAcceptedChannelRequestCount (max_values: None, max_size: None, mode: Measured)
-	fn force_open_hrmp_channel() -> Weight {
+	fn force_open_hrmp_channel(_c: u32, ) -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `307`
 		//  Estimated: `6247`