From c644c39f3d911103cc5d4b9ec6511a07dcdbbcc7 Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Wed, 13 Jan 2021 17:35:32 +0100 Subject: [PATCH] HRMP channel deposits (#2225) * Drive by fixes The visibility modifiers are remnants of the previous structure where HRMP wasn't a standalone module, by rather a submodule of the router module. * Add Currency assoc type to Config This would allow us to reserve balance for deposits. This commit also integrates the HRMP module in rococo, test-runtime and mocks to use the balances pallet. * Fix a bug that doesn't increment the age In case the request is not confirmed, the age would be incremented but not persisted. * Fix cleaning the indexes Before that change, the cleaning of the channel indexes was wrong, because it naively removed entire rows that was pertaining to the para we delete. This approach is flawed because it doesn't account for the rows that are pertaining to other paras that contain the outgoing one. This clearly violates the invariant imposed on the indexes, that all the index rows must contain alive paras, but apart from that it also lead to the situation where ingress index would contain the a different set of channels that an egress have. * Reserve currency for opening the channels Note the ugly `unique_saturated_into` calls. The reason for them is the currency trait accepts and defines the `Balance` associated type and the deposit values are coming from the `HostConfiguration` where they are defined using the `Balance`. I figured that parameterising `HostConfiguration` would be annoying. On the other hand, I don't expect these `unique_saturated_into` calls to give us problems since it seems to be a reasonable assumption that this module will be instantiated within a runtime where the Currency provided will have a Balance that matches the one used in the configuration. * Tests: Adapt `run_to_block` so that it submits a proper config * Tests: exercise the deposit logic --- polkadot/parachain/src/primitives.rs | 2 +- .../runtime/common/src/paras_registrar.rs | 1 + polkadot/runtime/parachains/src/hrmp.rs | 314 +++++++++++++++--- polkadot/runtime/parachains/src/mock.rs | 18 +- polkadot/runtime/rococo/src/lib.rs | 1 + polkadot/runtime/test-runtime/src/lib.rs | 1 + 6 files changed, 290 insertions(+), 47 deletions(-) diff --git a/polkadot/parachain/src/primitives.rs b/polkadot/parachain/src/primitives.rs index 91250b75f5..a9b8f13c80 100644 --- a/polkadot/parachain/src/primitives.rs +++ b/polkadot/parachain/src/primitives.rs @@ -231,7 +231,7 @@ impl AccountIdConversion for Id { /// that we use the first item tuple for the sender and the second for the recipient. Only one channel /// is allowed between two participants in one direction, i.e. there cannot be 2 different channels /// identified by `(A, B)`. -#[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)] +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, RuntimeDebug)] #[cfg_attr(feature = "std", derive(Hash))] pub struct HrmpChannelId { /// The para that acts as the sender in this channel. diff --git a/polkadot/runtime/common/src/paras_registrar.rs b/polkadot/runtime/common/src/paras_registrar.rs index 1d46a43c01..a14c46bf86 100644 --- a/polkadot/runtime/common/src/paras_registrar.rs +++ b/polkadot/runtime/common/src/paras_registrar.rs @@ -435,6 +435,7 @@ mod tests { impl hrmp::Config for Test { type Origin = Origin; + type Currency = pallet_balances::Module; } impl pallet_session::historical::Config for Test { diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index 4c9341652a..b3682345cf 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -21,14 +21,14 @@ use crate::{ }; use parity_scale_codec::{Decode, Encode}; use frame_support::{ - decl_storage, decl_module, decl_error, ensure, traits::Get, weights::Weight, StorageMap, - StorageValue, dispatch::DispatchResult, + decl_storage, decl_module, decl_error, ensure, traits::{Get, ReservableCurrency}, weights::Weight, + StorageMap, StorageValue, dispatch::DispatchResult, }; use primitives::v1::{ Balance, Hash, HrmpChannelId, Id as ParaId, InboundHrmpMessage, OutboundHrmpMessage, SessionIndex, }; -use sp_runtime::traits::{BlakeTwo256, Hash as HashT}; +use sp_runtime::traits::{UniqueSaturatedInto, AccountIdConversion, BlakeTwo256, Hash as HashT}; use sp_std::{ mem, fmt, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, @@ -218,6 +218,13 @@ pub trait Config: frame_system::Config + configuration::Config + paras::Config + type Origin: From + From<::Origin> + Into::Origin>>; + + /// An interface for reserving deposits for opening channels. + /// + /// NOTE that this Currency instance will be charged with the amounts defined in the `Configuration` + /// module. Specifically, that means that the `Balance` of the `Currency` implementation should + /// be the same as `Balance` as used in the `Configuration`. + type Currency: ReservableCurrency; } decl_storage! { @@ -226,7 +233,6 @@ decl_storage! { /// The entries are sorted ascending by the para id. OutgoingParas: Vec; - /// The set of pending HRMP open channel requests. /// /// The set is accompanied by a list for iteration. @@ -423,23 +429,28 @@ impl Module { } /// Remove all storage entries associated with the given para. - pub(super) fn clean_hrmp_after_outgoing(outgoing_para: ParaId) { + fn clean_hrmp_after_outgoing(outgoing_para: ParaId) { ::HrmpOpenChannelRequestCount::remove(&outgoing_para); ::HrmpAcceptedChannelRequestCount::remove(&outgoing_para); - // close all channels where the outgoing para acts as the recipient. - for sender in ::HrmpIngressChannelsIndex::take(&outgoing_para) { - Self::close_hrmp_channel(&HrmpChannelId { + let ingress = ::HrmpIngressChannelsIndex::take(&outgoing_para) + .into_iter() + .map(|sender| HrmpChannelId { sender, recipient: outgoing_para.clone(), }); - } - // close all channels where the outgoing para acts as the sender. - for recipient in ::HrmpEgressChannelsIndex::take(&outgoing_para) { - Self::close_hrmp_channel(&HrmpChannelId { + let egress = ::HrmpEgressChannelsIndex::take(&outgoing_para) + .into_iter() + .map(|recipient| HrmpChannelId { sender: outgoing_para.clone(), recipient, }); + let mut to_close = ingress.chain(egress).collect::>(); + to_close.sort(); + to_close.dedup(); + + for channel in to_close { + Self::close_hrmp_channel(&channel); } } @@ -447,7 +458,7 @@ impl Module { /// /// - prune the stale requests /// - enact the confirmed requests - pub(super) fn process_hrmp_open_channel_requests(config: &HostConfiguration) { + fn process_hrmp_open_channel_requests(config: &HostConfiguration) { let mut open_req_channels = ::HrmpOpenChannelRequestsList::get(); if open_req_channels.is_empty() { return; @@ -528,15 +539,21 @@ impl Module { request.age += 1; if request.age == config.hrmp_open_request_ttl { // got stale - ::HrmpOpenChannelRequestCount::mutate(&channel_id.sender, |v| { *v -= 1; }); - // TODO: return deposit https://github.com/paritytech/polkadot/issues/1907 - let _ = open_req_channels.swap_remove(idx); - ::HrmpOpenChannelRequests::remove(&channel_id); + if let Some(HrmpOpenChannelRequest { sender_deposit, .. }) = + ::HrmpOpenChannelRequests::take(&channel_id) + { + T::Currency::unreserve( + &channel_id.sender.into_account(), + sender_deposit.unique_saturated_into(), + ); + } + } else { + ::HrmpOpenChannelRequests::insert(&channel_id, request); } } } @@ -545,35 +562,49 @@ impl Module { } /// Iterate over all close channel requests unconditionally closing the channels. - pub(super) fn process_hrmp_close_channel_requests() { + fn process_hrmp_close_channel_requests() { let close_reqs = ::HrmpCloseChannelRequestsList::take(); for condemned_ch_id in close_reqs { ::HrmpCloseChannelRequests::remove(&condemned_ch_id); Self::close_hrmp_channel(&condemned_ch_id); - - // clean up the indexes. - ::HrmpEgressChannelsIndex::mutate(&condemned_ch_id.sender, |v| { - if let Ok(i) = v.binary_search(&condemned_ch_id.recipient) { - v.remove(i); - } - }); - ::HrmpIngressChannelsIndex::mutate(&condemned_ch_id.recipient, |v| { - if let Ok(i) = v.binary_search(&condemned_ch_id.sender) { - v.remove(i); - } - }); } } /// Close and remove the designated HRMP channel. /// - /// This includes returning the deposits. However, it doesn't include updating the ingress/egress - /// indicies. - pub(super) fn close_hrmp_channel(channel_id: &HrmpChannelId) { - // TODO: return deposit https://github.com/paritytech/polkadot/issues/1907 + /// This includes returning the deposits. + /// + /// This function is indempotent, meaning that after the first application it should have no + /// effect (i.e. it won't return the deposits twice). + fn close_hrmp_channel(channel_id: &HrmpChannelId) { + if let Some(HrmpChannel { + sender_deposit, + recipient_deposit, + .. + }) = ::HrmpChannels::take(channel_id) + { + T::Currency::unreserve( + &channel_id.sender.into_account(), + sender_deposit.unique_saturated_into(), + ); + T::Currency::unreserve( + &channel_id.recipient.into_account(), + recipient_deposit.unique_saturated_into(), + ); + } - ::HrmpChannels::remove(channel_id); ::HrmpChannelContents::remove(channel_id); + + ::HrmpEgressChannelsIndex::mutate(&channel_id.sender, |v| { + if let Ok(i) = v.binary_search(&channel_id.recipient) { + v.remove(i); + } + }); + ::HrmpIngressChannelsIndex::mutate(&channel_id.recipient, |v| { + if let Ok(i) = v.binary_search(&channel_id.sender) { + v.remove(i); + } + }); } /// Check that the candidate of the given recipient controls the HRMP watermark properly. @@ -845,7 +876,7 @@ impl Module { recipient: ParaId, proposed_max_capacity: u32, proposed_max_message_size: u32, - ) -> Result<(), Error> { + ) -> DispatchResult { ensure!(origin != recipient, Error::::OpenHrmpChannelToSelf); ensure!( >::is_valid_para(recipient), @@ -896,7 +927,10 @@ impl Module { Error::::OpenHrmpChannelLimitExceeded, ); - // TODO: Deposit https://github.com/paritytech/polkadot/issues/1907 + T::Currency::reserve( + &origin.into_account(), + config.hrmp_sender_deposit.unique_saturated_into(), + )?; ::HrmpOpenChannelRequestCount::insert(&origin, open_req_cnt + 1); ::HrmpOpenChannelRequests::insert( @@ -936,9 +970,9 @@ impl Module { /// Accept a pending open channel request from the given sender. /// - /// Basically the same as [`hrmp_accept_open_channel`](Module::hrmp_accept_open_channel) but intendend for calling directly from - /// other pallets rather than dispatched. - pub fn accept_open_channel(origin: ParaId, sender: ParaId) -> Result<(), Error> { + /// Basically the same as [`hrmp_accept_open_channel`](Module::hrmp_accept_open_channel) but + /// intendend for calling directly from other pallets rather than dispatched. + pub fn accept_open_channel(origin: ParaId, sender: ParaId) -> DispatchResult { let channel_id = HrmpChannelId { sender, recipient: origin, @@ -966,7 +1000,10 @@ impl Module { Error::::AcceptHrmpChannelLimitExceeded, ); - // TODO: Deposit https://github.com/paritytech/polkadot/issues/1907 + T::Currency::reserve( + &origin.into_account(), + config.hrmp_recipient_deposit.unique_saturated_into(), + )?; // persist the updated open channel request and then increment the number of accepted // channels. @@ -1086,14 +1123,16 @@ impl Module { mod tests { use super::*; use crate::mock::{ - new_test_ext, Configuration, Paras, Hrmp, System, GenesisConfig as MockGenesisConfig, + new_test_ext, Test, Configuration, Paras, Hrmp, System, GenesisConfig as MockGenesisConfig, }; + use frame_support::{assert_err, traits::Currency as _}; use primitives::v1::BlockNumber; use std::collections::{BTreeMap, HashSet}; fn run_to_block(to: BlockNumber, new_session: Option>) { use frame_support::traits::{OnFinalize as _, OnInitialize as _}; + let config = Configuration::config(); while System::block_number() < to { let b = System::block_number(); @@ -1102,9 +1141,15 @@ mod tests { Paras::initializer_finalize(); if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) { + let notification = crate::initializer::SessionChangeNotification { + prev_config: config.clone(), + new_config: config.clone(), + ..Default::default() + }; + // NOTE: this is in initialization order. - Paras::initializer_on_new_session(&Default::default()); - Hrmp::initializer_on_new_session(&Default::default()); + Paras::initializer_on_new_session(¬ification); + Hrmp::initializer_on_new_session(¬ification); } System::on_finalize(b); @@ -1118,6 +1163,7 @@ mod tests { } } + #[derive(Debug)] struct GenesisConfigBuilder { hrmp_channel_max_capacity: u32, hrmp_channel_max_message_size: u32, @@ -1127,6 +1173,9 @@ mod tests { hrmp_max_parachain_inbound_channels: u32, hrmp_max_message_num_per_candidate: u32, hrmp_channel_max_total_size: u32, + hrmp_sender_deposit: Balance, + hrmp_recipient_deposit: Balance, + hrmp_open_request_ttl: u32, } impl Default for GenesisConfigBuilder { @@ -1140,6 +1189,9 @@ mod tests { hrmp_max_parachain_inbound_channels: 2, hrmp_max_message_num_per_candidate: 2, hrmp_channel_max_total_size: 16, + hrmp_sender_deposit: 100, + hrmp_recipient_deposit: 100, + hrmp_open_request_ttl: 3, } } } @@ -1157,6 +1209,9 @@ mod tests { config.hrmp_max_parachain_inbound_channels = self.hrmp_max_parachain_inbound_channels; config.hrmp_max_message_num_per_candidate = self.hrmp_max_message_num_per_candidate; config.hrmp_channel_max_total_size = self.hrmp_channel_max_total_size; + config.hrmp_sender_deposit = self.hrmp_sender_deposit; + config.hrmp_recipient_deposit = self.hrmp_recipient_deposit; + config.hrmp_open_request_ttl = self.hrmp_open_request_ttl; genesis } } @@ -1173,7 +1228,7 @@ mod tests { } } - fn register_parachain(id: ParaId) { + fn register_parachain_with_balance(id: ParaId, balance: Balance) { Paras::schedule_para_initialize( id, crate::paras::ParaGenesisArgs { @@ -1182,10 +1237,16 @@ mod tests { validation_code: vec![1].into(), }, ); + ::Currency::make_free_balance_be(&id.into_account(), balance); + } + + fn register_parachain(id: ParaId) { + register_parachain_with_balance(id, 1000); } fn deregister_parachain(id: ParaId) { Paras::schedule_para_cleanup(id); + Hrmp::schedule_para_cleanup(id); } fn channel_exists(sender: ParaId, recipient: ParaId) -> bool { @@ -1622,4 +1683,167 @@ mod tests { ); }); } + + #[test] + fn charging_deposits() { + let para_a = 32.into(); + let para_b = 64.into(); + + new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { + register_parachain_with_balance(para_a, 0); + register_parachain(para_b); + run_to_block(5, Some(vec![5])); + + assert_err!( + Hrmp::init_open_channel(para_a, para_b, 2, 8), + pallet_balances::Error::::InsufficientBalance + ); + }); + + new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { + register_parachain(para_a); + register_parachain_with_balance(para_b, 0); + run_to_block(5, Some(vec![5])); + + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + + assert_err!( + Hrmp::accept_open_channel(para_b, para_a), + pallet_balances::Error::::InsufficientBalance + ); + }); + } + + #[test] + fn refund_deposit_on_normal_closure() { + let para_a = 32.into(); + let para_b = 64.into(); + + let mut genesis = GenesisConfigBuilder::default(); + genesis.hrmp_sender_deposit = 20; + genesis.hrmp_recipient_deposit = 15; + new_test_ext(genesis.build()).execute_with(|| { + // Register two parachains funded with different amounts of funds and arrange a channel. + register_parachain_with_balance(para_a, 100); + register_parachain_with_balance(para_b, 110); + run_to_block(5, Some(vec![5])); + 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()), + 80 + ); + assert_eq!( + ::Currency::free_balance(¶_b.into_account()), + 95 + ); + run_to_block(8, Some(vec![8])); + + // Now, we close the channel and wait until the next session. + Hrmp::close_channel( + para_b, + HrmpChannelId { + sender: para_a, + recipient: para_b, + }, + ) + .unwrap(); + run_to_block(10, Some(vec![10])); + assert_eq!( + ::Currency::free_balance(¶_a.into_account()), + 100 + ); + assert_eq!( + ::Currency::free_balance(¶_b.into_account()), + 110 + ); + }); + } + + #[test] + fn refund_deposit_on_request_expiry() { + let para_a = 32.into(); + let para_b = 64.into(); + + let mut genesis = GenesisConfigBuilder::default(); + genesis.hrmp_sender_deposit = 20; + genesis.hrmp_recipient_deposit = 15; + genesis.hrmp_open_request_ttl = 2; + new_test_ext(genesis.build()).execute_with(|| { + // Register two parachains funded with different amounts of funds, send an open channel + // request but do not accept it. + register_parachain_with_balance(para_a, 100); + register_parachain_with_balance(para_b, 110); + run_to_block(5, Some(vec![5])); + Hrmp::init_open_channel(para_a, para_b, 2, 8).unwrap(); + assert_eq!( + ::Currency::free_balance(¶_a.into_account()), + 80 + ); + assert_eq!( + ::Currency::free_balance(¶_b.into_account()), + 110 + ); + + // Request age is 1 out of 2 + run_to_block(10, Some(vec![10])); + assert_eq!( + ::Currency::free_balance(¶_a.into_account()), + 80 + ); + + // Request age is 2 out of 2. The request should expire. + run_to_block(20, Some(vec![20])); + assert_eq!( + ::Currency::free_balance(¶_a.into_account()), + 100 + ); + }); + } + + #[test] + fn refund_deposit_on_offboarding() { + let para_a = 32.into(); + let para_b = 64.into(); + + let mut genesis = GenesisConfigBuilder::default(); + genesis.hrmp_sender_deposit = 20; + genesis.hrmp_recipient_deposit = 15; + new_test_ext(genesis.build()).execute_with(|| { + // Register two parachains and open a channel between them. + register_parachain_with_balance(para_a, 100); + register_parachain_with_balance(para_b, 110); + run_to_block(5, Some(vec![5])); + 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()), + 80 + ); + assert_eq!( + ::Currency::free_balance(¶_b.into_account()), + 95 + ); + run_to_block(8, Some(vec![8])); + assert!(channel_exists(para_a, para_b)); + + // Then deregister one parachain. + deregister_parachain(para_a); + run_to_block(10, Some(vec![10])); + + // The channel should be removed. + assert!(!Paras::is_valid_para(para_a)); + assert!(!channel_exists(para_a, para_b)); + assert_storage_consistency_exhaustive(); + + assert_eq!( + ::Currency::free_balance(¶_a.into_account()), + 100 + ); + assert_eq!( + ::Currency::free_balance(¶_b.into_account()), + 110 + ); + }); + } } diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 7ae33c01b3..b219a70a2d 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -21,7 +21,7 @@ use sp_core::H256; use sp_runtime::traits::{ BlakeTwo256, IdentityLookup, }; -use primitives::v1::{AuthorityDiscoveryId, BlockNumber, Header, ValidatorIndex}; +use primitives::v1::{AuthorityDiscoveryId, Balance, BlockNumber, Header, ValidatorIndex}; use frame_support::{ impl_outer_origin, impl_outer_dispatch, impl_outer_event, parameter_types, traits::Randomness as RandomnessT, @@ -50,6 +50,7 @@ impl_outer_dispatch! { impl_outer_event! { pub enum TestEvent for Test { frame_system, + pallet_balances, inclusion, } } @@ -93,6 +94,20 @@ impl frame_system::Config for Test { type SS58Prefix = (); } +parameter_types! { + pub static ExistentialDeposit: u64 = 0; +} + +impl pallet_balances::Config for Test { + type MaxLocks = (); + type Balance = Balance; + type Event = TestEvent; + type DustRemoval = (); + type ExistentialDeposit = ExistentialDeposit; + type AccountStore = System; + type WeightInfo = (); +} + impl crate::initializer::Config for Test { type Randomness = TestRandomness; } @@ -111,6 +126,7 @@ impl crate::ump::Config for Test { impl crate::hrmp::Config for Test { type Origin = Origin; + type Currency = pallet_balances::Module; } impl crate::scheduler::Config for Test { } diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index d03c7bca4d..6ae369c307 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -562,6 +562,7 @@ impl parachains_dmp::Config for Runtime {} impl parachains_hrmp::Config for Runtime { type Origin = Origin; + type Currency = Balances; } impl parachains_inclusion_inherent::Config for Runtime {} diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index 58b0df5dff..30545a3da9 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/polkadot/runtime/test-runtime/src/lib.rs @@ -473,6 +473,7 @@ impl parachains_ump::Config for Runtime { impl parachains_hrmp::Config for Runtime { type Origin = Origin; + type Currency = Balances; } impl parachains_scheduler::Config for Runtime {}