From b477394ce6f6cd7859718c4c86da21c6792e2c07 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 24 Jul 2023 20:32:13 +0300 Subject: [PATCH 01/13] Introduce bags-list pallet to collator selection Signed-off-by: georgepisaltu --- Cargo.lock | 2 + pallets/collator-selection/Cargo.toml | 3 + pallets/collator-selection/src/lib.rs | 94 ++++++++++++++++++++++++--- parachain-template/runtime/Cargo.toml | 4 ++ parachain-template/runtime/src/lib.rs | 19 ++++++ 5 files changed, 113 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d9223cabd7f..a806d49b9d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7200,6 +7200,7 @@ name = "pallet-collator-selection" version = "3.0.0" dependencies = [ "frame-benchmarking", + "frame-election-provider-support", "frame-support", "frame-system", "log", @@ -8310,6 +8311,7 @@ dependencies = [ "log", "pallet-aura", "pallet-authorship", + "pallet-bags-list", "pallet-balances", "pallet-collator-selection", "pallet-parachain-template", diff --git a/pallets/collator-selection/Cargo.toml b/pallets/collator-selection/Cargo.toml index dbb7f452162..6160c14b970 100644 --- a/pallets/collator-selection/Cargo.toml +++ b/pallets/collator-selection/Cargo.toml @@ -21,6 +21,7 @@ scale-info = { version = "2.9.0", default-features = false, features = ["derive" sp-std = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } sp-runtime = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } sp-staking = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } +frame-election-provider-support = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } frame-support = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } frame-system = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } pallet-authorship = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } @@ -42,6 +43,7 @@ pallet-aura = { git = "https://github.com/paritytech/substrate", branch = "maste default = ["std"] runtime-benchmarks = [ "frame-benchmarking/runtime-benchmarks", + "frame-election-provider-support/runtime-benchmarks", "frame-support/runtime-benchmarks", "frame-system/runtime-benchmarks", ] @@ -53,6 +55,7 @@ std = [ "sp-runtime/std", "sp-staking/std", "sp-std/std", + "frame-election-provider-support/std", "frame-support/std", "frame-system/std", "frame-benchmarking/std", diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 539a4d8bd95..ce424f515d7 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -80,6 +80,7 @@ const LOG_TARGET: &str = "runtime::collator-selection"; pub mod pallet { pub use crate::weights::WeightInfo; use core::ops::Div; + use frame_election_provider_support::{ScoreProvider, SortedListProvider}; use frame_support::{ dispatch::{DispatchClass, DispatchResultWithPostInfo}, pallet_prelude::*, @@ -142,6 +143,8 @@ pub mod pallet { /// Maximum number of invulnerables. type MaxInvulnerables: Get; + type CandidateList: SortedListProvider>; + // Will be kicked if block is not produced in threshold. type KickThreshold: Get>; @@ -204,6 +207,10 @@ pub mod pallet { #[pallet::getter(fn desired_candidates)] pub type DesiredCandidates = StorageValue<_, u32, ValueQuery>; + // #[pallet::storage] + // #[pallet::getter(fn minimum_deposit)] + // pub type MinimumDeposit = StorageValue<_, BalanceOf, ValueQuery>; + /// Fixed amount to deposit to become a collator. /// /// When a collator calls `leave_intent` they immediately receive the deposit back. @@ -289,6 +296,12 @@ pub mod pallet { NoAssociatedValidatorId, /// Validator ID is not yet registered. ValidatorNotRegistered, + /// Some doc. + OnInsert, + /// Some doc. + OnRemove, + /// Some doc. + OnIncrease, } #[pallet::hooks] @@ -451,6 +464,8 @@ pub mod pallet { who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), ); + T::CandidateList::on_insert(who.clone(), deposit) + .map_err(|_| Error::::OnInsert)?; Ok(candidates.len()) } })?; @@ -555,6 +570,28 @@ pub mod pallet { Self::deposit_event(Event::InvulnerableRemoved { account_id: who }); Ok(()) } + + /// Todo + /// + /// Todo + #[pallet::call_index(7)] + #[pallet::weight(T::WeightInfo::set_candidacy_bond())] + pub fn increase_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { + let who = ensure_signed(origin)?; + + >::try_mutate(|candidates| -> DispatchResult { + let candidate_info = candidates + .iter_mut() + .find(|candidate_info| candidate_info.who == who) + .ok_or_else(|| Error::::NotCandidate)?; + candidate_info.deposit.saturating_add(bond); + T::CandidateList::on_increase(&who, bond).map_err(|_| Error::::OnIncrease)?; + Ok(()) + })?; + + Self::deposit_event(Event::InvulnerableRemoved { account_id: who }); + Ok(()) + } } impl Pallet { @@ -584,6 +621,7 @@ pub mod pallet { .ok_or(Error::::NotCandidate)?; let candidate = candidates.remove(index); T::Currency::unreserve(who, candidate.deposit); + T::CandidateList::on_remove(&who).map_err(|_| Error::::OnRemove)?; if remove_last_authored { >::remove(who.clone()) }; @@ -596,11 +634,12 @@ pub mod pallet { /// Assemble the current set of candidates and invulnerables into the next collator set. /// /// This is done on the fly, as frequent as we are told to do so, as the session manager. - pub fn assemble_collators( - candidates: BoundedVec, - ) -> Vec { + pub fn assemble_collators() -> Vec { + // TODO[GMP] revisit + let desired_candidates: usize = + >::get().try_into().unwrap_or(usize::MAX); let mut collators = Self::invulnerables().to_vec(); - collators.extend(candidates); + collators.extend(T::CandidateList::iter().take(desired_candidates as usize)); collators } @@ -608,7 +647,7 @@ pub mod pallet { /// their deposits. pub fn kick_stale_candidates( candidates: BoundedVec>, T::MaxCandidates>, - ) -> BoundedVec { + ) -> usize { let now = frame_system::Pallet::::block_number(); let kick_threshold = T::KickThreshold::get(); let min_collators = T::MinEligibleCollators::get(); @@ -639,7 +678,7 @@ pub mod pallet { } } }) - .collect::>() + .count() .try_into() .expect("filter_map operation can't result in a bounded vec larger than its original; qed") } @@ -680,9 +719,9 @@ pub mod pallet { let candidates = Self::candidates(); let candidates_len_before = candidates.len(); - let active_candidates = Self::kick_stale_candidates(candidates); - let removed = candidates_len_before - active_candidates.len(); - let result = Self::assemble_collators(active_candidates); + let active_candidates_count = Self::kick_stale_candidates(candidates); + let removed = candidates_len_before.saturating_sub(active_candidates_count); + let result = Self::assemble_collators(); frame_system::Pallet::::register_extra_weight_unchecked( T::WeightInfo::new_session(candidates_len_before as u32, removed as u32), @@ -697,4 +736,41 @@ pub mod pallet { // we don't care. } } + + impl ScoreProvider for Pallet { + type Score = BalanceOf; + + fn score(who: &T::AccountId) -> Self::Score { + let candidates = Self::candidates(); + candidates + .iter() + .find(|&candidate_info| candidate_info.who == who.clone()) + .map(|candidate_info| candidate_info.deposit) + .unwrap_or_default() + } + + #[cfg(feature = "runtime-benchmarks")] + fn set_score_of(who: &T::AccountId, weight: Self::Score) { + // // this will clearly results in an inconsistent state, but it should not matter for a + // // benchmark. + // let active: BalanceOf = weight.try_into().map_err(|_| ()).unwrap(); + // let mut ledger = match Self::ledger(who) { + // None => StakingLedger::default_from(who.clone()), + // Some(l) => l, + // }; + // ledger.active = active; + + // >::insert(who, ledger); + // >::insert(who, who); + + // // also, we play a trick to make sure that a issuance based-`CurrencyToVote` behaves well: + // // This will make sure that total issuance is zero, thus the currency to vote will be a 1-1 + // // conversion. + // let imbalance = T::Currency::burn(T::Currency::total_issuance()); + // // kinda ugly, but gets the job done. The fact that this works here is a HUGE exception. + // // Don't try this pattern in other places. + // sp_std::mem::forget(imbalance); + todo!(); + } + } } diff --git a/parachain-template/runtime/Cargo.toml b/parachain-template/runtime/Cargo.toml index 59bd61124a2..68456a7ce35 100644 --- a/parachain-template/runtime/Cargo.toml +++ b/parachain-template/runtime/Cargo.toml @@ -34,6 +34,7 @@ frame-system-rpc-runtime-api = { git = "https://github.com/paritytech/substrate" frame-try-runtime = { git = "https://github.com/paritytech/substrate", default-features = false, optional = true, branch = "master" } pallet-aura = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-authorship = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } +pallet-bags-list = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-balances = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-session = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-sudo = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } @@ -95,6 +96,7 @@ std = [ "frame-system/std", "pallet-aura/std", "pallet-authorship/std", + "pallet-bags-list/std", "pallet-balances/std", "pallet-collator-selection/std", "pallet-session/std", @@ -131,6 +133,7 @@ runtime-benchmarks = [ "frame-system-benchmarking/runtime-benchmarks", "frame-system/runtime-benchmarks", "pallet-balances/runtime-benchmarks", + "pallet-bags-list/runtime-benchmarks", "pallet-collator-selection/runtime-benchmarks", "pallet-parachain-template/runtime-benchmarks", "pallet-timestamp/runtime-benchmarks", @@ -155,6 +158,7 @@ try-runtime = [ "pallet-aura/try-runtime", "pallet-authorship/try-runtime", "pallet-balances/try-runtime", + "pallet-bags-list/try-runtime", "pallet-collator-selection/try-runtime", "pallet-session/try-runtime", "pallet-sudo/try-runtime", diff --git a/parachain-template/runtime/src/lib.rs b/parachain-template/runtime/src/lib.rs index 761a3944afb..ad35e60e31b 100644 --- a/parachain-template/runtime/src/lib.rs +++ b/parachain-template/runtime/src/lib.rs @@ -450,6 +450,7 @@ impl pallet_collator_selection::Config for Runtime { type MaxCandidates = ConstU32<100>; type MinEligibleCollators = ConstU32<4>; type MaxInvulnerables = ConstU32<20>; + type CandidateList = CandidateList; // should be a multiple of session or things will get inconsistent type KickThreshold = Period; type ValidatorId = ::AccountId; @@ -458,6 +459,23 @@ impl pallet_collator_selection::Config for Runtime { type WeightInfo = (); } +parameter_types! { + // TODO[GMP] come back to this + pub const BagThresholds: &'static [u128] = &[0]; +} + +type CandidateBagsListInstance = pallet_bags_list::Instance1; +impl pallet_bags_list::Config for Runtime { + type RuntimeEvent = RuntimeEvent; + /// The voter bags-list is loosely kept up to date, and the real source of truth for the score + /// of each node is the staking pallet. + type ScoreProvider = CollatorSelection; + type BagThresholds = BagThresholds; + // TODO[GMP] come back to this + type Score = Balance; + type WeightInfo = pallet_bags_list::weights::SubstrateWeight; +} + /// Configure the pallet template in pallets/template. impl pallet_parachain_template::Config for Runtime { type RuntimeEvent = RuntimeEvent; @@ -486,6 +504,7 @@ construct_runtime!( Session: pallet_session = 22, Aura: pallet_aura = 23, AuraExt: cumulus_pallet_aura_ext = 24, + CandidateList: pallet_bags_list:: = 25, // XCM helpers. XcmpQueue: cumulus_pallet_xcmp_queue = 30, From a218cda049582eb5b53ed5dbbee39aa0e8d96a0c Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 25 Jul 2023 18:33:52 +0300 Subject: [PATCH 02/13] Add increase/decrease bond and benches Signed-off-by: georgepisaltu --- Cargo.lock | 3 + pallets/collator-selection/Cargo.toml | 1 + .../collator-selection/src/benchmarking.rs | 64 ++++++++++++++++++- pallets/collator-selection/src/lib.rs | 57 ++++++++++++++++- pallets/collator-selection/src/mock.rs | 15 +++++ pallets/collator-selection/src/weights.rs | 30 +++++++++ parachain-template/runtime/Cargo.toml | 9 +++ 7 files changed, 175 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a806d49b9d8..e9fdbda5522 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7206,6 +7206,7 @@ dependencies = [ "log", "pallet-aura", "pallet-authorship", + "pallet-bags-list", "pallet-balances", "pallet-session", "pallet-timestamp", @@ -8301,6 +8302,7 @@ dependencies = [ "cumulus-primitives-timestamp", "cumulus-primitives-utility", "frame-benchmarking", + "frame-election-provider-support", "frame-executive", "frame-support", "frame-system", @@ -8316,6 +8318,7 @@ dependencies = [ "pallet-collator-selection", "pallet-parachain-template", "pallet-session", + "pallet-staking", "pallet-sudo", "pallet-timestamp", "pallet-transaction-payment", diff --git a/pallets/collator-selection/Cargo.toml b/pallets/collator-selection/Cargo.toml index 6160c14b970..b3a49854950 100644 --- a/pallets/collator-selection/Cargo.toml +++ b/pallets/collator-selection/Cargo.toml @@ -36,6 +36,7 @@ sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master" } pallet-timestamp = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-consensus-aura = { git = "https://github.com/paritytech/substrate", branch = "master" } +pallet-bags-list = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "master" } pallet-aura = { git = "https://github.com/paritytech/substrate", branch = "master" } diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index 5fc92f8a783..704fd08174b 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -94,7 +94,8 @@ fn register_candidates(count: u32) { assert!(>::get() > 0u32.into(), "Bond cannot be zero!"); for who in candidates { - T::Currency::make_free_balance_be(&who, >::get() * 2u32.into()); + // TODO[GMP] revisit this, need it for Currency reserve in increase_bid + T::Currency::make_free_balance_be(&who, >::get() * 3u32.into()); >::register_as_candidate(RawOrigin::Signed(who).into()).unwrap(); } } @@ -238,6 +239,67 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn increase_bond(c: Linear<1, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + >::put(T::Currency::minimum_balance()); + >::put(c); + + register_candidates::(c); + + let caller = >::get().last().unwrap().who.clone(); + v2::whitelist!(caller); + + let bond_amount: BalanceOf = T::Currency::minimum_balance(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), bond_amount); + + assert_last_event::( + Event::CandidateBondIncreased { + account_id: caller, + deposit: T::Currency::minimum_balance() + bond_amount, + } + .into(), + ); + assert!( + >::get().last().unwrap().deposit == + T::Currency::minimum_balance() * 2u32.into() + ); + Ok(()) + } + + #[benchmark] + fn decrease_bond(c: Linear<1, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + >::put(T::Currency::minimum_balance()); + >::put(c); + + register_candidates::(c); + + let caller = >::get().last().unwrap().who.clone(); + v2::whitelist!(caller); + + >::increase_bond( + RawOrigin::Signed(caller.clone()).into(), + >::get(), + ) + .unwrap(); + + let bond_amount: BalanceOf = T::Currency::minimum_balance(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), bond_amount); + + assert_last_event::( + Event::CandidateBondDecreased { + account_id: caller, + deposit: T::Currency::minimum_balance(), + } + .into(), + ); + assert!(>::get().last().unwrap().deposit == T::Currency::minimum_balance()); + Ok(()) + } + // worse case is when we have all the max-candidate slots filled except one, and we fill that // one. #[benchmark] diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index ce424f515d7..843eccdab8d 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -269,6 +269,10 @@ pub mod pallet { NewCandidacyBond { bond_amount: BalanceOf }, /// A new candidate joined. CandidateAdded { account_id: T::AccountId, deposit: BalanceOf }, + /// A new candidate joined. + CandidateBondIncreased { account_id: T::AccountId, deposit: BalanceOf }, + /// A new candidate joined. + CandidateBondDecreased { account_id: T::AccountId, deposit: BalanceOf }, /// A candidate was removed. CandidateRemoved { account_id: T::AccountId }, /// An account was unable to be added to the Invulnerables because they did not have keys @@ -301,6 +305,8 @@ pub mod pallet { /// Some doc. OnRemove, /// Some doc. + OnRemoveInsufficientFunds, + /// Some doc. OnIncrease, } @@ -575,7 +581,7 @@ pub mod pallet { /// /// Todo #[pallet::call_index(7)] - #[pallet::weight(T::WeightInfo::set_candidacy_bond())] + #[pallet::weight(T::WeightInfo::increase_bond(T::MaxCandidates::get()))] pub fn increase_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; @@ -584,12 +590,57 @@ pub mod pallet { .iter_mut() .find(|candidate_info| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit.saturating_add(bond); + candidate_info.deposit = candidate_info.deposit.saturating_add(bond); + T::Currency::reserve(&who, bond)?; T::CandidateList::on_increase(&who, bond).map_err(|_| Error::::OnIncrease)?; Ok(()) })?; - Self::deposit_event(Event::InvulnerableRemoved { account_id: who }); + let new_bond = >::get() + .iter() + .find(|candidate_info| candidate_info.who == who) + .map(|candidate_info| candidate_info.deposit) + .unwrap(); + + Self::deposit_event(Event::CandidateBondIncreased { + account_id: who, + deposit: new_bond, + }); + Ok(()) + } + + /// Todo + /// + /// Todo + #[pallet::call_index(8)] + #[pallet::weight(T::WeightInfo::decrease_bond(T::MaxCandidates::get()))] + pub fn decrease_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { + let who = ensure_signed(origin)?; + + >::try_mutate(|candidates| -> DispatchResult { + let candidate_info = candidates + .iter_mut() + .find(|candidate_info| candidate_info.who == who) + .ok_or_else(|| Error::::NotCandidate)?; + candidate_info.deposit = candidate_info.deposit.saturating_sub(bond); + if candidate_info.deposit < >::get() { + return Err(Error::::OnRemoveInsufficientFunds.into()) + } + T::Currency::unreserve(&who, bond); + T::CandidateList::on_decrease(&who, bond).map_err(|_| Error::::OnIncrease)?; + Ok(()) + })?; + + let new_bond = >::get() + .iter() + .find(|candidate_info| candidate_info.who == who) + .map(|candidate_info| candidate_info.deposit) + .unwrap(); + + Self::deposit_event(Event::CandidateBondDecreased { + account_id: who, + deposit: new_bond, + }); Ok(()) } } diff --git a/pallets/collator-selection/src/mock.rs b/pallets/collator-selection/src/mock.rs index 7e8b1595d2c..94aa785b9bb 100644 --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -40,6 +40,7 @@ frame_support::construct_runtime!( Session: pallet_session, Aura: pallet_aura, Balances: pallet_balances, + CandidateList: pallet_bags_list::, CollatorSelection: collator_selection, Authorship: pallet_authorship, } @@ -177,12 +178,25 @@ impl pallet_session::Config for Test { type WeightInfo = (); } +type CandidateBagsListInstance = pallet_bags_list::Instance1; +impl pallet_bags_list::Config for Test { + type RuntimeEvent = RuntimeEvent; + /// The voter bags-list is loosely kept up to date, and the real source of truth for the score + /// of each node is the staking pallet. + type ScoreProvider = CollatorSelection; + type BagThresholds = BagThresholds; + // TODO[GMP] come back to this + type Score = u64; + type WeightInfo = pallet_bags_list::weights::SubstrateWeight; +} + ord_parameter_types! { pub const RootAccount: u64 = 777; } parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); + pub const BagThresholds: &'static [u64] = &[0]; } pub struct IsRegistered; @@ -201,6 +215,7 @@ impl Config for Test { type MinEligibleCollators = ConstU32<1>; type MaxInvulnerables = ConstU32<20>; type KickThreshold = Period; + type CandidateList = CandidateList; type ValidatorId = ::AccountId; type ValidatorIdOf = IdentityCollator; type ValidatorRegistration = IsRegistered; diff --git a/pallets/collator-selection/src/weights.rs b/pallets/collator-selection/src/weights.rs index 7d227da291a..67b8072fa42 100644 --- a/pallets/collator-selection/src/weights.rs +++ b/pallets/collator-selection/src/weights.rs @@ -33,6 +33,8 @@ pub trait WeightInfo { fn set_candidacy_bond() -> Weight; fn register_as_candidate(_c: u32) -> Weight; fn leave_intent(_c: u32) -> Weight; + fn increase_bond(_c: u32) -> Weight; + fn decrease_bond(_c: u32) -> Weight; fn note_author() -> Weight; fn new_session(_c: u32, _r: u32) -> Weight; } @@ -66,6 +68,20 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } + fn increase_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } + fn decrease_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } fn note_author() -> Weight { Weight::from_parts(71_461_000_u64, 0) .saturating_add(T::DbWeight::get().reads(3_u64)) @@ -153,6 +169,20 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } + fn increase_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) + } + fn decrease_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(1_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) + } fn note_author() -> Weight { Weight::from_parts(71_461_000_u64, 0) .saturating_add(RocksDbWeight::get().reads(3_u64)) diff --git a/parachain-template/runtime/Cargo.toml b/parachain-template/runtime/Cargo.toml index 68456a7ce35..311df7ae0e0 100644 --- a/parachain-template/runtime/Cargo.toml +++ b/parachain-template/runtime/Cargo.toml @@ -26,6 +26,8 @@ pallet-parachain-template = { path = "../pallets/template", default-features = f # Substrate frame-benchmarking = { git = "https://github.com/paritytech/substrate", default-features = false, optional = true, branch = "master" } +# TODO[GMP] maybe unnecessary +frame-election-provider-support = { default-features = false, git = "https://github.com/paritytech/substrate", branch = "master" } frame-executive = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } frame-support = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } frame-system = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } @@ -38,6 +40,7 @@ pallet-bags-list = { git = "https://github.com/paritytech/substrate", default-fe pallet-balances = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-session = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-sudo = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } +pallet-staking = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-timestamp = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-transaction-payment = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } pallet-transaction-payment-rpc-runtime-api = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } @@ -91,6 +94,7 @@ std = [ "cumulus-primitives-timestamp/std", "cumulus-primitives-utility/std", "frame-executive/std", + "frame-election-provider-support/std", "frame-support/std", "frame-system-rpc-runtime-api/std", "frame-system/std", @@ -101,6 +105,7 @@ std = [ "pallet-collator-selection/std", "pallet-session/std", "pallet-sudo/std", + "pallet-staking/std", "pallet-parachain-template/std", "pallet-timestamp/std", "pallet-transaction-payment-rpc-runtime-api/std", @@ -130,6 +135,7 @@ runtime-benchmarks = [ "hex-literal", "frame-benchmarking/runtime-benchmarks", "frame-support/runtime-benchmarks", + "frame-election-provider-support/runtime-benchmarks", "frame-system-benchmarking/runtime-benchmarks", "frame-system/runtime-benchmarks", "pallet-balances/runtime-benchmarks", @@ -138,6 +144,7 @@ runtime-benchmarks = [ "pallet-parachain-template/runtime-benchmarks", "pallet-timestamp/runtime-benchmarks", "pallet-sudo/runtime-benchmarks", + "pallet-staking/runtime-benchmarks", "pallet-xcm/runtime-benchmarks", "sp-runtime/runtime-benchmarks", "xcm-builder/runtime-benchmarks", @@ -152,6 +159,7 @@ try-runtime = [ "cumulus-pallet-parachain-system/try-runtime", "cumulus-pallet-xcm/try-runtime", "cumulus-pallet-xcmp-queue/try-runtime", + "frame-election-provider-support/try-runtime", "frame-executive/try-runtime", "frame-system/try-runtime", "frame-try-runtime/try-runtime", @@ -162,6 +170,7 @@ try-runtime = [ "pallet-collator-selection/try-runtime", "pallet-session/try-runtime", "pallet-sudo/try-runtime", + "pallet-staking/try-runtime", "pallet-parachain-template/try-runtime", "pallet-timestamp/try-runtime", "pallet-transaction-payment/try-runtime", From bd4064ff6eb2c70a23f5a57383862110d6ae0bfb Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Wed, 26 Jul 2023 23:50:10 +0300 Subject: [PATCH 03/13] UT and some bench fixes Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 10 +- pallets/collator-selection/src/lib.rs | 2 +- pallets/collator-selection/src/mock.rs | 2 +- pallets/collator-selection/src/tests.rs | 228 ++++++++++++++++++ 4 files changed, 238 insertions(+), 4 deletions(-) diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index 704fd08174b..d597e763e0f 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -240,10 +240,13 @@ mod benchmarks { } #[benchmark] - fn increase_bond(c: Linear<1, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + fn increase_bond( + c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>, + ) -> Result<(), BenchmarkError> { >::put(T::Currency::minimum_balance()); >::put(c); + register_validators::(c); register_candidates::(c); let caller = >::get().last().unwrap().who.clone(); @@ -269,10 +272,13 @@ mod benchmarks { } #[benchmark] - fn decrease_bond(c: Linear<1, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + fn decrease_bond( + c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>, + ) -> Result<(), BenchmarkError> { >::put(T::Currency::minimum_balance()); >::put(c); + register_validators::(c); register_candidates::(c); let caller = >::get().last().unwrap().who.clone(); diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 843eccdab8d..3dd6c1dd676 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -445,7 +445,7 @@ pub mod pallet { // ensure we are below limit. let length = >::decode_len().unwrap_or_default(); - ensure!((length as u32) < Self::desired_candidates(), Error::::TooManyCandidates); + ensure!((length as u32) < T::MaxCandidates::get(), Error::::TooManyCandidates); ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); let validator_key = T::ValidatorIdOf::convert(who.clone()) diff --git a/pallets/collator-selection/src/mock.rs b/pallets/collator-selection/src/mock.rs index 94aa785b9bb..a6d0b482410 100644 --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -196,7 +196,7 @@ ord_parameter_types! { parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); - pub const BagThresholds: &'static [u64] = &[0]; + pub const BagThresholds: &'static [u64] = &[]; } pub struct IsRegistered; diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index cbfbde743f0..766d650775a 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -19,6 +19,7 @@ use frame_support::{ assert_noop, assert_ok, traits::{Currency, OnInitialize}, }; +use pallet_bags_list::{Error as BagsListError, ListError}; use pallet_balances::Error as BalancesError; use sp_runtime::{testing::UintAuthorityId, traits::BadOrigin, BuildStorage}; @@ -283,6 +284,7 @@ fn set_candidacy_bond() { }); } +// TODO[GMP] fix this with MaxCandidates #[test] fn cannot_register_candidate_if_too_many() { new_test_ext().execute_with(|| { @@ -421,6 +423,232 @@ fn register_as_candidate_works() { }); } +#[test] +fn increase_candidacy_bond_non_candidate_account() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + + assert_noop!( + CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 10), + Error::::NotCandidate + ); + }); +} + +#[test] +fn increase_candidacy_bond_insufficient_balance() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take two endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_noop!( + CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 100), + BalancesError::::InsufficientBalance + ); + }); +} + +#[test] +fn increase_candidacy_bond_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 20)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 30)); + + assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(Balances::free_balance(3), 80); + assert_eq!(Balances::free_balance(4), 70); + assert_eq!(Balances::free_balance(5), 60); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 30)); + + assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(Balances::free_balance(3), 60); + assert_eq!(Balances::free_balance(4), 40); + assert_eq!(Balances::free_balance(5), 60); + }); +} + +#[test] +fn decrease_candidacy_bond_non_candidate_account() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + + assert_noop!( + CollatorSelection::decrease_bond(RuntimeOrigin::signed(5), 10), + Error::::NotCandidate + ); + }); +} + +#[test] +fn decrease_candidacy_bond_insufficient_funds() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take two endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 50)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 50)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 40); + assert_eq!(Balances::free_balance(5), 40); + + assert_noop!( + CollatorSelection::decrease_bond(RuntimeOrigin::signed(3), 100), + Error::::OnRemoveInsufficientFunds + ); + + assert_noop!( + CollatorSelection::decrease_bond(RuntimeOrigin::signed(4), 60), + Error::::OnRemoveInsufficientFunds + ); + + assert_noop!( + CollatorSelection::decrease_bond(RuntimeOrigin::signed(5), 51), + Error::::OnRemoveInsufficientFunds + ); + }); +} + +#[test] +fn decrease_candidacy_bond_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 20)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 30)); + + assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(Balances::free_balance(3), 80); + assert_eq!(Balances::free_balance(4), 70); + assert_eq!(Balances::free_balance(5), 60); + + assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(4), 10)); + + assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 80); + assert_eq!(Balances::free_balance(5), 60); + }); +} + +#[test] +fn candidate_list_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_noop!( + CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3), + BagsListError::::List(ListError::NotHeavier) + ); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 10)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 4)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3)); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(3), 5)); + + assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3)); + }); +} + #[test] fn leave_intent() { new_test_ext().execute_with(|| { From 54d8e58d5e95346625f1df54d6978e682a004321 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 27 Jul 2023 22:33:57 +0300 Subject: [PATCH 04/13] Work on session UTs Signed-off-by: georgepisaltu --- pallets/collator-selection/src/lib.rs | 4 +- pallets/collator-selection/src/tests.rs | 257 ++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 2 deletions(-) diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 3dd6c1dd676..35057b52587 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -690,7 +690,7 @@ pub mod pallet { let desired_candidates: usize = >::get().try_into().unwrap_or(usize::MAX); let mut collators = Self::invulnerables().to_vec(); - collators.extend(T::CandidateList::iter().take(desired_candidates as usize)); + collators.extend(T::CandidateList::iter().take(desired_candidates)); collators } @@ -797,7 +797,7 @@ pub mod pallet { .iter() .find(|&candidate_info| candidate_info.who == who.clone()) .map(|candidate_info| candidate_info.deposit) - .unwrap_or_default() + .unwrap_or_else(|| Zero::zero()) } #[cfg(feature = "runtime-benchmarks")] diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 766d650775a..178dd85618e 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -758,6 +758,263 @@ fn session_management_works() { }); } +#[test] +fn session_management_works_1() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 4]); + }); +} + +#[test] +fn session_management_works_2() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 4]); + }); +} + +#[test] +fn session_management_works_3() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5, 3]); + }); +} + +#[test] +fn session_management_works_4() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 4)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 5]); + }); +} + +#[test] +fn session_management_works_5() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3)); + + initialize_to_block(5); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 60)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(4), 5)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 60)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(3), 5)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 4, 3]); + }); +} + +#[test] +fn session_management_works_6() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 3)); + + initialize_to_block(5); + + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 60)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(4), 5)); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 60)); + assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(3), 5)); + + initialize_to_block(5); + + assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(5), 50)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(CollatorSelection::candidates().len(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 4, 3]); + }); +} + #[test] fn kick_mechanism() { new_test_ext().execute_with(|| { From 637112b573960dfd67bc470095bf3febc1fbb26a Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Fri, 28 Jul 2023 18:21:41 +0300 Subject: [PATCH 05/13] Benchmarks and some refactoring Signed-off-by: georgepisaltu --- pallets/collator-selection/src/lib.rs | 90 +++++++++++---------------- 1 file changed, 35 insertions(+), 55 deletions(-) diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 35057b52587..36faa04043e 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -585,22 +585,18 @@ pub mod pallet { pub fn increase_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; - >::try_mutate(|candidates| -> DispatchResult { - let candidate_info = candidates - .iter_mut() - .find(|candidate_info| candidate_info.who == who) - .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_add(bond); - T::Currency::reserve(&who, bond)?; - T::CandidateList::on_increase(&who, bond).map_err(|_| Error::::OnIncrease)?; - Ok(()) - })?; - - let new_bond = >::get() - .iter() - .find(|candidate_info| candidate_info.who == who) - .map(|candidate_info| candidate_info.deposit) - .unwrap(); + let new_bond = + >::try_mutate(|candidates| -> Result, DispatchError> { + let candidate_info = candidates + .iter_mut() + .find(|candidate_info| candidate_info.who == who) + .ok_or_else(|| Error::::NotCandidate)?; + candidate_info.deposit = candidate_info.deposit.saturating_add(bond); + T::Currency::reserve(&who, bond)?; + T::CandidateList::on_increase(&who, bond) + .map_err(|_| Error::::OnIncrease)?; + Ok(candidate_info.deposit) + })?; Self::deposit_event(Event::CandidateBondIncreased { account_id: who, @@ -617,25 +613,21 @@ pub mod pallet { pub fn decrease_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; - >::try_mutate(|candidates| -> DispatchResult { - let candidate_info = candidates - .iter_mut() - .find(|candidate_info| candidate_info.who == who) - .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_sub(bond); - if candidate_info.deposit < >::get() { - return Err(Error::::OnRemoveInsufficientFunds.into()) - } - T::Currency::unreserve(&who, bond); - T::CandidateList::on_decrease(&who, bond).map_err(|_| Error::::OnIncrease)?; - Ok(()) - })?; - - let new_bond = >::get() - .iter() - .find(|candidate_info| candidate_info.who == who) - .map(|candidate_info| candidate_info.deposit) - .unwrap(); + let new_bond = + >::try_mutate(|candidates| -> Result, DispatchError> { + let candidate_info = candidates + .iter_mut() + .find(|candidate_info| candidate_info.who == who) + .ok_or_else(|| Error::::NotCandidate)?; + candidate_info.deposit = candidate_info.deposit.saturating_sub(bond); + if candidate_info.deposit < >::get() { + return Err(Error::::OnRemoveInsufficientFunds.into()) + } + T::Currency::unreserve(&who, bond); + T::CandidateList::on_decrease(&who, bond) + .map_err(|_| Error::::OnIncrease)?; + Ok(candidate_info.deposit) + })?; Self::deposit_event(Event::CandidateBondDecreased { account_id: who, @@ -802,26 +794,14 @@ pub mod pallet { #[cfg(feature = "runtime-benchmarks")] fn set_score_of(who: &T::AccountId, weight: Self::Score) { - // // this will clearly results in an inconsistent state, but it should not matter for a - // // benchmark. - // let active: BalanceOf = weight.try_into().map_err(|_| ()).unwrap(); - // let mut ledger = match Self::ledger(who) { - // None => StakingLedger::default_from(who.clone()), - // Some(l) => l, - // }; - // ledger.active = active; - - // >::insert(who, ledger); - // >::insert(who, who); - - // // also, we play a trick to make sure that a issuance based-`CurrencyToVote` behaves well: - // // This will make sure that total issuance is zero, thus the currency to vote will be a 1-1 - // // conversion. - // let imbalance = T::Currency::burn(T::Currency::total_issuance()); - // // kinda ugly, but gets the job done. The fact that this works here is a HUGE exception. - // // Don't try this pattern in other places. - // sp_std::mem::forget(imbalance); - todo!(); + let active: BalanceOf = weight.try_into().map_err(|_| ()).unwrap(); + Candidates::::mutate(|candidates| { + if let Some(candidate) = + candidates.iter_mut().find(|candidate_info| candidate_info.who == who.clone()) + { + candidate.deposit = active; + } + }); } } } From 6fc4026617f604e7600cd65e12a6f7761718b9a5 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 1 Aug 2023 14:13:38 +0300 Subject: [PATCH 06/13] Add extrinsic for buying slot Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 29 +++++++++ pallets/collator-selection/src/lib.rs | 63 +++++++++++++++++++ pallets/collator-selection/src/weights.rs | 15 +++++ 3 files changed, 107 insertions(+) diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index d597e763e0f..d3f7edb0c89 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -335,6 +335,35 @@ mod benchmarks { ); } + #[benchmark] + fn buy_slot(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { + >::put(T::Currency::minimum_balance()); + >::put(1); + + register_validators::(c); + register_candidates::(c); + + let caller: T::AccountId = whitelisted_caller(); + let bond: BalanceOf = T::Currency::minimum_balance() * 10u32.into(); + T::Currency::make_free_balance_be(&caller, bond); + + >::set_keys( + RawOrigin::Signed(caller.clone()).into(), + keys::(c + 1), + Vec::new(), + ) + .unwrap(); + + let target = >::get().last().unwrap().who.clone(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), bond / 2u32.into(), target); + + assert_last_event::( + Event::CandidateAdded { account_id: caller, deposit: bond / 2u32.into() }.into(), + ); + } + // worse case is the last candidate leaving. #[benchmark] fn leave_intent(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 36faa04043e..c38aae8b991 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -308,6 +308,8 @@ pub mod pallet { OnRemoveInsufficientFunds, /// Some doc. OnIncrease, + /// Some doc + InsufficientBond, } #[pallet::hooks] @@ -635,6 +637,67 @@ pub mod pallet { }); Ok(()) } + + #[pallet::call_index(9)] + #[pallet::weight(T::WeightInfo::buy_slot(T::MaxCandidates::get()))] + pub fn buy_slot( + origin: OriginFor, + deposit: BalanceOf, + target: T::AccountId, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + + // ensure we are below limit. + let length = >::decode_len().unwrap_or_default(); + ensure!((length as u32) <= T::MaxCandidates::get(), Error::::TooManyCandidates); + ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); + + let validator_key = T::ValidatorIdOf::convert(who.clone()) + .ok_or(Error::::NoAssociatedValidatorId)?; + ensure!( + T::ValidatorRegistration::is_registered(&validator_key), + Error::::ValidatorNotRegistered + ); + + ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); + + >::try_mutate(|candidates| -> Result<(), DispatchError> { + let mut kicked = false; + for candidate_info in candidates.iter_mut() { + if candidate_info.who == who { + Err(Error::::AlreadyCandidate)? + } + + if !kicked && candidate_info.who == target { + if deposit <= candidate_info.deposit { + Err(Error::::InsufficientBond)? + } + + T::Currency::unreserve(&candidate_info.who, candidate_info.deposit); + T::CandidateList::on_remove(&candidate_info.who) + .map_err(|_| Error::::OnRemove)?; + >::remove(candidate_info.who.clone()); + + kicked = true; + candidate_info.who = who.clone(); + candidate_info.deposit = deposit; + + T::Currency::reserve(&who, deposit)?; + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); + T::CandidateList::on_insert(who.clone(), deposit) + .map_err(|_| Error::::OnInsert)?; + } + } + Ok(()) + })?; + + Self::deposit_event(Event::CandidateRemoved { account_id: target }); + Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); + Ok(Some(T::WeightInfo::buy_slot(length as u32)).into()) + } } impl Pallet { diff --git a/pallets/collator-selection/src/weights.rs b/pallets/collator-selection/src/weights.rs index 67b8072fa42..ab10f5ee681 100644 --- a/pallets/collator-selection/src/weights.rs +++ b/pallets/collator-selection/src/weights.rs @@ -35,6 +35,7 @@ pub trait WeightInfo { fn leave_intent(_c: u32) -> Weight; fn increase_bond(_c: u32) -> Weight; fn decrease_bond(_c: u32) -> Weight; + fn buy_slot(_c: u32) -> Weight; fn note_author() -> Weight; fn new_session(_c: u32, _r: u32) -> Weight; } @@ -82,6 +83,13 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } + fn buy_slot(c: u32) -> Weight { + Weight::from_parts(71_196_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(T::DbWeight::get().reads(4_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } fn note_author() -> Weight { Weight::from_parts(71_461_000_u64, 0) .saturating_add(T::DbWeight::get().reads(3_u64)) @@ -183,6 +191,13 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } + fn buy_slot(c: u32) -> Weight { + Weight::from_parts(71_196_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(4_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) + } fn note_author() -> Weight { Weight::from_parts(71_461_000_u64, 0) .saturating_add(RocksDbWeight::get().reads(3_u64)) From 99556110097b60825de9f79c3135fe1b18c08369 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 1 Aug 2023 16:13:31 +0300 Subject: [PATCH 07/13] Add UT for buy_slot Signed-off-by: georgepisaltu --- pallets/collator-selection/src/tests.rs | 129 ++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 178dd85618e..0eace70b074 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -423,6 +423,135 @@ fn register_as_candidate_works() { }); } +#[test] +fn cannot_buy_slot_if_invulnerable() { + new_test_ext().execute_with(|| { + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // can't 1 because it is invulnerable. + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(1), 50u64.into(), 2), + Error::::AlreadyInvulnerable, + ); + }) +} + +#[test] +fn cannot_buy_slot_if_keys_not_registered() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(42), 50u64.into(), 3), + Error::::ValidatorNotRegistered + ); + }) +} + +#[test] +fn cannot_buy_slot_if_duplicate() { + new_test_ext().execute_with(|| { + // can add 3 as candidate + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 10 }; + assert_eq!(CollatorSelection::candidates(), vec![candidate_3, candidate_4]); + assert_eq!(CollatorSelection::last_authored_block(3), 10); + assert_eq!(CollatorSelection::last_authored_block(4), 10); + assert_eq!(Balances::free_balance(3), 90); + + // but no more + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(3), 50u64.into(), 4), + Error::::AlreadyCandidate, + ); + }) +} + +#[test] +fn cannot_buy_slot_if_poor() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(33), 0); + + // works + assert_ok!(CollatorSelection::buy_slot(RuntimeOrigin::signed(3), 20u64.into(), 4)); + + // poor + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(33), 30u64.into(), 3), + BalancesError::::InsufficientBalance, + ); + }); +} + +#[test] +fn cannot_buy_slot_if_insufficient_deposit() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 50u64.into())); + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(4), 5u64.into(), 3), + Error::::InsufficientBond, + ); + }); +} + +#[test] +fn cannot_buy_slot_if_deposit_less_than_target() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 50u64.into())); + assert_noop!( + CollatorSelection::buy_slot(RuntimeOrigin::signed(4), 20u64.into(), 3), + Error::::InsufficientBond, + ); + }); +} + +#[test] +fn buy_slot_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take two endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 100); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_eq!(CollatorSelection::candidates().len(), 3); + + Balances::make_free_balance_be(&6, 100); + let key = MockSessionKeys { aura: UintAuthorityId(6) }; + Session::set_keys(RuntimeOrigin::signed(6).into(), key, Vec::new()).unwrap(); + + assert_ok!(CollatorSelection::buy_slot(RuntimeOrigin::signed(6), 50u64.into(), 4)); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 90); + assert_eq!(Balances::free_balance(6), 50); + + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_6 = CandidateInfo { who: 6, deposit: 50 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 10 }; + assert_eq!(CollatorSelection::candidates(), vec![candidate_3, candidate_6, candidate_5]); + }); +} + #[test] fn increase_candidacy_bond_non_candidate_account() { new_test_ext().execute_with(|| { From 54dd1c5c13d40eb974a32a7ac1a4cdbce2ae7118 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 1 Aug 2023 19:31:42 +0300 Subject: [PATCH 08/13] Fix register candidate UT Signed-off-by: georgepisaltu --- pallets/collator-selection/src/tests.rs | 26 ++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 0eace70b074..ed2f6c983ed 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -284,26 +284,26 @@ fn set_candidacy_bond() { }); } -// TODO[GMP] fix this with MaxCandidates #[test] fn cannot_register_candidate_if_too_many() { new_test_ext().execute_with(|| { - // reset desired candidates: - >::put(0); + >::put(1); - // can't accept anyone anymore. - assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)), - Error::::TooManyCandidates, - ); + // MaxCandidates: u32 = 20 + // Aside from 3, 4, and 5, create enough accounts to have 21 potential + // candidates. + for i in 6..=23 { + Balances::make_free_balance_be(&i, 100); + let key = MockSessionKeys { aura: UintAuthorityId(i) }; + Session::set_keys(RuntimeOrigin::signed(i).into(), key, Vec::new()).unwrap(); + } - // reset desired candidates: - >::put(1); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + for c in 3..=22 { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(c))); + } - // but no more assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5)), + CollatorSelection::register_as_candidate(RuntimeOrigin::signed(23)), Error::::TooManyCandidates, ); }) From 5d928f5d2892df3364fb2e68334b1ef4ed1fe4a4 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 1 Aug 2023 19:51:36 +0300 Subject: [PATCH 09/13] Rename `buy_slot` to `take_candidate_slot` Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 2 +- pallets/collator-selection/src/lib.rs | 6 +-- pallets/collator-selection/src/tests.rs | 38 +++++++++++-------- pallets/collator-selection/src/weights.rs | 6 +-- 4 files changed, 30 insertions(+), 22 deletions(-) diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index d3f7edb0c89..ff28eeaa18e 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -336,7 +336,7 @@ mod benchmarks { } #[benchmark] - fn buy_slot(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { + fn take_candidate_slot(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { >::put(T::Currency::minimum_balance()); >::put(1); diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index c38aae8b991..766ff519dc1 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -639,8 +639,8 @@ pub mod pallet { } #[pallet::call_index(9)] - #[pallet::weight(T::WeightInfo::buy_slot(T::MaxCandidates::get()))] - pub fn buy_slot( + #[pallet::weight(T::WeightInfo::take_candidate_slot(T::MaxCandidates::get()))] + pub fn take_candidate_slot( origin: OriginFor, deposit: BalanceOf, target: T::AccountId, @@ -696,7 +696,7 @@ pub mod pallet { Self::deposit_event(Event::CandidateRemoved { account_id: target }); Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); - Ok(Some(T::WeightInfo::buy_slot(length as u32)).into()) + Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into()) } } diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index ed2f6c983ed..2bda501ecc7 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -424,31 +424,31 @@ fn register_as_candidate_works() { } #[test] -fn cannot_buy_slot_if_invulnerable() { +fn cannot_take_candidate_slot_if_invulnerable() { new_test_ext().execute_with(|| { assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // can't 1 because it is invulnerable. assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(1), 50u64.into(), 2), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(1), 50u64.into(), 2), Error::::AlreadyInvulnerable, ); }) } #[test] -fn cannot_buy_slot_if_keys_not_registered() { +fn cannot_take_candidate_slot_if_keys_not_registered() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(42), 50u64.into(), 3), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(42), 50u64.into(), 3), Error::::ValidatorNotRegistered ); }) } #[test] -fn cannot_buy_slot_if_duplicate() { +fn cannot_take_candidate_slot_if_duplicate() { new_test_ext().execute_with(|| { // can add 3 as candidate assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); @@ -462,56 +462,60 @@ fn cannot_buy_slot_if_duplicate() { // but no more assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(3), 50u64.into(), 4), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(3), 50u64.into(), 4), Error::::AlreadyCandidate, ); }) } #[test] -fn cannot_buy_slot_if_poor() { +fn cannot_take_candidate_slot_if_poor() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Balances::free_balance(33), 0); // works - assert_ok!(CollatorSelection::buy_slot(RuntimeOrigin::signed(3), 20u64.into(), 4)); + assert_ok!(CollatorSelection::take_candidate_slot( + RuntimeOrigin::signed(3), + 20u64.into(), + 4 + )); // poor assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(33), 30u64.into(), 3), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(33), 30u64.into(), 3), BalancesError::::InsufficientBalance, ); }); } #[test] -fn cannot_buy_slot_if_insufficient_deposit() { +fn cannot_take_candidate_slot_if_insufficient_deposit() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 50u64.into())); assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(4), 5u64.into(), 3), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 5u64.into(), 3), Error::::InsufficientBond, ); }); } #[test] -fn cannot_buy_slot_if_deposit_less_than_target() { +fn cannot_take_candidate_slot_if_deposit_less_than_target() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 50u64.into())); assert_noop!( - CollatorSelection::buy_slot(RuntimeOrigin::signed(4), 20u64.into(), 3), + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 20u64.into(), 3), Error::::InsufficientBond, ); }); } #[test] -fn buy_slot_works() { +fn take_candidate_slot_works() { new_test_ext().execute_with(|| { // given assert_eq!(CollatorSelection::desired_candidates(), 2); @@ -538,7 +542,11 @@ fn buy_slot_works() { let key = MockSessionKeys { aura: UintAuthorityId(6) }; Session::set_keys(RuntimeOrigin::signed(6).into(), key, Vec::new()).unwrap(); - assert_ok!(CollatorSelection::buy_slot(RuntimeOrigin::signed(6), 50u64.into(), 4)); + assert_ok!(CollatorSelection::take_candidate_slot( + RuntimeOrigin::signed(6), + 50u64.into(), + 4 + )); assert_eq!(Balances::free_balance(3), 90); assert_eq!(Balances::free_balance(4), 100); diff --git a/pallets/collator-selection/src/weights.rs b/pallets/collator-selection/src/weights.rs index ab10f5ee681..878906863ad 100644 --- a/pallets/collator-selection/src/weights.rs +++ b/pallets/collator-selection/src/weights.rs @@ -35,7 +35,7 @@ pub trait WeightInfo { fn leave_intent(_c: u32) -> Weight; fn increase_bond(_c: u32) -> Weight; fn decrease_bond(_c: u32) -> Weight; - fn buy_slot(_c: u32) -> Weight; + fn take_candidate_slot(_c: u32) -> Weight; fn note_author() -> Weight; fn new_session(_c: u32, _r: u32) -> Weight; } @@ -83,7 +83,7 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } - fn buy_slot(c: u32) -> Weight { + fn take_candidate_slot(c: u32) -> Weight { Weight::from_parts(71_196_000_u64, 0) // Standard Error: 0 .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) @@ -191,7 +191,7 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } - fn buy_slot(c: u32) -> Weight { + fn take_candidate_slot(c: u32) -> Weight { Weight::from_parts(71_196_000_u64, 0) // Standard Error: 0 .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) From ca2185542367d811f4465a762e42f1c369798751 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 7 Aug 2023 13:32:01 +0300 Subject: [PATCH 10/13] Renames and minor refactoring Signed-off-by: georgepisaltu --- pallets/collator-selection/src/lib.rs | 105 +++++++++++++----------- pallets/collator-selection/src/mock.rs | 5 +- pallets/collator-selection/src/tests.rs | 28 ++++--- parachain-template/runtime/src/lib.rs | 8 +- 4 files changed, 78 insertions(+), 68 deletions(-) diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 766ff519dc1..27996b388dc 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -207,10 +207,6 @@ pub mod pallet { #[pallet::getter(fn desired_candidates)] pub type DesiredCandidates = StorageValue<_, u32, ValueQuery>; - // #[pallet::storage] - // #[pallet::getter(fn minimum_deposit)] - // pub type MinimumDeposit = StorageValue<_, BalanceOf, ValueQuery>; - /// Fixed amount to deposit to become a collator. /// /// When a collator calls `leave_intent` they immediately receive the deposit back. @@ -269,9 +265,9 @@ pub mod pallet { NewCandidacyBond { bond_amount: BalanceOf }, /// A new candidate joined. CandidateAdded { account_id: T::AccountId, deposit: BalanceOf }, - /// A new candidate joined. + /// Bond of a candidate increased. CandidateBondIncreased { account_id: T::AccountId, deposit: BalanceOf }, - /// A new candidate joined. + /// Bond of a candidate decreased. CandidateBondDecreased { account_id: T::AccountId, deposit: BalanceOf }, /// A candidate was removed. CandidateRemoved { account_id: T::AccountId }, @@ -300,15 +296,16 @@ pub mod pallet { NoAssociatedValidatorId, /// Validator ID is not yet registered. ValidatorNotRegistered, - /// Some doc. - OnInsert, - /// Some doc. - OnRemove, - /// Some doc. - OnRemoveInsufficientFunds, - /// Some doc. - OnIncrease, - /// Some doc + /// Could not insert in the candidate list. + InsertToCandidateListFailed, + /// Could not remove from the candidate list. + RemoveFromCandidateListFailed, + /// New deposit amount would be below the minimum candidacy bond. + DepositTooLow, + /// Could not update the candidate list. + UpdateCandidateListFailed, + /// Deposit amount is too low to take the target's slot in the + /// candidate list. InsufficientBond, } @@ -473,7 +470,7 @@ pub mod pallet { frame_system::Pallet::::block_number() + T::KickThreshold::get(), ); T::CandidateList::on_insert(who.clone(), deposit) - .map_err(|_| Error::::OnInsert)?; + .map_err(|_| Error::::InsertToCandidateListFailed)?; Ok(candidates.len()) } })?; @@ -579,65 +576,78 @@ pub mod pallet { Ok(()) } - /// Todo + /// Increase the candidacy bond of collator candidate `origin` by an + /// `amount`. /// - /// Todo + /// This call will fail if `origin` is not a collator candidate and/or + /// the amount cannot be reserved. #[pallet::call_index(7)] #[pallet::weight(T::WeightInfo::increase_bond(T::MaxCandidates::get()))] - pub fn increase_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { + pub fn increase_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; - let new_bond = + let new_amount = >::try_mutate(|candidates| -> Result, DispatchError> { let candidate_info = candidates .iter_mut() .find(|candidate_info| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_add(bond); - T::Currency::reserve(&who, bond)?; - T::CandidateList::on_increase(&who, bond) - .map_err(|_| Error::::OnIncrease)?; + candidate_info.deposit = candidate_info.deposit.saturating_add(amount); + T::Currency::reserve(&who, amount)?; + T::CandidateList::on_increase(&who, amount) + .map_err(|_| Error::::UpdateCandidateListFailed)?; Ok(candidate_info.deposit) })?; Self::deposit_event(Event::CandidateBondIncreased { account_id: who, - deposit: new_bond, + deposit: new_amount, }); Ok(()) } - /// Todo + /// Decrease the candidacy bond of collator candidate `origin` by an + /// `amount`. /// - /// Todo + /// This call will fail if `origin` is not a collator candidate and/or + /// the remaining bond is lower than the minimum candidacy bond. #[pallet::call_index(8)] #[pallet::weight(T::WeightInfo::decrease_bond(T::MaxCandidates::get()))] - pub fn decrease_bond(origin: OriginFor, bond: BalanceOf) -> DispatchResult { + pub fn decrease_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; - let new_bond = + let new_amount = >::try_mutate(|candidates| -> Result, DispatchError> { let candidate_info = candidates .iter_mut() .find(|candidate_info| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_sub(bond); + candidate_info.deposit = candidate_info.deposit.saturating_sub(amount); if candidate_info.deposit < >::get() { - return Err(Error::::OnRemoveInsufficientFunds.into()) + return Err(Error::::DepositTooLow.into()) } - T::Currency::unreserve(&who, bond); - T::CandidateList::on_decrease(&who, bond) - .map_err(|_| Error::::OnIncrease)?; + T::Currency::unreserve(&who, amount); + T::CandidateList::on_decrease(&who, amount) + .map_err(|_| Error::::UpdateCandidateListFailed)?; Ok(candidate_info.deposit) })?; Self::deposit_event(Event::CandidateBondDecreased { account_id: who, - deposit: new_bond, + deposit: new_amount, }); Ok(()) } + /// The caller `origin` replaces a candidate `target` in the collator + /// candidate list by reserving `deposit`. The amount `deposit` + /// reserved by the caller must be greater than the existing bond of + /// the target it is trying to replace. + /// + /// This call will fail if the caller is already a collator candidate + /// or invulnerable, the caller does not have registered session keys, + /// the target is not a collator candidate, and/or the `deposit` amount + /// cannot be reserved. #[pallet::call_index(9)] #[pallet::weight(T::WeightInfo::take_candidate_slot(T::MaxCandidates::get()))] pub fn take_candidate_slot( @@ -647,10 +657,9 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - // ensure we are below limit. let length = >::decode_len().unwrap_or_default(); - ensure!((length as u32) <= T::MaxCandidates::get(), Error::::TooManyCandidates); ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); + ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); let validator_key = T::ValidatorIdOf::convert(who.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; @@ -659,36 +668,30 @@ pub mod pallet { Error::::ValidatorNotRegistered ); - ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); - >::try_mutate(|candidates| -> Result<(), DispatchError> { let mut kicked = false; for candidate_info in candidates.iter_mut() { - if candidate_info.who == who { - Err(Error::::AlreadyCandidate)? - } + ensure!(candidate_info.who != who, Error::::AlreadyCandidate); if !kicked && candidate_info.who == target { - if deposit <= candidate_info.deposit { - Err(Error::::InsufficientBond)? - } + ensure!(deposit > candidate_info.deposit, Error::::InsufficientBond); T::Currency::unreserve(&candidate_info.who, candidate_info.deposit); T::CandidateList::on_remove(&candidate_info.who) - .map_err(|_| Error::::OnRemove)?; + .map_err(|_| Error::::RemoveFromCandidateListFailed)?; >::remove(candidate_info.who.clone()); kicked = true; + T::Currency::reserve(&who, deposit)?; candidate_info.who = who.clone(); candidate_info.deposit = deposit; - T::Currency::reserve(&who, deposit)?; >::insert( who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), ); T::CandidateList::on_insert(who.clone(), deposit) - .map_err(|_| Error::::OnInsert)?; + .map_err(|_| Error::::InsertToCandidateListFailed)?; } } Ok(()) @@ -727,7 +730,8 @@ pub mod pallet { .ok_or(Error::::NotCandidate)?; let candidate = candidates.remove(index); T::Currency::unreserve(who, candidate.deposit); - T::CandidateList::on_remove(&who).map_err(|_| Error::::OnRemove)?; + T::CandidateList::on_remove(&who) + .map_err(|_| Error::::RemoveFromCandidateListFailed)?; if remove_last_authored { >::remove(who.clone()) }; @@ -741,7 +745,6 @@ pub mod pallet { /// /// This is done on the fly, as frequent as we are told to do so, as the session manager. pub fn assemble_collators() -> Vec { - // TODO[GMP] revisit let desired_candidates: usize = >::get().try_into().unwrap_or(usize::MAX); let mut collators = Self::invulnerables().to_vec(); @@ -751,6 +754,8 @@ pub mod pallet { /// Kicks out candidates that did not produce a block in the kick threshold and refunds /// their deposits. + /// + /// Return value is the number of candidates left in the list. pub fn kick_stale_candidates( candidates: BoundedVec>, T::MaxCandidates>, ) -> usize { diff --git a/pallets/collator-selection/src/mock.rs b/pallets/collator-selection/src/mock.rs index a6d0b482410..ff8c23278eb 100644 --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -181,11 +181,10 @@ impl pallet_session::Config for Test { type CandidateBagsListInstance = pallet_bags_list::Instance1; impl pallet_bags_list::Config for Test { type RuntimeEvent = RuntimeEvent; - /// The voter bags-list is loosely kept up to date, and the real source of truth for the score - /// of each node is the staking pallet. + /// The candidate bags-list is loosely kept up to date, and the real source + /// of truth for the score of each node is the collator-selection pallet. type ScoreProvider = CollatorSelection; type BagThresholds = BagThresholds; - // TODO[GMP] come back to this type Score = u64; type WeightInfo = pallet_bags_list::weights::SubstrateWeight; } diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 2bda501ecc7..300b6f1d07e 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -696,17 +696,17 @@ fn decrease_candidacy_bond_insufficient_funds() { assert_noop!( CollatorSelection::decrease_bond(RuntimeOrigin::signed(3), 100), - Error::::OnRemoveInsufficientFunds + Error::::DepositTooLow ); assert_noop!( CollatorSelection::decrease_bond(RuntimeOrigin::signed(4), 60), - Error::::OnRemoveInsufficientFunds + Error::::DepositTooLow ); assert_noop!( CollatorSelection::decrease_bond(RuntimeOrigin::signed(5), 51), - Error::::OnRemoveInsufficientFunds + Error::::DepositTooLow ); }); } @@ -859,7 +859,7 @@ fn fees_edgecases() { } #[test] -fn session_management_works() { +fn session_management_single_candidate() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -896,7 +896,7 @@ fn session_management_works() { } #[test] -fn session_management_works_1() { +fn session_management_max_candidates() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -934,7 +934,7 @@ fn session_management_works_1() { } #[test] -fn session_management_works_2() { +fn session_management_increase_bid_without_list_update() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -973,7 +973,7 @@ fn session_management_works_2() { } #[test] -fn session_management_works_3() { +fn session_management_increase_bid_with_list_update() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -1013,7 +1013,7 @@ fn session_management_works_3() { } #[test] -fn session_management_works_4() { +fn session_management_candidate_list_lazy_sort() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -1029,6 +1029,9 @@ fn session_management_works_4() { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 50)); + // even though candidate 5 has a higher bid than both 3 and 4, we try + // to put it in front of 4 and not 3; the list should be lazily sorted + // and not reorder candidate 5 ahead of 4. assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(5), 4)); // session won't see this. @@ -1053,7 +1056,7 @@ fn session_management_works_4() { } #[test] -fn session_management_works_5() { +fn session_management_reciprocal_outbidding() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -1074,6 +1077,8 @@ fn session_management_works_5() { initialize_to_block(5); + // candidates 3 and 4 saw they were outbid and preemptively bid more + // than 5 in the next block. assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 60)); assert_ok!(CandidateList::put_in_front_of(RuntimeOrigin::signed(4), 5)); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 60)); @@ -1101,7 +1106,7 @@ fn session_management_works_5() { } #[test] -fn session_management_works_6() { +fn session_management_decrease_bid_after_auction() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -1129,6 +1134,9 @@ fn session_management_works_6() { initialize_to_block(5); + // candidate 5 saw it was outbid and wants to take back its bid, but + // not entirely so they still keep their place in the candidate list + // in case there is an opportunity in the future. assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(5), 50)); // session won't see this. diff --git a/parachain-template/runtime/src/lib.rs b/parachain-template/runtime/src/lib.rs index ad35e60e31b..f63bb538ef7 100644 --- a/parachain-template/runtime/src/lib.rs +++ b/parachain-template/runtime/src/lib.rs @@ -460,18 +460,16 @@ impl pallet_collator_selection::Config for Runtime { } parameter_types! { - // TODO[GMP] come back to this - pub const BagThresholds: &'static [u128] = &[0]; + pub const BagThresholds: &'static [u128] = &[]; } type CandidateBagsListInstance = pallet_bags_list::Instance1; impl pallet_bags_list::Config for Runtime { type RuntimeEvent = RuntimeEvent; - /// The voter bags-list is loosely kept up to date, and the real source of truth for the score - /// of each node is the staking pallet. + /// The candidate bags-list is loosely kept up to date, and the real source + /// of truth for the score of each node is the collator-selection pallet. type ScoreProvider = CollatorSelection; type BagThresholds = BagThresholds; - // TODO[GMP] come back to this type Score = Balance; type WeightInfo = pallet_bags_list::weights::SubstrateWeight; } From 5f89bc3e7b48e132cc527424257c1220075daa73 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 7 Aug 2023 17:22:12 +0300 Subject: [PATCH 11/13] Refund weight for bid extrinsics Signed-off-by: georgepisaltu --- pallets/collator-selection/src/lib.rs | 52 +++++++++++++++++---------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 27996b388dc..b7d14a01523 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -583,27 +583,35 @@ pub mod pallet { /// the amount cannot be reserved. #[pallet::call_index(7)] #[pallet::weight(T::WeightInfo::increase_bond(T::MaxCandidates::get()))] - pub fn increase_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { + pub fn increase_bond( + origin: OriginFor, + amount: BalanceOf, + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - - let new_amount = - >::try_mutate(|candidates| -> Result, DispatchError> { - let candidate_info = candidates + // The function below will try to mutate the `Candidates` entry for + // the caller to increase their bond by `amount`. The return value + // is a tuple of the position of the entry in the list, used for + // weight calculation, and the new bonded amount. + let (idx, new_amount) = >::try_mutate( + |candidates| -> Result<(usize, BalanceOf), DispatchError> { + let (idx, candidate_info) = candidates .iter_mut() - .find(|candidate_info| candidate_info.who == who) + .enumerate() + .find(|(_, candidate_info)| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; candidate_info.deposit = candidate_info.deposit.saturating_add(amount); T::Currency::reserve(&who, amount)?; T::CandidateList::on_increase(&who, amount) .map_err(|_| Error::::UpdateCandidateListFailed)?; - Ok(candidate_info.deposit) - })?; + Ok((idx, candidate_info.deposit)) + }, + )?; Self::deposit_event(Event::CandidateBondIncreased { account_id: who, deposit: new_amount, }); - Ok(()) + Ok(Some(T::WeightInfo::increase_bond(idx as u32)).into()) } /// Decrease the candidacy bond of collator candidate `origin` by an @@ -613,14 +621,21 @@ pub mod pallet { /// the remaining bond is lower than the minimum candidacy bond. #[pallet::call_index(8)] #[pallet::weight(T::WeightInfo::decrease_bond(T::MaxCandidates::get()))] - pub fn decrease_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { + pub fn decrease_bond( + origin: OriginFor, + amount: BalanceOf, + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - - let new_amount = - >::try_mutate(|candidates| -> Result, DispatchError> { - let candidate_info = candidates + // The function below will try to mutate the `Candidates` entry for + // the caller to decrease their bond by `amount`. The return value + // is a tuple of the position of the entry in the list, used for + // weight calculation, and the new bonded amount. + let (idx, new_amount) = >::try_mutate( + |candidates| -> Result<(usize, BalanceOf), DispatchError> { + let (idx, candidate_info) = candidates .iter_mut() - .find(|candidate_info| candidate_info.who == who) + .enumerate() + .find(|(_, candidate_info)| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; candidate_info.deposit = candidate_info.deposit.saturating_sub(amount); if candidate_info.deposit < >::get() { @@ -629,14 +644,15 @@ pub mod pallet { T::Currency::unreserve(&who, amount); T::CandidateList::on_decrease(&who, amount) .map_err(|_| Error::::UpdateCandidateListFailed)?; - Ok(candidate_info.deposit) - })?; + Ok((idx, candidate_info.deposit)) + }, + )?; Self::deposit_event(Event::CandidateBondDecreased { account_id: who, deposit: new_amount, }); - Ok(()) + Ok(Some(T::WeightInfo::decrease_bond(idx as u32)).into()) } /// The caller `origin` replaces a candidate `target` in the collator From 3e5494a7978367a54020acb91f404e122ebfd7ec Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 8 Aug 2023 10:36:33 +0300 Subject: [PATCH 12/13] Collapse `take_candidate_slot` events Signed-off-by: georgepisaltu --- pallets/collator-selection/src/benchmarking.rs | 5 +++-- pallets/collator-selection/src/lib.rs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index ff28eeaa18e..a9daad1ba53 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -357,10 +357,11 @@ mod benchmarks { let target = >::get().last().unwrap().who.clone(); #[extrinsic_call] - _(RawOrigin::Signed(caller.clone()), bond / 2u32.into(), target); + _(RawOrigin::Signed(caller.clone()), bond / 2u32.into(), target.clone()); assert_last_event::( - Event::CandidateAdded { account_id: caller, deposit: bond / 2u32.into() }.into(), + Event::CandidateReplaced { old: target, new: caller, deposit: bond / 2u32.into() } + .into(), ); } diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index b7d14a01523..10b317d7599 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -271,6 +271,8 @@ pub mod pallet { CandidateBondDecreased { account_id: T::AccountId, deposit: BalanceOf }, /// A candidate was removed. CandidateRemoved { account_id: T::AccountId }, + /// An account was replaced in the candidate list by another one. + CandidateReplaced { old: T::AccountId, new: T::AccountId, deposit: BalanceOf }, /// An account was unable to be added to the Invulnerables because they did not have keys /// registered. Other Invulnerables may have been set. InvalidInvulnerableSkipped { account_id: T::AccountId }, @@ -713,8 +715,7 @@ pub mod pallet { Ok(()) })?; - Self::deposit_event(Event::CandidateRemoved { account_id: target }); - Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); + Self::deposit_event(Event::CandidateReplaced { old: target, new: who, deposit }); Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into()) } } From 75269102f63d96282c7e830aa8e6fe76875890c4 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 8 Aug 2023 10:18:34 +0300 Subject: [PATCH 13/13] Move `Candidates` to `StorageMap` Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 57 ++-- pallets/collator-selection/src/lib.rs | 256 ++++++++---------- pallets/collator-selection/src/tests.rs | 148 ++++++---- 3 files changed, 238 insertions(+), 223 deletions(-) diff --git a/pallets/collator-selection/src/benchmarking.rs b/pallets/collator-selection/src/benchmarking.rs index a9daad1ba53..2d63eacc9d0 100644 --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -24,9 +24,9 @@ use crate::Pallet as CollatorSelection; use frame_benchmarking::{ account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError, }; +use frame_election_provider_support::SortedListProvider; use frame_support::{ codec::Decode, - dispatch::DispatchResult, traits::{Currency, EnsureOrigin, Get, ReservableCurrency}, }; use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, RawOrigin}; @@ -108,8 +108,8 @@ fn min_candidates() -> u32 { fn min_invulnerables() -> u32 { let min_collators = T::MinEligibleCollators::get(); - let candidates_length = >::get().len(); - min_collators.saturating_sub(candidates_length.try_into().unwrap()) + let candidates_length = >::get(); + min_collators.saturating_sub(candidates_length) } #[benchmarks(where T: pallet_authorship::Config + session::Config)] @@ -164,19 +164,13 @@ mod benchmarks { for (who, _) in candidates { let deposit = >::get(); T::Currency::make_free_balance_be(&who, deposit * 1000_u32.into()); - let incoming = CandidateInfo { who: who.clone(), deposit }; - >::try_mutate(|candidates| -> DispatchResult { - if !candidates.iter().any(|candidate| candidate.who == who) { - T::Currency::reserve(&who, deposit)?; - candidates.try_push(incoming).expect("we've respected the bounded vec limit"); - >::insert( - who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), - ); - } - Ok(()) - }) - .expect("only returns ok"); + >::insert(who.clone(), deposit); + T::CandidateList::on_insert(who.clone(), deposit).unwrap(); + T::Currency::reserve(&who, deposit)?; + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); } // now we need to fill up invulnerables @@ -249,7 +243,7 @@ mod benchmarks { register_validators::(c); register_candidates::(c); - let caller = >::get().last().unwrap().who.clone(); + let caller = >::iter().last().unwrap().0.clone(); v2::whitelist!(caller); let bond_amount: BalanceOf = T::Currency::minimum_balance(); @@ -265,7 +259,7 @@ mod benchmarks { .into(), ); assert!( - >::get().last().unwrap().deposit == + >::iter().last().unwrap().1 == T::Currency::minimum_balance() * 2u32.into() ); Ok(()) @@ -281,7 +275,7 @@ mod benchmarks { register_validators::(c); register_candidates::(c); - let caller = >::get().last().unwrap().who.clone(); + let caller = >::iter().last().unwrap().0.clone(); v2::whitelist!(caller); >::increase_bond( @@ -302,7 +296,7 @@ mod benchmarks { } .into(), ); - assert!(>::get().last().unwrap().deposit == T::Currency::minimum_balance()); + assert!(>::iter().last().unwrap().1 == T::Currency::minimum_balance()); Ok(()) } @@ -354,7 +348,7 @@ mod benchmarks { ) .unwrap(); - let target = >::get().last().unwrap().who.clone(); + let target = >::iter().last().unwrap().0.clone(); #[extrinsic_call] _(RawOrigin::Signed(caller.clone()), bond / 2u32.into(), target.clone()); @@ -374,7 +368,7 @@ mod benchmarks { register_validators::(c); register_candidates::(c); - let leaving = >::get().last().unwrap().who.clone(); + let leaving = >::iter().last().unwrap().0.clone(); v2::whitelist!(leaving); #[extrinsic_call] @@ -421,30 +415,31 @@ mod benchmarks { let new_block: BlockNumberFor = 1800u32.into(); let zero_block: BlockNumberFor = 0u32.into(); - let candidates = >::get(); + let candidates: Vec = >::iter().map(|(who, _)| who).collect(); let non_removals = c.saturating_sub(r); for i in 0..c { - >::insert(candidates[i as usize].who.clone(), zero_block); + >::insert(candidates[i as usize].clone(), zero_block); } if non_removals > 0 { for i in 0..non_removals { - >::insert(candidates[i as usize].who.clone(), new_block); + >::insert(candidates[i as usize].clone(), new_block); } } else { for i in 0..c { - >::insert(candidates[i as usize].who.clone(), new_block); + >::insert(candidates[i as usize].clone(), new_block); } } let min_candidates = min_candidates::(); - let pre_length = >::get().len(); + let pre_length = >::get(); + assert!(pre_length as usize == >::iter().count()); frame_system::Pallet::::set_block_number(new_block); - assert!(>::get().len() == c as usize); + assert!(>::get() == c); #[block] { @@ -455,16 +450,16 @@ mod benchmarks { // candidates > removals and remaining candidates > min candidates // => remaining candidates should be shorter than before removal, i.e. some were // actually removed. - assert!(>::get().len() < pre_length); + assert!(>::get() < pre_length); } else if c > r && non_removals < min_candidates { // candidates > removals and remaining candidates would be less than min candidates // => remaining candidates should equal min candidates, i.e. some were removed up to // the minimum, but then any more were "forced" to stay in candidates. - assert!(>::get().len() == min_candidates as usize); + assert!(>::get() == min_candidates); } else { // removals >= candidates, non removals must == 0 // can't remove more than exist - assert!(>::get().len() == pre_length); + assert!(>::get() == pre_length); } } diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 10b317d7599..5cf62b49e57 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -184,15 +184,19 @@ pub mod pallet { pub type Invulnerables = StorageValue<_, BoundedVec, ValueQuery>; + /// Current number of candidates in the `Candidates` map. + /// + /// This must always be less than [`Config::MaxCandidates`]. + #[pallet::storage] + #[pallet::getter(fn candidate_count)] + pub type CandidateCount = StorageValue<_, u32, ValueQuery>; + /// The (community, limited) collation candidates. `Candidates` and `Invulnerables` should be /// mutually exclusive. #[pallet::storage] #[pallet::getter(fn candidates)] - pub type Candidates = StorageValue< - _, - BoundedVec>, T::MaxCandidates>, - ValueQuery, - >; + pub type Candidates = + StorageMap<_, Identity, T::AccountId, BalanceOf, ValueQuery>; /// Last block authored by collator. #[pallet::storage] @@ -309,6 +313,9 @@ pub mod pallet { /// Deposit amount is too low to take the target's slot in the /// candidate list. InsufficientBond, + /// The target account to be replaced in the candidate list is not a + /// candidate. + TargetIsNotCandidate, } #[pallet::hooks] @@ -341,8 +348,7 @@ pub mod pallet { // don't wipe out the collator set if new.is_empty() { ensure!( - Candidates::::decode_len().unwrap_or_default() >= - T::MinEligibleCollators::get() as usize, + CandidateCount::::get() >= T::MinEligibleCollators::get(), Error::::TooFewEligibleCollators ); } @@ -445,8 +451,8 @@ pub mod pallet { let who = ensure_signed(origin)?; // ensure we are below limit. - let length = >::decode_len().unwrap_or_default(); - ensure!((length as u32) < T::MaxCandidates::get(), Error::::TooManyCandidates); + let length = >::get(); + ensure!(length < T::MaxCandidates::get(), Error::::TooManyCandidates); ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); let validator_key = T::ValidatorIdOf::convert(who.clone()) @@ -458,27 +464,26 @@ pub mod pallet { let deposit = Self::candidacy_bond(); // First authored block is current block plus kick threshold to handle session delay - let incoming = CandidateInfo { who: who.clone(), deposit }; - - let current_count = - >::try_mutate(|candidates| -> Result { - if candidates.iter().any(|candidate| candidate.who == who) { - Err(Error::::AlreadyCandidate)? - } else { - T::Currency::reserve(&who, deposit)?; - candidates.try_push(incoming).map_err(|_| Error::::TooManyCandidates)?; - >::insert( - who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), - ); - T::CandidateList::on_insert(who.clone(), deposit) - .map_err(|_| Error::::InsertToCandidateListFailed)?; - Ok(candidates.len()) - } - })?; + let new_length = >::try_mutate_exists( + &who, + |maybe_bond| -> Result { + ensure!(maybe_bond.is_none(), Error::::AlreadyCandidate); + T::Currency::reserve(&who, deposit)?; + *maybe_bond = Some(deposit); + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); + T::CandidateList::on_insert(who.clone(), deposit) + .map_err(|_| Error::::InsertToCandidateListFailed)?; + let new_length = length + 1; + >::put(new_length); + Ok(new_length) + }, + )?; Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); - Ok(Some(T::WeightInfo::register_as_candidate(current_count as u32)).into()) + Ok(Some(T::WeightInfo::register_as_candidate(new_length)).into()) } /// Deregister `origin` as a collator candidate. Note that the collator can only leave on @@ -491,13 +496,14 @@ pub mod pallet { pub fn leave_intent(origin: OriginFor) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; ensure!( - Self::eligible_collators() > T::MinEligibleCollators::get() as usize, + Self::eligible_collators() > T::MinEligibleCollators::get(), Error::::TooFewEligibleCollators ); + let length = >::get(); // Do remove their last authored block. - let current_count = Self::try_remove_candidate(&who, true)?; + Self::try_remove_candidate(&who, true)?; - Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) + Ok(Some(T::WeightInfo::leave_intent(length.saturating_sub(1) as u32)).into()) } /// Add a new account `who` to the list of `Invulnerables` collators. `who` must have @@ -544,10 +550,7 @@ pub mod pallet { .unwrap_or_default() .try_into() .unwrap_or(T::MaxInvulnerables::get().saturating_sub(1)), - Candidates::::decode_len() - .unwrap_or_default() - .try_into() - .unwrap_or(T::MaxCandidates::get()), + >::get(), ); Ok(Some(weight_used).into()) @@ -563,7 +566,7 @@ pub mod pallet { T::UpdateOrigin::ensure_origin(origin)?; ensure!( - Self::eligible_collators() > T::MinEligibleCollators::get() as usize, + Self::eligible_collators() > T::MinEligibleCollators::get(), Error::::TooFewEligibleCollators ); @@ -585,27 +588,23 @@ pub mod pallet { /// the amount cannot be reserved. #[pallet::call_index(7)] #[pallet::weight(T::WeightInfo::increase_bond(T::MaxCandidates::get()))] - pub fn increase_bond( - origin: OriginFor, - amount: BalanceOf, - ) -> DispatchResultWithPostInfo { + pub fn increase_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; // The function below will try to mutate the `Candidates` entry for // the caller to increase their bond by `amount`. The return value // is a tuple of the position of the entry in the list, used for // weight calculation, and the new bonded amount. - let (idx, new_amount) = >::try_mutate( - |candidates| -> Result<(usize, BalanceOf), DispatchError> { - let (idx, candidate_info) = candidates - .iter_mut() - .enumerate() - .find(|(_, candidate_info)| candidate_info.who == who) - .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_add(amount); + let new_amount = >::try_mutate_exists( + &who, + |maybe_deposit| -> Result, DispatchError> { + let new_deposit = maybe_deposit + .ok_or_else(|| Error::::NotCandidate)? + .saturating_add(amount); + *maybe_deposit = Some(new_deposit); T::Currency::reserve(&who, amount)?; T::CandidateList::on_increase(&who, amount) .map_err(|_| Error::::UpdateCandidateListFailed)?; - Ok((idx, candidate_info.deposit)) + Ok(new_deposit) }, )?; @@ -613,7 +612,7 @@ pub mod pallet { account_id: who, deposit: new_amount, }); - Ok(Some(T::WeightInfo::increase_bond(idx as u32)).into()) + Ok(()) } /// Decrease the candidacy bond of collator candidate `origin` by an @@ -623,30 +622,24 @@ pub mod pallet { /// the remaining bond is lower than the minimum candidacy bond. #[pallet::call_index(8)] #[pallet::weight(T::WeightInfo::decrease_bond(T::MaxCandidates::get()))] - pub fn decrease_bond( - origin: OriginFor, - amount: BalanceOf, - ) -> DispatchResultWithPostInfo { + pub fn decrease_bond(origin: OriginFor, amount: BalanceOf) -> DispatchResult { let who = ensure_signed(origin)?; // The function below will try to mutate the `Candidates` entry for // the caller to decrease their bond by `amount`. The return value // is a tuple of the position of the entry in the list, used for // weight calculation, and the new bonded amount. - let (idx, new_amount) = >::try_mutate( - |candidates| -> Result<(usize, BalanceOf), DispatchError> { - let (idx, candidate_info) = candidates - .iter_mut() - .enumerate() - .find(|(_, candidate_info)| candidate_info.who == who) - .ok_or_else(|| Error::::NotCandidate)?; - candidate_info.deposit = candidate_info.deposit.saturating_sub(amount); - if candidate_info.deposit < >::get() { - return Err(Error::::DepositTooLow.into()) - } + let new_amount = >::try_mutate_exists( + &who, + |maybe_deposit| -> Result, DispatchError> { + let new_deposit = maybe_deposit + .ok_or_else(|| Error::::NotCandidate)? + .saturating_sub(amount); + ensure!(new_deposit >= >::get(), Error::::DepositTooLow); + *maybe_deposit = Some(new_deposit); T::Currency::unreserve(&who, amount); T::CandidateList::on_decrease(&who, amount) .map_err(|_| Error::::UpdateCandidateListFailed)?; - Ok((idx, candidate_info.deposit)) + Ok(new_deposit) }, )?; @@ -654,7 +647,7 @@ pub mod pallet { account_id: who, deposit: new_amount, }); - Ok(Some(T::WeightInfo::decrease_bond(idx as u32)).into()) + Ok(()) } /// The caller `origin` replaces a candidate `target` in the collator @@ -675,9 +668,9 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - let length = >::decode_len().unwrap_or_default(); ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); + ensure!(!>::contains_key(&who), Error::::AlreadyCandidate); let validator_key = T::ValidatorIdOf::convert(who.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; @@ -686,37 +679,34 @@ pub mod pallet { Error::::ValidatorNotRegistered ); - >::try_mutate(|candidates| -> Result<(), DispatchError> { - let mut kicked = false; - for candidate_info in candidates.iter_mut() { - ensure!(candidate_info.who != who, Error::::AlreadyCandidate); - - if !kicked && candidate_info.who == target { - ensure!(deposit > candidate_info.deposit, Error::::InsufficientBond); - - T::Currency::unreserve(&candidate_info.who, candidate_info.deposit); - T::CandidateList::on_remove(&candidate_info.who) - .map_err(|_| Error::::RemoveFromCandidateListFailed)?; - >::remove(candidate_info.who.clone()); - - kicked = true; - T::Currency::reserve(&who, deposit)?; - candidate_info.who = who.clone(); - candidate_info.deposit = deposit; - - >::insert( - who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), - ); - T::CandidateList::on_insert(who.clone(), deposit) - .map_err(|_| Error::::InsertToCandidateListFailed)?; - } - } - Ok(()) - })?; + ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); + + let length = >::get(); + let target_deposit = >::try_mutate_exists( + &target, + |maybe_target_deposit| -> Result, DispatchError> { + let target_deposit = + maybe_target_deposit.ok_or(Error::::TargetIsNotCandidate)?; + T::Currency::unreserve(&target, target_deposit); + T::CandidateList::on_remove(&target) + .map_err(|_| Error::::RemoveFromCandidateListFailed)?; + >::remove(target.clone()); + *maybe_target_deposit = None; + Ok(target_deposit) + }, + )?; + ensure!(deposit > target_deposit, Error::::InsufficientBond); + T::Currency::reserve(&who, deposit)?; + >::insert(&who, deposit); + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); + T::CandidateList::on_insert(who.clone(), deposit) + .map_err(|_| Error::::InsertToCandidateListFailed)?; Self::deposit_event(Event::CandidateReplaced { old: target, new: who, deposit }); - Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into()) + Ok(Some(T::WeightInfo::take_candidate_slot(length)).into()) } } @@ -728,34 +718,37 @@ pub mod pallet { /// Return the total number of accounts that are eligible collators (candidates and /// invulnerables). - fn eligible_collators() -> usize { - Candidates::::decode_len() - .unwrap_or_default() - .saturating_add(Invulnerables::::decode_len().unwrap_or_default()) + fn eligible_collators() -> u32 { + >::get().saturating_add( + Invulnerables::::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or(u32::MAX), + ) } /// Removes a candidate if they exist and sends them back their deposit. fn try_remove_candidate( who: &T::AccountId, remove_last_authored: bool, - ) -> Result { - let current_count = - >::try_mutate(|candidates| -> Result { - let index = candidates - .iter() - .position(|candidate| candidate.who == *who) - .ok_or(Error::::NotCandidate)?; - let candidate = candidates.remove(index); - T::Currency::unreserve(who, candidate.deposit); + ) -> Result<(), DispatchError> { + >::try_mutate_exists( + &who, + |maybe_deposit| -> Result<(), DispatchError> { + let deposit = maybe_deposit.ok_or(Error::::NotCandidate)?; + T::Currency::unreserve(who, deposit); + *maybe_deposit = None; T::CandidateList::on_remove(&who) .map_err(|_| Error::::RemoveFromCandidateListFailed)?; if remove_last_authored { >::remove(who.clone()) }; - Ok(candidates.len()) - })?; + >::mutate(|length| *length = length.saturating_sub(1)); + Ok(()) + }, + )?; Self::deposit_event(Event::CandidateRemoved { account_id: who.clone() }); - Ok(current_count) + Ok(()) } /// Assemble the current set of candidates and invulnerables into the next collator set. @@ -773,35 +766,33 @@ pub mod pallet { /// their deposits. /// /// Return value is the number of candidates left in the list. - pub fn kick_stale_candidates( - candidates: BoundedVec>, T::MaxCandidates>, - ) -> usize { + pub fn kick_stale_candidates(candidates: impl IntoIterator) -> u32 { let now = frame_system::Pallet::::block_number(); let kick_threshold = T::KickThreshold::get(); let min_collators = T::MinEligibleCollators::get(); candidates .into_iter() .filter_map(|c| { - let last_block = >::get(c.who.clone()); + let last_block = >::get(c.clone()); let since_last = now.saturating_sub(last_block); - let is_invulnerable = Self::invulnerables().contains(&c.who); + let is_invulnerable = Self::invulnerables().contains(&c); let is_lazy = since_last >= kick_threshold; if is_invulnerable { // They are invulnerable. No reason for them to be in Candidates also. // We don't even care about the min collators here, because an Account // should not be a collator twice. - let _ = Self::try_remove_candidate(&c.who, false); + let _ = Self::try_remove_candidate(&c, false); None } else { - if Self::eligible_collators() <= min_collators as usize || !is_lazy { + if Self::eligible_collators() <= min_collators || !is_lazy { // Either this is a good collator (not lazy) or we are at the minimum // that the system needs. They get to stay. - Some(c.who) + Some(c) } else { // This collator has not produced a block recently enough. Bye bye. - let _ = Self::try_remove_candidate(&c.who, true); + let _ = Self::try_remove_candidate(&c, true); None } } @@ -845,9 +836,9 @@ pub mod pallet { >::block_number(), ); - let candidates = Self::candidates(); - let candidates_len_before = candidates.len(); - let active_candidates_count = Self::kick_stale_candidates(candidates); + let candidates_len_before = >::get(); + let active_candidates_count = + Self::kick_stale_candidates(>::iter().map(|(who, _)| who)); let removed = candidates_len_before.saturating_sub(active_candidates_count); let result = Self::assemble_collators(); @@ -869,23 +860,14 @@ pub mod pallet { type Score = BalanceOf; fn score(who: &T::AccountId) -> Self::Score { - let candidates = Self::candidates(); - candidates - .iter() - .find(|&candidate_info| candidate_info.who == who.clone()) - .map(|candidate_info| candidate_info.deposit) - .unwrap_or_else(|| Zero::zero()) + >::get(who) } #[cfg(feature = "runtime-benchmarks")] fn set_score_of(who: &T::AccountId, weight: Self::Score) { let active: BalanceOf = weight.try_into().map_err(|_| ()).unwrap(); - Candidates::::mutate(|candidates| { - if let Some(candidate) = - candidates.iter_mut().find(|candidate_info| candidate_info.who == who.clone()) - { - candidate.deposit = active; - } + Candidates::::mutate(who, |deposit| { + *deposit = active; }); } } diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 300b6f1d07e..380fa2841cd 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -14,7 +14,8 @@ // limitations under the License. use crate as collator_selection; -use crate::{mock::*, CandidateInfo, Error}; +use crate::{mock::*, Error}; +use frame_election_provider_support::SortedListProvider; use frame_support::{ assert_noop, assert_ok, traits::{Currency, OnInitialize}, @@ -29,7 +30,7 @@ fn basic_setup_works() { assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert!(CollatorSelection::candidates().is_empty()); + assert_eq!(>::iter().count(), 0); // genesis should sort input assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); }); @@ -203,7 +204,7 @@ fn candidate_to_invulnerable_works() { initialize_to_block(1); assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_eq!(Balances::free_balance(3), 100); @@ -227,7 +228,7 @@ fn candidate_to_invulnerable_works() { )); assert!(CollatorSelection::invulnerables().to_vec().contains(&3)); assert_eq!(Balances::free_balance(3), 100); - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::iter().count(), 1); assert_ok!(CollatorSelection::add_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -241,7 +242,7 @@ fn candidate_to_invulnerable_works() { )); assert!(CollatorSelection::invulnerables().to_vec().contains(&4)); assert_eq!(Balances::free_balance(4), 100); - assert_eq!(CollatorSelection::candidates().len(), 0); + assert_eq!(>::iter().count(), 0); }); } @@ -312,7 +313,7 @@ fn cannot_register_candidate_if_too_many() { #[test] fn cannot_unregister_candidate_if_too_few() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::remove_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -370,8 +371,9 @@ fn cannot_register_dupe_candidate() { new_test_ext().execute_with(|| { // can add 3 as candidate assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); - let addition = CandidateInfo { who: 3, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![addition]); + // tuple of (id, deposit). + let addition = (3, 10); + assert_eq!(>::iter().collect::>(), vec![addition]); assert_eq!(CollatorSelection::last_authored_block(3), 10); assert_eq!(Balances::free_balance(3), 90); @@ -406,7 +408,7 @@ fn register_as_candidate_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take two endowed, non-invulnerables accounts. @@ -419,7 +421,7 @@ fn register_as_candidate_works() { assert_eq!(Balances::free_balance(3), 90); assert_eq!(Balances::free_balance(4), 90); - assert_eq!(CollatorSelection::candidates().len(), 2); + assert_eq!(>::iter().count(), 2); }); } @@ -453,9 +455,12 @@ fn cannot_take_candidate_slot_if_duplicate() { // can add 3 as candidate assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); - let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; - let candidate_4 = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![candidate_3, candidate_4]); + // tuple of (id, deposit). + let candidate_3 = (3, 10); + let candidate_4 = (4, 10); + let mut actual_candidates = >::iter().collect::>(); + actual_candidates.sort(); + assert_eq!(actual_candidates, vec![candidate_3, candidate_4]); assert_eq!(CollatorSelection::last_authored_block(3), 10); assert_eq!(CollatorSelection::last_authored_block(4), 10); assert_eq!(Balances::free_balance(3), 90); @@ -468,6 +473,25 @@ fn cannot_take_candidate_slot_if_duplicate() { }) } +#[test] +fn cannot_take_candidate_slot_if_target_invalid() { + new_test_ext().execute_with(|| { + // can add 3 as candidate + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + // tuple of (id, deposit). + let candidate_3 = (3, 10); + assert_eq!(>::iter().collect::>(), vec![candidate_3]); + assert_eq!(CollatorSelection::last_authored_block(3), 10); + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 100); + + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 50u64.into(), 5), + Error::::TargetIsNotCandidate, + ); + }) +} + #[test] fn cannot_take_candidate_slot_if_poor() { new_test_ext().execute_with(|| { @@ -520,7 +544,7 @@ fn take_candidate_slot_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take two endowed, non-invulnerables accounts. @@ -536,7 +560,7 @@ fn take_candidate_slot_works() { assert_eq!(Balances::free_balance(4), 90); assert_eq!(Balances::free_balance(5), 90); - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); Balances::make_free_balance_be(&6, 100); let key = MockSessionKeys { aura: UintAuthorityId(6) }; @@ -553,10 +577,17 @@ fn take_candidate_slot_works() { assert_eq!(Balances::free_balance(5), 90); assert_eq!(Balances::free_balance(6), 50); - let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; - let candidate_6 = CandidateInfo { who: 6, deposit: 50 }; - let candidate_5 = CandidateInfo { who: 5, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![candidate_3, candidate_6, candidate_5]); + // tuple of (id, deposit). + let candidate_3 = (3, 10); + let candidate_6 = (6, 50); + let candidate_5 = (5, 10); + let mut actual_candidates = >::iter().collect::>(); + actual_candidates.sort(); + assert_eq!( + >::iter().collect::>(), + vec![candidate_3, candidate_5, candidate_6] + ); + assert_eq!(CandidateList::iter().collect::>(), vec![3, 5, 6]); }); } @@ -566,7 +597,7 @@ fn increase_candidacy_bond_non_candidate_account() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); @@ -585,7 +616,7 @@ fn increase_candidacy_bond_insufficient_balance() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take two endowed, non-invulnerables accounts. @@ -614,7 +645,7 @@ fn increase_candidacy_bond_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take three endowed, non-invulnerables accounts. @@ -634,7 +665,7 @@ fn increase_candidacy_bond_works() { assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 20)); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 30)); - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); assert_eq!(Balances::free_balance(3), 80); assert_eq!(Balances::free_balance(4), 70); assert_eq!(Balances::free_balance(5), 60); @@ -642,7 +673,7 @@ fn increase_candidacy_bond_works() { assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(3), 20)); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 30)); - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); assert_eq!(Balances::free_balance(3), 60); assert_eq!(Balances::free_balance(4), 40); assert_eq!(Balances::free_balance(5), 60); @@ -655,7 +686,7 @@ fn decrease_candidacy_bond_non_candidate_account() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); @@ -674,7 +705,7 @@ fn decrease_candidacy_bond_insufficient_funds() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take two endowed, non-invulnerables accounts. @@ -717,7 +748,7 @@ fn decrease_candidacy_bond_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take three endowed, non-invulnerables accounts. @@ -737,7 +768,7 @@ fn decrease_candidacy_bond_works() { assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(4), 20)); assert_ok!(CollatorSelection::increase_bond(RuntimeOrigin::signed(5), 30)); - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); assert_eq!(Balances::free_balance(3), 80); assert_eq!(Balances::free_balance(4), 70); assert_eq!(Balances::free_balance(5), 60); @@ -745,7 +776,7 @@ fn decrease_candidacy_bond_works() { assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(3), 10)); assert_ok!(CollatorSelection::decrease_bond(RuntimeOrigin::signed(4), 10)); - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); assert_eq!(Balances::free_balance(3), 90); assert_eq!(Balances::free_balance(4), 80); assert_eq!(Balances::free_balance(5), 60); @@ -758,7 +789,7 @@ fn candidate_list_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); // take three endowed, non-invulnerables accounts. @@ -822,9 +853,10 @@ fn authorship_event_handler() { // triggers `note_author` Authorship::on_initialize(1); - let collator = CandidateInfo { who: 4, deposit: 10 }; + // tuple of (id, deposit). + let collator = (4, 10); - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!(>::iter().collect::>(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 0); // half of the pot goes to the collator who's the author (4 in tests). @@ -847,9 +879,10 @@ fn fees_edgecases() { // triggers `note_author` Authorship::on_initialize(1); - let collator = CandidateInfo { who: 4, deposit: 10 }; + // tuple of (id, deposit). + let collator = (4, 10); - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!(>::iter().collect::>(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 0); // Nothing received assert_eq!(Balances::free_balance(4), 90); @@ -877,7 +910,7 @@ fn session_management_single_candidate() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::iter().count(), 1); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -915,7 +948,7 @@ fn session_management_max_candidates() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -954,7 +987,7 @@ fn session_management_increase_bid_without_list_update() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -994,7 +1027,7 @@ fn session_management_increase_bid_with_list_update() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -1037,7 +1070,7 @@ fn session_management_candidate_list_lazy_sort() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -1087,7 +1120,7 @@ fn session_management_reciprocal_outbidding() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -1142,7 +1175,7 @@ fn session_management_decrease_bid_after_auction() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 3); + assert_eq!(>::iter().count(), 3); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -1167,15 +1200,16 @@ fn kick_mechanism() { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); initialize_to_block(10); - assert_eq!(CollatorSelection::candidates().len(), 2); + assert_eq!(>::iter().count(), 2); initialize_to_block(20); assert_eq!(SessionChangeBlock::get(), 20); // 4 authored this block, gets to stay 3 was kicked - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::iter().count(), 1); // 3 will be kicked after 1 session delay assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 4]); - let collator = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + // tuple of (id, deposit). + let collator = (4, 10); + assert_eq!(>::iter().collect::>(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 20); initialize_to_block(30); // 3 gets kicked after 1 session delay @@ -1189,7 +1223,7 @@ fn kick_mechanism() { fn should_not_kick_mechanism_too_few() { new_test_ext().execute_with(|| { // remove the invulnerables and add new collators 3 and 5 - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::remove_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -1203,16 +1237,17 @@ fn should_not_kick_mechanism_too_few() { )); initialize_to_block(10); - assert_eq!(CollatorSelection::candidates().len(), 2); + assert_eq!(>::iter().count(), 2); initialize_to_block(20); assert_eq!(SessionChangeBlock::get(), 20); // 4 authored this block, 3 is kicked, 5 stays because of too few collators - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::iter().count(), 1); // 3 will be kicked after 1 session delay assert_eq!(SessionHandlerCollators::get(), vec![3, 5]); - let collator = CandidateInfo { who: 5, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + // tuple of (id, deposit). + let collator = (5, 10); + assert_eq!(>::iter().collect::>(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 20); initialize_to_block(30); @@ -1226,7 +1261,7 @@ fn should_not_kick_mechanism_too_few() { #[test] fn should_kick_invulnerables_from_candidates_on_session_change() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::iter().count(), 0); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); assert_eq!(Balances::free_balance(3), 90); @@ -1236,16 +1271,19 @@ fn should_kick_invulnerables_from_candidates_on_session_change() { vec![1, 2, 3] )); - let collator_3 = CandidateInfo { who: 3, deposit: 10 }; - let collator_4 = CandidateInfo { who: 4, deposit: 10 }; + // tuple of (id, deposit). + let collator_3 = (3, 10); + let collator_4 = (4, 10); - assert_eq!(CollatorSelection::candidates(), vec![collator_3, collator_4.clone()]); + let mut actual_candidates = >::iter().collect::>(); + actual_candidates.sort(); + assert_eq!(actual_candidates, vec![collator_3, collator_4.clone()]); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2, 3]); // session change initialize_to_block(10); // 3 is removed from candidates - assert_eq!(CollatorSelection::candidates(), vec![collator_4]); + assert_eq!(>::iter().collect::>(), vec![collator_4]); // but not from invulnerables assert_eq!(CollatorSelection::invulnerables(), vec![1, 2, 3]); // and it got its deposit back