From 9dd686b1f814141d35fa8e04d9b852715d37b32b Mon Sep 17 00:00:00 2001 From: Gregory Hill Date: Mon, 23 Jan 2023 17:08:55 +0000 Subject: [PATCH] refactor: bound entries in collator-selection Signed-off-by: Gregory Hill --- crates/collator-selection/src/lib.rs | 58 +++++++++++++++------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/crates/collator-selection/src/lib.rs b/crates/collator-selection/src/lib.rs index db079fb94c..f6d85071ae 100644 --- a/crates/collator-selection/src/lib.rs +++ b/crates/collator-selection/src/lib.rs @@ -70,7 +70,7 @@ pub mod pallet { RuntimeDebug, }, traits::{Currency, EnsureOrigin, ExistenceRequirement::KeepAlive, ReservableCurrency, ValidatorRegistration}, - PalletId, + BoundedVec, PalletId, }; use frame_system::{pallet_prelude::*, Config as SystemConfig}; use pallet_session::SessionManager; @@ -106,8 +106,7 @@ pub mod pallet { /// Account Identifier from which the internal Pot is generated. type PotId: Get; - /// Maximum number of candidates that we should have. This is used for benchmarking and is not - /// enforced. + /// Maximum number of candidates that we should have. This is enforced in code. /// /// This does not take into account the invulnerables. type MaxCandidates: Get; @@ -117,9 +116,7 @@ pub mod pallet { /// This does not take into account the invulnerables. type MinCandidates: Get; - /// Maximum number of invulnerables. - /// - /// Used only for benchmarking. + /// Maximum number of invulnerables. This is enforced in code. type MaxInvulnerables: Get; // Will be kicked if block is not produced in threshold. @@ -141,7 +138,7 @@ pub mod pallet { } /// Basic information about a collation candidate. - #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, scale_info::TypeInfo)] + #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, scale_info::TypeInfo, MaxEncodedLen)] pub struct CandidateInfo { /// Account identifier. pub who: AccountId, @@ -151,18 +148,19 @@ pub mod pallet { #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] - #[pallet::without_storage_info] pub struct Pallet(_); /// The invulnerable, fixed collators. #[pallet::storage] #[pallet::getter(fn invulnerables)] - pub type Invulnerables = StorageValue<_, Vec, ValueQuery>; + pub type Invulnerables = + StorageValue<_, BoundedVec, ValueQuery>; /// The (community, limited) collation candidates. #[pallet::storage] #[pallet::getter(fn candidates)] - pub type Candidates = StorageValue<_, Vec>>, ValueQuery>; + pub type Candidates = + StorageValue<_, BoundedVec>, T::MaxCandidates>, ValueQuery>; /// Last block authored by collator. #[pallet::storage] @@ -210,10 +208,8 @@ pub mod pallet { "duplicate invulnerables in genesis." ); - assert!( - T::MaxInvulnerables::get() >= (self.invulnerables.len() as u32), - "genesis invulnerables are more than T::MaxInvulnerables", - ); + let bounded_invulnerables = BoundedVec::<_, T::MaxInvulnerables>::try_from(self.invulnerables.clone()) + .expect("genesis invulnerables are more than T::MaxInvulnerables"); assert!( T::MaxCandidates::get() >= self.desired_candidates, "genesis desired_candidates are more than T::MaxCandidates", @@ -221,7 +217,7 @@ pub mod pallet { >::put(&self.desired_candidates); >::put(&self.candidacy_bond); - >::put(&self.invulnerables); + >::put(bounded_invulnerables); } } @@ -261,6 +257,8 @@ pub mod pallet { AlreadyCandidate, /// User is not a candidate NotCandidate, + /// Too many invulnerables + TooManyInvulnerables, /// User is already an Invulnerable AlreadyInvulnerable, /// Account has no associated validator ID @@ -279,13 +277,11 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_invulnerables(new.len() as u32))] pub fn set_invulnerables(origin: OriginFor, new: Vec) -> DispatchResultWithPostInfo { T::UpdateOrigin::ensure_origin(origin)?; - // we trust origin calls, this is just a for more accurate benchmarking - if (new.len() as u32) > T::MaxInvulnerables::get() { - log::warn!("invulnerables > T::MaxInvulnerables; you might need to run benchmarks again"); - } + let bounded_invulnerables = + BoundedVec::<_, T::MaxInvulnerables>::try_from(new).map_err(|_| Error::::TooManyInvulnerables)?; // check if the invulnerables have associated validator keys before they are set - for account_id in &new { + for account_id in bounded_invulnerables.iter() { let validator_key = T::ValidatorIdOf::convert(account_id.clone()).ok_or(Error::::NoAssociatedValidatorId)?; ensure!( @@ -294,8 +290,10 @@ pub mod pallet { ); } - >::put(&new); - Self::deposit_event(Event::NewInvulnerables { invulnerables: new }); + >::put(&bounded_invulnerables); + Self::deposit_event(Event::NewInvulnerables { + invulnerables: bounded_invulnerables.to_vec(), + }); Ok(().into()) } @@ -362,7 +360,9 @@ pub mod pallet { Err(Error::::AlreadyCandidate)? } else { T::StakingCurrency::reserve(&who, deposit)?; - candidates.push(incoming); + candidates + .try_push(incoming) + .map_err(|_| Error::::TooManyCandidates)?; >::insert( who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), @@ -425,15 +425,17 @@ 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: Vec) -> Vec { - let mut collators = Self::invulnerables(); + pub fn assemble_collators(candidates: BoundedVec) -> Vec { + let mut collators = Self::invulnerables().to_vec(); collators.extend(candidates); collators } /// Kicks out candidates that did not produce a block in the kick threshold /// or whose free balance drops below the minimum and refund their deposits. - pub fn kick_stale_candidates(candidates: Vec>>) -> Vec { + pub fn kick_stale_candidates( + candidates: BoundedVec>, T::MaxCandidates>, + ) -> BoundedVec { let now = frame_system::Pallet::::block_number(); let kick_threshold = T::KickThreshold::get(); let candidacy_bond = Self::candidacy_bond(); @@ -460,7 +462,9 @@ pub mod pallet { None } }) - .collect() + .collect::>() + .try_into() + .expect("filter_map operation can't result in a bounded vec larger than its original; qed") } }