From d41364fb01e467c1b87fd8b5803389957e7ca3bc Mon Sep 17 00:00:00 2001 From: Fahrrader Date: Thu, 22 Dec 2022 22:42:21 +0000 Subject: [PATCH] feat(collator-selection): method refactoring + unit tests complete --- --- a/pallets/collator-selection/src/benchmarking.rs +++ b/pallets/collator-selection/src/benchmarking.rs @@ -130,7 +130,7 @@ where_clause { where T: pallet_authorship::Config + session::Config } set_invulnerables { - let b in 1 .. T::MaxInvulnerables::get(); + let b in 1 .. T::MaxCollators::get(); let new_invulnerables = register_validators::(b); let origin = T::UpdateOrigin::successful_origin(); }: { @@ -142,16 +142,16 @@ assert_last_event::(Event::NewInvulnerables{invulnerables: new_invulnerables}.into()); } - set_desired_candidates { + set_desired_collators { let max: u32 = 999; let origin = T::UpdateOrigin::successful_origin(); }: { assert_ok!( - >::set_desired_candidates(origin, max.clone()) + >::set_desired_collators(origin, max.clone()) ); } verify { - assert_last_event::(Event::NewDesiredCandidates{desired_candidates: max}.into()); + assert_last_event::(Event::NewDesiredCollators{desired_collators: max}.into()); } set_license_bond { @@ -169,10 +169,10 @@ // 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::MaxCollators::get(); >::put(T::Currency::minimum_balance()); - >::put(c + 1); + >::put(c + 1); register_validators::(c); register_candidates::(c); @@ -194,9 +194,9 @@ // worse case is the last candidate leaving. leave_intent { - let c in (T::MinCandidates::get() + 1) .. T::MaxCandidates::get(); + let c in (T::MinCandidates::get() + 1) .. T::MaxCollators::get(); >::put(T::Currency::minimum_balance()); - >::put(c); + >::put(c); register_validators::(c); register_candidates::(c); @@ -230,11 +230,11 @@ // worst case for new session. new_session { - let r in 1 .. T::MaxCandidates::get(); - let c in 1 .. T::MaxCandidates::get(); + let r in 1 .. T::MaxCollators::get(); + let c in 1 .. T::MaxCollators::get(); >::put(T::Currency::minimum_balance()); - >::put(c); + >::put(c); frame_system::Pallet::::set_block_number(0u32.into()); register_validators::(c); --- a/pallets/collator-selection/src/lib.rs +++ b/pallets/collator-selection/src/lib.rs @@ -98,10 +98,7 @@ dispatch::{DispatchClass, DispatchResultWithPostInfo}, inherent::Vec, pallet_prelude::*, - sp_runtime::{ - traits::{AccountIdConversion, CheckedSub, Saturating, Zero}, - RuntimeDebug, - }, + sp_runtime::traits::{AccountIdConversion, CheckedSub, Saturating, Zero}, traits::{ Currency, EnsureOrigin, ExistenceRequirement::KeepAlive, ReservableCurrency, ValidatorRegistration, @@ -145,19 +142,9 @@ /// Account Identifier from which the internal Pot is generated. type PotId: Get; - - /// Maximum number of candidates that we should have. This is enforced in code. - /// - /// 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. This is enforced in code. - type MaxInvulnerables: Get; + /// Maximum number of candidates and invulnerables that we should have. This is enforced in code. + type MaxCollators: Get; /// If kicked, how much of the collator's deposit will be slashed and sent to the slash destination. type SlashRatio: Get; @@ -175,17 +162,6 @@ /// The weight information of this pallet. type WeightInfo: WeightInfo; - } - - /// Basic information about a collation candidate. - #[derive( - PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, scale_info::TypeInfo, MaxEncodedLen, - )] - pub struct LicenseInfo { - /// Account identifier. - pub who: AccountId, - /// Reserved deposit. - pub deposit: Balance, } #[pallet::pallet] @@ -196,7 +172,7 @@ #[pallet::storage] #[pallet::getter(fn invulnerables)] pub type Invulnerables = - StorageValue<_, BoundedVec, ValueQuery>; + StorageValue<_, BoundedVec, ValueQuery>; /// The (community) collation license holders. #[pallet::storage] @@ -209,7 +185,7 @@ #[pallet::getter(fn candidates)] pub type Candidates = StorageValue< _, - BoundedVec, //LicenseInfo>, T::MaxCandidates>, // license ID? + BoundedVec, //LicenseInfo>, T::MaxCollators>, // license ID? ValueQuery, >; @@ -228,10 +204,10 @@ /// Desired number of candidates. /// - /// This should ideally always be less than [`Config::MaxCandidates`] for weights to be correct. + /// This should ideally always be less than [`Config::MaxCollators`] for weights to be correct. #[pallet::storage] - #[pallet::getter(fn desired_candidates)] - pub type DesiredCandidates = StorageValue<_, u32, ValueQuery>; + #[pallet::getter(fn desired_collators)] + pub type DesiredCollators = StorageValue<_, u32, ValueQuery>; /// Fixed amount to deposit to become a collator. /// @@ -245,7 +221,7 @@ pub invulnerables: Vec, pub license_bond: BalanceOf, pub kick_threshold: T::BlockNumber, - pub desired_candidates: u32, + pub desired_collators: u32, } #[cfg(feature = "std")] @@ -255,7 +231,7 @@ invulnerables: Default::default(), license_bond: Default::default(), kick_threshold: T::BlockNumber::one(), - desired_candidates: Default::default(), + desired_collators: Default::default(), } } } @@ -273,16 +249,16 @@ ); let bounded_invulnerables = - BoundedVec::<_, T::MaxInvulnerables>::try_from(self.invulnerables.clone()) - .expect("genesis invulnerables are more than T::MaxInvulnerables"); + BoundedVec::<_, T::MaxCollators>::try_from(self.invulnerables.clone()) + .expect("genesis invulnerables are more than T::MaxCollators"); assert!( - T::MaxCandidates::get() >= self.desired_candidates, - "genesis desired_candidates are more than T::MaxCandidates", + T::MaxCollators::get() >= self.desired_collators, + "genesis desired_collators are more than T::MaxCollators", ); - >::put(&self.desired_candidates); - >::put(&self.license_bond); - >::put(&self.kick_threshold); + >::put(self.desired_collators); + >::put(self.license_bond); + >::put(self.kick_threshold); >::put(bounded_invulnerables); } } @@ -290,8 +266,8 @@ #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { - NewDesiredCandidates { - desired_candidates: u32, + NewDesiredCollators { + desired_collators: u32, }, NewLicenseBond { bond_amount: BalanceOf, @@ -326,14 +302,12 @@ pub enum Error { /// Too many candidates TooManyCandidates, - /// Too few candidates - TooFewCandidates, /// Unknown error Unknown, /// Permission issue Permission, /// User already holds license to collate - AlreadyLicenseHolder, + AlreadyHoldingLicense, /// User does not hold a license to collate NoLicense, /// User is already a candidate @@ -360,7 +334,7 @@ #[pallet::call] impl Pallet { /// Add a collator to the list of invulnerable (fixed) collators. - #[pallet::weight(T::WeightInfo::set_invulnerables(1 as u32))] // todo:collator weight + #[pallet::weight(T::WeightInfo::set_invulnerables(1u32))] // todo:collator weight pub fn add_invulnerable( origin: OriginFor, new: T::AccountId, @@ -378,12 +352,13 @@ if Self::invulnerables().contains(&new) { return Ok(().into()); } - - // todo:collator check license holders, release moneys, promotion! - // force_release_license? Error::::lreadyLicenseHolder? >::try_append(new.clone()) .map_err(|_| Error::::TooManyInvulnerables)?; + + // try to offboard the new invulnerable if it was a collator candidate before + let _ = Self::try_remove_candidate(&new); + Self::deposit_event(Event::InvulnerableAdded { invulnerable: new }); Ok(().into()) } @@ -417,22 +392,19 @@ 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. + /// Set the ideal number of collators. 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. - #[pallet::weight(T::WeightInfo::set_desired_candidates())] - pub fn set_desired_candidates( - origin: OriginFor, - max: u32, - ) -> DispatchResultWithPostInfo { + #[pallet::weight(T::WeightInfo::set_desired_collators())] + pub fn set_desired_collators(origin: OriginFor, max: u32) -> DispatchResultWithPostInfo { T::UpdateOrigin::ensure_origin(origin)?; // we trust origin calls, this is just a for more accurate benchmarking - if max > T::MaxCandidates::get() { - log::warn!("max > T::MaxCandidates; you might need to run benchmarks again"); + if max > T::MaxCollators::get() { + log::warn!("max > T::MaxCollators; you might need to run benchmarks again"); } - >::put(&max); - Self::deposit_event(Event::NewDesiredCandidates { - desired_candidates: max, + >::put(max); + Self::deposit_event(Event::NewDesiredCollators { + desired_collators: max, }); Ok(().into()) } @@ -444,7 +416,7 @@ bond: BalanceOf, ) -> DispatchResultWithPostInfo { T::UpdateOrigin::ensure_origin(origin)?; - >::put(&bond); + >::put(bond); Self::deposit_event(Event::NewLicenseBond { bond_amount: bond }); Ok(().into()) } @@ -470,19 +442,19 @@ /// (a) already have registered session keys and (b) be able to reserve the `LicenseBond`. /// /// This call is not available to `Invulnerable` collators. - #[pallet::weight(T::WeightInfo::register_as_candidate(T::MaxCandidates::get()))] // todo:collator weight + #[pallet::weight(T::WeightInfo::register_as_candidate(T::MaxCollators::get()))] // todo:collator weight pub fn get_license(origin: OriginFor) -> DispatchResultWithPostInfo { // register_as_candidate let who = ensure_signed(origin)?; if Licenses::::contains_key(&who) { - return Ok(().into()); + return Err(Error::::AlreadyHoldingLicense.into()); } - ensure!( + /*ensure!( !Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable - ); + );*/ let validator_key = T::ValidatorIdOf::convert(who.clone()) .ok_or(Error::::NoAssociatedValidatorId)?; @@ -507,7 +479,7 @@ return Err(BadOrigin.into()); } if candidates.iter().any(|candidate| *candidate == who) { - Err(Error::::AlreadyLicenseHolder)? + Err(Error::::AlreadyHoldingLicense)? } else { T::Currency::reserve(&who, deposit)?; candidates @@ -532,7 +504,7 @@ /// The account must already hold a license, and cannot offboard immediately during a session. /// /// This call is not available to `Invulnerable` collators. - #[pallet::weight(T::WeightInfo::register_as_candidate(T::MaxCandidates::get()))] // todo:collator weight + #[pallet::weight(T::WeightInfo::register_as_candidate(T::MaxCollators::get()))] // todo:collator weight pub fn onboard(origin: OriginFor) -> DispatchResultWithPostInfo { // register_as_candidate let who = ensure_signed(origin)?; @@ -540,9 +512,10 @@ // ensure the user obtained the license. ensure!(Licenses::::contains_key(&who), Error::::NoLicense); // ensure we are below limit. - let length = >::decode_len().unwrap_or_default(); + let length = >::decode_len().unwrap_or_default() + + >::decode_len().unwrap_or_default(); ensure!( - (length as u32) < Self::desired_candidates(), + (length as u32) < Self::desired_collators(), Error::::TooManyCandidates ); // todo:collator really need it? @@ -551,8 +524,6 @@ Error::::AlreadyInvulnerable ); - let deposit = Self::license_bond(); - // First authored block is current block plus kick threshold to handle session delay /*let incoming = LicenseInfo { who: who.clone(), deposit, @@ -563,10 +534,10 @@ if candidates.iter().any(|candidate| *candidate == who) { Err(Error::::AlreadyCandidate)? } else { - T::Currency::reserve(&who, deposit)?; candidates .try_push(who.clone()) .map_err(|_| Error::::TooManyCandidates)?; + // First authored block is current block plus kick threshold to handle session delay >::insert( who.clone(), frame_system::Pallet::::block_number() + Self::kick_threshold(), @@ -583,31 +554,31 @@ /// session change. The license to `onboard` later at any other time will remain. /// /// This call will fail if the total number of candidates would drop below `MinCandidates`. todo:collator maybe not - #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] // todo:collator weight + #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCollators::get()))] // todo:collator weight pub fn offboard(origin: OriginFor) -> DispatchResultWithPostInfo { // leave_intent let who = ensure_signed(origin)?; - // todo:collator invulnerables and candidates should count against min candidates together + /* 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)?; - Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) + Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) // todo:collator weight } /// Forfeit `origin`'s own license. The `LicenseBond` will be unreserved immediately. /// /// This call is not available to `Invulnerable` collators. - #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] // todo:collator weight + #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCollators::get()))] // todo:collator weight pub fn release_license(origin: OriginFor) -> DispatchResultWithPostInfo { // leave_intent let who = ensure_signed(origin)?; - // let current_count = Self::try_remove_candidate(&who, false)?; - Self::try_release_license(&who, false)?; - Ok(().into()) + let current_count = Self::try_remove_candidate_and_release_license(&who, false, true)?; + + Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) // todo:collator weight } /// Force deregister `origin` as a collator candidate as a governing authority, and revoke its license. @@ -615,16 +586,15 @@ /// The `LicenseBond` will be unreserved and returned immediately. /// /// This call is not available to `Invulnerable` collators. - #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCandidates::get()))] // todo:collator weight - pub fn force_release_license( + #[pallet::weight(T::WeightInfo::leave_intent(T::MaxCollators::get()))] // todo:collator weight + pub fn force_revoke_license( origin: OriginFor, who: T::AccountId, ) -> DispatchResultWithPostInfo { // leave_intent T::UpdateOrigin::ensure_origin(origin)?; - let current_count = Self::try_remove_candidate(&who)?; - Self::try_release_license(&who, false)?; + let current_count = Self::try_remove_candidate_and_release_license(&who, false, true)?; Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) // todo:collator weight } @@ -636,6 +606,23 @@ T::PotId::get().into_account_truncating() } + fn try_remove_candidate_and_release_license( + who: &T::AccountId, + should_slash: bool, + ignore_if_not_candidate: bool, + ) -> Result { + let current_count = Self::try_remove_candidate(who); + let current_count = if ignore_if_not_candidate + && current_count == Err(Error::::NotCandidate.into()) + { + >::decode_len().unwrap_or_default() + } else { + current_count? + }; + Self::try_release_license(who, should_slash)?; + Ok(current_count) + } + /// Removes a candidate from the collator pool for the next session if they exist. fn try_remove_candidate(who: &T::AccountId) -> Result { let current_count = @@ -657,7 +644,7 @@ /// Removes a candidate if they exist and sends them back their deposit, optionally slashed. fn try_release_license(who: &T::AccountId, should_slash: bool) -> DispatchResult { let mut deposit_returned = BalanceOf::::default(); - Licenses::::try_mutate_exists(&who, |deposit| -> DispatchResult { + Licenses::::try_mutate_exists(who, |deposit| -> DispatchResult { if let Some(deposit) = deposit.take() { if should_slash { let slashed = T::SlashRatio::get() * deposit; @@ -690,7 +677,7 @@ /// /// This is done on the fly, as frequent as we are told to do so, as the session manager. pub fn assemble_collators( - candidates: BoundedVec, + candidates: BoundedVec, ) -> Vec { let mut collators = Self::invulnerables().to_vec(); collators.extend(candidates); @@ -700,8 +687,8 @@ /// Kicks out candidates that did not produce a block in the kick threshold /// and **confiscates** their deposits to the treasury. pub fn kick_stale_candidates( - candidates: BoundedVec, //LicenseInfo> - ) -> BoundedVec { + candidates: BoundedVec, //LicenseInfo> + ) -> BoundedVec { let now = frame_system::Pallet::::block_number(); let kick_threshold = Self::kick_threshold(); candidates @@ -709,21 +696,13 @@ .filter_map(|c| { let last_block = >::get(c.clone()); let since_last = now.saturating_sub(last_block); - if since_last < kick_threshold || - Self::candidates().len() as u32 <= T::MinCandidates::get() - { + if since_last < kick_threshold { Some(c) } else { - let outcome = Self::try_remove_candidate(&c); + let outcome = Self::try_remove_candidate_and_release_license(&c, true, false); if let Err(why) = outcome { - log::warn!("Failed to remove candidate {:?}", why); - debug_assert!(false, "failed to remove candidate {:?}", why); - return None; - } - let outcome = Self::try_release_license(&c, true); - if let Err(why) = outcome { - log::warn!("Failed to release license {:?}", why); - debug_assert!(false, "failed to release license {:?}", why); + log::warn!("Failed to kick collator and release license {:?}", why); + debug_assert!(false, "failed to kick collator and release license {why:?}"); } None } --- a/pallets/collator-selection/src/mock.rs +++ b/pallets/collator-selection/src/mock.rs @@ -206,9 +206,7 @@ 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 const MaxCollators: u32 = 20; pub const MaxAuthorities: u32 = 100_000; pub const SlashRatio: Perbill = Perbill::one(); } @@ -230,9 +228,7 @@ type Currency = Balances; type UpdateOrigin = EnsureSignedBy; type PotId = PotId; - type MaxCandidates = MaxCandidates; - type MinCandidates = MinCandidates; - type MaxInvulnerables = MaxInvulnerables; + type MaxCollators = MaxCollators; // type KickThreshold = Period; type SlashRatio = SlashRatio; type TreasuryAccountId = (); @@ -263,9 +259,9 @@ }) .collect::>(); let collator_selection = collator_selection::GenesisConfig:: { - desired_candidates: 2, + desired_collators: 5, license_bond: 10, - kick_threshold: 1, + kick_threshold: 10, invulnerables, }; let session = pallet_session::GenesisConfig:: { keys }; --- a/pallets/collator-selection/src/tests.rs +++ b/pallets/collator-selection/src/tests.rs @@ -31,7 +31,7 @@ // limitations under the License. use crate as collator_selection; -use crate::{mock::*, LicenseInfo, Error}; +use crate::{mock::*, Error}; use frame_support::{ assert_noop, assert_ok, traits::{Currency, GenesisBuild, OnInitialize}, @@ -39,10 +39,19 @@ use pallet_balances::Error as BalancesError; use sp_runtime::traits::BadOrigin; +fn get_license_and_onboard(account_id: ::AccountId) { + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed( + account_id + ))); + assert_ok!(CollatorSelection::onboard(RuntimeOrigin::signed( + account_id + ))); +} + #[test] fn basic_setup_works() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::desired_collators(), 5); assert_eq!(CollatorSelection::license_bond(), 10); assert!(CollatorSelection::candidates().is_empty()); @@ -51,6 +60,7 @@ } // todo:collator add more tests later +// invulnerable after onboard + invulnerables can bypass desired_candidates #[test] fn it_should_add_invulnerables() { @@ -112,21 +122,21 @@ } #[test] -fn set_desired_candidates_works() { +fn set_desired_collators_works() { new_test_ext().execute_with(|| { // given - assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::desired_collators(), 5); // can set - assert_ok!(CollatorSelection::set_desired_candidates( + assert_ok!(CollatorSelection::set_desired_collators( RuntimeOrigin::signed(RootAccount::get()), 7 )); - assert_eq!(CollatorSelection::desired_candidates(), 7); + assert_eq!(CollatorSelection::desired_collators(), 7); // rejects bad origin assert_noop!( - CollatorSelection::set_desired_candidates(RuntimeOrigin::signed(1), 8), + CollatorSelection::set_desired_collators(RuntimeOrigin::signed(1), 8), BadOrigin ); }); @@ -154,166 +164,246 @@ } #[test] -fn cannot_register_candidate_if_too_many() { +fn cannot_onboard_candidate_with_no_license() { new_test_ext().execute_with(|| { - // reset desired candidates: - >::put(0); + // can't onboard a candidate who did not get a license. + assert_noop!( + CollatorSelection::onboard(RuntimeOrigin::signed(3)), + Error::::NoLicense, + ); + + // but give it a license and welcome aboard. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::onboard(RuntimeOrigin::signed(3))); + }) +} + +#[test] +fn cannot_onboard_candidate_if_too_many() { + new_test_ext().execute_with(|| { + // reset desired candidates + >::put(0); + + // can still get a license. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(4))); // can't accept anyone anymore. assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)), + CollatorSelection::onboard(RuntimeOrigin::signed(4)), Error::::TooManyCandidates, ); - // reset desired candidates: - >::put(1); - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(4) - )); + // reset desired candidates to invulnerables + 1 + >::put(3); + assert_ok!(CollatorSelection::onboard(RuntimeOrigin::signed(4))); - // but no more + // but no more. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(5))); assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5)), + CollatorSelection::onboard(RuntimeOrigin::signed(5)), Error::::TooManyCandidates, ); }) } #[test] -fn cannot_unregister_candidate_if_too_few() { +fn cannot_obtain_license_if_keys_not_registered() { new_test_ext().execute_with(|| { - // reset desired candidates: - >::put(1); - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(4) - )); - - // can not remove too few + // can't 7 because keys not registered. assert_noop!( - CollatorSelection::leave_intent(RuntimeOrigin::signed(4)), - Error::::TooFewCandidates, + CollatorSelection::get_license(RuntimeOrigin::signed(7)), + Error::::ValidatorNotRegistered ); }) } #[test] -fn cannot_register_as_candidate_if_invulnerable() { +fn cannot_obtain_license_if_poor() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + assert_eq!(Balances::free_balance(&3), 100); + assert_eq!(Balances::free_balance(&33), 0); - // can't 1 because it is invulnerable. - assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(1)), - Error::::AlreadyInvulnerable, - ); - }) -} + // works + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(3))); -#[test] -fn cannot_register_as_candidate_if_keys_not_registered() { - new_test_ext().execute_with(|| { - // can't 7 because keys not registered. + // poor assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(7)), - Error::::ValidatorNotRegistered + CollatorSelection::get_license(RuntimeOrigin::signed(33)), + BalancesError::::InsufficientBalance, ); - }) + }); } #[test] -fn cannot_register_dupe_candidate() { +fn cannot_onboard_dupe_candidate() { new_test_ext().execute_with(|| { // can add 3 as candidate - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(3) - )); - let addition = LicenseInfo { - who: 3, - deposit: 10, - }; - assert_eq!(CollatorSelection::candidates(), vec![addition]); + get_license_and_onboard(3); + assert_eq!(CollatorSelection::licenses(3), 10); + assert_eq!(CollatorSelection::candidates(), vec![3]); assert_eq!(CollatorSelection::last_authored_block(3), 10); assert_eq!(Balances::free_balance(3), 90); // but no more assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)), + CollatorSelection::get_license(RuntimeOrigin::signed(3)), + Error::::AlreadyHoldingLicense, + ); + assert_noop!( + CollatorSelection::onboard(RuntimeOrigin::signed(3)), Error::::AlreadyCandidate, ); }) } #[test] -fn cannot_register_as_candidate_if_poor() { +fn becoming_candidate_works() { new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_collators(), 5); + assert_eq!(CollatorSelection::license_bond(), 10); + assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take two endowed, non-invulnerables accounts. assert_eq!(Balances::free_balance(&3), 100); - assert_eq!(Balances::free_balance(&33), 0); + assert_eq!(Balances::free_balance(&4), 100); - // works - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(3) - )); + get_license_and_onboard(3); + get_license_and_onboard(4); + + assert_eq!(Balances::free_balance(&3), 90); + assert_eq!(Balances::free_balance(&4), 90); - // poor - assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(33)), - BalancesError::::InsufficientBalance, - ); + assert_eq!(CollatorSelection::candidates().len(), 2); }); } #[test] -fn register_as_candidate_works() { +fn cannot_become_candidate_if_invulnerable() { new_test_ext().execute_with(|| { - // given - assert_eq!(CollatorSelection::desired_candidates(), 2); - assert_eq!(CollatorSelection::license_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); - // take two endowed, non-invulnerables accounts. - assert_eq!(Balances::free_balance(&3), 100); - assert_eq!(Balances::free_balance(&4), 100); + // can obtain a license even if is invulnerable. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(1))); + // but cannot onboard + assert_noop!( + CollatorSelection::onboard(RuntimeOrigin::signed(1)), + Error::::AlreadyInvulnerable, + ); - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(3) + // get a license and then become invulnerable. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::add_invulnerable( + RuntimeOrigin::signed(RootAccount::get()), + 3 )); - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(4) + assert_noop!( + CollatorSelection::onboard(RuntimeOrigin::signed(3)), + Error::::AlreadyInvulnerable, + ); + }) +} + +#[test] +fn can_become_invulnerable_if_candidate() { + new_test_ext().execute_with(|| { + // become a candidate and then become invulnerable. + get_license_and_onboard(3); + assert_eq!(CollatorSelection::candidates(), vec![3]); + + assert_ok!(CollatorSelection::add_invulnerable( + RuntimeOrigin::signed(RootAccount::get()), + 3 )); + // should exclude from candidates, but not revoke the license + assert_eq!(CollatorSelection::candidates(), vec![]); + assert_eq!(CollatorSelection::licenses(3), 10); + assert_eq!(Balances::free_balance(3), 90); + }); +} - assert_eq!(Balances::free_balance(&3), 90); - assert_eq!(Balances::free_balance(&4), 90); +#[test] +fn offboard() { + new_test_ext().execute_with(|| { + // register a candidate. + get_license_and_onboard(3); + assert_eq!(Balances::free_balance(3), 90); + + // cannot leave if holds license but not yet candidate. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(4))); + assert_noop!( + CollatorSelection::offboard(RuntimeOrigin::signed(4)), + Error::::NotCandidate + ); + // cannot leave if does not hold license. + assert_noop!( + CollatorSelection::offboard(RuntimeOrigin::signed(5)), + Error::::NotCandidate + ); - assert_eq!(CollatorSelection::candidates().len(), 2); + // bond is returned - only after releasing the license + assert_ok!(CollatorSelection::offboard(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(CollatorSelection::last_authored_block(3), 0); + assert_ok!(CollatorSelection::release_license(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 100); }); } #[test] -fn leave_intent() { +fn release_license() { new_test_ext().execute_with(|| { + // obtain a license to collate and reserve the bond. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 90); + + // release the license and get the bond back. + assert_ok!(CollatorSelection::release_license(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 100); + // register a candidate. - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(3) - )); + get_license_and_onboard(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_eq!(Balances::free_balance(5), 90); + // can release license even if onboarded. + assert_ok!(CollatorSelection::release_license(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(CollatorSelection::candidates(), vec![]); + }); +} + +#[test] +fn force_revoke_license() { + new_test_ext().execute_with(|| { + // obtain a license to collate and reserve the bond. + assert_ok!(CollatorSelection::get_license(RuntimeOrigin::signed(3))); + assert_eq!(Balances::free_balance(3), 90); - // cannot leave if not candidate. + // cannot execute the operation as non-root assert_noop!( - CollatorSelection::leave_intent(RuntimeOrigin::signed(4)), - Error::::NotCandidate + CollatorSelection::force_revoke_license(RuntimeOrigin::signed(3), 3), + BadOrigin ); - // bond is returned - assert_ok!(CollatorSelection::leave_intent(RuntimeOrigin::signed(3))); + // release the license and get the bond back. + assert_ok!(CollatorSelection::force_revoke_license( + RuntimeOrigin::signed(RootAccount::get()), + 3 + )); + assert_eq!(Balances::free_balance(3), 100); + + // register a candidate. + get_license_and_onboard(3); + assert_eq!(Balances::free_balance(3), 90); + + // can release license even if onboarded. + assert_ok!(CollatorSelection::force_revoke_license( + RuntimeOrigin::signed(RootAccount::get()), + 3 + )); assert_eq!(Balances::free_balance(3), 100); - assert_eq!(CollatorSelection::last_authored_block(3), 0); + assert_eq!(CollatorSelection::candidates(), vec![]); }); } @@ -325,18 +415,11 @@ // 4 is the default author. assert_eq!(Balances::free_balance(4), 100); - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(4) - )); + get_license_and_onboard(4); // triggers `note_author` Authorship::on_initialize(1); - let collator = LicenseInfo { - who: 4, - deposit: 10, - }; - - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!(CollatorSelection::candidates(), vec![4]); assert_eq!(CollatorSelection::last_authored_block(4), 0); // half of the pot goes to the collator who's the author (4 in tests). @@ -355,18 +438,11 @@ 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) - )); + get_license_and_onboard(4); // triggers `note_author` Authorship::on_initialize(1); - let collator = LicenseInfo { - who: 4, - deposit: 10, - }; - - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!(CollatorSelection::candidates(), vec![4]); assert_eq!(CollatorSelection::last_authored_block(4), 0); // Nothing received assert_eq!(Balances::free_balance(4), 90); @@ -389,9 +465,7 @@ assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // add a new collator - assert_ok!(CollatorSelection::register_as_candidate( - RuntimeOrigin::signed(3) - )); + get_license_and_onboard(5); // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); @@ -410,7 +484,7 @@ initialize_to_block(20); assert_eq!(SessionChangeBlock::get(), 20); // changed are now reflected to session handlers. - assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3]); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5]); }); } @@ -418,64 +492,28 @@ 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) - )); + get_license_and_onboard(3); + get_license_and_onboard(4); + initialize_to_block(10); assert_eq!(CollatorSelection::candidates().len(), 2); + initialize_to_block(20); assert_eq!(SessionChangeBlock::get(), 20); // 4 authored this block, gets to stay 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, 4]); - let collator = LicenseInfo { - who: 4, - deposit: 10, - }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); - assert_eq!(CollatorSelection::kick_threshold(), 1); + + assert_eq!(CollatorSelection::candidates(), vec![4]); + assert_eq!(CollatorSelection::kick_threshold(), 10); 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, 4]); - // kicked collator gets funds back - assert_eq!(Balances::free_balance(3), 100); - }); -} - -#[test] -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) - )); - 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 = LicenseInfo { - 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); + // kicked collator gets their funds slashed, the deposit going to treasury + assert_eq!(Balances::free_balance(3), 90); }); } @@ -489,9 +527,9 @@ let invulnerables = vec![1, 1]; let collator_selection = collator_selection::GenesisConfig:: { - desired_candidates: 2, + desired_collators: 5, license_bond: 10, - kick_threshold: 1, + kick_threshold: 10, invulnerables, }; // collator selection must be initialized before session. --- a/pallets/collator-selection/src/weights.rs +++ b/pallets/collator-selection/src/weights.rs @@ -45,7 +45,7 @@ // The weight info trait for `pallet_collator_selection`. pub trait WeightInfo { fn set_invulnerables(_b: u32) -> Weight; - fn set_desired_candidates() -> Weight; + fn set_desired_collators() -> Weight; fn set_license_bond() -> Weight; fn register_as_candidate(_c: u32) -> Weight; fn leave_intent(_c: u32) -> Weight; @@ -62,7 +62,7 @@ .saturating_add(Weight::from_ref_time(68_000 as u64).saturating_mul(b as u64)) .saturating_add(T::DbWeight::get().writes(1 as u64)) } - fn set_desired_candidates() -> Weight { + fn set_desired_collators() -> Weight { Weight::from_ref_time(16_363_000 as u64).saturating_add(T::DbWeight::get().writes(1 as u64)) } fn set_license_bond() -> Weight { @@ -108,7 +108,7 @@ .saturating_add(Weight::from_ref_time(68_000 as u64).saturating_mul(b as u64)) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } - fn set_desired_candidates() -> Weight { + fn set_desired_collators() -> Weight { Weight::from_ref_time(16_363_000 as u64) .saturating_add(RocksDbWeight::get().writes(1 as u64)) } --- a/primitives/common/src/constants.rs +++ b/primitives/common/src/constants.rs @@ -45,9 +45,9 @@ /// Minimum balance required to create or keep an account open. pub const EXISTENTIAL_DEPOSIT: u128 = 0; /// Amount of Balance reserved for candidate registration. -pub const GENESIS_LICENSE_BOND: u128 = EXISTENTIAL_DEPOSIT; +pub const GENESIS_LICENSE_BOND: u128 = 1_000_000_000_000 * UNIQUE; /// How long a periodic session lasts in blocks. -pub const SESSION_LENGTH: BlockNumber = MINUTES; +pub const SESSION_LENGTH: BlockNumber = HOURS; // 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 @@ -55,9 +55,7 @@ parameter_types! { pub const PotId: PalletId = PalletId(*b"PotStake"); - 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 MaxCollators: u32 = 10; pub const SlashRatio: Perbill = Perbill::from_percent(100); } @@ -68,9 +66,7 @@ type UpdateOrigin = EnsureRoot; type TreasuryAccountId = TreasuryAccountId; type PotId = PotId; - type MaxCandidates = MaxCandidates; - type MinCandidates = MinCandidates; - type MaxInvulnerables = MaxInvulnerables; + type MaxCollators = MaxCollators; // todo:collator kick threshold should be in storage and configured only by root -- or rather UpdateOrigin type SlashRatio = SlashRatio; type ValidatorId = ::AccountId; --- a/runtime/common/mod.rs +++ b/runtime/common/mod.rs @@ -192,7 +192,7 @@ }; use pallet_session::SessionManager; use up_common::constants::GENESIS_LICENSE_BOND; - use crate::config::pallets::collator_selection::MaxInvulnerables; + use crate::config::pallets::collator_selection::MaxCollators; let mut weight = ::DbWeight::get().reads(1); @@ -231,17 +231,17 @@ }) .collect::>(); - let bounded_invulnerables = BoundedVec::<_, MaxInvulnerables>::try_from( + let bounded_invulnerables = BoundedVec::<_, MaxCollators>::try_from( invulnerables .iter() .cloned() .map(|(acc, _)| acc) .collect::>(), ) - .expect("Existing collators/invulnerables are more than MaxInvulnerables"); + .expect("Existing collators/invulnerables are more than MaxCollators"); >::put(bounded_invulnerables); - >::put(0); + >::put(MaxCollators::get()); >::put(GENESIS_LICENSE_BOND); let keys = invulnerables --- a/runtime/common/tests/mod.rs +++ b/runtime/common/tests/mod.rs @@ -18,7 +18,7 @@ use sp_core::{Public, Pair}; use sp_std::vec; use up_common::types::AuraId; -use crate::{GenesisConfig, ParachainInfoConfig, AuraConfig}; +use crate::{GenesisConfig, ParachainInfoConfig}; pub mod xcm; @@ -28,7 +28,61 @@ .public() } +#[cfg(feature = "collator-selection")] +fn new_test_ext(para_id: u32) -> sp_io::TestExternalities { + use sp_core::{sr25519}; + use sp_runtime::traits::{IdentifyAccount, Verify}; + use crate::{AccountId, Signature, SessionKeys, CollatorSelectionConfig, SessionConfig}; + + type AccountPublic = ::Signer; + + fn get_account_id_from_seed(seed: &str) -> AccountId + where + AccountPublic: From<::Public>, + { + AccountPublic::from(get_from_seed::(seed)).into_account() + } + + let accounts = vec!["Alice", "Bob"]; + let keys = accounts + .iter() + .map(|&acc| { + let account_id = get_account_id_from_seed::(acc); + ( + account_id.clone(), + account_id, + SessionKeys { + aura: get_from_seed::(acc), + }, + ) + }) + .collect::>(); + let invulnerables = accounts + .iter() + .map(|acc| get_account_id_from_seed::(acc)) + .collect::>(); + + let cfg = GenesisConfig { + collator_selection: CollatorSelectionConfig { + desired_collators: 2, + license_bond: 10, + kick_threshold: 10, + invulnerables, + }, + session: SessionConfig { keys }, + parachain_info: ParachainInfoConfig { + parachain_id: para_id.into(), + }, + ..GenesisConfig::default() + }; + + cfg.build_storage().unwrap().into() +} + +#[cfg(not(feature = "collator-selection"))] fn new_test_ext(para_id: u32) -> sp_io::TestExternalities { + use crate::AuraConfig; + let cfg = GenesisConfig { aura: AuraConfig { authorities: vec![ -- gitstuff