Skip to content

refactor: bound entries in collator-selection #879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gregdhill merged 2 commits into from
Jan 24, 2023
Merged

refactor: bound entries in collator-selection #879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
733497a
refactor: bound entries in collator-selection
gregdhill Jan 23, 2023
c6fc462
fix: only register MaxCandidates-1 candidates initially
gregdhill Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/collator-selection/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ benchmarks! {
// worse case is when we have all the max-candidate slots filled except one, and we fill that
// one.
register_as_candidate {
let c in 1 .. T::MaxCandidates::get();
let c in 1 .. T::MaxCandidates::get() - 1;

<CandidacyBond<T>>::put(T::StakingCurrency::minimum_balance());
<DesiredCandidates<T>>::put(c + 1);
Expand Down
57 changes: 30 additions & 27 deletions crates/collator-selection/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -106,8 +106,7 @@ pub mod pallet {
/// Account Identifier from which the internal Pot is generated.
type PotId: Get<PalletId>;

/// 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<u32>;
Expand All @@ -117,9 +116,7 @@ pub mod pallet {
/// This does not take into account the invulnerables.
type MinCandidates: Get<u32>;

/// Maximum number of invulnerables.
///
/// Used only for benchmarking.
/// Maximum number of invulnerables. This is enforced in code.
type MaxInvulnerables: Get<u32>;

// Will be kicked if block is not produced in threshold.
Expand All @@ -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<AccountId, Balance> {
/// Account identifier.
pub who: AccountId,
Expand All @@ -151,18 +148,18 @@ pub mod pallet {

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
#[pallet::without_storage_info]
pub struct Pallet<T>(_);

/// The invulnerable, fixed collators.
#[pallet::storage]
#[pallet::getter(fn invulnerables)]
pub type Invulnerables<T: Config> = StorageValue<_, Vec<T::AccountId>, ValueQuery>;
pub type Invulnerables<T: Config> = StorageValue<_, BoundedVec<T::AccountId, T::MaxInvulnerables>, ValueQuery>;

/// The (community, limited) collation candidates.
#[pallet::storage]
#[pallet::getter(fn candidates)]
pub type Candidates<T: Config> = StorageValue<_, Vec<CandidateInfo<T::AccountId, BalanceOf<T>>>, ValueQuery>;
pub type Candidates<T: Config> =
StorageValue<_, BoundedVec<CandidateInfo<T::AccountId, BalanceOf<T>>, T::MaxCandidates>, ValueQuery>;

/// Last block authored by collator.
#[pallet::storage]
Expand Down Expand Up @@ -210,18 +207,16 @@ 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",
);

<DesiredCandidates<T>>::put(&self.desired_candidates);
<CandidacyBond<T>>::put(&self.candidacy_bond);
<Invulnerables<T>>::put(&self.invulnerables);
<Invulnerables<T>>::put(bounded_invulnerables);
}
}

Expand Down Expand Up @@ -261,6 +256,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
Expand All @@ -279,13 +276,11 @@ pub mod pallet {
#[pallet::weight(T::WeightInfo::set_invulnerables(new.len() as u32))]
pub fn set_invulnerables(origin: OriginFor<T>, new: Vec<T::AccountId>) -> 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::<T>::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::<T>::NoAssociatedValidatorId)?;
ensure!(
Expand All @@ -294,8 +289,10 @@ pub mod pallet {
);
}

<Invulnerables<T>>::put(&new);
Self::deposit_event(Event::NewInvulnerables { invulnerables: new });
<Invulnerables<T>>::put(&bounded_invulnerables);
Self::deposit_event(Event::NewInvulnerables {
invulnerables: bounded_invulnerables.to_vec(),
});
Ok(().into())
}

Expand Down Expand Up @@ -362,7 +359,9 @@ pub mod pallet {
Err(Error::<T>::AlreadyCandidate)?
} else {
T::StakingCurrency::reserve(&who, deposit)?;
candidates.push(incoming);
candidates
.try_push(incoming)
.map_err(|_| Error::<T>::TooManyCandidates)?;
<LastAuthoredBlock<T>>::insert(
who.clone(),
frame_system::Pallet::<T>::block_number() + T::KickThreshold::get(),
Expand Down Expand Up @@ -425,15 +424,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<T::AccountId>) -> Vec<T::AccountId> {
let mut collators = Self::invulnerables();
pub fn assemble_collators(candidates: BoundedVec<T::AccountId, T::MaxCandidates>) -> Vec<T::AccountId> {
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<CandidateInfo<T::AccountId, BalanceOf<T>>>) -> Vec<T::AccountId> {
pub fn kick_stale_candidates(
candidates: BoundedVec<CandidateInfo<T::AccountId, BalanceOf<T>>, T::MaxCandidates>,
) -> BoundedVec<T::AccountId, T::MaxCandidates> {
let now = frame_system::Pallet::<T>::block_number();
let kick_threshold = T::KickThreshold::get();
let candidacy_bond = Self::candidacy_bond();
Expand All @@ -460,7 +461,9 @@ pub mod pallet {
None
}
})
.collect()
.collect::<Vec<_>>()
.try_into()
.expect("filter_map operation can't result in a bounded vec larger than its original; qed")
}
}

Expand Down