diff --git a/pallets/parachain-staking/src/lib.rs b/pallets/parachain-staking/src/lib.rs index 6780a3a6e3..5f63ecf016 100644 --- a/pallets/parachain-staking/src/lib.rs +++ b/pallets/parachain-staking/src/lib.rs @@ -80,7 +80,7 @@ pub mod pallet { use crate::delegation_requests::{ CancelledScheduledRequest, DelegationAction, ScheduledRequest, }; - use crate::{set::OrderedSet, traits::*, types::*, InflationInfo, Range, WeightInfo}; + use crate::{set::BoundedOrderedSet, traits::*, types::*, InflationInfo, Range, WeightInfo}; use crate::{AutoCompoundConfig, AutoCompoundDelegations}; use frame_support::pallet_prelude::*; use frame_support::traits::{ @@ -177,6 +177,9 @@ pub mod pallet { type OnNewRound: OnNewRound; /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + /// Maximum candidates + #[pallet::constant] + type MaxCandidates: Get; } #[pallet::error] @@ -229,8 +232,8 @@ pub mod pallet { TooLowCandidateAutoCompoundingDelegationCountToLeaveCandidates, TooLowCandidateCountWeightHint, TooLowCandidateCountWeightHintGoOffline, - TooLowCandidateCountWeightHintGoOnline, - TooLowCandidateCountWeightHintCandidateBondMore, + CandidateLimitReached, + CannotSetAboveMaxCandidates, } #[pallet::event] @@ -579,7 +582,8 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn selected_candidates)] /// The collator candidates selected for the current round - type SelectedCandidates = StorageValue<_, Vec, ValueQuery>; + type SelectedCandidates = + StorageValue<_, BoundedVec, ValueQuery>; #[pallet::storage] #[pallet::getter(fn total)] @@ -589,8 +593,11 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn candidate_pool)] /// The pool of collator candidates, each with their total backing stake - pub(crate) type CandidatePool = - StorageValue<_, OrderedSet>>, ValueQuery>; + pub(crate) type CandidatePool = StorageValue< + _, + BoundedOrderedSet>, T::MaxCandidates>, + ValueQuery, + >; #[pallet::storage] #[pallet::getter(fn at_stake)] @@ -764,6 +771,11 @@ pub mod pallet { "{:?}", Error::::CannotSetBelowMin ); + assert!( + self.num_selected_candidates <= T::MaxCandidates::get(), + "{:?}", + Error::::CannotSetAboveMaxCandidates + ); >::put(self.num_selected_candidates); // Choose top TotalSelected collator candidates let (_, v_count, _, total_staked) = >::select_top_candidates(1u32); @@ -886,6 +898,10 @@ pub mod pallet { new >= T::MinSelectedCandidates::get(), Error::::CannotSetBelowMin ); + ensure!( + new <= T::MaxCandidates::get(), + Error::::CannotSetAboveMaxCandidates + ); let old = >::get(); ensure!(old != new, Error::::NoWritingSameValue); ensure!( @@ -970,13 +986,14 @@ pub mod pallet { candidate_count >= old_count, Error::::TooLowCandidateCountWeightHintJoinCandidates ); - ensure!( - candidates.insert(Bond { + let maybe_inserted_candidate = candidates + .try_insert(Bond { owner: acc.clone(), - amount: bond - }), - Error::::CandidateExists - ); + amount: bond, + }) + .map_err(|_| Error::::CandidateLimitReached)?; + ensure!(maybe_inserted_candidate, Error::::CandidateExists); + ensure!( Self::get_collator_stakable_free_balance(&acc) >= bond, Error::::InsufficientBalance, @@ -1065,13 +1082,13 @@ pub mod pallet { candidates.0.len() as u32 <= candidate_count, Error::::TooLowCandidateCountWeightHintCancelLeaveCandidates ); - ensure!( - candidates.insert(Bond { + let maybe_inserted_candidate = candidates + .try_insert(Bond { owner: collator.clone(), - amount: state.total_counted - }), - Error::::AlreadyActive - ); + amount: state.total_counted, + }) + .map_err(|_| Error::::CandidateLimitReached)?; + ensure!(maybe_inserted_candidate, Error::::AlreadyActive); >::put(candidates); >::insert(&collator, state); Self::deposit_event(Event::CancelledCandidateExit { @@ -1447,16 +1464,20 @@ pub mod pallet { ); state.go_online(); - ensure!( - candidates.insert(Bond { + let maybe_inserted_candidate = candidates + .try_insert(Bond { owner: collator.clone(), - amount: state.total_counted - }), + amount: state.total_counted, + }) + .map_err(|_| Error::::CandidateLimitReached)?; + ensure!( + maybe_inserted_candidate, DispatchErrorWithPostInfo { post_info: Some(actual_weight).into(), error: >::AlreadyActive.into(), }, ); + >::put(candidates); >::insert(&collator, state); Self::deposit_event(Event::CandidateBackOnline { @@ -1619,10 +1640,14 @@ pub mod pallet { pub(crate) fn update_active(candidate: T::AccountId, total: BalanceOf) { let mut candidates = >::get(); candidates.remove(&Bond::from_owner(candidate.clone())); - candidates.insert(Bond { - owner: candidate, - amount: total, - }); + candidates + .try_insert(Bond { + owner: candidate, + amount: total, + }) + .expect( + "the candidate is removed in previous step so the length cannot increase; qed", + ); >::put(candidates); } @@ -1853,27 +1878,31 @@ pub mod pallet { return vec![]; } - let mut candidates = >::get().0; + let candidates = >::get().0; // If the number of candidates is greater than top_n, select the candidates with higher // amount. Otherwise, return all the candidates. if candidates.len() > top_n { // Partially sort candidates such that element at index `top_n - 1` is sorted, and // all the elements in the range 0..top_n are the top n elements. - candidates.select_nth_unstable_by(top_n - 1, |a, b| { - // Order by amount, then owner. The owner is needed to ensure a stable order - // when two accounts have the same amount. - a.amount - .cmp(&b.amount) - .then_with(|| a.owner.cmp(&b.owner)) - .reverse() - }); + let sorted_candidates = candidates + .try_mutate(|inner| { + inner.select_nth_unstable_by(top_n - 1, |a, b| { + // Order by amount, then owner. The owner is needed to ensure a stable order + // when two accounts have the same amount. + a.amount + .cmp(&b.amount) + .then_with(|| a.owner.cmp(&b.owner)) + .reverse() + }); + }) + .expect("sort cannot increase item count; qed"); - let mut collators = candidates + let mut collators = sorted_candidates .into_iter() .take(top_n) .map(|x| x.owner) - .collect::>(); + .collect::>(); // Sort collators by AccountId collators.sort(); @@ -1882,10 +1911,7 @@ pub mod pallet { } else { // Return all candidates // The candidates are already sorted by AccountId, so no need to sort again - candidates - .into_iter() - .map(|x| x.owner) - .collect::>() + candidates.into_iter().map(|x| x.owner).collect::>() } } /// Best as in most cumulatively supported in terms of stake @@ -1967,7 +1993,10 @@ pub mod pallet { }); } // insert canonical collator set - >::put(collators); + >::put( + BoundedVec::try_from(collators) + .expect("subset of collators is always less than or equal to max candidates"), + ); let avg_delegator_count = delegation_count.checked_div(collator_count).unwrap_or(0); let weight = T::WeightInfo::select_top_candidates(collator_count, avg_delegator_count); @@ -2149,7 +2178,7 @@ pub mod pallet { impl Get> for Pallet { fn get() -> Vec { - Self::selected_candidates() + Self::selected_candidates().into_inner() } } } diff --git a/pallets/parachain-staking/src/mock.rs b/pallets/parachain-staking/src/mock.rs index 7a07f9d2a3..41a5a38eab 100644 --- a/pallets/parachain-staking/src/mock.rs +++ b/pallets/parachain-staking/src/mock.rs @@ -120,6 +120,7 @@ parameter_types! { pub const MinCandidateStk: u128 = 10; pub const MinDelegatorStk: u128 = 5; pub const MinDelegation: u128 = 3; + pub const MaxCandidates: u32 = 10; } impl Config for Test { type RuntimeEvent = RuntimeEvent; @@ -144,6 +145,7 @@ impl Config for Test { type PayoutCollatorReward = (); type OnNewRound = (); type WeightInfo = (); + type MaxCandidates = MaxCandidates; } pub(crate) struct ExtBuilder { diff --git a/pallets/parachain-staking/src/set.rs b/pallets/parachain-staking/src/set.rs index 8e4e29648f..2fb15ffadf 100644 --- a/pallets/parachain-staking/src/set.rs +++ b/pallets/parachain-staking/src/set.rs @@ -15,11 +15,13 @@ // along with Moonbeam. If not, see . /* TODO: use orml_utilities::OrderedSet without leaking substrate v2.0 dependencies*/ + +use frame_support::traits::Get; use parity_scale_codec::{Decode, Encode}; use scale_info::TypeInfo; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; -use sp_runtime::RuntimeDebug; +use sp_runtime::{BoundedVec, RuntimeDebug}; use sp_std::prelude::*; /// An ordered set backed by `Vec` @@ -87,3 +89,78 @@ impl From> for OrderedSet { Self::from(v) } } + +/// An ordered set backed by `BoundedVec` +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +#[derive(RuntimeDebug, PartialEq, Eq, Encode, Decode, Clone, TypeInfo)] +#[scale_info(skip_type_params(S))] +pub struct BoundedOrderedSet>(pub BoundedVec); + +impl> sp_std::default::Default for BoundedOrderedSet { + fn default() -> Self { + BoundedOrderedSet(BoundedVec::default()) + } +} + +impl> BoundedOrderedSet { + /// Create a new empty set + pub fn new() -> Self { + Self(BoundedVec::default()) + } + + /// Create a set from a `Vec`. + /// `v` will be sorted and dedup first. + pub fn from(mut v: BoundedVec) -> Self { + v.sort(); + let v = v.try_mutate(|inner| inner.dedup()).expect( + "input is a valid BoundedVec and deduping can only reduce the number of entires; qed", + ); + Self::from_sorted_set(v) + } + + /// Create a set from a `Vec`. + /// Assume `v` is sorted and contain unique elements. + pub fn from_sorted_set(v: BoundedVec) -> Self { + Self(v) + } + + /// Insert an element. + /// Return true if insertion happened. + pub fn try_insert(&mut self, value: T) -> Result { + match self.0.binary_search(&value) { + Ok(_) => Ok(false), + Err(loc) => self.0.try_insert(loc, value).map(|_| true).map_err(|_| ()), + } + } + + /// Remove an element. + /// Return true if removal happened. + pub fn remove(&mut self, value: &T) -> bool { + match self.0.binary_search(value) { + Ok(loc) => { + self.0.remove(loc); + true + } + Err(_) => false, + } + } + + /// Return if the set contains `value` + pub fn contains(&self, value: &T) -> bool { + self.0.binary_search(value).is_ok() + } + + /// Clear the set + pub fn clear(mut self) { + let v = self.0.try_mutate(|inner| inner.clear()).expect( + "input is a valid BoundedVec and clearing can only reduce the number of entires; qed", + ); + self.0 = v; + } +} + +impl> From> for BoundedOrderedSet { + fn from(v: BoundedVec) -> Self { + Self::from(v) + } +} diff --git a/pallets/parachain-staking/src/tests.rs b/pallets/parachain-staking/src/tests.rs index a95283c2da..f080d570b7 100644 --- a/pallets/parachain-staking/src/tests.rs +++ b/pallets/parachain-staking/src/tests.rs @@ -92,6 +92,17 @@ fn set_total_selected_fails_if_above_blocks_per_round() { }); } +#[test] +fn set_total_selected_fails_if_above_max_candidates() { + ExtBuilder::default().build().execute_with(|| { + assert_eq!(::MaxCandidates::get(), 10); // test relies on this + assert_noop!( + ParachainStaking::set_total_selected(RuntimeOrigin::root(), 15u32), + Error::::CannotSetAboveMaxCandidates, + ); + }); +} + #[test] fn set_total_selected_fails_if_equal_to_blocks_per_round() { ExtBuilder::default().build().execute_with(|| { @@ -129,10 +140,10 @@ fn set_blocks_per_round_fails_if_below_total_selected() { )); assert_ok!(ParachainStaking::set_total_selected( RuntimeOrigin::root(), - 15u32 + 10u32 )); assert_noop!( - ParachainStaking::set_blocks_per_round(RuntimeOrigin::root(), 14u32), + ParachainStaking::set_blocks_per_round(RuntimeOrigin::root(), 9u32), Error::::RoundLengthMustBeGreaterThanTotalSelectedCollators, ); }); @@ -860,6 +871,43 @@ fn sufficient_join_candidates_weight_hint_succeeds() { }); } +#[test] +fn join_candidates_fails_if_above_max_candidate_count() { + ExtBuilder::default() + .with_balances(vec![ + (1, 20), + (2, 20), + (3, 20), + (4, 20), + (5, 20), + (6, 20), + (7, 20), + (8, 20), + (9, 20), + (10, 20), + (11, 20), + ]) + .with_candidates(vec![ + (1, 20), + (2, 20), + (3, 20), + (4, 20), + (5, 20), + (6, 20), + (7, 20), + (8, 20), + (9, 20), + (10, 20), + ]) + .build() + .execute_with(|| { + assert_noop!( + ParachainStaking::join_candidates(RuntimeOrigin::signed(11), 20, 10), + Error::::CandidateLimitReached, + ); + }); +} + // SCHEDULE LEAVE CANDIDATES #[test] diff --git a/precompiles/parachain-staking/src/mock.rs b/precompiles/parachain-staking/src/mock.rs index b74576eb91..63a31ce9d7 100644 --- a/precompiles/parachain-staking/src/mock.rs +++ b/precompiles/parachain-staking/src/mock.rs @@ -172,6 +172,7 @@ parameter_types! { pub const MinCandidateStk: u128 = 10; pub const MinDelegatorStk: u128 = 5; pub const MinDelegation: u128 = 3; + pub const MaxCandidates: u32 = 10; pub BlockAuthor: AccountId = Alice.into(); } impl pallet_parachain_staking::Config for Runtime { @@ -197,6 +198,7 @@ impl pallet_parachain_staking::Config for Runtime { type OnCollatorPayout = (); type OnNewRound = (); type WeightInfo = (); + type MaxCandidates = MaxCandidates; } pub(crate) struct ExtBuilder { diff --git a/runtime/moonbase/src/lib.rs b/runtime/moonbase/src/lib.rs index 3ac5f0fbeb..13f0df3bbc 100644 --- a/runtime/moonbase/src/lib.rs +++ b/runtime/moonbase/src/lib.rs @@ -741,6 +741,7 @@ impl pallet_parachain_staking::Config for Runtime { type PayoutCollatorReward = PayoutCollatorOrOrbiterReward; type OnNewRound = OnNewRound; type WeightInfo = pallet_parachain_staking::weights::SubstrateWeight; + type MaxCandidates = ConstU32<200>; } impl pallet_author_inherent::Config for Runtime { diff --git a/runtime/moonbeam/src/lib.rs b/runtime/moonbeam/src/lib.rs index d391bc8d10..d7f0a6cba4 100644 --- a/runtime/moonbeam/src/lib.rs +++ b/runtime/moonbeam/src/lib.rs @@ -730,6 +730,7 @@ impl pallet_parachain_staking::Config for Runtime { type PayoutCollatorReward = PayoutCollatorOrOrbiterReward; type OnNewRound = OnNewRound; type WeightInfo = pallet_parachain_staking::weights::SubstrateWeight; + type MaxCandidates = ConstU32<200>; } impl pallet_author_inherent::Config for Runtime { diff --git a/runtime/moonriver/src/lib.rs b/runtime/moonriver/src/lib.rs index 4262099fa7..89c8469c68 100644 --- a/runtime/moonriver/src/lib.rs +++ b/runtime/moonriver/src/lib.rs @@ -731,6 +731,7 @@ impl pallet_parachain_staking::Config for Runtime { type PayoutCollatorReward = PayoutCollatorOrOrbiterReward; type OnNewRound = OnNewRound; type WeightInfo = pallet_parachain_staking::weights::SubstrateWeight; + type MaxCandidates = ConstU32<200>; } impl pallet_author_inherent::Config for Runtime {