From c206d9b37599c64dc8effaf140504ea1438f1da2 Mon Sep 17 00:00:00 2001
From: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Date: Wed, 21 Jun 2023 16:57:05 +0200
Subject: [PATCH] Clear Existing HRMP Channel Request When Force Opening
(#7389)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* 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>
---
.../src/weights/runtime_parachains_hrmp.rs | 2 +-
polkadot/runtime/parachains/src/hrmp.rs | 28 +++++++++---
.../parachains/src/hrmp/benchmarking.rs | 30 ++++++++++++-
polkadot/runtime/parachains/src/hrmp/tests.rs | 44 +++++++++++++++++++
.../src/weights/runtime_parachains_hrmp.rs | 2 +-
.../src/weights/runtime_parachains_hrmp.rs | 2 +-
.../src/weights/runtime_parachains_hrmp.rs | 2 +-
7 files changed, 98 insertions(+), 12 deletions(-)
diff --git a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs
index 64efe63c956..c13a8413e41 100644
--- a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs
+++ b/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`
diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs
index ee725da37f1..0d62ec16d74 100644
--- a/polkadot/runtime/parachains/src/hrmp.rs
+++ b/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())
}
}
}
diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs
index 4ac524b124d..3fe347a7bcb 100644
--- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs
+++ b/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 {
diff --git a/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs
index 709d56109b7..78fd983e25b 100644
--- a/polkadot/runtime/parachains/src/hrmp/tests.rs
+++ b/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();
diff --git a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs
index e2fdb821d6e..82d8c30baca 100644
--- a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs
+++ b/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`
diff --git a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs
index d3fba805d47..9f1cf65efa6 100644
--- a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs
+++ b/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`
diff --git a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs
index 9e10f35b6c1..e6ff97fa087 100644
--- a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs
+++ b/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`
--
GitLab