From 1e14fe80093b32d1d5066698ce3511e5b07065a8 Mon Sep 17 00:00:00 2001 From: Fahrrader Date: Wed, 21 Dec 2022 08:12:50 +0000 Subject: [PATCH] feat(collator-selection): add+remove invulnerable methods + tests + miscellaneous changes before more refactoring --- --- a/node/cli/src/chain_spec.rs +++ b/node/cli/src/chain_spec.rs @@ -24,7 +24,7 @@ use serde_json::map::Map; use up_common::types::opaque::*; -use up_common::constants::GENESIS_CANDIDACY_BOND; +use up_common::constants::{GENESIS_CANDIDACY_BOND, SESSION_LENGTH}; #[cfg(feature = "unique-runtime")] pub use unique_runtime as default_runtime; @@ -197,6 +197,7 @@ .map(|(acc, _)| acc) .collect(), candidacy_bond: GENESIS_CANDIDACY_BOND, + kick_threshold: SESSION_LENGTH, ..Default::default() }, session: SessionConfig { --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -112,8 +112,13 @@ } fn register_candidates(count: u32) { - let candidates = (0..count).map(|c| account("candidate", c, SEED)).collect::>(); - assert!(>::get() > 0u32.into(), "Bond cannot be zero!"); + let candidates = (0..count) + .map(|c| account("candidate", c, SEED)) + .collect::>(); + assert!( + >::get() > 0u32.into(), + "Bond cannot be zero!" + ); for who in candidates { T::Currency::make_free_balance_be(&who, >::get() * 2u32.into()); @@ -200,7 +205,8 @@ whitelist!(leaving); }: _(RawOrigin::Signed(leaving.clone())) verify { - assert_last_event::(Event::CandidateRemoved{account_id: leaving}.into()); + // todo:collator verify these + assert_last_event::(Event::CandidateRemoved{account_id: leaving, deposit_returned: bond / 2u32.into() }.into()); } // worse case is paying a non-existing candidate account. @@ -272,4 +278,8 @@ } } -impl_benchmark_test_suite!(CollatorSelection, crate::mock::new_test_ext(), crate::mock::Test,); +impl_benchmark_test_suite!( + CollatorSelection, + crate::mock::new_test_ext(), + crate::mock::Test, +); --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -30,6 +30,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +// todo:collator documentation //! Collator Selection pallet. //! //! A pallet to manage collators in a parachain. @@ -109,7 +110,10 @@ }; use frame_system::{pallet_prelude::*, Config as SystemConfig}; use pallet_session::SessionManager; - use sp_runtime::traits::Convert; + use sp_runtime::{ + Perbill, + traits::{One, Convert}, + }; use sp_staking::SessionIndex; type BalanceOf = @@ -136,6 +140,9 @@ /// Origin that can dictate updating parameters of this pallet. type UpdateOrigin: EnsureOrigin; + /// Account Identifier that holds the chain's treasury. + type TreasuryAccountId: Get; + /// Account Identifier from which the internal Pot is generated. type PotId: Get; @@ -152,8 +159,8 @@ /// Maximum number of invulnerables. This is enforced in code. type MaxInvulnerables: Get; - // Will be kicked if block is not produced in threshold. - type KickThreshold: Get; + /// If kicked, how much of the collator's deposit will be slashed and sent to the slash destination. + type SlashRatio: Get; /// A stable ID for a validator. type ValidatorId: Member + Parameter; @@ -200,6 +207,13 @@ ValueQuery, >; + /// Collator will be kicked if it does not produce a block within the threshold (does not apply to invulnerables). + /// + /// Should be a multiple of session or things will get inconsistent. todo:collator reword? + #[pallet::storage] + #[pallet::getter(fn kick_threshold)] + pub type KickThreshold = StorageValue<_, T::BlockNumber, ValueQuery>; + /// Last block authored by collator. #[pallet::storage] #[pallet::getter(fn last_authored_block)] @@ -224,6 +238,7 @@ pub struct GenesisConfig { pub invulnerables: Vec, pub candidacy_bond: BalanceOf, + pub kick_threshold: T::BlockNumber, pub desired_candidates: u32, } @@ -233,6 +248,7 @@ Self { invulnerables: Default::default(), candidacy_bond: Default::default(), + kick_threshold: T::BlockNumber::one(), desired_candidates: Default::default(), } } @@ -241,8 +257,10 @@ #[pallet::genesis_build] impl GenesisBuild for GenesisConfig { fn build(&self) { - let duplicate_invulnerables = - self.invulnerables.iter().collect::>(); + let duplicate_invulnerables = self + .invulnerables + .iter() + .collect::>(); assert!( duplicate_invulnerables.len() == self.invulnerables.len(), "duplicate invulnerables in genesis." @@ -258,6 +276,7 @@ >::put(&self.desired_candidates); >::put(&self.candidacy_bond); + >::put(&self.kick_threshold); >::put(bounded_invulnerables); } } @@ -265,11 +284,29 @@ #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { - NewInvulnerables { invulnerables: Vec }, - NewDesiredCandidates { desired_candidates: u32 }, - NewCandidacyBond { bond_amount: BalanceOf }, - CandidateAdded { account_id: T::AccountId, deposit: BalanceOf }, - CandidateRemoved { account_id: T::AccountId }, + NewDesiredCandidates { + desired_candidates: u32, + }, + NewCandidacyBond { + bond_amount: BalanceOf, + }, + NewKickThreshold { + length_in_blocks: T::BlockNumber, + }, + InvulnerableAdded { + invulnerable: T::AccountId, + }, + InvulnerableRemoved { + invulnerable: T::AccountId, + }, + CandidateAdded { + account_id: T::AccountId, + deposit: BalanceOf, + }, + CandidateRemoved { + account_id: T::AccountId, + deposit_returned: BalanceOf, + }, } // Errors inform users that something went wrong. @@ -289,8 +326,12 @@ NotCandidate, /// Too many invulnerables TooManyInvulnerables, + /// Too few invulnerables + TooFewInvulnerables, /// User is already an Invulnerable AlreadyInvulnerable, + /// User is not an Invulnerable + NotInvulnerable, /// Account has no associated validator ID NoAssociatedValidatorId, /// Validator ID is not yet registered @@ -302,33 +343,61 @@ #[pallet::call] impl Pallet { - /// Set the list of invulnerable (fixed) collators. - #[pallet::weight(T::WeightInfo::set_invulnerables(new.len() as u32))] - pub fn set_invulnerables( + /// Add a collator to the list of invulnerable (fixed) collators. + #[pallet::weight(T::WeightInfo::set_invulnerables(1 as u32))] // todo:collator weight + pub fn add_invulnerable( origin: OriginFor, - new: Vec, + new: T::AccountId, ) -> DispatchResultWithPostInfo { T::UpdateOrigin::ensure_origin(origin)?; - 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 bounded_invulnerables.iter() { - let validator_key = T::ValidatorIdOf::convert(account_id.clone()) - .ok_or(Error::::NoAssociatedValidatorId)?; - ensure!( - T::ValidatorRegistration::is_registered(&validator_key), - Error::::ValidatorNotRegistered - ); + // check if the new invulnerable has associated validator keys before it is added + let validator_key = T::ValidatorIdOf::convert(new.clone()) + .ok_or(Error::::NoAssociatedValidatorId)?; + ensure!( + T::ValidatorRegistration::is_registered(&validator_key), + Error::::ValidatorNotRegistered + ); + // ensure!(!Self::invulnerables().contains(&new), Error::::AlreadyInvulnerable); + if Self::invulnerables().contains(&new) { + return Ok(().into()); } - >::put(&bounded_invulnerables); - Self::deposit_event(Event::NewInvulnerables { - invulnerables: bounded_invulnerables.to_vec(), - }); + >::try_append(new.clone()) + .map_err(|_| Error::::TooManyInvulnerables)?; + Self::deposit_event(Event::InvulnerableAdded { invulnerable: new }); Ok(().into()) } + /// Remove a collator from the list of invulnerable (fixed) collators. + #[pallet::weight(T::WeightInfo::set_invulnerables(1))] // todo:collator weight + pub fn remove_invulnerable( + origin: OriginFor, + who: T::AccountId, + ) -> DispatchResultWithPostInfo { + T::UpdateOrigin::ensure_origin(origin)?; + + // let index = Self::invulnerables().into_iter().position(|r| r == who).ok_or(Error::::NotInvulnerable)?; + >::try_mutate(|invulnerables| -> DispatchResult { + if invulnerables.len() <= 1 { + return Err(Error::::TooFewInvulnerables.into()); + } + + let index = invulnerables + .into_iter() + .position(|r| *r == who) + .ok_or(Error::::NotInvulnerable)?; + invulnerables.remove(index); + Ok(()) + })?; + /*let bounded_invulnerables = BoundedVec::<_, T::MaxInvulnerables>::try_from(new) + .map_err(|_| Error::::TooManyInvulnerables)?; + + >::put(&bounded_invulnerables);*/ + Self::deposit_event(Event::InvulnerableRemoved { invulnerable: who }); + Ok(().into()) + } + /// Set the ideal number of collators (not including the invulnerables). /// If lowering this number, then the number of running collators could be higher than this figure. /// Aside from that edge case, there should be no other way to have more collators than the desired number. @@ -343,7 +412,9 @@ log::warn!("max > T::MaxCandidates; you might need to run benchmarks again"); } >::put(&max); - Self::deposit_event(Event::NewDesiredCandidates { desired_candidates: max }); + Self::deposit_event(Event::NewDesiredCandidates { + desired_candidates: max, + }); Ok(().into()) } @@ -359,6 +430,22 @@ Ok(().into()) } + /// Set the length of the kick threshold. + /// Note that if the length is not a multiple of the session period, it might get inconsistent. + #[pallet::weight(T::WeightInfo::set_candidacy_bond())] // todo:collator weight + pub fn set_kick_threshold( + origin: OriginFor, + kick_threshold: T::BlockNumber, + ) -> DispatchResultWithPostInfo { + T::UpdateOrigin::ensure_origin(origin)?; + // todo:collator insert something to guarantee consistency? + >::put(kick_threshold); + Self::deposit_event(Event::NewKickThreshold { + length_in_blocks: kick_threshold, + }); + Ok(().into()) + } + /// Register this account as a collator candidate. The account must (a) already have /// registered session keys and (b) be able to reserve the `CandidacyBond`. /// @@ -369,8 +456,15 @@ // ensure we are below limit. let length = >::decode_len().unwrap_or_default(); - ensure!((length as u32) < Self::desired_candidates(), Error::::TooManyCandidates); - ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); + ensure!( + (length as u32) < Self::desired_candidates(), + Error::::TooManyCandidates + ); + // todo:collator really need it? + ensure!( + !Self::invulnerables().contains(&who), + Error::::AlreadyInvulnerable + ); let validator_key = T::ValidatorIdOf::convert(who.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; @@ -381,7 +475,10 @@ 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 incoming = CandidateInfo { + who: who.clone(), + deposit, + }; let current_count = >::try_mutate(|candidates| -> Result { @@ -389,16 +486,21 @@ Err(Error::::AlreadyCandidate)? } else { T::Currency::reserve(&who, deposit)?; - candidates.try_push(incoming).map_err(|_| Error::::TooManyCandidates)?; + candidates + .try_push(incoming) + .map_err(|_| Error::::TooManyCandidates)?; >::insert( who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), + frame_system::Pallet::::block_number() + Self::kick_threshold(), ); Ok(candidates.len()) } })?; - Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); + Self::deposit_event(Event::CandidateAdded { + account_id: who, + deposit, + }); Ok(Some(T::WeightInfo::register_as_candidate(current_count as u32)).into()) } @@ -411,11 +513,12 @@ #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] pub fn leave_intent(origin: OriginFor) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; + // todo:collator invulnerables and candidates should count against min candidates together ensure!( Self::candidates().len() as u32 > T::MinCandidates::get(), Error::::TooFewCandidates ); - let current_count = Self::try_remove_candidate(&who)?; + let current_count = Self::try_remove_candidate(&who, false)?; Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) } @@ -427,8 +530,12 @@ T::PotId::get().into_account_truncating() } - /// Removes a candidate if they exist and sends them back their deposit - fn try_remove_candidate(who: &T::AccountId) -> Result { + /// Removes a candidate if they exist and sends them back their deposit, optionally slashed. + fn try_remove_candidate( + who: &T::AccountId, + should_slash: bool, + ) -> Result { + let mut deposit_returned = BalanceOf::::default(); let current_count = >::try_mutate(|candidates| -> Result { let index = candidates @@ -436,11 +543,33 @@ .position(|candidate| candidate.who == *who) .ok_or(Error::::NotCandidate)?; let candidate = candidates.remove(index); - T::Currency::unreserve(who, candidate.deposit); + let deposit = candidate.deposit; + + if should_slash { + let slashed = T::SlashRatio::get() * deposit; + let remaining = deposit - slashed; + + let (imbalance, _) = T::Currency::slash_reserved(who, slashed); + //T::Currency::unreserve(who, remaining); + deposit_returned = remaining; + + T::Currency::resolve_creating(&T::TreasuryAccountId::get(), imbalance); + + // Self::deposit_event(Event::CandidateSlashed(who.clone())); + } else { + //T::Currency::unreserve(who, deposit); + deposit_returned = deposit; + } + + T::Currency::unreserve(who, deposit_returned); + // candidates.remove(index); >::remove(who.clone()); Ok(candidates.len()) })?; - Self::deposit_event(Event::CandidateRemoved { account_id: who.clone() }); + Self::deposit_event(Event::CandidateRemoved { + account_id: who.clone(), + deposit_returned, + }); Ok(current_count) } @@ -456,12 +585,12 @@ } /// Kicks out candidates that did not produce a block in the kick threshold - /// and refund their deposits. + /// and **confiscates** their deposits to the treasury. pub fn kick_stale_candidates( candidates: BoundedVec>, T::MaxCandidates>, ) -> BoundedVec { let now = frame_system::Pallet::::block_number(); - let kick_threshold = T::KickThreshold::get(); + let kick_threshold = Self::kick_threshold(); candidates .into_iter() .filter_map(|c| { @@ -472,7 +601,7 @@ { Some(c.who) } else { - let outcome = Self::try_remove_candidate(&c.who); + let outcome = Self::try_remove_candidate(&c.who, true); if let Err(why) = outcome { log::warn!("Failed to remove candidate {:?}", why); debug_assert!(false, "failed to remove candidate {:?}", why); --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -43,7 +43,7 @@ use sp_runtime::{ testing::{Header, UintAuthorityId}, traits::{BlakeTwo256, IdentityLookup, OpaqueKeys}, - RuntimeAppPublic, + Perbill, RuntimeAppPublic, }; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -210,6 +210,7 @@ pub const MaxInvulnerables: u32 = 20; pub const MinCandidates: u32 = 1; pub const MaxAuthorities: u32 = 100_000; + pub const SlashRatio: Perbill = Perbill::one(); } pub struct IsRegistered; @@ -224,6 +225,7 @@ } impl Config for Test { + // todo:collator mocks and stocks type RuntimeEvent = RuntimeEvent; type Currency = Balances; type UpdateOrigin = EnsureSignedBy; @@ -231,7 +233,9 @@ type MaxCandidates = MaxCandidates; type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; - type KickThreshold = Period; + // type KickThreshold = Period; + type SlashRatio = SlashRatio; + type TreasuryAccountId = (); type ValidatorId = ::AccountId; type ValidatorIdOf = IdentityCollator; type ValidatorRegistration = IsRegistered; @@ -240,17 +244,28 @@ pub fn new_test_ext() -> sp_io::TestExternalities { sp_tracing::try_init_simple(); - let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); + let mut t = frame_system::GenesisConfig::default() + .build_storage::() + .unwrap(); let invulnerables = vec![1, 2]; let balances = vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100)]; let keys = balances .iter() - .map(|&(i, _)| (i, i, MockSessionKeys { aura: UintAuthorityId(i) })) + .map(|&(i, _)| { + ( + i, + i, + MockSessionKeys { + aura: UintAuthorityId(i), + }, + ) + }) .collect::>(); let collator_selection = collator_selection::GenesisConfig:: { desired_candidates: 2, candidacy_bond: 10, + kick_threshold: 1, invulnerables, }; let session = pallet_session::GenesisConfig:: { keys }; --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -113,7 +113,10 @@ assert_eq!(CollatorSelection::candidacy_bond(), 7); // rejects bad origin. - assert_noop!(CollatorSelection::set_candidacy_bond(RuntimeOrigin::signed(1), 8), BadOrigin); + assert_noop!( + CollatorSelection::set_candidacy_bond(RuntimeOrigin::signed(1), 8), + BadOrigin + ); }); } @@ -131,7 +134,9 @@ // reset desired candidates: >::put(1); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); // but no more assert_noop!( @@ -146,7 +151,9 @@ new_test_ext().execute_with(|| { // reset desired candidates: >::put(1); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); // can not remove too few assert_noop!( @@ -184,8 +191,13 @@ 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_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(3) + )); + let addition = CandidateInfo { + who: 3, + deposit: 10, + }; assert_eq!(CollatorSelection::candidates(), vec![addition]); assert_eq!(CollatorSelection::last_authored_block(3), 10); assert_eq!(Balances::free_balance(3), 90); @@ -205,7 +217,9 @@ assert_eq!(Balances::free_balance(&33), 0); // works - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(3) + )); // poor assert_noop!( @@ -228,8 +242,12 @@ assert_eq!(Balances::free_balance(&3), 100); assert_eq!(Balances::free_balance(&4), 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(3) + )); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); assert_eq!(Balances::free_balance(&3), 90); assert_eq!(Balances::free_balance(&4), 90); @@ -242,11 +260,15 @@ fn leave_intent() { new_test_ext().execute_with(|| { // register a candidate. - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(3) + )); assert_eq!(Balances::free_balance(3), 90); // register too so can leave above min candidates - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(5) + )); assert_eq!(Balances::free_balance(5), 90); // cannot leave if not candidate. @@ -270,11 +292,16 @@ // 4 is the default author. assert_eq!(Balances::free_balance(4), 100); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); // triggers `note_author` Authorship::on_initialize(1); - let collator = CandidateInfo { who: 4, deposit: 10 }; + let collator = CandidateInfo { + who: 4, + deposit: 10, + }; assert_eq!(CollatorSelection::candidates(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 0); @@ -295,11 +322,16 @@ Balances::make_free_balance_be(&CollatorSelection::account_id(), 5); // 4 is the default author. assert_eq!(Balances::free_balance(4), 100); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); // triggers `note_author` Authorship::on_initialize(1); - let collator = CandidateInfo { who: 4, deposit: 10 }; + let collator = CandidateInfo { + who: 4, + deposit: 10, + }; assert_eq!(CollatorSelection::candidates(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 0); @@ -324,7 +356,9 @@ assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // add a new collator - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(3) + )); // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); @@ -351,8 +385,12 @@ fn kick_mechanism() { new_test_ext().execute_with(|| { // add a new collator - 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(3) + )); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(4) + )); initialize_to_block(10); assert_eq!(CollatorSelection::candidates().len(), 2); initialize_to_block(20); @@ -361,7 +399,10 @@ assert_eq!(CollatorSelection::candidates().len(), 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 }; + let collator = CandidateInfo { + who: 4, + deposit: 10, + }; assert_eq!(CollatorSelection::candidates(), vec![collator]); assert_eq!(CollatorSelection::last_authored_block(4), 20); initialize_to_block(30); @@ -376,8 +417,12 @@ fn should_not_kick_mechanism_too_few() { new_test_ext().execute_with(|| { // add a new collator - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5))); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(3) + )); + assert_ok!(CollatorSelection::register_as_candidate( + RuntimeOrigin::signed(5) + )); initialize_to_block(10); assert_eq!(CollatorSelection::candidates().len(), 2); initialize_to_block(20); @@ -386,7 +431,10 @@ 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 }; + 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); @@ -401,7 +449,9 @@ #[should_panic = "duplicate invulnerables in genesis."] fn cannot_set_genesis_value_twice() { sp_tracing::try_init_simple(); - let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); + let mut t = frame_system::GenesisConfig::default() + .build_storage::() + .unwrap(); let invulnerables = vec![1, 1]; let collator_selection = collator_selection::GenesisConfig:: { --- a/pallets/collator-selection/src/weights.rs +++ b/pallets/collator-selection/src/weights.rs @@ -41,6 +41,7 @@ }; use sp_std::marker::PhantomData; +// todo:collator re-generate weights // The weight info trait for `pallet_collator_selection`. pub trait WeightInfo { fn set_invulnerables(_b: u32) -> Weight; --- a/primitives/common/src/constants.rs +++ b/primitives/common/src/constants.rs @@ -46,6 +46,8 @@ pub const EXISTENTIAL_DEPOSIT: u128 = 0; /// Amount of Balance reserved for candidate registration. pub const GENESIS_CANDIDACY_BOND: u128 = EXISTENTIAL_DEPOSIT; +/// How long a periodic session lasts in blocks. +pub const SESSION_LENGTH: BlockNumber = MINUTES; // Targeting 0.1 UNQ per transfer pub const WEIGHT_TO_FEE_COEFF: u32 = /**/175_199_920/**/; --- a/runtime/common/config/pallets/collator_selection.rs +++ b/runtime/common/config/pallets/collator_selection.rs @@ -18,12 +18,13 @@ use frame_system::EnsureRoot; use crate::{ AccountId, BlockNumber, Runtime, RuntimeEvent, Balances, Aura, Session, SessionKeys, - CollatorSelection, + CollatorSelection, config::pallets::TreasuryAccountId, }; +use sp_runtime::Perbill; use up_common::constants::*; parameter_types! { - pub const SessionPeriod: BlockNumber = HOURS; + pub const SessionPeriod: BlockNumber = SESSION_LENGTH; pub const SessionOffset: BlockNumber = 0; } @@ -54,9 +55,10 @@ parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); - pub const MaxCandidates: u32 = 1000; - pub const MinCandidates: u32 = 5; - pub const MaxInvulnerables: u32 = 100; + pub const MaxCandidates: u32 = 30; // todo:collator 30 collator slots - 3 planned invulnerables + pub const MinCandidates: u32 = 1; + pub const MaxInvulnerables: u32 = 30; + pub const SlashRatio: Perbill = Perbill::from_percent(100); } impl pallet_collator_selection::Config for Runtime { @@ -64,13 +66,13 @@ type Currency = Balances; // We allow root only to execute privileged collator selection operations. type UpdateOrigin = EnsureRoot; + type TreasuryAccountId = TreasuryAccountId; type PotId = PotId; type MaxCandidates = MaxCandidates; type MinCandidates = MinCandidates; type MaxInvulnerables = MaxInvulnerables; // todo:collator kick threshold should be in storage and configured only by root -- or rather UpdateOrigin - // Should be a multiple of session or things will get inconsistent. - type KickThreshold = SessionPeriod; + type SlashRatio = SlashRatio; type ValidatorId = ::AccountId; type ValidatorIdOf = pallet_collator_selection::IdentityCollator; type ValidatorRegistration = Session; --- a/runtime/common/runtime_apis.rs +++ b/runtime/common/runtime_apis.rs @@ -703,6 +703,10 @@ #[cfg(feature = "rmrk")] list_benchmark!(list, extra, pallet_proxy_rmrk_equip, RmrkEquip); + // todo:collator check benchmarks + #[cfg(feature = "collator-selection")] + list_benchmark!(list, extra, pallet_collator_selection, CollatorSelection); + #[cfg(feature = "foreign-assets")] list_benchmark!(list, extra, pallet_foreign_assets, ForeignAssets); @@ -766,6 +770,10 @@ #[cfg(feature = "rmrk")] add_benchmark!(params, batches, pallet_proxy_rmrk_equip, RmrkEquip); + // todo:collator check benchmarks + #[cfg(feature = "collator-selection")] + add_benchmark!(params, batches, pallet_collator_selection, CollatorSelection); + #[cfg(feature = "foreign-assets")] add_benchmark!(params, batches, pallet_foreign_assets, ForeignAssets); --- a/tests/src/collatorSelection.test.ts +++ b/tests/src/collatorSelection.test.ts @@ -17,90 +17,256 @@ import {IKeyringPair} from '@polkadot/types/types'; import {usingPlaygrounds, expect, itSub, Pallets, requirePalletsOrSkip} from './util'; -// todo Most preferable to launch this test in parallel somehow -- or change the session period (1 hr). -describe('Integration Test: Dynamic shuffling of collators', () => { +async function resetInvulnerables() { + await usingPlaygrounds(async (helper, privateKey) => { + const superuser = await privateKey('//Alice'); + const alice = await privateKey('//Alice'); + const bob = await privateKey('//Bob'); + const invulnerables = await helper.collatorSelection.getInvulnerables(); + if (!invulnerables.includes(alice.address) || !invulnerables.includes(bob.address) || invulnerables.length != 2) { + console.warn('Alice and Bob are not the invulnerables! Reinstating them back. ' + + 'Current invulnerables\' size: ' + invulnerables.length); + + let nonce = await helper.chain.getNonce(alice.address); + await Promise.all([ + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [alice.address], true, {nonce: nonce++}), + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [bob.address], true, {nonce: nonce++}), + ]); + + nonce = await helper.chain.getNonce(alice.address); + await Promise.all(invulnerables.map((invulnerable: any) => { + if (invulnerable == alice.address || invulnerable == bob.address) return new Promise(res => res()); + return helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [invulnerable], true, {nonce: nonce++}); + })); + } + }); +} + +// todo:collator Most preferable to launch this test in parallel somehow -- or change the session period (1 hr). +// + 18 tests: 5 (1+4) on session change +describe('Integration Test: Collator Selection', () => { let superuser: IKeyringPair; // These are the default invulnerables, and should return to be invulnerables after this suite. - let aliceAddress: string; - let bobAddress: string; + let alice: IKeyringPair; + let bob: IKeyringPair; let charlie: IKeyringPair; let dave: IKeyringPair; //let eve: IKeyringPair; - before(async function() { + before(async function() { await usingPlaygrounds(async (helper, privateKey) => { requirePalletsOrSkip(this, helper, [Pallets.CollatorSelection]); + //todo:collator //const donor = await privateKey({filename: __filename}); //[charlie, dave] = await helper.arrange.createAccounts([100n, 100n], donor); + alice = await privateKey('//Alice'); + bob = await privateKey('//Bob'); charlie = await privateKey('//Charlie'); dave = await privateKey('//Dave'); superuser = await privateKey('//Alice'); - aliceAddress = (await privateKey('//Alice')).address; - bobAddress = (await privateKey('//Bob')).address; + }); + }); + + describe('Dynamic shuffling of collators', () => { + before(async function() { + await usingPlaygrounds(async (helper) => { + expect((await helper.collatorSelection.setOwnKeys(charlie)) + .status.toLowerCase()).to.be.equal('success'); + expect((await helper.collatorSelection.setOwnKeys(dave)) + .status.toLowerCase()).to.be.equal('success'); + + // todo:collator check necessity + add RPC for invulnerables / just improve in general + // validators = await helper.callRpc('api.query.session.validators'); + const invulnerables = await helper.callRpc('api.query.collatorSelection.invulnerables'); + if (!invulnerables.includes(alice.address) || !invulnerables.includes(bob.address) || invulnerables.length != 2) { + console.warn('Alice and Bob are not the invulnerables! Reinstating them back. ' + + 'Current invulnerables\' size: ' + invulnerables.length); + + await Promise.all([ + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [alice.address], true, {nonce: 0}), + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [bob.address], true, {nonce: 1}), + ]); + + let nonce = 0; + await Promise.all(invulnerables.map((invulnerable: any) => { + if (invulnerable == alice.address || invulnerable == bob.address) return new Promise((res) => res); + return helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [invulnerable], true, {nonce: nonce++}); + })); + } + }); + }); + + itSub('Change invulnerables and make sure they start producing blocks', async ({helper}) => { + await expect(Promise.all([ + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [charlie.address], true, {nonce: 0}), + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [dave.address], true, {nonce: 1}), + ])).to.be.fulfilled; + + await expect(Promise.all([ + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [alice.address], true, {nonce: 0}), + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [bob.address], true, {nonce: 1}), + ])).to.be.fulfilled; + + const newInvulnerables = await helper.callRpc('api.query.collatorSelection.invulnerables'); + expect(newInvulnerables).to.contain(charlie.address).and.contain(dave.address).and.be.length(2); + + const expectedSessionIndex = (await helper.callRpc('api.query.session.currentIndex')).toNumber() + 2; + let currentSessionIndex = -1; + console.log('Waiting for the session after the next.' + + ' This might take a while -- check SessionPeriod in pallet_session::Config for session time.'); + + while (currentSessionIndex < expectedSessionIndex) { + // eslint-disable-next-line no-async-promise-executor + currentSessionIndex = await expect(helper.wait.withTimeout(new Promise(async (resolve) => { + //todo:collator + console.log('starting wait...'); + console.time('ein'); + await helper.wait.newBlocks(1); + console.timeLog('ein'); + const res = (await helper.callRpc('api.query.session.currentIndex')).toNumber(); + console.timeEnd('ein'); + resolve(res); + }), 24000, 'The chain has stopped producing blocks!')).to.be.fulfilled; + } + + const newValidators = await helper.callRpc('api.query.session.validators'); + expect(newValidators).to.contain(charlie.address).and.contain(dave.address).and.be.length(2); + + const lastBlockNumber = await helper.chain.getLatestBlockNumber(); + await helper.wait.newBlocks(1); + const lastCharlieBlock = (await helper.callRpc('api.query.collatorSelection.lastAuthoredBlock', [charlie.address])).toNumber(); + const lastDaveBlock = (await helper.callRpc('api.query.collatorSelection.lastAuthoredBlock', [dave.address])).toNumber(); + expect(lastCharlieBlock >= lastBlockNumber || lastDaveBlock >= lastBlockNumber).to.be.true; + }); + + // todo:collator keyless invulnerables? will hang, so, a breaking test, eh + // register candidate without sudos and the like + + after(async () => { + await usingPlaygrounds(async (helper) => { + if (helper.fetchMissingPalletNames([Pallets.CollatorSelection]).length != 0) return; + + await Promise.all([ + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [alice.address], true, {nonce: 0}), + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [bob.address], true, {nonce: 1}), + ]); + + await Promise.all([ + await helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [charlie.address], true, {nonce: 0}), + await helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [dave.address], true, {nonce: 1}), + ]); + }); + }); + }); + + // todo:collator make sure that there is enough session time for a set of tests + // 28 non-functioning collators, teehee. + + describe('Addition and removal of invulnerables', () => { + before(async function() { + await resetInvulnerables(); + }); - expect((await helper.executeExtrinsic(charlie, 'api.tx.session.setKeys', [ - '0x' + Buffer.from(charlie.addressRaw).toString('hex'), - '0x0', - ])).status.toLowerCase()).to.be.equal('success'); + describe('Positive', () => { + itSub('Adds an invulnerable', async ({helper}) => { + const [account] = await helper.arrange.createAccounts([10n], superuser); + const invulnerables = await helper.collatorSelection.getInvulnerables(); - expect((await helper.executeExtrinsic(dave, 'api.tx.session.setKeys', [ - '0x' + Buffer.from(dave.addressRaw).toString('hex'), - '0x0', - ])).status.toLowerCase()).to.be.equal('success'); + await helper.collatorSelection.setOwnKeys(account); + await helper.getSudo().collatorSelection.addInvulnerable(superuser, account.address); + + const newInvulnerables = await helper.collatorSelection.getInvulnerables(); + expect(invulnerables.concat(account.address)).to.have.all.members(newInvulnerables); + }); - const validators = await helper.callRpc('api.query.session.validators'); - expect(validators).to.not.contain(charlie.address).and.not.contain(dave.address); + itSub('Removes an invulnerable', async ({helper}) => { + const invulnerables = await helper.collatorSelection.getInvulnerables(); + const lastInvulnerable = invulnerables.pop(); + + await helper.getSudo().collatorSelection.removeInvulnerable(superuser, lastInvulnerable); + const newInvulnerables = await helper.collatorSelection.getInvulnerables(); + // invulnerables had its last element removed, so they should be equal + expect(newInvulnerables).to.have.all.members(invulnerables); + }); }); - }); - itSub('Change invulnerables and make sure they start producing blocks', async ({helper}) => { + describe('Negative', () => { + itSub('Does not duplicate an invulnerable', async ({helper}) => { + const invulnerables = await helper.collatorSelection.getInvulnerables(); + // adding an already invulnerable should not fail, but should not duplicate it either + await expect(helper.getSudo().collatorSelection.addInvulnerable(superuser, invulnerables[0])) + .to.be.fulfilled; + const newInvulnerables = await helper.collatorSelection.getInvulnerables(); + expect(newInvulnerables).to.have.all.members(invulnerables); + }); + + itSub('Cannot allow invulnerables to be empty', async ({helper}) => { + const invulnerables = await helper.collatorSelection.getInvulnerables(); + const lastInvulnerable = invulnerables.pop(); + + let nonce = await helper.chain.getNonce(superuser.address); + await Promise.all(invulnerables.map((i: any) => + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [i], true, {nonce: nonce++}))); - const tx = helper.constructApiCall('api.tx.collatorSelection.setInvulnerables', [[ - charlie.address, - dave.address, - ]]); - await expect(helper.executeExtrinsic(superuser, 'api.tx.sudo.sudo', [tx])).to.be.fulfilled; + await expect(helper.getSudo().collatorSelection.removeInvulnerable(superuser, lastInvulnerable)) + .to.be.rejected;//todo:collator With(/collatorSelection.TooFewInvulnerables/); + + const newInvulnerables = await helper.collatorSelection.getInvulnerables(); + expect(newInvulnerables).to.be.deep.equal([lastInvulnerable]); + + // restore the invulnerables to the previous state + nonce = await helper.chain.getNonce(superuser.address); + await Promise.all(invulnerables.map((i: any) => + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [i], true, {nonce: nonce++}))); + }); + + itSub('Cannot have too many invulnerables', async ({helper}) => { + const invulnerablesLength = (await helper.collatorSelection.getInvulnerables()).length; + const invulnerablesUntilLimit = 30 - invulnerablesLength; + const newInvulnerables = await helper.arrange.createAccounts(Array(invulnerablesUntilLimit).fill(10n), superuser); + const [lastInvulnerable] = await helper.arrange.createAccounts([10n], superuser); - const newInvulnerables = await helper.callRpc('api.query.collatorSelection.invulnerables'); - expect(newInvulnerables).to.contain(charlie.address).and.contain(dave.address).and.be.length(2); + await Promise.all(newInvulnerables.map((i: IKeyringPair) => + helper.collatorSelection.setOwnKeys(i))); + await helper.collatorSelection.setOwnKeys(lastInvulnerable); - const expectedSessionIndex = (await helper.callRpc('api.query.session.currentIndex')).toNumber() + 2; - let currentSessionIndex = -1; - console.log('Waiting for the session after the next.' - + ' This might take a while -- check SessionPeriod in pallet_session::Config for session time.'); + let nonce = await helper.chain.getNonce(superuser.address); + await Promise.all(newInvulnerables.map((i: IKeyringPair) => + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.addInvulnerable', [i.address], true, {nonce: nonce++}))); - while (currentSessionIndex < expectedSessionIndex) { - // eslint-disable-next-line no-async-promise-executor - currentSessionIndex = await expect(helper.wait.withTimeout(new Promise(async (resolve) => { - await helper.wait.newBlocks(1); - const res = (await helper.callRpc('api.query.session.currentIndex')).toNumber(); - resolve(res); - }), 24000, 'The chain has stopped producing blocks!')).to.be.fulfilled; - } + await expect(helper.getSudo().collatorSelection.addInvulnerable(superuser, lastInvulnerable.address)) + .to.be.rejected; // todo:collator With(/collatorSelection.TooManyInvulnerables/); + + // restore the invulnerables to the previous state + nonce = await helper.chain.getNonce(superuser.address); + await Promise.all(newInvulnerables.map((i: IKeyringPair) => + helper.getSudo().executeExtrinsic(superuser, 'api.tx.collatorSelection.removeInvulnerable', [i.address], true, {nonce: nonce++}))); + }); - const newValidators = await helper.callRpc('api.query.session.validators'); - expect(newValidators).to.contain(charlie.address).and.contain(dave.address).and.be.length(2); + itSub('Forbids a non-sudo to add an invulnerable', async ({helper}) => { + const [account] = await helper.arrange.createAccounts([10n], bob); + const invulnerables = await helper.collatorSelection.getInvulnerables(); - const lastBlockNumber = await helper.chain.getLatestBlockNumber(); - await helper.wait.newBlocks(1); - const lastCharlieBlock = (await helper.callRpc('api.query.collatorSelection.lastAuthoredBlock', [charlie.address])).toNumber(); - const lastDaveBlock = (await helper.callRpc('api.query.collatorSelection.lastAuthoredBlock', [dave.address])).toNumber(); - expect(lastCharlieBlock >= lastBlockNumber || lastDaveBlock >= lastBlockNumber).to.be.true; - }); + await helper.collatorSelection.setOwnKeys(account); + await expect(helper.collatorSelection.addInvulnerable(bob, account.address)) + .to.be.rejectedWith(/BadOrigin/); - after(async () => { - await usingPlaygrounds(async (helper) => { - if (helper.fetchMissingPalletNames([Pallets.AppPromotion]).length != 0) return; + const newInvulnerables = await helper.collatorSelection.getInvulnerables(); + expect(newInvulnerables).to.be.members(invulnerables); + }); - const tx = helper.constructApiCall('api.tx.collatorSelection.setInvulnerables', [[ - aliceAddress, - bobAddress, - ]]); - await expect(helper.executeExtrinsic(superuser, 'api.tx.sudo.sudo', [tx])).to.be.fulfilled; + itSub('Forbids a non-sudo to remove an invulnerable', async ({helper}) => { + const invulnerables = await helper.collatorSelection.getInvulnerables(); + await expect(helper.collatorSelection.removeInvulnerable(superuser, invulnerables[0])) + .to.be.rejectedWith(/BadOrigin/); + expect(await helper.collatorSelection.getInvulnerables()).to.have.all.members(invulnerables); + }); }); + + // todo:collator after }); -}); +}); \ No newline at end of file --- a/tests/src/util/playgrounds/unique.ts +++ b/tests/src/util/playgrounds/unique.ts @@ -11,6 +11,7 @@ import {IKeyringPair} from '@polkadot/types/types'; import {hexToU8a} from '@polkadot/util/hex'; import {u8aConcat} from '@polkadot/util/u8a'; +import {BN} from '@polkadot/util/bn'; import { IApiListeners, IBlock, @@ -2642,6 +2643,37 @@ } } +class CollatorSelectionGroup extends HelperGroup { + //todo:collator documentation + setKeys(signer: TSigner, key: string) { + return this.helper.executeExtrinsic( + signer, + 'api.tx.session.setKeys', + [ + key, + '0x0', + ], + true, + ); + } + + setOwnKeys(signer: TSigner) { + return this.setKeys(signer, '0x' + Buffer.from(signer.addressRaw).toString('hex')); + } + + addInvulnerable(signer: TSigner, address: string) { + return this.helper.executeExtrinsic(signer, 'api.tx.collatorSelection.addInvulnerable', [address]); + } + + removeInvulnerable(signer: TSigner, address: string) { + return this.helper.executeExtrinsic(signer, 'api.tx.collatorSelection.removeInvulnerable', [address]); + } + + async getInvulnerables() { + return (await this.helper.callRpc('api.query.collatorSelection.invulnerables')).map((x: any) => x.toHuman()); + } +} + class ForeignAssetsGroup extends HelperGroup { async register(signer: TSigner, ownerAddress: TSubstrateAccount, location: any, metadata: IForeignAssetMetadata) { await this.helper.executeExtrinsic( @@ -2808,6 +2840,7 @@ ft: FTGroup; staking: StakingGroup; scheduler: SchedulerGroup; + collatorSelection: CollatorSelectionGroup; foreignAssets: ForeignAssetsGroup; xcm: XcmGroup; xTokens: XTokensGroup; @@ -2823,6 +2856,7 @@ this.ft = new FTGroup(this); this.staking = new StakingGroup(this); this.scheduler = new SchedulerGroup(this); + this.collatorSelection = new CollatorSelectionGroup(this); this.foreignAssets = new ForeignAssetsGroup(this); this.xcm = new XcmGroup(this, 'polkadotXcm'); this.xTokens = new XTokensGroup(this); @@ -2988,19 +3022,32 @@ super(...args); } - executeExtrinsic ( + async executeExtrinsic( sender: IKeyringPair, extrinsic: string, params: any[], expectSuccess?: boolean, + options: Partial|null = null, ): Promise { const call = this.constructApiCall(extrinsic, params); - return super.executeExtrinsic( + const result = await super.executeExtrinsic( sender, 'api.tx.sudo.sudo', [call], expectSuccess, + options, ); + + if (result.status === 'Fail') return result; + + const data = this.eventHelper.extractEvents(result.result.events).find(x => x.section == 'sudo')?.data[0]; + if (data.err) { + const error = data.err.module; + // todo:collator + const metaError = super.getApi()?.registry.findMetaError({index: new BN(error.index), error: new BN(9)}); + throw new Error(`${data.err.module.error} ${metaError.section}.${metaError.name}`); + } + return result; } }; } -- gitstuff