From a03fd64ad59389e7a9c1b6c97f7e50ce59150fcf Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 8 Sep 2023 17:00:24 +0200 Subject: [PATCH 01/16] initial solution --- .../runtime/common/src/paras_sudo_wrapper.rs | 1 + polkadot/runtime/parachains/src/hrmp.rs | 109 +++++++++++++++--- .../parachains/src/hrmp/benchmarking.rs | 6 +- polkadot/runtime/parachains/src/hrmp/tests.rs | 28 ++--- 4 files changed, 109 insertions(+), 35 deletions(-) diff --git a/polkadot/runtime/common/src/paras_sudo_wrapper.rs b/polkadot/runtime/common/src/paras_sudo_wrapper.rs index 0fc2644b2a0b..5704e04f2492 100644 --- a/polkadot/runtime/common/src/paras_sudo_wrapper.rs +++ b/polkadot/runtime/common/src/paras_sudo_wrapper.rs @@ -166,6 +166,7 @@ pub mod pallet { recipient, max_capacity, max_message_size, + false, // although set by root, this wrapper is used to simulate real networks )?; >::accept_open_channel(recipient, sender)?; Ok(()) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index cdb38ba6a8ea..900fde3c9745 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -21,13 +21,15 @@ use crate::{ use frame_support::{pallet_prelude::*, traits::ReservableCurrency, DefaultNoBound}; use frame_system::pallet_prelude::*; use parity_scale_codec::{Decode, Encode}; -use polkadot_parachain_primitives::primitives::HorizontalMessages; +use polkadot_parachain_primitives::primitives::{HorizontalMessages, IsSystem}; use primitives::{ Balance, Hash, HrmpChannelId, Id as ParaId, InboundHrmpMessage, OutboundHrmpMessage, SessionIndex, }; use scale_info::TypeInfo; -use sp_runtime::traits::{AccountIdConversion, BlakeTwo256, Hash as HashT, UniqueSaturatedInto}; +use sp_runtime::traits::{ + AccountIdConversion, BlakeTwo256, Hash as HashT, UniqueSaturatedInto, Zero, +}; use sp_std::{ collections::{btree_map::BTreeMap, btree_set::BTreeSet}, fmt, mem, @@ -279,6 +281,9 @@ pub mod pallet { /// An HRMP channel was opened via Root origin. /// `[sender, recipient, proposed_max_capacity, proposed_max_message_size]` HrmpChannelForceOpened(ParaId, ParaId, u32, u32), + /// An HRMP channel was opened between two system chains. + /// `[sender, recipient, proposed_max_capacity, proposed_max_message_size]` + HrmpSystemChannelOpened(ParaId, ParaId, u32, u32), } #[pallet::error] @@ -321,6 +326,8 @@ pub mod pallet { OpenHrmpChannelAlreadyConfirmed, /// The provided witness data is wrong. WrongWitness, + /// The channel between these two chains cannot be authorized. + ChannelCreationNotAuthorized, } /// The set of pending HRMP open channel requests. @@ -479,6 +486,7 @@ pub mod pallet { recipient, proposed_max_capacity, proposed_max_message_size, + false, )?; Self::deposit_event(Event::OpenChannelRequested( origin, @@ -600,8 +608,8 @@ pub mod pallet { /// the `max_capacity` and `max_message_size` are still subject to the Relay 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 system parachain. + /// Expected use is when one (and only one) of the `ParaId`s involved in the channel is + /// governed by the system, e.g. a system parachain. /// /// Origin must be the `ChannelManager`. #[pallet::call_index(7)] @@ -628,8 +636,9 @@ pub mod pallet { 0 }; - // Now we proceed with normal init/accept. - Self::init_open_channel(sender, recipient, max_capacity, max_message_size)?; + // Now we proceed with normal init/accept, except that we set `no_deposit` to true such + // that it will not require deposits from either member. + Self::init_open_channel(sender, recipient, max_capacity, max_message_size, true)?; Self::accept_open_channel(recipient, sender)?; Self::deposit_event(Event::HrmpChannelForceOpened( sender, @@ -640,6 +649,50 @@ pub mod pallet { Ok(Some(::WeightInfo::force_open_hrmp_channel(cancel_request)).into()) } + + /// Establish an HRMP channel between two system chains. If the channel does not already + /// exist, the transaction fees will be refunded to the caller. The system does not take + /// deposits for channels between system chains, and automatically sets the message number + /// and size limits to the maximum allowed by the network's configuration. + /// + /// Arguments: + /// + /// - `sender`: A system chain, `ParaId`. + /// - `recipient`: A system chain, `ParaId`. + /// + /// Any signed origin can call this function. + #[pallet::call_index(8)] + #[pallet::weight(1)] // todo + pub fn establish_system_channel( + origin: OriginFor, + sender: ParaId, + recipient: ParaId, + ) -> DispatchResultWithPostInfo { + let _caller = ensure_signed(origin)?; + + // to recognize current chains needs + // https://github.com/paritytech/polkadot-sdk/pull/1406 + ensure!( + sender.is_system() && recipient.is_system(), + Error::::ChannelCreationNotAuthorized + ); + + let config = >::config(); + let max_message_size = config.hrmp_channel_max_message_size; + let max_capacity = config.hrmp_channel_max_capacity; + + Self::init_open_channel(sender, recipient, max_capacity, max_message_size, true)?; + Self::accept_open_channel(recipient, sender)?; + + Self::deposit_event(Event::HrmpSystemChannelOpened( + sender, + recipient, + max_capacity, + max_message_size, + )); + + Ok(Pays::No.into()) + } } } @@ -661,7 +714,7 @@ fn preopen_hrmp_channel( max_capacity: u32, max_message_size: u32, ) -> DispatchResult { - >::init_open_channel(sender, recipient, max_capacity, max_message_size)?; + >::init_open_channel(sender, recipient, max_capacity, max_message_size, false)?; >::accept_open_channel(recipient, sender)?; Ok(()) } @@ -817,6 +870,10 @@ impl Pallet { "can't be `None` due to the invariant that the list contains the same items as the set; qed", ); + let system_channel = request.sender_deposit.is_zero(); + let sender_deposit = request.sender_deposit; + let recipient_deposit = if system_channel { 0 } else { config.hrmp_recipient_deposit }; + if request.confirmed { if >::is_valid_para(channel_id.sender) && >::is_valid_para(channel_id.recipient) @@ -824,8 +881,8 @@ impl Pallet { HrmpChannels::::insert( &channel_id, HrmpChannel { - sender_deposit: request.sender_deposit, - recipient_deposit: config.hrmp_recipient_deposit, + sender_deposit, + recipient_deposit, max_capacity: request.max_capacity, max_total_size: request.max_total_size, max_message_size: request.max_message_size, @@ -1177,11 +1234,16 @@ impl Pallet { /// /// Basically the same as [`hrmp_init_open_channel`](Pallet::hrmp_init_open_channel) but /// intended for calling directly from other pallets rather than dispatched. + /// + /// The parameter `no_deposit` allows the caller to establish the channel without deposits. + /// This should be used only for channels established via governance or channels between system + /// chains. All other channels should use `false`. pub fn init_open_channel( origin: ParaId, recipient: ParaId, proposed_max_capacity: u32, proposed_max_message_size: u32, + no_deposit: bool, ) -> DispatchResult { ensure!(origin != recipient, Error::::OpenHrmpChannelToSelf); ensure!( @@ -1219,10 +1281,14 @@ impl Pallet { Error::::OpenHrmpChannelLimitExceeded, ); - T::Currency::reserve( - &origin.into_account_truncating(), - config.hrmp_sender_deposit.unique_saturated_into(), - )?; + // In practice should only be zero (for governance/system chains) or the default. + let deposit = if no_deposit { 0 } else { config.hrmp_sender_deposit }; + if !deposit.is_zero() { + T::Currency::reserve( + &origin.into_account_truncating(), + deposit.unique_saturated_into(), + )?; + } // mutating storage directly now -- shall not bail henceforth. @@ -1232,7 +1298,7 @@ impl Pallet { HrmpOpenChannelRequest { confirmed: false, _age: 0, - sender_deposit: config.hrmp_sender_deposit, + sender_deposit: deposit, max_capacity: proposed_max_capacity, max_message_size: proposed_max_message_size, max_total_size: config.hrmp_channel_max_total_size, @@ -1254,7 +1320,7 @@ impl Pallet { if let Err(dmp::QueueDownwardMessageError::ExceedsMaxMessageSize) = >::queue_downward_message(&config, recipient, notification_bytes) { - // this should never happen unless the max downward message size is configured to an + // this should never happen unless the max downward message size is configured to a // jokingly small number. log::error!( target: "runtime::hrmp", @@ -1287,10 +1353,15 @@ impl Pallet { Error::::AcceptHrmpChannelLimitExceeded, ); - T::Currency::reserve( - &origin.into_account_truncating(), - config.hrmp_recipient_deposit.unique_saturated_into(), - )?; + // The channel cleanup / para offboarding does not store recipient deposits. If the deposit + // was overridden for the sender, which should only happen for channels established by + // governance or for the system itself, then it also does not take a recipient deposit. + if !channel_req.sender_deposit.is_zero() { + T::Currency::reserve( + &origin.into_account_truncating(), + config.hrmp_recipient_deposit.unique_saturated_into(), + )?; + } // persist the updated open channel request and then increment the number of accepted // channels. diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index 3fe347a7bcba..d4f2e3034e11 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -94,7 +94,8 @@ where sender_origin.clone().into(), recipient, capacity, - message_size + message_size, + false )); if matches!(until, ParachainSetupStep::Requested) { @@ -328,7 +329,8 @@ frame_benchmarking::benchmarks! { sender_origin.clone().into(), recipient_id, capacity, - message_size + message_size, + false )); assert!(HrmpOpenChannelRequests::::get(&channel_id).is_some()); } else { diff --git a/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs index 8cfaf48d10ef..485f615809b6 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/runtime/parachains/src/hrmp/tests.rs @@ -251,7 +251,7 @@ fn close_channel_works() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(6, Some(vec![6])); @@ -286,7 +286,7 @@ fn send_recv_messages() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); // On Block 6: @@ -324,7 +324,7 @@ fn hrmp_mqc_head_fixture() { register_parachain(para_b); run_to_block(2, Some(vec![1, 2])); - Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(3, Some(vec![3])); @@ -366,7 +366,7 @@ fn accept_incoming_request_and_offboard() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); deregister_parachain(para_a); @@ -393,9 +393,9 @@ fn check_sent_messages() { // Open two channels to the same receiver, b: // a -> b, c -> b - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); - Hrmp::init_open_channel(para_c, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_c, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_c).unwrap(); // On Block 6: session change. @@ -453,7 +453,7 @@ fn verify_externally_accessible() { register_parachain(para_a); register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(8, Some(vec![8])); @@ -511,7 +511,7 @@ fn charging_deposits() { run_to_block(5, Some(vec![4, 5])); assert_noop!( - Hrmp::init_open_channel(para_a, para_b, 2, 8), + Hrmp::init_open_channel(para_a, para_b, 2, 8, false), pallet_balances::Error::::InsufficientBalance ); }); @@ -521,7 +521,7 @@ fn charging_deposits() { register_parachain_with_balance(para_b, 0); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); assert_noop!( Hrmp::accept_open_channel(para_b, para_a), @@ -543,7 +543,7 @@ fn refund_deposit_on_normal_closure() { register_parachain_with_balance(para_a, 100); register_parachain_with_balance(para_b, 110); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); assert_eq!(::Currency::free_balance(¶_b.into_account_truncating()), 95); @@ -576,7 +576,7 @@ fn refund_deposit_on_offboarding() { register_parachain_with_balance(para_a, 100); register_parachain_with_balance(para_b, 110); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); assert_eq!(::Currency::free_balance(¶_b.into_account_truncating()), 95); @@ -618,7 +618,7 @@ fn no_dangling_open_requests() { run_to_block(5, Some(vec![4, 5])); // Start opening a channel a->b - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); // Then deregister one parachain, but don't wait two sessions until it takes effect. @@ -656,7 +656,7 @@ fn cancel_pending_open_channel_request() { run_to_block(5, Some(vec![4, 5])); // Start opening a channel a->b - Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); // Cancel opening the channel @@ -686,7 +686,7 @@ fn watermark_maxed_out_at_relay_parent() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); // On Block 6: From ffc4fdf9fcb114e66e9d5d46ba16a6fad1bacb6b Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 8 Sep 2023 19:33:05 +0200 Subject: [PATCH 02/16] fix signatures in benchmarks --- polkadot/runtime/parachains/src/hrmp/benchmarking.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index d4f2e3034e11..3fe347a7bcba 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -94,8 +94,7 @@ where sender_origin.clone().into(), recipient, capacity, - message_size, - false + message_size )); if matches!(until, ParachainSetupStep::Requested) { @@ -329,8 +328,7 @@ frame_benchmarking::benchmarks! { sender_origin.clone().into(), recipient_id, capacity, - message_size, - false + message_size )); assert!(HrmpOpenChannelRequests::::get(&channel_id).is_some()); } else { From fd32120a043a0ec289e27026398c0210173e516d Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 8 Sep 2023 19:49:47 +0200 Subject: [PATCH 03/16] add benchmark --- polkadot/runtime/parachains/src/hrmp.rs | 6 +++- .../parachains/src/hrmp/benchmarking.rs | 30 ++++++++++++++----- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 900fde3c9745..1e555d6ebfba 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -62,6 +62,7 @@ pub trait WeightInfo { fn hrmp_cancel_open_request(c: u32) -> Weight; fn clean_open_channel_requests(c: u32) -> Weight; fn force_open_hrmp_channel(c: u32) -> Weight; + fn establish_system_channel() -> Weight; } /// A weight info that is only suitable for testing. @@ -95,6 +96,9 @@ impl WeightInfo for TestWeightInfo { fn force_open_hrmp_channel(_: u32) -> Weight { Weight::MAX } + fn establish_system_channel() -> Weight { + Weight::MAX + } } /// A description of a request to open an HRMP channel. @@ -662,7 +666,7 @@ pub mod pallet { /// /// Any signed origin can call this function. #[pallet::call_index(8)] - #[pallet::weight(1)] // todo + #[pallet::weight(::WeightInfo::establish_system_channel())] pub fn establish_system_channel( origin: OriginFor, sender: ParaId, diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index 3fe347a7bcba..aec86bfe96c8 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -304,14 +304,9 @@ frame_benchmarking::benchmarks! { let sender_origin: crate::Origin = 1u32.into(); let recipient_id: ParaId = 2u32.into(); - // make sure para is registered, and has enough balance. - let ed = T::Currency::minimum_balance(); - let sender_deposit: BalanceOf = - Configuration::::config().hrmp_sender_deposit.unique_saturated_into(); - let recipient_deposit: BalanceOf = - Configuration::::config().hrmp_recipient_deposit.unique_saturated_into(); - register_parachain_with_balance::(sender_id, sender_deposit + ed); - register_parachain_with_balance::(recipient_id, recipient_deposit + ed); + // make sure para is registered, and has zero balance. + register_parachain_with_balance::(sender_id, Zero::zero()); + register_parachain_with_balance::(recipient_id, Zero::zero()); let capacity = Configuration::::config().hrmp_channel_max_capacity; let message_size = Configuration::::config().hrmp_channel_max_message_size; @@ -351,6 +346,25 @@ frame_benchmarking::benchmarks! { Event::::HrmpChannelForceOpened(sender_id, recipient_id, capacity, message_size).into() ); } + + establish_system_channel { + let sender_id: ParaId = 1u32.into(); + let recipient_id: ParaId = 2u32.into(); + + let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); + + // make sure para is registered, and has zero balance. + register_parachain_with_balance::(sender_id, Zero::zero()); + register_parachain_with_balance::(recipient_id, Zero::zero()); + + let capacity = Configuration::::config().hrmp_channel_max_capacity; + let message_size = Configuration::::config().hrmp_channel_max_message_size; + }: _(frame_system::RawOrigin::Signed(caller), sender_id, recipient_id) + verify { + assert_last_event::( + Event::::HrmpSystemChannelOpened(sender_id, recipient_id, capacity, message_size).into() + ); + } } frame_benchmarking::impl_benchmark_test_suite!( From 5f0bfd9144bbcd9342869072b3a46f775621d368 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 8 Sep 2023 20:45:29 +0200 Subject: [PATCH 04/16] add dummy to weight impl --- polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs | 3 +++ .../runtime/polkadot/src/weights/runtime_parachains_hrmp.rs | 3 +++ polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs | 3 +++ .../runtime/westend/src/weights/runtime_parachains_hrmp.rs | 3 +++ 4 files changed, 12 insertions(+) diff --git a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs index c13a8413e410..b575174bb812 100644 --- a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs @@ -282,4 +282,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(13)) .saturating_add(T::DbWeight::get().writes(8)) } + fn establish_system_channel() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs index 82d8c30bacad..b77447341046 100644 --- a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs @@ -292,4 +292,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(14)) .saturating_add(T::DbWeight::get().writes(8)) } + fn establish_system_channel() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs index 9f1cf65efa64..d019526b2541 100644 --- a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs @@ -289,4 +289,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(14)) .saturating_add(T::DbWeight::get().writes(8)) } + fn establish_system_channel() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs index e6ff97fa0878..d8a194cce86f 100644 --- a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs @@ -282,4 +282,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(13)) .saturating_add(T::DbWeight::get().writes(8)) } + fn establish_system_channel() -> Weight { + Weight::from_parts(1, 1) + } } From 8a4d5792265a2a9ec62461bf183843274aa1f736 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sat, 9 Sep 2023 08:21:20 +0200 Subject: [PATCH 05/16] fix and add tests --- .../src/tests/hrmp_channels.rs | 14 ---- .../src/tests/hrmp_channels.rs | 14 ---- polkadot/runtime/parachains/src/hrmp/tests.rs | 66 +++++++++++++++++++ 3 files changed, 66 insertions(+), 28 deletions(-) diff --git a/cumulus/parachains/integration-tests/emulated/assets/asset-hub-kusama/src/tests/hrmp_channels.rs b/cumulus/parachains/integration-tests/emulated/assets/asset-hub-kusama/src/tests/hrmp_channels.rs index 623b3ff599c8..7bb64333db52 100644 --- a/cumulus/parachains/integration-tests/emulated/assets/asset-hub-kusama/src/tests/hrmp_channels.rs +++ b/cumulus/parachains/integration-tests/emulated/assets/asset-hub-kusama/src/tests/hrmp_channels.rs @@ -159,12 +159,6 @@ fn force_open_hrmp_channel_for_system_para_works() { // Parachain A init values let para_a_id = PenpalKusamaA::para_id(); - let fund_amount = KUSAMA_ED * 1000_000_000; - - // Fund Parachain's Sovereign accounts to be able to reserve the deposit - let para_a_sovereign_account = Kusama::fund_para_sovereign(fund_amount, para_a_id); - let system_para_sovereign_account = Kusama::fund_para_sovereign(fund_amount, system_para_id); - Kusama::execute_with(|| { assert_ok!(::Hrmp::force_open_hrmp_channel( relay_root_origin, @@ -179,14 +173,6 @@ fn force_open_hrmp_channel_for_system_para_works() { assert_expected_events!( Kusama, vec![ - // Sender deposit is reserved for System Parachain's Sovereign account - RuntimeEvent::Balances(pallet_balances::Event::Reserved { who, .. }) =>{ - who: *who == system_para_sovereign_account, - }, - // Recipient deposit is reserved for Parachain's Sovereign account - RuntimeEvent::Balances(pallet_balances::Event::Reserved { who, .. }) =>{ - who: *who == para_a_sovereign_account, - }, // HRMP channel forced opened RuntimeEvent::Hrmp( polkadot_runtime_parachains::hrmp::Event::HrmpChannelForceOpened( diff --git a/cumulus/parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/hrmp_channels.rs b/cumulus/parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/hrmp_channels.rs index a1423f2ea90b..a6286c619f64 100644 --- a/cumulus/parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/hrmp_channels.rs +++ b/cumulus/parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/hrmp_channels.rs @@ -159,12 +159,6 @@ fn force_open_hrmp_channel_for_system_para_works() { // Parachain A init values let para_a_id = PenpalPolkadotA::para_id(); - let fund_amount = POLKADOT_ED * 1000_000_000; - - // Fund Parachain's Sovereign accounts to be able to reserve the deposit - let system_para_sovereign_account = Polkadot::fund_para_sovereign(fund_amount, system_para_id); - let para_a_sovereign_account = Polkadot::fund_para_sovereign(fund_amount, para_a_id); - Polkadot::execute_with(|| { assert_ok!(::Hrmp::force_open_hrmp_channel( relay_root_origin, @@ -179,14 +173,6 @@ fn force_open_hrmp_channel_for_system_para_works() { assert_expected_events!( Polkadot, vec![ - // Sender deposit is reserved for System Parachain's Sovereign account - RuntimeEvent::Balances(pallet_balances::Event::Reserved { who, .. }) =>{ - who: *who == system_para_sovereign_account, - }, - // Recipient deposit is reserved for Parachain's Sovereign account - RuntimeEvent::Balances(pallet_balances::Event::Reserved { who, .. }) =>{ - who: *who == para_a_sovereign_account, - }, // HRMP channel forced opened RuntimeEvent::Hrmp( polkadot_runtime_parachains::hrmp::Event::HrmpChannelForceOpened( diff --git a/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs index 485f615809b6..8daf24abe894 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/runtime/parachains/src/hrmp/tests.rs @@ -240,6 +240,72 @@ fn force_open_channel_works_with_existing_request() { }); } +#[test] +fn open_system_channel_works() { + let para_a = 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); + + run_to_block(5, Some(vec![4, 5])); + Hrmp::establish_system_channel(RuntimeOrigin::signed(1), para_a, para_b).unwrap(); + Hrmp::assert_storage_consistency_exhaustive(); + assert!(System::events().iter().any(|record| record.event == + MockEvent::Hrmp(Event::HrmpSystemChannelOpened(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 open_system_channel_does_not_work_for_non_system_chains() { + let para_a = 2001.into(); + let para_b = 2003.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); + + run_to_block(5, Some(vec![4, 5])); + assert_noop!( + Hrmp::establish_system_channel(RuntimeOrigin::signed(1), para_a, para_b), + Error::::ChannelCreationNotAuthorized + ); + Hrmp::assert_storage_consistency_exhaustive(); + }); +} + +#[test] +fn open_system_channel_does_not_work_with_one_non_system_chain() { + let para_a = 1.into(); + let para_b = 2003.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); + + run_to_block(5, Some(vec![4, 5])); + assert_noop!( + Hrmp::establish_system_channel(RuntimeOrigin::signed(1), para_a, para_b), + Error::::ChannelCreationNotAuthorized + ); + Hrmp::assert_storage_consistency_exhaustive(); + }); +} + #[test] fn close_channel_works() { let para_a = 5.into(); From 7cfd74e133043f2926ebd0489edc5776cd558fe3 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sat, 9 Sep 2023 14:43:33 +0200 Subject: [PATCH 06/16] fix benchmark --- polkadot/runtime/parachains/src/hrmp/benchmarking.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index aec86bfe96c8..3e8307fa0e5c 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -304,8 +304,12 @@ frame_benchmarking::benchmarks! { let sender_origin: crate::Origin = 1u32.into(); let recipient_id: ParaId = 2u32.into(); - // make sure para is registered, and has zero balance. - register_parachain_with_balance::(sender_id, Zero::zero()); + // Make sure para is registered. The sender does actually need the normal deposit because it + // is first going the "init" route. + let ed = T::Currency::minimum_balance(); + let sender_deposit: BalanceOf = + Configuration::::config().hrmp_sender_deposit.unique_saturated_into(); + register_parachain_with_balance::(sender_id, sender_deposit + ed); register_parachain_with_balance::(recipient_id, Zero::zero()); let capacity = Configuration::::config().hrmp_channel_max_capacity; From e3bc6eac5adf61127f88e970e5c7d420916381c1 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 10 Sep 2023 13:55:55 +0200 Subject: [PATCH 07/16] add poke fn to update deposits when config changes --- .../src/weights/runtime_parachains_hrmp.rs | 3 + polkadot/runtime/parachains/src/hrmp.rs | 88 +++++++++++++++++++ .../parachains/src/hrmp/benchmarking.rs | 44 +++++++++- .../src/weights/runtime_parachains_hrmp.rs | 3 + .../src/weights/runtime_parachains_hrmp.rs | 3 + .../src/weights/runtime_parachains_hrmp.rs | 3 + 6 files changed, 142 insertions(+), 2 deletions(-) diff --git a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs index b575174bb812..89c055ca0caf 100644 --- a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs @@ -285,4 +285,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf fn establish_system_channel() -> Weight { Weight::from_parts(1, 1) } + fn poke_channel_deposits() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 1e555d6ebfba..093ce9478698 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -63,6 +63,7 @@ pub trait WeightInfo { fn clean_open_channel_requests(c: u32) -> Weight; fn force_open_hrmp_channel(c: u32) -> Weight; fn establish_system_channel() -> Weight; + fn poke_channel_deposits() -> Weight; } /// A weight info that is only suitable for testing. @@ -99,6 +100,9 @@ impl WeightInfo for TestWeightInfo { fn establish_system_channel() -> Weight { Weight::MAX } + fn poke_channel_deposits() -> Weight { + Weight::MAX + } } /// A description of a request to open an HRMP channel. @@ -288,6 +292,9 @@ pub mod pallet { /// An HRMP channel was opened between two system chains. /// `[sender, recipient, proposed_max_capacity, proposed_max_message_size]` HrmpSystemChannelOpened(ParaId, ParaId, u32, u32), + /// An HRMP channel's deposits were updated. + /// `[sender, recipient]` + OpenChannelDepositsUpdated(ParaId, ParaId), } #[pallet::error] @@ -697,6 +704,87 @@ pub mod pallet { Ok(Pays::No.into()) } + + /// Update the deposits held for an HRMP channel. If at least one of the chains is a system + /// chain, any deposits held will be unreserved. + /// + /// Arguments: + /// + /// - `sender`: A chain, `ParaId`. + /// - `recipient`: A chain, `ParaId`. + /// + /// Any signed origin can call this function. + #[pallet::call_index(9)] + #[pallet::weight(::WeightInfo::poke_channel_deposits())] + pub fn poke_channel_deposits( + origin: OriginFor, + sender: ParaId, + recipient: ParaId, + ) -> DispatchResult { + let _caller = ensure_signed(origin)?; + let channel_id = HrmpChannelId { sender, recipient }; + let is_system = sender.is_system() || recipient.is_system(); + + let config = >::config(); + + // Channels with and amongst the system do not require a deposit. + let (new_sender_deposit, new_recipient_deposit) = if is_system { + (0, 0) + } else { + (config.hrmp_sender_deposit, config.hrmp_recipient_deposit) + }; + + let _ = HrmpChannels::::mutate(&channel_id, |channel| -> DispatchResult { + if let Some(ref mut channel) = channel { + let current_sender_deposit = channel.sender_deposit; + let current_recipient_deposit = channel.recipient_deposit; + + // nothing to update + if current_sender_deposit == new_sender_deposit && + current_recipient_deposit == new_recipient_deposit + { + return Ok(()) + } + + // sender + if current_sender_deposit > new_sender_deposit { + T::Currency::unreserve( + &channel_id.sender.into_account_truncating(), + (current_sender_deposit - new_sender_deposit).unique_saturated_into(), + ); + } else if current_sender_deposit < new_sender_deposit { + T::Currency::reserve( + &channel_id.sender.into_account_truncating(), + (new_sender_deposit - current_sender_deposit).unique_saturated_into(), + )?; + } + + // recipient + if current_recipient_deposit > new_recipient_deposit { + T::Currency::unreserve( + &channel_id.recipient.into_account_truncating(), + (current_recipient_deposit - new_recipient_deposit) + .unique_saturated_into(), + ); + } else if current_recipient_deposit < new_recipient_deposit { + T::Currency::reserve( + &channel_id.recipient.into_account_truncating(), + (new_recipient_deposit - current_recipient_deposit) + .unique_saturated_into(), + )?; + } + + // update storage + channel.sender_deposit = new_sender_deposit; + channel.recipient_deposit = new_recipient_deposit; + } + Ok(()) + }); + + Self::deposit_event(Event::OpenChannelDepositsUpdated(sender, recipient)); + + Ok(()) + } } } diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index 3e8307fa0e5c..4091896b412e 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -356,19 +356,59 @@ frame_benchmarking::benchmarks! { let recipient_id: ParaId = 2u32.into(); let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); + let config = Configuration::::config(); // make sure para is registered, and has zero balance. register_parachain_with_balance::(sender_id, Zero::zero()); register_parachain_with_balance::(recipient_id, Zero::zero()); - let capacity = Configuration::::config().hrmp_channel_max_capacity; - let message_size = Configuration::::config().hrmp_channel_max_message_size; + let capacity = config.hrmp_channel_max_capacity; + let message_size = config.hrmp_channel_max_message_size; }: _(frame_system::RawOrigin::Signed(caller), sender_id, recipient_id) verify { assert_last_event::( Event::::HrmpSystemChannelOpened(sender_id, recipient_id, capacity, message_size).into() ); } + + poke_channel_deposits { + let sender_id: ParaId = 1u32.into(); + let recipient_id: ParaId = 2u32.into(); + let channel_id = HrmpChannelId {sender: sender_id, recipient: recipient_id }; + + let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); + let config = Configuration::::config(); + + // make sure para is registered, and has balance to reserve. + let ed = T::Currency::minimum_balance(); + let sender_deposit: BalanceOf = config.hrmp_sender_deposit.unique_saturated_into(); + let recipient_deposit: BalanceOf = config.hrmp_recipient_deposit.unique_saturated_into(); + register_parachain_with_balance::(sender_id, ed + sender_deposit); + register_parachain_with_balance::(recipient_id, ed + recipient_deposit); + + // Our normal establishment won't actually reserve deposits, so just insert them directly. + HrmpChannels::::insert( + &channel_id, + HrmpChannel { + sender_deposit: config.hrmp_sender_deposit, + recipient_deposit: config.hrmp_recipient_deposit, + max_capacity: config.hrmp_channel_max_capacity, + max_total_size: config.hrmp_channel_max_total_size, + max_message_size: config.hrmp_channel_max_message_size, + msg_count: 0, + total_size: 0, + mqc_head: None, + }, + ); + }: _(frame_system::RawOrigin::Signed(caller), sender_id, recipient_id) + verify { + assert_last_event::( + Event::::OpenChannelDepositsUpdated(sender_id, recipient_id).into() + ); + let channel = HrmpChannels::::get(&channel_id).unwrap(); + assert_eq!(channel.sender_deposit, 0); + assert_eq!(channel.recipient_deposit, 0); + } } frame_benchmarking::impl_benchmark_test_suite!( diff --git a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs index b77447341046..5d5e005fbf0f 100644 --- a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs @@ -295,4 +295,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf fn establish_system_channel() -> Weight { Weight::from_parts(1, 1) } + fn poke_channel_deposits() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs index d019526b2541..d67667482f9c 100644 --- a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs @@ -292,4 +292,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf fn establish_system_channel() -> Weight { Weight::from_parts(1, 1) } + fn poke_channel_deposits() -> Weight { + Weight::from_parts(1, 1) + } } diff --git a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs index d8a194cce86f..6c4c53fb0531 100644 --- a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs @@ -285,4 +285,7 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf fn establish_system_channel() -> Weight { Weight::from_parts(1, 1) } + fn poke_channel_deposits() -> Weight { + Weight::from_parts(1, 1) + } } From 71a387d1c501f202a96ef78e68f52325742287ce Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 10 Sep 2023 17:41:44 +0200 Subject: [PATCH 08/16] reasonable weights --- .../src/weights/runtime_parachains_hrmp.rs | 40 +++++++++++++++++- .../src/weights/runtime_parachains_hrmp.rs | 41 +++++++++++++++++-- .../src/weights/runtime_parachains_hrmp.rs | 40 +++++++++++++++++- .../src/weights/runtime_parachains_hrmp.rs | 40 +++++++++++++++++- 4 files changed, 152 insertions(+), 9 deletions(-) diff --git a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs index 89c055ca0caf..f2bc2aa2b085 100644 --- a/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/kusama/src/weights/runtime_parachains_hrmp.rs @@ -282,10 +282,46 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(13)) .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Paras::ParaLifecycles` (r:1 w:0) + /// Proof: `Paras::ParaLifecycles` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequests` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequests` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpChannels` (r:1 w:0) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpEgressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpEgressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestsList` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestsList` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueues` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueues` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueueHeads` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueueHeads` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpIngressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpIngressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpAcceptedChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpAcceptedChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) fn establish_system_channel() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `417` + // Estimated: `6357` + // Minimum execution time: 629_674_000 picoseconds. + Weight::from_parts(640_174_000, 0) + .saturating_add(Weight::from_parts(0, 6357)) + .saturating_add(T::DbWeight::get().reads(12)) + .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Hrmp::HrmpChannels` (r:1 w:1) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) fn poke_channel_deposits() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `263` + // Estimated: `3728` + // Minimum execution time: 173_371_000 picoseconds. + Weight::from_parts(175_860_000, 0) + .saturating_add(Weight::from_parts(0, 3728)) + .saturating_add(T::DbWeight::get().reads(1)) + .saturating_add(T::DbWeight::get().writes(1)) } } diff --git a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs index 5d5e005fbf0f..2b5746a5cc86 100644 --- a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs @@ -292,10 +292,45 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(14)) .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Paras::ParaLifecycles` (r:1 w:0) + /// Proof: `Paras::ParaLifecycles` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequests` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequests` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpChannels` (r:1 w:0) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpEgressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpEgressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestsList` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestsList` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueues` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueues` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueueHeads` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueueHeads` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpIngressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpIngressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpAcceptedChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpAcceptedChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) fn establish_system_channel() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `417` + // Estimated: `6357` + // Minimum execution time: 629_674_000 picoseconds. + Weight::from_parts(640_174_000, 0) + .saturating_add(Weight::from_parts(0, 6357)) + .saturating_add(T::DbWeight::get().reads(12)) + .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Hrmp::HrmpChannels` (r:1 w:1) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) fn poke_channel_deposits() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `263` + // Estimated: `3728` + // Minimum execution time: 173_371_000 picoseconds. + Weight::from_parts(175_860_000, 0) + .saturating_add(Weight::from_parts(0, 3728)) + .saturating_add(T::DbWeight::get().reads(1)) + .saturating_add(T::DbWeight::get().writes(1)) } -} diff --git a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs index d67667482f9c..417820e6627f 100644 --- a/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/rococo/src/weights/runtime_parachains_hrmp.rs @@ -289,10 +289,46 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(14)) .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Paras::ParaLifecycles` (r:1 w:0) + /// Proof: `Paras::ParaLifecycles` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequests` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequests` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpChannels` (r:1 w:0) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpEgressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpEgressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestsList` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestsList` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueues` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueues` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueueHeads` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueueHeads` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpIngressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpIngressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpAcceptedChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpAcceptedChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) fn establish_system_channel() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `417` + // Estimated: `6357` + // Minimum execution time: 629_674_000 picoseconds. + Weight::from_parts(640_174_000, 0) + .saturating_add(Weight::from_parts(0, 6357)) + .saturating_add(T::DbWeight::get().reads(12)) + .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Hrmp::HrmpChannels` (r:1 w:1) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) fn poke_channel_deposits() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `263` + // Estimated: `3728` + // Minimum execution time: 173_371_000 picoseconds. + Weight::from_parts(175_860_000, 0) + .saturating_add(Weight::from_parts(0, 3728)) + .saturating_add(T::DbWeight::get().reads(1)) + .saturating_add(T::DbWeight::get().writes(1)) } } diff --git a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs index 6c4c53fb0531..9beb15303d87 100644 --- a/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/westend/src/weights/runtime_parachains_hrmp.rs @@ -282,10 +282,46 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(13)) .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Paras::ParaLifecycles` (r:1 w:0) + /// Proof: `Paras::ParaLifecycles` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequests` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequests` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpChannels` (r:1 w:0) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpEgressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpEgressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpOpenChannelRequestsList` (r:1 w:1) + /// Proof: `Hrmp::HrmpOpenChannelRequestsList` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueues` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueues` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Dmp::DownwardMessageQueueHeads` (r:2 w:2) + /// Proof: `Dmp::DownwardMessageQueueHeads` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpIngressChannelsIndex` (r:1 w:0) + /// Proof: `Hrmp::HrmpIngressChannelsIndex` (`max_values`: None, `max_size`: None, mode: `Measured`) + /// Storage: `Hrmp::HrmpAcceptedChannelRequestCount` (r:1 w:1) + /// Proof: `Hrmp::HrmpAcceptedChannelRequestCount` (`max_values`: None, `max_size`: None, mode: `Measured`) fn establish_system_channel() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `417` + // Estimated: `6357` + // Minimum execution time: 629_674_000 picoseconds. + Weight::from_parts(640_174_000, 0) + .saturating_add(Weight::from_parts(0, 6357)) + .saturating_add(T::DbWeight::get().reads(12)) + .saturating_add(T::DbWeight::get().writes(8)) } + /// Storage: `Hrmp::HrmpChannels` (r:1 w:1) + /// Proof: `Hrmp::HrmpChannels` (`max_values`: None, `max_size`: None, mode: `Measured`) fn poke_channel_deposits() -> Weight { - Weight::from_parts(1, 1) + // Proof Size summary in bytes: + // Measured: `263` + // Estimated: `3728` + // Minimum execution time: 173_371_000 picoseconds. + Weight::from_parts(175_860_000, 0) + .saturating_add(Weight::from_parts(0, 3728)) + .saturating_add(T::DbWeight::get().reads(1)) + .saturating_add(T::DbWeight::get().writes(1)) } } From e514265a1cc3c5899e83387d5fcf657affd81213 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 10 Sep 2023 19:01:24 +0200 Subject: [PATCH 09/16] missing brace --- polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs index 2b5746a5cc86..73a08d33eed8 100644 --- a/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs +++ b/polkadot/runtime/polkadot/src/weights/runtime_parachains_hrmp.rs @@ -334,3 +334,4 @@ impl runtime_parachains::hrmp::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().reads(1)) .saturating_add(T::DbWeight::get().writes(1)) } +} From 7bbaf665483750bac9056803c6e495c9f34b06a3 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 10 Sep 2023 20:44:28 +0200 Subject: [PATCH 10/16] do system checks in init/accept, not caller --- .../runtime/common/src/paras_sudo_wrapper.rs | 1 - polkadot/runtime/parachains/src/hrmp.rs | 32 +++--- polkadot/runtime/parachains/src/hrmp/tests.rs | 104 ++++++++++-------- 3 files changed, 73 insertions(+), 64 deletions(-) diff --git a/polkadot/runtime/common/src/paras_sudo_wrapper.rs b/polkadot/runtime/common/src/paras_sudo_wrapper.rs index 5704e04f2492..0fc2644b2a0b 100644 --- a/polkadot/runtime/common/src/paras_sudo_wrapper.rs +++ b/polkadot/runtime/common/src/paras_sudo_wrapper.rs @@ -166,7 +166,6 @@ pub mod pallet { recipient, max_capacity, max_message_size, - false, // although set by root, this wrapper is used to simulate real networks )?; >::accept_open_channel(recipient, sender)?; Ok(()) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 093ce9478698..5d1fdef35e7c 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -497,7 +497,6 @@ pub mod pallet { recipient, proposed_max_capacity, proposed_max_message_size, - false, )?; Self::deposit_event(Event::OpenChannelRequested( origin, @@ -649,7 +648,7 @@ pub mod pallet { // Now we proceed with normal init/accept, except that we set `no_deposit` to true such // that it will not require deposits from either member. - Self::init_open_channel(sender, recipient, max_capacity, max_message_size, true)?; + Self::init_open_channel(sender, recipient, max_capacity, max_message_size)?; Self::accept_open_channel(recipient, sender)?; Self::deposit_event(Event::HrmpChannelForceOpened( sender, @@ -671,7 +670,8 @@ pub mod pallet { /// - `sender`: A system chain, `ParaId`. /// - `recipient`: A system chain, `ParaId`. /// - /// Any signed origin can call this function. + /// Any signed origin can call this function, but _both_ inputs MUST be system chains. If + /// the channel does not exist yet, there is no fee. #[pallet::call_index(8)] #[pallet::weight(::WeightInfo::establish_system_channel())] pub fn establish_system_channel( @@ -692,7 +692,7 @@ pub mod pallet { let max_message_size = config.hrmp_channel_max_message_size; let max_capacity = config.hrmp_channel_max_capacity; - Self::init_open_channel(sender, recipient, max_capacity, max_message_size, true)?; + Self::init_open_channel(sender, recipient, max_capacity, max_message_size)?; Self::accept_open_channel(recipient, sender)?; Self::deposit_event(Event::HrmpSystemChannelOpened( @@ -806,7 +806,7 @@ fn preopen_hrmp_channel( max_capacity: u32, max_message_size: u32, ) -> DispatchResult { - >::init_open_channel(sender, recipient, max_capacity, max_message_size, false)?; + >::init_open_channel(sender, recipient, max_capacity, max_message_size)?; >::accept_open_channel(recipient, sender)?; Ok(()) } @@ -1322,20 +1322,17 @@ impl Pallet { } /// Initiate opening a channel from a parachain to a given recipient with given channel - /// parameters. + /// parameters. If neither chain is part of the system, then a deposit from the `Configuration` + /// will be required for `origin` (the sender) upon opening the request and the `recipient` upon + /// accepting it. /// /// Basically the same as [`hrmp_init_open_channel`](Pallet::hrmp_init_open_channel) but /// intended for calling directly from other pallets rather than dispatched. - /// - /// The parameter `no_deposit` allows the caller to establish the channel without deposits. - /// This should be used only for channels established via governance or channels between system - /// chains. All other channels should use `false`. pub fn init_open_channel( origin: ParaId, recipient: ParaId, proposed_max_capacity: u32, proposed_max_message_size: u32, - no_deposit: bool, ) -> DispatchResult { ensure!(origin != recipient, Error::::OpenHrmpChannelToSelf); ensure!( @@ -1373,8 +1370,9 @@ impl Pallet { Error::::OpenHrmpChannelLimitExceeded, ); - // In practice should only be zero (for governance/system chains) or the default. - let deposit = if no_deposit { 0 } else { config.hrmp_sender_deposit }; + // Do not require deposits for channels with or amongst the system. + let is_system = origin.is_system() || recipient.is_system(); + let deposit = if is_system { 0 } else { config.hrmp_sender_deposit }; if !deposit.is_zero() { T::Currency::reserve( &origin.into_account_truncating(), @@ -1445,10 +1443,10 @@ impl Pallet { Error::::AcceptHrmpChannelLimitExceeded, ); - // The channel cleanup / para offboarding does not store recipient deposits. If the deposit - // was overridden for the sender, which should only happen for channels established by - // governance or for the system itself, then it also does not take a recipient deposit. - if !channel_req.sender_deposit.is_zero() { + // Do not require deposits for channels with or amongst the system. + let is_system = origin.is_system() || sender.is_system(); + let deposit = if is_system { 0 } else { config.hrmp_recipient_deposit }; + if !deposit.is_zero() { T::Currency::reserve( &origin.into_account_truncating(), config.hrmp_recipient_deposit.unique_saturated_into(), diff --git a/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs index 8daf24abe894..e68287456df8 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/runtime/parachains/src/hrmp/tests.rs @@ -14,6 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +// NOTE: System chains, identified by ParaId < 2000, are treated as special in HRMP channel +// initialization. Namely, they do not require a deposit if even one ParaId is a system para. If +// both paras are system chains, then they are also configured to the system's max configuration. + use super::*; use crate::mock::{ deregister_parachain, new_test_ext, register_parachain, register_parachain_with_balance, @@ -133,10 +137,10 @@ fn empty_state_consistent_state() { #[test] fn open_channel_works() { - let para_a = 1.into(); - let para_a_origin: crate::Origin = 1.into(); - let para_b = 3.into(); - let para_b_origin: crate::Origin = 3.into(); + let para_a = 2001.into(); + let para_a_origin: crate::Origin = 2001.into(); + let para_b = 2003.into(); + let para_b_origin: crate::Origin = 2003.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { // We need both A & B to be registered and alive parachains. @@ -171,13 +175,16 @@ fn open_channel_works() { #[test] fn force_open_channel_works() { let para_a = 1.into(); - let para_b = 3.into(); + let para_b = 2003.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); + let para_b_free_balance = + ::Currency::free_balance(¶_b.into_account_truncating()); + run_to_block(5, Some(vec![4, 5])); Hrmp::force_open_hrmp_channel(RuntimeOrigin::root(), para_a, para_b, 2, 8).unwrap(); Hrmp::assert_storage_consistency_exhaustive(); @@ -193,14 +200,19 @@ fn force_open_channel_works() { // 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)); + // Because para_a is a system chain, para_b's free balance should not have changed. + assert_eq!( + ::Currency::free_balance(¶_b.into_account_truncating()), + para_b_free_balance + ); }); } #[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(); + let para_a = 2001.into(); + let para_a_origin: crate::Origin = 2001.into(); + let para_b = 2003.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { // We need both A & B to be registered and live parachains. @@ -308,16 +320,16 @@ fn open_system_channel_does_not_work_with_one_non_system_chain() { #[test] fn close_channel_works() { - let para_a = 5.into(); - let para_b = 2.into(); - let para_b_origin: crate::Origin = 2.into(); + let para_a = 2005.into(); + let para_b = 2002.into(); + let para_b_origin: crate::Origin = 2002.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { register_parachain(para_a); register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(6, Some(vec![6])); @@ -341,8 +353,8 @@ fn close_channel_works() { #[test] fn send_recv_messages() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_channel_max_message_size = 20; @@ -352,7 +364,7 @@ fn send_recv_messages() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); // On Block 6: @@ -390,7 +402,7 @@ fn hrmp_mqc_head_fixture() { register_parachain(para_b); run_to_block(2, Some(vec![1, 2])); - Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(3, Some(vec![3])); @@ -424,15 +436,15 @@ fn hrmp_mqc_head_fixture() { #[test] fn accept_incoming_request_and_offboard() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { register_parachain(para_a); register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); deregister_parachain(para_a); @@ -446,9 +458,9 @@ fn accept_incoming_request_and_offboard() { #[test] fn check_sent_messages() { - let para_a = 32.into(); - let para_b = 64.into(); - let para_c = 97.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); + let para_c = 2097.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { register_parachain(para_a); @@ -459,9 +471,9 @@ fn check_sent_messages() { // Open two channels to the same receiver, b: // a -> b, c -> b - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); - Hrmp::init_open_channel(para_c, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_c, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_c).unwrap(); // On Block 6: session change. @@ -510,8 +522,8 @@ fn check_sent_messages() { fn verify_externally_accessible() { use primitives::{well_known_keys, AbridgedHrmpChannel}; - let para_a = 20.into(); - let para_b = 21.into(); + let para_a = 2020.into(); + let para_b = 2021.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { // Register two parachains, wait until a session change, then initiate channel open @@ -519,7 +531,7 @@ fn verify_externally_accessible() { register_parachain(para_a); register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); run_to_block(8, Some(vec![8])); @@ -568,8 +580,8 @@ fn verify_externally_accessible() { #[test] fn charging_deposits() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { register_parachain_with_balance(para_a, 0); @@ -577,7 +589,7 @@ fn charging_deposits() { run_to_block(5, Some(vec![4, 5])); assert_noop!( - Hrmp::init_open_channel(para_a, para_b, 2, 8, false), + Hrmp::init_open_channel(para_a, para_b, 2, 8), pallet_balances::Error::::InsufficientBalance ); }); @@ -587,7 +599,7 @@ fn charging_deposits() { register_parachain_with_balance(para_b, 0); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); assert_noop!( Hrmp::accept_open_channel(para_b, para_a), @@ -598,8 +610,8 @@ fn charging_deposits() { #[test] fn refund_deposit_on_normal_closure() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_sender_deposit = 20; @@ -609,7 +621,7 @@ fn refund_deposit_on_normal_closure() { register_parachain_with_balance(para_a, 100); register_parachain_with_balance(para_b, 110); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); assert_eq!(::Currency::free_balance(¶_b.into_account_truncating()), 95); @@ -631,8 +643,8 @@ fn refund_deposit_on_normal_closure() { #[test] fn refund_deposit_on_offboarding() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_sender_deposit = 20; @@ -642,7 +654,7 @@ fn refund_deposit_on_offboarding() { register_parachain_with_balance(para_a, 100); register_parachain_with_balance(para_b, 110); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); assert_eq!(::Currency::free_balance(¶_b.into_account_truncating()), 95); @@ -671,8 +683,8 @@ fn refund_deposit_on_offboarding() { #[test] fn no_dangling_open_requests() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_sender_deposit = 20; @@ -684,7 +696,7 @@ fn no_dangling_open_requests() { run_to_block(5, Some(vec![4, 5])); // Start opening a channel a->b - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); // Then deregister one parachain, but don't wait two sessions until it takes effect. @@ -709,8 +721,8 @@ fn no_dangling_open_requests() { #[test] fn cancel_pending_open_channel_request() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_sender_deposit = 20; @@ -722,7 +734,7 @@ fn cancel_pending_open_channel_request() { run_to_block(5, Some(vec![4, 5])); // Start opening a channel a->b - Hrmp::init_open_channel(para_a, para_b, 2, 8, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); assert_eq!(::Currency::free_balance(¶_a.into_account_truncating()), 80); // Cancel opening the channel @@ -741,8 +753,8 @@ fn cancel_pending_open_channel_request() { #[test] fn watermark_maxed_out_at_relay_parent() { - let para_a = 32.into(); - let para_b = 64.into(); + let para_a = 2032.into(); + let para_b = 2064.into(); let mut genesis = GenesisConfigBuilder::default(); genesis.hrmp_channel_max_message_size = 20; @@ -752,7 +764,7 @@ fn watermark_maxed_out_at_relay_parent() { register_parachain(para_b); run_to_block(5, Some(vec![4, 5])); - Hrmp::init_open_channel(para_a, para_b, 2, 20, false).unwrap(); + Hrmp::init_open_channel(para_a, para_b, 2, 20).unwrap(); Hrmp::accept_open_channel(para_b, para_a).unwrap(); // On Block 6: From c5e1621de35e17c8665cb3465d4034391d6664ec Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Sep 2023 07:41:58 +0200 Subject: [PATCH 11/16] add PR1407 change to identify system chains --- polkadot/parachain/src/primitives.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/polkadot/parachain/src/primitives.rs b/polkadot/parachain/src/primitives.rs index 5cea9d3bbf4e..588e1e427e8b 100644 --- a/polkadot/parachain/src/primitives.rs +++ b/polkadot/parachain/src/primitives.rs @@ -199,13 +199,14 @@ impl From for Id { } } -const USER_INDEX_START: u32 = 1000; +// System parachain ID is considered `< 2000`. +const SYSTEM_INDEX_END: u32 = 1999; const PUBLIC_INDEX_START: u32 = 2000; /// The ID of the first user (non-system) parachain. -pub const LOWEST_USER_ID: Id = Id(USER_INDEX_START); +pub const LOWEST_USER_ID: Id = Id(PUBLIC_INDEX_START); -/// The ID of the first publicly registerable parachain. +/// The ID of the first publicly registrable parachain. pub const LOWEST_PUBLIC_ID: Id = Id(PUBLIC_INDEX_START); impl Id { @@ -223,7 +224,7 @@ pub trait IsSystem { impl IsSystem for Id { fn is_system(&self) -> bool { - self.0 < USER_INDEX_START + self.0 <= SYSTEM_INDEX_END } } From b1be601e92d70ff37fbad2eb26c6d5013e7e2295 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 13 Sep 2023 08:25:02 +0200 Subject: [PATCH 12/16] add test for poke --- .../parachains/src/hrmp/benchmarking.rs | 7 +++ polkadot/runtime/parachains/src/hrmp/tests.rs | 52 ++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs index 4091896b412e..9ffa264a7dc8 100644 --- a/polkadot/runtime/parachains/src/hrmp/benchmarking.rs +++ b/polkadot/runtime/parachains/src/hrmp/benchmarking.rs @@ -400,14 +400,21 @@ frame_benchmarking::benchmarks! { mqc_head: None, }, ); + // Actually reserve the deposits. + let _ = T::Currency::reserve(&sender_id.into_account_truncating(), sender_deposit); + let _ = T::Currency::reserve(&recipient_id.into_account_truncating(), recipient_deposit); }: _(frame_system::RawOrigin::Signed(caller), sender_id, recipient_id) verify { assert_last_event::( Event::::OpenChannelDepositsUpdated(sender_id, recipient_id).into() ); let channel = HrmpChannels::::get(&channel_id).unwrap(); + // Check that the deposit was updated in the channel state. assert_eq!(channel.sender_deposit, 0); assert_eq!(channel.recipient_deposit, 0); + // And that the funds were unreserved. + assert_eq!(T::Currency::reserved_balance(&sender_id.into_account_truncating()), 0u128.unique_saturated_into()); + assert_eq!(T::Currency::reserved_balance(&recipient_id.into_account_truncating()), 0u128.unique_saturated_into()); } } diff --git a/polkadot/runtime/parachains/src/hrmp/tests.rs b/polkadot/runtime/parachains/src/hrmp/tests.rs index e68287456df8..236745b7cc35 100644 --- a/polkadot/runtime/parachains/src/hrmp/tests.rs +++ b/polkadot/runtime/parachains/src/hrmp/tests.rs @@ -24,7 +24,7 @@ use crate::mock::{ Configuration, Hrmp, MockGenesisConfig, Paras, ParasShared, RuntimeEvent as MockEvent, RuntimeOrigin, System, Test, }; -use frame_support::assert_noop; +use frame_support::{assert_noop, assert_ok}; use primitives::BlockNumber; use std::collections::BTreeMap; @@ -318,6 +318,56 @@ fn open_system_channel_does_not_work_with_one_non_system_chain() { }); } +#[test] +fn poke_deposits_works() { + let para_a = 1.into(); + let para_b = 2001.into(); + + new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { + // We need both A & B to be registered and live parachains. + register_parachain_with_balance(para_a, 200); + register_parachain_with_balance(para_b, 200); + + let config = Configuration::config(); + let channel_id = HrmpChannelId { sender: para_a, recipient: para_b }; + + // Our normal establishment won't actually reserve deposits, so just insert them directly. + HrmpChannels::::insert( + &channel_id, + HrmpChannel { + sender_deposit: config.hrmp_sender_deposit, + recipient_deposit: config.hrmp_recipient_deposit, + max_capacity: config.hrmp_channel_max_capacity, + max_total_size: config.hrmp_channel_max_total_size, + max_message_size: config.hrmp_channel_max_message_size, + msg_count: 0, + total_size: 0, + mqc_head: None, + }, + ); + // reserve funds + assert_ok!(::Currency::reserve( + ¶_a.into_account_truncating(), + config.hrmp_sender_deposit + )); + assert_ok!(::Currency::reserve( + ¶_b.into_account_truncating(), + config.hrmp_recipient_deposit + )); + + assert_ok!(Hrmp::poke_channel_deposits(RuntimeOrigin::signed(1), para_a, para_b)); + + assert_eq!( + ::Currency::reserved_balance(¶_a.into_account_truncating()), + 0 + ); + assert_eq!( + ::Currency::reserved_balance(¶_b.into_account_truncating()), + 0 + ); + }); +} + #[test] fn close_channel_works() { let para_a = 2005.into(); From 9fa4cbf29ac05b3090ba715f08cc74f2f5f7fb6f Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 13 Sep 2023 08:40:23 +0200 Subject: [PATCH 13/16] update comment --- polkadot/runtime/parachains/src/hrmp.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 5d1fdef35e7c..c1950b904a7d 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -681,8 +681,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let _caller = ensure_signed(origin)?; - // to recognize current chains needs - // https://github.com/paritytech/polkadot-sdk/pull/1406 + // both chains must be system ensure!( sender.is_system() && recipient.is_system(), Error::::ChannelCreationNotAuthorized From 8b262818b69e62ee645fbfafd7b7cc8ed186df04 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Mon, 18 Sep 2023 14:43:37 +0200 Subject: [PATCH 14/16] Update polkadot/runtime/parachains/src/hrmp.rs Co-authored-by: Muharem Ismailov --- polkadot/runtime/parachains/src/hrmp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index c1950b904a7d..d80483729058 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -778,7 +778,7 @@ pub mod pallet { channel.recipient_deposit = new_recipient_deposit; } Ok(()) - }); + })?; Self::deposit_event(Event::OpenChannelDepositsUpdated(sender, recipient)); From 67815c066c3c833f36e63886d0c1ddcfc30272c1 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 18 Sep 2023 20:08:42 +0200 Subject: [PATCH 15/16] don't saturate in conversion --- polkadot/runtime/parachains/src/hrmp.rs | 34 ++++++++++++++++++------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index d80483729058..b8f7d82e340e 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -27,8 +27,9 @@ use primitives::{ SessionIndex, }; use scale_info::TypeInfo; -use sp_runtime::traits::{ - AccountIdConversion, BlakeTwo256, Hash as HashT, UniqueSaturatedInto, Zero, +use sp_runtime::{ + traits::{AccountIdConversion, BlakeTwo256, Hash as HashT, UniqueSaturatedInto, Zero}, + ArithmeticError, }; use sp_std::{ collections::{btree_map::BTreeMap, btree_set::BTreeSet}, @@ -747,35 +748,50 @@ pub mod pallet { // sender if current_sender_deposit > new_sender_deposit { + // Can never underflow, but be paranoid. + let amount = current_sender_deposit + .checked_sub(new_sender_deposit) + .ok_or(ArithmeticError::Underflow)?; T::Currency::unreserve( &channel_id.sender.into_account_truncating(), - (current_sender_deposit - new_sender_deposit).unique_saturated_into(), + // The difference should always be convertable into `Balance`, but be + // paranoid and do nothing in case. + amount.try_into().unwrap_or(Zero::zero()), ); } else if current_sender_deposit < new_sender_deposit { + let amount = new_sender_deposit + .checked_sub(current_sender_deposit) + .ok_or(ArithmeticError::Underflow)?; T::Currency::reserve( &channel_id.sender.into_account_truncating(), - (new_sender_deposit - current_sender_deposit).unique_saturated_into(), + amount.try_into().unwrap_or(Zero::zero()), )?; } // recipient if current_recipient_deposit > new_recipient_deposit { + let amount = current_recipient_deposit + .checked_sub(new_recipient_deposit) + .ok_or(ArithmeticError::Underflow)?; T::Currency::unreserve( &channel_id.recipient.into_account_truncating(), - (current_recipient_deposit - new_recipient_deposit) - .unique_saturated_into(), + amount.try_into().unwrap_or(Zero::zero()), ); } else if current_recipient_deposit < new_recipient_deposit { + let amount = new_recipient_deposit + .checked_sub(current_recipient_deposit) + .ok_or(ArithmeticError::Underflow)?; T::Currency::reserve( &channel_id.recipient.into_account_truncating(), - (new_recipient_deposit - current_recipient_deposit) - .unique_saturated_into(), + amount.try_into().unwrap_or(Zero::zero()), )?; } // update storage channel.sender_deposit = new_sender_deposit; channel.recipient_deposit = new_recipient_deposit; + } else { + return Err(Error::::OpenHrmpChannelDoesntExist.into()) } Ok(()) })?; @@ -961,7 +977,7 @@ impl Pallet { "can't be `None` due to the invariant that the list contains the same items as the set; qed", ); - let system_channel = request.sender_deposit.is_zero(); + let system_channel = channel_id.sender.is_system() || channel_id.recipient.is_system(); let sender_deposit = request.sender_deposit; let recipient_deposit = if system_channel { 0 } else { config.hrmp_recipient_deposit }; From 6740c682517ae0bd5ef27ce02f152ad8b03ac0cc Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 19 Sep 2023 13:01:00 +0200 Subject: [PATCH 16/16] docs update --- polkadot/runtime/parachains/src/hrmp.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index b8f7d82e340e..3f0a5e0830cb 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -705,8 +705,8 @@ pub mod pallet { Ok(Pays::No.into()) } - /// Update the deposits held for an HRMP channel. If at least one of the chains is a system - /// chain, any deposits held will be unreserved. + /// Update the deposits held for an HRMP channel to the latest `Configuration`. Channels + /// with system chains do not require a deposit. /// /// Arguments: /// @@ -1464,7 +1464,7 @@ impl Pallet { if !deposit.is_zero() { T::Currency::reserve( &origin.into_account_truncating(), - config.hrmp_recipient_deposit.unique_saturated_into(), + deposit.unique_saturated_into(), )?; }