From dd38694c4d6c3f82070815cf5b8098f8127fffff Mon Sep 17 00:00:00 2001 From: JesseAbram <33698952+JesseAbram@users.noreply.github.com> Date: Tue, 22 Jun 2021 18:59:14 +0200 Subject: [PATCH] min collator check (#498) * min collator check * change statemint/mine min candidates * Ci pass * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi * Apply suggestions from code review * build fixes * add error messages to errors * added validator register check Co-authored-by: Shawn Tabrizi --- pallets/collator-selection/src/lib.rs | 44 ++++++++++++-- pallets/collator-selection/src/mock.rs | 18 +++++- pallets/collator-selection/src/tests.rs | 58 +++++++++++++++++++ .../statemine-runtime/src/lib.rs | 5 ++ .../statemint-common/src/impls.rs | 15 ++++- .../statemint-runtime/src/lib.rs | 5 ++ .../westmint-runtime/src/lib.rs | 5 ++ 7 files changed, 144 insertions(+), 6 deletions(-) diff --git a/pallets/collator-selection/src/lib.rs b/pallets/collator-selection/src/lib.rs index 8285c67dede..371154d0432 100644 --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -41,6 +41,9 @@ //! The current implementation resolves congestion of [`Candidates`] in a first-come-first-serve //! manner. //! +//! Candidates will not be allowed to get kicked or leave_intent if the total number of candidates +//! fall below MinCandidates. This is for potential disaster recovery scenarios. +//! //! ### Rewards //! //! The Collator Selection pallet maintains an on-chain account (the "Pot"). In each block, the @@ -76,7 +79,7 @@ pub mod pallet { pallet_prelude::*, inherent::Vec, traits::{ - Currency, ReservableCurrency, EnsureOrigin, ExistenceRequirement::KeepAlive, + Currency, ReservableCurrency, EnsureOrigin, ExistenceRequirement::KeepAlive, ValidatorRegistration }, PalletId, }; @@ -89,6 +92,7 @@ pub mod pallet { }, weights::DispatchClass, }; + use sp_runtime::traits::Convert; use core::ops::Div; use pallet_session::SessionManager; use sp_staking::SessionIndex; @@ -127,6 +131,12 @@ pub mod pallet { /// This does not take into account the invulnerables. type MaxCandidates: Get; + /// Minimum number of candidates that we should have. This is used for disaster recovery. + /// + /// This does not take into account the invulnerables. + type MinCandidates: Get; + + /// Maximum number of invulnerables. /// /// Used only for benchmarking. @@ -135,6 +145,18 @@ pub mod pallet { // Will be kicked if block is not produced in threshold. type KickThreshold: Get; + /// A stable ID for a validator. + type ValidatorId: Member + Parameter; + + /// A conversion from account ID to validator ID. + /// + /// Its cost must be at most one storage read. + type ValidatorIdOf: Convert>; + + /// Validate a user is registered + type ValidatorRegistration: ValidatorRegistration; + + /// The weight information of this pallet. type WeightInfo: WeightInfo; } @@ -238,13 +260,24 @@ pub mod pallet { // Errors inform users that something went wrong. #[pallet::error] pub enum Error { + /// Too many candidates TooManyCandidates, + /// Too few candidates + TooFewCandidates, + /// Unknown error Unknown, + /// Permission issue Permission, + /// User is already a candidate AlreadyCandidate, + /// User is not a candidate NotCandidate, + /// User is already an Invulnerable AlreadyInvulnerable, - InvalidProof, + /// Account has no associated validator ID + NoAssociatedValidatorId, + /// Validator ID is not yet registered + ValidatorNotRegistered } #[pallet::hooks] @@ -300,6 +333,9 @@ pub mod pallet { ensure!((length as u32) < Self::desired_candidates(), 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); + 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 }; @@ -323,7 +359,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] pub fn leave_intent(origin: OriginFor) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - + ensure!(Self::candidates().len() as u32 > T::MinCandidates::get(), Error::::TooFewCandidates); let current_count = Self::try_remove_candidate(&who)?; Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) @@ -365,7 +401,7 @@ pub mod pallet { let new_candidates = candidates.into_iter().filter_map(|c| { let last_block = >::get(c.who.clone()); let since_last = now.saturating_sub(last_block); - if since_last < kick_threshold { + if since_last < kick_threshold || Self::candidates().len() as u32 <= T::MinCandidates::get() { Some(c.who) } else { let outcome = Self::try_remove_candidate(&c.who); diff --git a/pallets/collator-selection/src/mock.rs b/pallets/collator-selection/src/mock.rs index 4f48d16cd68..bbc63295814 100644 --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -18,7 +18,7 @@ use crate as collator_selection; use sp_core::H256; use frame_support::{ parameter_types, ord_parameter_types, - traits::{FindAuthor, GenesisBuild}, + traits::{FindAuthor, GenesisBuild, ValidatorRegistration}, PalletId }; use sp_runtime::{ @@ -188,6 +188,18 @@ parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); pub const MaxCandidates: u32 = 20; pub const MaxInvulnerables: u32 = 20; + pub const MinCandidates: u32 = 1; +} + +pub struct IsRegistered; +impl ValidatorRegistration for IsRegistered { + fn is_registered(id: &u64) -> bool { + if *id == 7u64 { + false + } else { + true + } + } } impl Config for Test { @@ -196,8 +208,12 @@ impl Config for Test { type UpdateOrigin = EnsureSignedBy; type PotId = PotId; type MaxCandidates = MaxCandidates; + type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; type KickThreshold = Period; + type ValidatorId = ::AccountId; + type ValidatorIdOf = IdentityCollator; + type ValidatorRegistration = IsRegistered; type WeightInfo = (); } diff --git a/pallets/collator-selection/src/tests.rs b/pallets/collator-selection/src/tests.rs index 47157fdbda4..3dc8c174269 100644 --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -106,6 +106,21 @@ fn cannot_register_candidate_if_too_many() { }) } +#[test] +fn cannot_unregister_candidate_if_too_few() { + new_test_ext().execute_with(|| { + // reset desired candidates: + >::put(1); + assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(4))); + + // can not remove too few + assert_noop!( + CollatorSelection::leave_intent(Origin::signed(4)), + Error::::TooFewCandidates, + ); + }) +} + #[test] fn cannot_register_as_candidate_if_invulnerable() { new_test_ext().execute_with(|| { @@ -119,6 +134,17 @@ fn cannot_register_as_candidate_if_invulnerable() { }) } +#[test] +fn cannot_register_as_candidate_if_keys_not_registered() { + new_test_ext().execute_with(|| { + // can't 7 because keys not registered. + assert_noop!( + CollatorSelection::register_as_candidate(Origin::signed(7)), + Error::::ValidatorNotRegistered + ); + }) +} + #[test] fn cannot_register_dupe_candidate() { new_test_ext().execute_with(|| { @@ -184,6 +210,10 @@ fn leave_intent() { assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3))); assert_eq!(Balances::free_balance(3), 90); + // register too so can leave above min candidates + assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(5))); + assert_eq!(Balances::free_balance(5), 90); + // cannot leave if not candidate. assert_noop!( CollatorSelection::leave_intent(Origin::signed(4)), @@ -318,6 +348,34 @@ fn kick_mechanism() { }); } +#[test] +fn should_not_kick_mechanism_too_few() { + new_test_ext().execute_with(|| { + // add a new collator + assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(Origin::signed(5))); + initialize_to_block(10); + assert_eq!(CollatorSelection::candidates().len(), 2); + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // 4 authored this block, 5 gets to stay too few 3 was kicked + assert_eq!(CollatorSelection::candidates().len(), 1); + // 3 will be kicked after 1 session delay + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 5]); + let collator = CandidateInfo { + who: 5, + deposit: 10, + }; + assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!(CollatorSelection::last_authored_block(4), 20); + initialize_to_block(30); + // 3 gets kicked after 1 session delay + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5]); + // kicked collator gets funds back + assert_eq!(Balances::free_balance(3), 100); + }); +} + #[test] #[should_panic = "duplicate invulnerables in genesis."] diff --git a/polkadot-parachains/statemine-runtime/src/lib.rs b/polkadot-parachains/statemine-runtime/src/lib.rs index e311f7974b8..3c0750a5f2b 100644 --- a/polkadot-parachains/statemine-runtime/src/lib.rs +++ b/polkadot-parachains/statemine-runtime/src/lib.rs @@ -638,6 +638,7 @@ impl pallet_aura::Config for Runtime { parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); pub const MaxCandidates: u32 = 1000; + pub const MinCandidates: u32 = 5; pub const SessionLength: BlockNumber = 6 * HOURS; pub const MaxInvulnerables: u32 = 100; } @@ -655,9 +656,13 @@ impl pallet_collator_selection::Config for Runtime { type UpdateOrigin = CollatorSelectionUpdateOrigin; type PotId = PotId; type MaxCandidates = MaxCandidates; + type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; // should be a multiple of session or things will get inconsistent type KickThreshold = Period; + type ValidatorId = ::AccountId; + type ValidatorIdOf = pallet_collator_selection::IdentityCollator; + type ValidatorRegistration = Session; type WeightInfo = weights::pallet_collator_selection::WeightInfo; } diff --git a/polkadot-parachains/statemint-common/src/impls.rs b/polkadot-parachains/statemint-common/src/impls.rs index 7694c89fac9..84eccd24666 100644 --- a/polkadot-parachains/statemint-common/src/impls.rs +++ b/polkadot-parachains/statemint-common/src/impls.rs @@ -65,7 +65,7 @@ where mod tests { use super::*; use frame_support::traits::FindAuthor; - use frame_support::{parameter_types, PalletId}; + use frame_support::{parameter_types, PalletId, traits::ValidatorRegistration}; use frame_system::{limits, EnsureRoot}; use polkadot_primitives::v1::AccountId; use sp_core::H256; @@ -74,6 +74,7 @@ mod tests { traits::{BlakeTwo256, IdentityLookup}, Perbill, }; + use pallet_collator_selection::IdentityCollator; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -145,10 +146,18 @@ mod tests { } } + pub struct IsRegistered; + impl ValidatorRegistration for IsRegistered { + fn is_registered(_id: &AccountId) -> bool { + true + } + } + parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); pub const MaxCandidates: u32 = 20; pub const MaxInvulnerables: u32 = 20; + pub const MinCandidates: u32 = 1; } impl pallet_collator_selection::Config for Test { @@ -157,7 +166,11 @@ mod tests { type UpdateOrigin = EnsureRoot; type PotId = PotId; type MaxCandidates = MaxCandidates; + type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; + type ValidatorId = ::AccountId; + type ValidatorIdOf = IdentityCollator; + type ValidatorRegistration = IsRegistered; type KickThreshold = (); type WeightInfo = (); } diff --git a/polkadot-parachains/statemint-runtime/src/lib.rs b/polkadot-parachains/statemint-runtime/src/lib.rs index 4eab30cd1eb..30a480341c3 100644 --- a/polkadot-parachains/statemint-runtime/src/lib.rs +++ b/polkadot-parachains/statemint-runtime/src/lib.rs @@ -579,6 +579,7 @@ impl pallet_aura::Config for Runtime { parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); pub const MaxCandidates: u32 = 1000; + pub const MinCandidates: u32 = 5; pub const SessionLength: BlockNumber = 6 * HOURS; pub const MaxInvulnerables: u32 = 100; } @@ -596,9 +597,13 @@ impl pallet_collator_selection::Config for Runtime { type UpdateOrigin = CollatorSelectionUpdateOrigin; type PotId = PotId; type MaxCandidates = MaxCandidates; + type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; // should be a multiple of session or things will get inconsistent type KickThreshold = Period; + type ValidatorId = ::AccountId; + type ValidatorIdOf = pallet_collator_selection::IdentityCollator; + type ValidatorRegistration = Session; type WeightInfo = weights::pallet_collator_selection::WeightInfo; } diff --git a/polkadot-parachains/westmint-runtime/src/lib.rs b/polkadot-parachains/westmint-runtime/src/lib.rs index bf8565d5dfa..f526882f39d 100644 --- a/polkadot-parachains/westmint-runtime/src/lib.rs +++ b/polkadot-parachains/westmint-runtime/src/lib.rs @@ -576,6 +576,7 @@ impl pallet_aura::Config for Runtime { parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); pub const MaxCandidates: u32 = 1000; + pub const MinCandidates: u32 = 1; pub const SessionLength: BlockNumber = 6 * HOURS; pub const MaxInvulnerables: u32 = 100; } @@ -586,9 +587,13 @@ impl pallet_collator_selection::Config for Runtime { type UpdateOrigin = EnsureRoot; type PotId = PotId; type MaxCandidates = MaxCandidates; + type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; // should be a multiple of session or things will get inconsistent type KickThreshold = Period; + type ValidatorId = ::AccountId; + type ValidatorIdOf = pallet_collator_selection::IdentityCollator; + type ValidatorRegistration = Session; type WeightInfo = weights::pallet_collator_selection::WeightInfo; }