From e09264489dabd068e50186dde8789bb2b9a3925c Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 31 Aug 2023 16:04:32 +0300 Subject: [PATCH 01/33] Introduce simple election to `collator-selection` Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 131 +++- cumulus/pallets/collator-selection/src/lib.rs | 304 ++++++-- .../pallets/collator-selection/src/tests.rs | 715 +++++++++++++++++- .../pallets/collator-selection/src/weights.rs | 30 + 4 files changed, 1045 insertions(+), 135 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 49999dc114df..ba2d86518a10 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -25,10 +25,7 @@ use codec::Decode; use frame_benchmarking::{ account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError, }; -use frame_support::{ - dispatch::DispatchResult, - traits::{Currency, EnsureOrigin, Get, ReservableCurrency}, -}; +use frame_support::traits::{Currency, EnsureOrigin, Get, ReservableCurrency}; use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, RawOrigin}; use pallet_authorship::EventHandler; use pallet_session::{self as session, SessionManager}; @@ -94,7 +91,8 @@ fn register_candidates(count: u32) { assert!(>::get() > 0u32.into(), "Bond cannot be zero!"); for who in candidates { - T::Currency::make_free_balance_be(&who, >::get() * 2u32.into()); + // TODO[GMP] revisit this, need it for Currency reserve in increase_bid + T::Currency::make_free_balance_be(&who, >::get() * 3u32.into()); >::register_as_candidate(RawOrigin::Signed(who).into()).unwrap(); } } @@ -107,8 +105,11 @@ fn min_candidates() -> u32 { fn min_invulnerables() -> u32 { let min_collators = T::MinEligibleCollators::get(); - let candidates_length = >::get().len(); - min_collators.saturating_sub(candidates_length.try_into().unwrap()) + let candidates_length = >::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or_default(); + min_collators.saturating_sub(candidates_length) } #[benchmarks(where T: pallet_authorship::Config + session::Config)] @@ -160,22 +161,19 @@ mod benchmarks { .unwrap(); } // ... and register them. - for (who, _) in candidates { + for (who, _) in candidates.iter() { let deposit = >::get(); - T::Currency::make_free_balance_be(&who, deposit * 1000_u32.into()); - let incoming = CandidateInfo { who: who.clone(), deposit }; - >::try_mutate(|candidates| -> DispatchResult { - if !candidates.iter().any(|candidate| candidate.who == who) { - T::Currency::reserve(&who, deposit)?; - candidates.try_push(incoming).expect("we've respected the bounded vec limit"); - >::insert( - who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), - ); - } - Ok(()) + T::Currency::make_free_balance_be(who, deposit * 1000_u32.into()); + >::try_mutate(|list| { + list.try_push(CandidateInfo { who: who.clone(), deposit }).unwrap(); + Ok::<(), BenchmarkError>(()) }) - .expect("only returns ok"); + .unwrap(); + T::Currency::reserve(who, deposit)?; + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); } // now we need to fill up invulnerables @@ -238,6 +236,35 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn update_bond( + c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>, + ) -> Result<(), BenchmarkError> { + >::put(T::Currency::minimum_balance()); + >::put(c); + + register_validators::(c); + register_candidates::(c); + + let caller = >::get()[0].who.clone(); + v2::whitelist!(caller); + + let bond_amount: BalanceOf = + T::Currency::minimum_balance() + T::Currency::minimum_balance(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), bond_amount); + + assert_last_event::( + Event::CandidateBondUpdated { account_id: caller, deposit: bond_amount }.into(), + ); + assert!( + >::get().iter().last().unwrap().deposit == + T::Currency::minimum_balance() * 2u32.into() + ); + Ok(()) + } + // worse case is when we have all the max-candidate slots filled except one, and we fill that // one. #[benchmark] @@ -267,6 +294,36 @@ mod benchmarks { ); } + #[benchmark] + fn take_candidate_slot(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { + >::put(T::Currency::minimum_balance()); + >::put(1); + + register_validators::(c); + register_candidates::(c); + + let caller: T::AccountId = whitelisted_caller(); + let bond: BalanceOf = T::Currency::minimum_balance() * 10u32.into(); + T::Currency::make_free_balance_be(&caller, bond); + + >::set_keys( + RawOrigin::Signed(caller.clone()).into(), + keys::(c + 1), + Vec::new(), + ) + .unwrap(); + + let target = >::get().iter().last().unwrap().who.clone(); + + #[extrinsic_call] + _(RawOrigin::Signed(caller.clone()), bond / 2u32.into(), target.clone()); + + assert_last_event::( + Event::CandidateReplaced { old: target, new: caller, deposit: bond / 2u32.into() } + .into(), + ); + } + // worse case is the last candidate leaving. #[benchmark] fn leave_intent(c: Linear<{ min_candidates::() + 1 }, { T::MaxCandidates::get() }>) { @@ -276,7 +333,7 @@ mod benchmarks { register_validators::(c); register_candidates::(c); - let leaving = >::get().last().unwrap().who.clone(); + let leaving = >::get().iter().last().unwrap().who.clone(); v2::whitelist!(leaving); #[extrinsic_call] @@ -323,31 +380,37 @@ mod benchmarks { let new_block: BlockNumberFor = 1800u32.into(); let zero_block: BlockNumberFor = 0u32.into(); - let candidates = >::get(); + let candidates: Vec = >::get() + .iter() + .map(|candidate_info| candidate_info.who.clone()) + .collect(); let non_removals = c.saturating_sub(r); for i in 0..c { - >::insert(candidates[i as usize].who.clone(), zero_block); + >::insert(candidates[i as usize].clone(), zero_block); } if non_removals > 0 { for i in 0..non_removals { - >::insert(candidates[i as usize].who.clone(), new_block); + >::insert(candidates[i as usize].clone(), new_block); } } else { for i in 0..c { - >::insert(candidates[i as usize].who.clone(), new_block); + >::insert(candidates[i as usize].clone(), new_block); } } let min_candidates = min_candidates::(); - let pre_length = >::get().len(); + let pre_length = >::decode_len().unwrap_or_default(); frame_system::Pallet::::set_block_number(new_block); - assert!(>::get().len() == c as usize); - + let current_length: u32 = >::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or_default(); + assert!(c == current_length); #[block] { as SessionManager<_>>::new_session(0); @@ -357,16 +420,20 @@ mod benchmarks { // candidates > removals and remaining candidates > min candidates // => remaining candidates should be shorter than before removal, i.e. some were // actually removed. - assert!(>::get().len() < pre_length); + assert!(>::decode_len().unwrap_or_default() < pre_length); } else if c > r && non_removals < min_candidates { // candidates > removals and remaining candidates would be less than min candidates // => remaining candidates should equal min candidates, i.e. some were removed up to // the minimum, but then any more were "forced" to stay in candidates. - assert!(>::get().len() == min_candidates as usize); + let current_length: u32 = >::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or_default(); + assert!(min_candidates == current_length); } else { // removals >= candidates, non removals must == 0 // can't remove more than exist - assert!(>::get().len() == pre_length); + assert!(>::decode_len().unwrap_or_default() == pre_length); } } diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 24493ce9d9cd..c43789ee0e87 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -183,8 +183,8 @@ pub mod pallet { /// The (community, limited) collation candidates. `Candidates` and `Invulnerables` should be /// mutually exclusive. #[pallet::storage] - #[pallet::getter(fn candidates)] - pub type Candidates = StorageValue< + #[pallet::getter(fn candidate_list)] + pub type CandidateList = StorageValue< _, BoundedVec>, T::MaxCandidates>, ValueQuery, @@ -261,8 +261,12 @@ pub mod pallet { NewCandidacyBond { bond_amount: BalanceOf }, /// A new candidate joined. CandidateAdded { account_id: T::AccountId, deposit: BalanceOf }, + /// Bond of a candidate updated. + CandidateBondUpdated { account_id: T::AccountId, deposit: BalanceOf }, /// A candidate was removed. CandidateRemoved { account_id: T::AccountId }, + /// An account was replaced in the candidate list by another one. + CandidateReplaced { old: T::AccountId, new: T::AccountId, deposit: BalanceOf }, /// An account was unable to be added to the Invulnerables because they did not have keys /// registered. Other Invulnerables may have been set. InvalidInvulnerableSkipped { account_id: T::AccountId }, @@ -288,6 +292,20 @@ pub mod pallet { NoAssociatedValidatorId, /// Validator ID is not yet registered. ValidatorNotRegistered, + /// Could not insert in the candidate list. + InsertToCandidateListFailed, + /// Could not remove from the candidate list. + RemoveFromCandidateListFailed, + /// New deposit amount would be below the minimum candidacy bond. + DepositTooLow, + /// Could not update the candidate list. + UpdateCandidateListFailed, + /// Deposit amount is too low to take the target's slot in the + /// candidate list. + InsufficientBond, + /// The target account to be replaced in the candidate list is not a + /// candidate. + TargetIsNotCandidate, } #[pallet::hooks] @@ -320,8 +338,8 @@ pub mod pallet { // don't wipe out the collator set if new.is_empty() { ensure!( - Candidates::::decode_len().unwrap_or_default() >= - T::MinEligibleCollators::get() as usize, + CandidateList::::decode_len().unwrap_or_default() >= + T::MinEligibleCollators::get().try_into().unwrap(), Error::::TooFewEligibleCollators ); } @@ -424,8 +442,11 @@ pub mod pallet { let who = ensure_signed(origin)?; // ensure we are below limit. - let length = >::decode_len().unwrap_or_default(); - ensure!((length as u32) < Self::desired_candidates(), Error::::TooManyCandidates); + let length: u32 = >::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or_default(); + ensure!(length < T::MaxCandidates::get(), Error::::TooManyCandidates); ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); let validator_key = T::ValidatorIdOf::convert(who.clone()) @@ -437,25 +458,24 @@ pub mod pallet { 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 current_count = - >::try_mutate(|candidates| -> Result { - if candidates.iter().any(|candidate| candidate.who == who) { - Err(Error::::AlreadyCandidate)? - } else { - T::Currency::reserve(&who, deposit)?; - candidates.try_push(incoming).map_err(|_| Error::::TooManyCandidates)?; - >::insert( - who.clone(), - frame_system::Pallet::::block_number() + T::KickThreshold::get(), - ); - Ok(candidates.len()) - } - })?; + >::try_mutate(|candidates| -> Result<(), DispatchError> { + ensure!( + !candidates.iter().any(|candidate_info| candidate_info.who == who), + Error::::AlreadyCandidate + ); + T::Currency::reserve(&who, deposit)?; + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); + candidates + .try_insert(0, CandidateInfo { who: who.clone(), deposit }) + .map_err(|_| Error::::InsertToCandidateListFailed)?; + Ok(()) + })?; Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); - Ok(Some(T::WeightInfo::register_as_candidate(current_count as u32)).into()) + Ok(Some(T::WeightInfo::register_as_candidate(length + 1)).into()) } /// Deregister `origin` as a collator candidate. Note that the collator can only leave on @@ -468,13 +488,14 @@ pub mod pallet { pub fn leave_intent(origin: OriginFor) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; ensure!( - Self::eligible_collators() > T::MinEligibleCollators::get() as usize, + Self::eligible_collators() > T::MinEligibleCollators::get(), Error::::TooFewEligibleCollators ); + let length = >::decode_len().unwrap_or_default(); // Do remove their last authored block. - let current_count = Self::try_remove_candidate(&who, true)?; + Self::try_remove_candidate(&who, true)?; - Ok(Some(T::WeightInfo::leave_intent(current_count as u32)).into()) + Ok(Some(T::WeightInfo::leave_intent(length.saturating_sub(1) as u32)).into()) } /// Add a new account `who` to the list of `Invulnerables` collators. `who` must have @@ -521,10 +542,10 @@ pub mod pallet { .unwrap_or_default() .try_into() .unwrap_or(T::MaxInvulnerables::get().saturating_sub(1)), - Candidates::::decode_len() + >::decode_len() .unwrap_or_default() .try_into() - .unwrap_or(T::MaxCandidates::get()), + .unwrap_or_default(), ); Ok(Some(weight_used).into()) @@ -540,7 +561,7 @@ pub mod pallet { T::UpdateOrigin::ensure_origin(origin)?; ensure!( - Self::eligible_collators() > T::MinEligibleCollators::get() as usize, + Self::eligible_collators() > T::MinEligibleCollators::get(), Error::::TooFewEligibleCollators ); @@ -554,6 +575,138 @@ pub mod pallet { Self::deposit_event(Event::InvulnerableRemoved { account_id: who }); Ok(()) } + + /// Update the candidacy bond of collator candidate `origin` to a + /// new amount `new_deposit`. + /// + /// This call will fail if `origin` is not a collator candidate, the + /// updated bond is lower than the minimum candidacy bond, and/or + /// the amount cannot be reserved. + #[pallet::call_index(7)] + #[pallet::weight(T::WeightInfo::update_bond(T::MaxCandidates::get()))] + pub fn update_bond( + origin: OriginFor, + new_deposit: BalanceOf, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + ensure!(new_deposit >= >::get(), Error::::DepositTooLow); + // The function below will try to mutate the `Candidates` entry for + // the caller to update their deposit to the new value of + // `new_deposit`. The return value is a tuple of the position of + // the entry in the list, used for weight calculation, and the new + // bonded amount. + let (new_amount, length) = >::try_mutate( + |candidates| -> Result<(BalanceOf, usize), DispatchError> { + let mut idx = candidates + .iter() + .position(|candidate_info| candidate_info.who == who) + .ok_or_else(|| Error::::NotCandidate)?; + let candidate_count = candidates.len(); + let old_deposit = candidates[idx].deposit; + if new_deposit > old_deposit { + T::Currency::reserve(&who, new_deposit - old_deposit)?; + } else if new_deposit < old_deposit { + T::Currency::unreserve(&who, old_deposit - new_deposit); + } + candidates[idx].deposit = new_deposit; + if new_deposit > old_deposit && idx < candidate_count { + idx += 1; + while idx < candidate_count && candidates[idx].deposit < new_deposit { + candidates.as_mut().swap(idx - 1, idx); + idx += 1; + } + } else { + while idx > 0 && candidates[idx].deposit >= new_deposit { + candidates.as_mut().swap(idx - 1, idx); + idx -= 1; + } + } + + Ok((new_deposit, candidate_count)) + }, + )?; + + Self::deposit_event(Event::CandidateBondUpdated { + account_id: who, + deposit: new_amount, + }); + Ok(Some(T::WeightInfo::update_bond(length as u32)).into()) + } + + /// The caller `origin` replaces a candidate `target` in the collator + /// candidate list by reserving `deposit`. The amount `deposit` + /// reserved by the caller must be greater than the existing bond of + /// the target it is trying to replace. + /// + /// This call will fail if the caller is already a collator candidate + /// or invulnerable, the caller does not have registered session keys, + /// the target is not a collator candidate, and/or the `deposit` amount + /// cannot be reserved. + #[pallet::call_index(8)] + #[pallet::weight(T::WeightInfo::take_candidate_slot(T::MaxCandidates::get()))] + pub fn take_candidate_slot( + origin: OriginFor, + deposit: BalanceOf, + target: T::AccountId, + ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + + ensure!(!Self::invulnerables().contains(&who), Error::::AlreadyInvulnerable); + ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); + + let validator_key = T::ValidatorIdOf::convert(who.clone()) + .ok_or(Error::::NoAssociatedValidatorId)?; + ensure!( + T::ValidatorRegistration::is_registered(&validator_key), + Error::::ValidatorNotRegistered + ); + + ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); + + let length = >::decode_len().unwrap_or_default(); + let (target_info_idx, target_info) = + >::try_mutate( + |candidates| -> Result< + (usize, CandidateInfo>), + DispatchError, + > { + let mut target_info_idx = None; + for (idx, candidate_info) in candidates.iter().enumerate() { + ensure!(candidate_info.who != who, Error::::AlreadyCandidate); + if candidate_info.who == target { + target_info_idx = Some(idx); + } + } + let target_info_idx = + target_info_idx.ok_or(Error::::TargetIsNotCandidate)?; + Ok((target_info_idx, candidates[target_info_idx].clone())) + }, + )?; + T::Currency::unreserve(&target_info.who, target_info.deposit); + >::remove(target_info.who.clone()); + ensure!(deposit > target_info.deposit, Error::::InsufficientBond); + T::Currency::reserve(&who, deposit)?; + >::insert( + who.clone(), + frame_system::Pallet::::block_number() + T::KickThreshold::get(), + ); + // Replace the old candidate in the list with the new candidate and + // then move them up the list to the correct place, if necessary. + >::try_mutate(|list| { + let mut idx = target_info_idx; + list[idx].who = who.clone(); + list[idx].deposit = deposit; + idx += 1; + while idx < list.len() && list[idx].deposit < deposit { + list.as_mut().swap(idx - 1, idx); + idx += 1; + } + Ok::<(), Error>(()) + })?; + + Self::deposit_event(Event::CandidateReplaced { old: target, new: who, deposit }); + Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into()) + } } impl Pallet { @@ -564,81 +717,96 @@ pub mod pallet { /// Return the total number of accounts that are eligible collators (candidates and /// invulnerables). - fn eligible_collators() -> usize { - Candidates::::decode_len() + fn eligible_collators() -> u32 { + // TODO[GMP]: rethink unwraps with `MaxCandidates` in mind. + >::decode_len() .unwrap_or_default() - .saturating_add(Invulnerables::::decode_len().unwrap_or_default()) + .try_into() + .unwrap_or(u32::MAX) + .saturating_add( + Invulnerables::::decode_len() + .unwrap_or_default() + .try_into() + .unwrap_or(u32::MAX), + ) } /// Removes a candidate if they exist and sends them back their deposit. fn try_remove_candidate( who: &T::AccountId, remove_last_authored: bool, - ) -> Result { - let current_count = - >::try_mutate(|candidates| -> Result { - let index = candidates - .iter() - .position(|candidate| candidate.who == *who) - .ok_or(Error::::NotCandidate)?; - let candidate = candidates.remove(index); - T::Currency::unreserve(who, candidate.deposit); - if remove_last_authored { - >::remove(who.clone()) - }; - Ok(candidates.len()) - })?; + ) -> Result<(), DispatchError> { + >::try_mutate(|candidates| -> Result<(), DispatchError> { + let idx = candidates + .iter() + .position(|candidate_info| candidate_info.who == *who) + .ok_or(Error::::NotCandidate)?; + let deposit = candidates[idx].deposit; + T::Currency::unreserve(who, deposit); + candidates.remove(idx); + if remove_last_authored { + >::remove(who.clone()) + }; + Ok(()) + })?; Self::deposit_event(Event::CandidateRemoved { account_id: who.clone() }); - Ok(current_count) + Ok(()) } /// Assemble the current set of candidates and invulnerables into the next collator set. /// /// This is done on the fly, as frequent as we are told to do so, as the session manager. - pub fn assemble_collators( - candidates: BoundedVec, - ) -> Vec { + pub fn assemble_collators() -> Vec { + let desired_candidates: usize = + >::get().try_into().unwrap_or(usize::MAX); let mut collators = Self::invulnerables().to_vec(); - collators.extend(candidates); + collators.extend( + >::get() + .iter() + .rev() + .cloned() + .take(desired_candidates) + .map(|candidate_info| candidate_info.who), + ); collators } /// Kicks out candidates that did not produce a block in the kick threshold and refunds /// their deposits. - pub fn kick_stale_candidates( - candidates: BoundedVec>, T::MaxCandidates>, - ) -> BoundedVec { + /// + /// Return value is the number of candidates left in the list. + pub fn kick_stale_candidates(candidates: impl IntoIterator) -> u32 { let now = frame_system::Pallet::::block_number(); let kick_threshold = T::KickThreshold::get(); let min_collators = T::MinEligibleCollators::get(); candidates .into_iter() .filter_map(|c| { - let last_block = >::get(c.who.clone()); + let last_block = >::get(c.clone()); let since_last = now.saturating_sub(last_block); - let is_invulnerable = Self::invulnerables().contains(&c.who); + let is_invulnerable = Self::invulnerables().contains(&c); let is_lazy = since_last >= kick_threshold; if is_invulnerable { // They are invulnerable. No reason for them to be in Candidates also. // We don't even care about the min collators here, because an Account // should not be a collator twice. - let _ = Self::try_remove_candidate(&c.who, false); + let _ = Self::try_remove_candidate(&c, false); None } else { - if Self::eligible_collators() <= min_collators as usize || !is_lazy { + if Self::eligible_collators() <= min_collators || !is_lazy { // Either this is a good collator (not lazy) or we are at the minimum // that the system needs. They get to stay. - Some(c.who) + Some(c) } else { // This collator has not produced a block recently enough. Bye bye. - let _ = Self::try_remove_candidate(&c.who, true); + let _ = Self::try_remove_candidate(&c, true); None } } }) - .collect::>() + .count() .try_into() .expect("filter_map operation can't result in a bounded vec larger than its original; qed") } @@ -677,11 +845,15 @@ pub mod pallet { >::block_number(), ); - let candidates = Self::candidates(); - let candidates_len_before = candidates.len(); - let active_candidates = Self::kick_stale_candidates(candidates); - let removed = candidates_len_before - active_candidates.len(); - let result = Self::assemble_collators(active_candidates); + let candidates_len_before = >::decode_len().unwrap_or_default(); + let active_candidates_count = Self::kick_stale_candidates( + >::get() + .iter() + .map(|candidate_info| candidate_info.who.clone()), + ); + let removed = candidates_len_before + .saturating_sub(active_candidates_count.try_into().unwrap_or_default()); + let result = Self::assemble_collators(); frame_system::Pallet::::register_extra_weight_unchecked( T::WeightInfo::new_session(candidates_len_before as u32, removed as u32), diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index d4dae513df37..38968e5a8d6f 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -28,7 +28,7 @@ fn basic_setup_works() { assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert!(CollatorSelection::candidates().is_empty()); + assert_eq!(>::get().iter().count(), 0); // genesis should sort input assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); }); @@ -202,7 +202,8 @@ fn candidate_to_invulnerable_works() { initialize_to_block(1); assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + + assert_eq!(>::get().iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_eq!(Balances::free_balance(3), 100); @@ -226,7 +227,7 @@ fn candidate_to_invulnerable_works() { )); assert!(CollatorSelection::invulnerables().to_vec().contains(&3)); assert_eq!(Balances::free_balance(3), 100); - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::get().iter().count(), 1); assert_ok!(CollatorSelection::add_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -240,7 +241,8 @@ fn candidate_to_invulnerable_works() { )); assert!(CollatorSelection::invulnerables().to_vec().contains(&4)); assert_eq!(Balances::free_balance(4), 100); - assert_eq!(CollatorSelection::candidates().len(), 0); + + assert_eq!(>::get().iter().count(), 0); }); } @@ -286,22 +288,23 @@ fn set_candidacy_bond() { #[test] fn cannot_register_candidate_if_too_many() { new_test_ext().execute_with(|| { - // reset desired candidates: - >::put(0); + >::put(1); - // can't accept anyone anymore. - assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3)), - Error::::TooManyCandidates, - ); + // MaxCandidates: u32 = 20 + // Aside from 3, 4, and 5, create enough accounts to have 21 potential + // candidates. + for i in 6..=23 { + Balances::make_free_balance_be(&i, 100); + let key = MockSessionKeys { aura: UintAuthorityId(i) }; + Session::set_keys(RuntimeOrigin::signed(i).into(), key, Vec::new()).unwrap(); + } - // reset desired candidates: - >::put(1); - assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + for c in 3..=22 { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(c))); + } - // but no more assert_noop!( - CollatorSelection::register_as_candidate(RuntimeOrigin::signed(5)), + CollatorSelection::register_as_candidate(RuntimeOrigin::signed(23)), Error::::TooManyCandidates, ); }) @@ -310,7 +313,7 @@ fn cannot_register_candidate_if_too_many() { #[test] fn cannot_unregister_candidate_if_too_few() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::get().iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::remove_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -368,8 +371,12 @@ fn cannot_register_dupe_candidate() { new_test_ext().execute_with(|| { // can add 3 as candidate assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + // tuple of (id, deposit). let addition = CandidateInfo { who: 3, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![addition]); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![addition] + ); assert_eq!(CollatorSelection::last_authored_block(3), 10); assert_eq!(Balances::free_balance(3), 90); @@ -404,20 +411,416 @@ fn register_as_candidate_works() { // given assert_eq!(CollatorSelection::desired_candidates(), 2); assert_eq!(CollatorSelection::candidacy_bond(), 10); - assert_eq!(CollatorSelection::candidates(), Vec::new()); + + assert_eq!(>::get().iter().count(), 0); + 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); + + 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); + + assert_eq!(>::get().iter().count(), 2); + }); +} + +#[test] +fn cannot_take_candidate_slot_if_invulnerable() { + new_test_ext().execute_with(|| { + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // can't 1 because it is invulnerable. + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(1), 50u64.into(), 2), + Error::::AlreadyInvulnerable, + ); + }) +} + +#[test] +fn cannot_take_candidate_slot_if_keys_not_registered() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(42), 50u64.into(), 3), + Error::::ValidatorNotRegistered + ); + }) +} + +#[test] +fn cannot_take_candidate_slot_if_duplicate() { + new_test_ext().execute_with(|| { + // can add 3 as candidate + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + // tuple of (id, deposit). + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 10 }; + let actual_candidates = + >::get().iter().cloned().collect::>(); + assert_eq!(actual_candidates, vec![candidate_4, candidate_3]); + assert_eq!(CollatorSelection::last_authored_block(3), 10); + assert_eq!(CollatorSelection::last_authored_block(4), 10); + assert_eq!(Balances::free_balance(3), 90); + + // but no more + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(3), 50u64.into(), 4), + Error::::AlreadyCandidate, + ); + }) +} + +#[test] +fn cannot_take_candidate_slot_if_target_invalid() { + new_test_ext().execute_with(|| { + // can add 3 as candidate + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + // tuple of (id, deposit). + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![candidate_3] + ); + assert_eq!(CollatorSelection::last_authored_block(3), 10); + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 100); + + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 50u64.into(), 5), + Error::::TargetIsNotCandidate, + ); + }) +} + +#[test] +fn cannot_take_candidate_slot_if_poor() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(33), 0); + + // works + assert_ok!(CollatorSelection::take_candidate_slot( + RuntimeOrigin::signed(3), + 20u64.into(), + 4 + )); + + // poor + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(33), 30u64.into(), 3), + BalancesError::::InsufficientBalance, + ); + }); +} + +#[test] +fn cannot_take_candidate_slot_if_insufficient_deposit() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 60u64.into())); + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 5u64.into(), 3), + Error::::InsufficientBond, + ); + }); +} + +#[test] +fn cannot_take_candidate_slot_if_deposit_less_than_target() { + new_test_ext().execute_with(|| { + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 60u64.into())); + assert_noop!( + CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 20u64.into(), 3), + Error::::InsufficientBond, + ); + }); +} + +#[test] +fn take_candidate_slot_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + 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); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_eq!(>::get().iter().count(), 3); + + Balances::make_free_balance_be(&6, 100); + let key = MockSessionKeys { aura: UintAuthorityId(6) }; + Session::set_keys(RuntimeOrigin::signed(6).into(), key, Vec::new()).unwrap(); + + assert_ok!(CollatorSelection::take_candidate_slot( + RuntimeOrigin::signed(6), + 50u64.into(), + 4 + )); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 90); + assert_eq!(Balances::free_balance(6), 50); + + // tuple of (id, deposit). + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_6 = CandidateInfo { who: 6, deposit: 50 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 10 }; + let mut actual_candidates = + >::get().iter().cloned().collect::>(); + actual_candidates.sort_by(|info_1, info_2| info_1.deposit.cmp(&info_2.deposit)); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![candidate_5, candidate_3, candidate_6] + ); + }); +} + +#[test] +fn increase_candidacy_bond_non_candidate_account() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(5), 20), + Error::::NotCandidate + ); + }); +} + +#[test] +fn increase_candidacy_bond_insufficient_balance() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + 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); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(3), 110), + BalancesError::::InsufficientBalance + ); + }); +} + +#[test] +fn increase_candidacy_bond_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 30)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 40)); + + assert_eq!(>::get().iter().count(), 3); + assert_eq!(Balances::free_balance(3), 80); + assert_eq!(Balances::free_balance(4), 70); + assert_eq!(Balances::free_balance(5), 60); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 40)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 60)); + + assert_eq!(>::get().iter().count(), 3); + assert_eq!(Balances::free_balance(3), 60); + assert_eq!(Balances::free_balance(4), 40); + assert_eq!(Balances::free_balance(5), 60); + }); +} + +#[test] +fn decrease_candidacy_bond_non_candidate_account() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(5), 10), + Error::::NotCandidate + ); + }); +} + +#[test] +fn decrease_candidacy_bond_insufficient_funds() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); 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); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 60)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 60)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60)); + + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 40); + assert_eq!(Balances::free_balance(5), 40); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(3), 0), + Error::::DepositTooLow + ); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(4), 5), + Error::::DepositTooLow + ); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(5), 9), + Error::::DepositTooLow + ); + }); +} + +#[test] +fn decrease_candidacy_bond_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 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(5))); assert_eq!(Balances::free_balance(3), 90); assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); - assert_eq!(CollatorSelection::candidates().len(), 2); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 30)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 40)); + + assert_eq!(>::get().iter().count(), 3); + assert_eq!(Balances::free_balance(3), 80); + assert_eq!(Balances::free_balance(4), 70); + assert_eq!(Balances::free_balance(5), 60); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 20)); + + assert_eq!(>::get().iter().count(), 3); + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 80); + assert_eq!(Balances::free_balance(5), 60); + }); +} + +#[test] +fn candidate_list_works() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 20)); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 30)); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10)); }); } @@ -457,9 +860,13 @@ fn authorship_event_handler() { // triggers `note_author` Authorship::on_initialize(1); + // tuple of (id, deposit). let collator = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![collator] + ); assert_eq!(CollatorSelection::last_authored_block(4), 0); // half of the pot goes to the collator who's the author (4 in tests). @@ -482,9 +889,13 @@ fn fees_edgecases() { // triggers `note_author` Authorship::on_initialize(1); + // tuple of (id, deposit). let collator = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![collator] + ); assert_eq!(CollatorSelection::last_authored_block(4), 0); // Nothing received assert_eq!(Balances::free_balance(4), 90); @@ -494,7 +905,7 @@ fn fees_edgecases() { } #[test] -fn session_management_works() { +fn session_management_single_candidate() { new_test_ext().execute_with(|| { initialize_to_block(1); @@ -512,7 +923,7 @@ fn session_management_works() { // session won't see this. assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); // but we have a new candidate. - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::get().iter().count(), 1); initialize_to_block(10); assert_eq!(SessionChangeBlock::get(), 10); @@ -530,6 +941,221 @@ fn session_management_works() { }); } +#[test] +fn session_management_max_candidates() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + 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(5))); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(>::get().iter().count(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 4]); + }); +} + +#[test] +fn session_management_increase_bid_with_list_update() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + 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(5))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(>::get().iter().count(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5, 3]); + }); +} + +#[test] +fn session_management_candidate_list_eager_sort() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + 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(5))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(>::get().iter().count(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 5, 3]); + }); +} + +#[test] +fn session_management_reciprocal_outbidding() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + 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(5))); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60)); + + initialize_to_block(5); + + // candidates 3 and 4 saw they were outbid and preemptively bid more + // than 5 in the next block. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 70)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 70)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(>::get().iter().count(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 4, 3]); + }); +} + +#[test] +fn session_management_decrease_bid_after_auction() { + new_test_ext().execute_with(|| { + initialize_to_block(1); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(4); + + assert_eq!(SessionChangeBlock::get(), 0); + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + 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(5))); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60)); + + initialize_to_block(5); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 70)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 70)); + + initialize_to_block(5); + + // candidate 5 saw it was outbid and wants to take back its bid, but + // not entirely so they still keep their place in the candidate list + // in case there is an opportunity in the future. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 10)); + + // session won't see this. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + // but we have a new candidate. + assert_eq!(>::get().iter().count(), 3); + + initialize_to_block(10); + assert_eq!(SessionChangeBlock::get(), 10); + // pallet-session has 1 session delay; current validators are the same. + assert_eq!(Session::validators(), vec![1, 2]); + // queued ones are changed, and now we have 4. + assert_eq!(Session::queued_keys().len(), 4); + // session handlers (aura, et. al.) cannot see this yet. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2]); + + initialize_to_block(20); + assert_eq!(SessionChangeBlock::get(), 20); + // changed are now reflected to session handlers. + assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 4, 3]); + }); +} + #[test] fn kick_mechanism() { new_test_ext().execute_with(|| { @@ -537,15 +1163,19 @@ fn kick_mechanism() { 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); + assert_eq!(>::get().iter().count(), 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); + assert_eq!(>::get().iter().count(), 1); // 3 will be kicked after 1 session delay assert_eq!(SessionHandlerCollators::get(), vec![1, 2, 3, 4]); + // tuple of (id, deposit). let collator = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![collator] + ); assert_eq!(CollatorSelection::last_authored_block(4), 20); initialize_to_block(30); // 3 gets kicked after 1 session delay @@ -559,7 +1189,8 @@ fn kick_mechanism() { fn should_not_kick_mechanism_too_few() { new_test_ext().execute_with(|| { // remove the invulnerables and add new collators 3 and 5 - assert_eq!(CollatorSelection::candidates(), Vec::new()); + + assert_eq!(>::get().iter().count(), 0); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); assert_ok!(CollatorSelection::remove_invulnerable( RuntimeOrigin::signed(RootAccount::get()), @@ -573,30 +1204,34 @@ fn should_not_kick_mechanism_too_few() { )); initialize_to_block(10); - assert_eq!(CollatorSelection::candidates().len(), 2); + assert_eq!(>::get().iter().count(), 2); initialize_to_block(20); assert_eq!(SessionChangeBlock::get(), 20); // 4 authored this block, 3 is kicked, 5 stays because of too few collators - assert_eq!(CollatorSelection::candidates().len(), 1); + assert_eq!(>::get().iter().count(), 1); // 3 will be kicked after 1 session delay assert_eq!(SessionHandlerCollators::get(), vec![3, 5]); - let collator = CandidateInfo { who: 5, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator]); + // tuple of (id, deposit). + let collator = CandidateInfo { who: 3, deposit: 10 }; + assert_eq!( + >::get().iter().cloned().collect::>(), + 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![5]); + assert_eq!(SessionHandlerCollators::get(), vec![3]); // kicked collator gets funds back - assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(5), 100); }); } #[test] fn should_kick_invulnerables_from_candidates_on_session_change() { new_test_ext().execute_with(|| { - assert_eq!(CollatorSelection::candidates(), Vec::new()); + assert_eq!(>::get().iter().count(), 0); 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); @@ -606,16 +1241,22 @@ fn should_kick_invulnerables_from_candidates_on_session_change() { vec![1, 2, 3] )); + // tuple of (id, deposit). let collator_3 = CandidateInfo { who: 3, deposit: 10 }; let collator_4 = CandidateInfo { who: 4, deposit: 10 }; - assert_eq!(CollatorSelection::candidates(), vec![collator_3, collator_4.clone()]); + let actual_candidates = + >::get().iter().cloned().collect::>(); + assert_eq!(actual_candidates, vec![collator_4.clone(), collator_3]); assert_eq!(CollatorSelection::invulnerables(), vec![1, 2, 3]); // session change initialize_to_block(10); // 3 is removed from candidates - assert_eq!(CollatorSelection::candidates(), vec![collator_4]); + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![collator_4] + ); // but not from invulnerables assert_eq!(CollatorSelection::invulnerables(), vec![1, 2, 3]); // and it got its deposit back diff --git a/cumulus/pallets/collator-selection/src/weights.rs b/cumulus/pallets/collator-selection/src/weights.rs index f8f86fb7dec2..0e08eda0dbb3 100644 --- a/cumulus/pallets/collator-selection/src/weights.rs +++ b/cumulus/pallets/collator-selection/src/weights.rs @@ -33,6 +33,8 @@ pub trait WeightInfo { fn set_candidacy_bond() -> Weight; fn register_as_candidate(_c: u32) -> Weight; fn leave_intent(_c: u32) -> Weight; + fn update_bond(_c: u32) -> Weight; + fn take_candidate_slot(_c: u32) -> Weight; fn note_author() -> Weight; fn new_session(_c: u32, _r: u32) -> Weight; } @@ -66,6 +68,20 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } + fn update_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } + fn take_candidate_slot(c: u32) -> Weight { + Weight::from_parts(71_196_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(T::DbWeight::get().reads(4_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) + } fn note_author() -> Weight { Weight::from_parts(71_461_000_u64, 0) .saturating_add(T::DbWeight::get().reads(3_u64)) @@ -158,6 +174,20 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(3_u64)) .saturating_add(RocksDbWeight::get().writes(4_u64)) } + fn update_bond(c: u32) -> Weight { + Weight::from_parts(55_336_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(151_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(3_u64)) + .saturating_add(RocksDbWeight::get().writes(4_u64)) + } + fn take_candidate_slot(c: u32) -> Weight { + Weight::from_parts(71_196_000_u64, 0) + // Standard Error: 0 + .saturating_add(Weight::from_parts(198_000_u64, 0).saturating_mul(c as u64)) + .saturating_add(RocksDbWeight::get().reads(3_u64)) + .saturating_add(RocksDbWeight::get().writes(4_u64)) + } fn new_session(r: u32, c: u32) -> Weight { Weight::from_parts(0_u64, 0) // Standard Error: 1_010_000 From 00a36714274e8661dc6ac8215f7bd89b816e0c8d Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Fri, 1 Sep 2023 11:18:23 +0300 Subject: [PATCH 02/33] Add new weights fn to runtimes Signed-off-by: georgepisaltu --- .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ 7 files changed, 168 insertions(+) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs index 7fe56ac31f7a..12d97284e8f3 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs index 53efb218440a..ce4a4ee607bf 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -176,6 +176,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs index 2473c58e4581..b62a87d3f0b2 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs index fa0ac199ca2c..3febabae4a58 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs index e0f4156fe4d4..f6aab0e35dd8 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs index 9cbfa6ce80e3..3fff5980c68c 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs index ea237d602a9b..abcceb5f9634 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs @@ -176,6 +176,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) From ea0b26979c4583a0ba0cc27f66492f4bf9474266 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 4 Sep 2023 16:46:54 +0300 Subject: [PATCH 03/33] Update `collator-selection` documentation Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index c43789ee0e87..691d2bfee0bb 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -35,16 +35,22 @@ //! //! 1. [`Invulnerables`]: a set of collators appointed by governance. These accounts will always be //! collators. -//! 2. [`Candidates`]: these are *candidates to the collation task* and may or may not be elected as -//! a final collator. +//! 2. [`CandidateList`]: these are *candidates to the collation task* and may or may not be elected +//! as a final collator. //! -//! The current implementation resolves congestion of [`Candidates`] in a first-come-first-serve -//! manner. +//! The current implementation resolves congestion of [`CandidateList`] through a simple election +//! mechanism. Initially candidates register by placing the minimum bond, up until the maximum +//! number of allowed candidates is reached. Then, if an account wants to register as a candidate, +//! they have to replace an existing one by placing a greater deposit. //! //! Candidates will not be allowed to get kicked or `leave_intent` if the total number of collators //! would fall below `MinEligibleCollators`. This is to ensure that some collators will always //! exist, i.e. someone is eligible to produce a block. //! +//! When a new session starts, candidates with the highest deposits will be selected in order until +//! the desired amount of collators is reached. Candidates can increase or decrease their deposits +//! between sessions in order to ensure they receive a slot in the collator list of that session. +//! //! ### Rewards //! //! The Collator Selection pallet maintains an on-chain account (the "Pot"). In each block, the From 5526ea291f94cfd823ad87f4dfc8dacdb7234248 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 12 Sep 2023 11:41:56 +0300 Subject: [PATCH 04/33] Minor code and docs cleanup Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 1 - cumulus/pallets/collator-selection/src/lib.rs | 20 +++++++------------ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index ba2d86518a10..5ead458e4473 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -91,7 +91,6 @@ fn register_candidates(count: u32) { assert!(>::get() > 0u32.into(), "Bond cannot be zero!"); for who in candidates { - // TODO[GMP] revisit this, need it for Currency reserve in increase_bid T::Currency::make_free_balance_be(&who, >::get() * 3u32.into()); >::register_as_candidate(RawOrigin::Signed(who).into()).unwrap(); } diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 691d2bfee0bb..5eefa63cbf30 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -48,8 +48,8 @@ //! exist, i.e. someone is eligible to produce a block. //! //! When a new session starts, candidates with the highest deposits will be selected in order until -//! the desired amount of collators is reached. Candidates can increase or decrease their deposits -//! between sessions in order to ensure they receive a slot in the collator list of that session. +//! the desired number of collators is reached. Candidates can increase or decrease their deposits +//! between sessions in order to ensure they receive a slot in the collator list. //! //! ### Rewards //! @@ -551,7 +551,7 @@ pub mod pallet { >::decode_len() .unwrap_or_default() .try_into() - .unwrap_or_default(), + .unwrap_or(T::MaxCandidates::get()), ); Ok(Some(weight_used).into()) @@ -596,7 +596,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; ensure!(new_deposit >= >::get(), Error::::DepositTooLow); - // The function below will try to mutate the `Candidates` entry for + // The function below will try to mutate the `CandidateList` entry for // the caller to update their deposit to the new value of // `new_deposit`. The return value is a tuple of the position of // the entry in the list, used for weight calculation, and the new @@ -688,10 +688,10 @@ pub mod pallet { Ok((target_info_idx, candidates[target_info_idx].clone())) }, )?; - T::Currency::unreserve(&target_info.who, target_info.deposit); - >::remove(target_info.who.clone()); ensure!(deposit > target_info.deposit, Error::::InsufficientBond); T::Currency::reserve(&who, deposit)?; + T::Currency::unreserve(&target_info.who, target_info.deposit); + >::remove(target_info.who.clone()); >::insert( who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), @@ -724,17 +724,11 @@ pub mod pallet { /// Return the total number of accounts that are eligible collators (candidates and /// invulnerables). fn eligible_collators() -> u32 { - // TODO[GMP]: rethink unwraps with `MaxCandidates` in mind. >::decode_len() .unwrap_or_default() + .saturating_add(Invulnerables::::decode_len().unwrap_or_default()) .try_into() .unwrap_or(u32::MAX) - .saturating_add( - Invulnerables::::decode_len() - .unwrap_or_default() - .try_into() - .unwrap_or(u32::MAX), - ) } /// Removes a candidate if they exist and sends them back their deposit. From 06b3c796e7613254815a925c689fa990d2df7398 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 12 Sep 2023 12:13:00 +0300 Subject: [PATCH 05/33] Fix doc line wrapping Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 5eefa63cbf30..d196dd69d2fb 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -306,11 +306,9 @@ pub mod pallet { DepositTooLow, /// Could not update the candidate list. UpdateCandidateListFailed, - /// Deposit amount is too low to take the target's slot in the - /// candidate list. + /// Deposit amount is too low to take the target's slot in the candidate list. InsufficientBond, - /// The target account to be replaced in the candidate list is not a - /// candidate. + /// The target account to be replaced in the candidate list is not a candidate. TargetIsNotCandidate, } @@ -331,8 +329,8 @@ pub mod pallet { /// acceptable Invulnerables, and is not proposing a _set_ of new Invulnerables. /// /// This call does not maintain mutual exclusivity of `Invulnerables` and `Candidates`. It - /// is recommended to use a batch of `add_invulnerable` and `remove_invulnerable` instead. - /// A `batch_all` can also be used to enforce atomicity. If any candidates are included in + /// is recommended to use a batch of `add_invulnerable` and `remove_invulnerable` instead. A + /// `batch_all` can also be used to enforce atomicity. If any candidates are included in /// `new`, they should be removed with `remove_invulnerable_candidate` after execution. /// /// Must be called by the `UpdateOrigin`. @@ -582,12 +580,10 @@ pub mod pallet { Ok(()) } - /// Update the candidacy bond of collator candidate `origin` to a - /// new amount `new_deposit`. + /// Update the candidacy bond of collator candidate `origin` to a new amount `new_deposit`. /// - /// This call will fail if `origin` is not a collator candidate, the - /// updated bond is lower than the minimum candidacy bond, and/or - /// the amount cannot be reserved. + /// This call will fail if `origin` is not a collator candidate, the updated bond is lower + /// than the minimum candidacy bond, and/or the amount cannot be reserved. #[pallet::call_index(7)] #[pallet::weight(T::WeightInfo::update_bond(T::MaxCandidates::get()))] pub fn update_bond( @@ -596,10 +592,9 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; ensure!(new_deposit >= >::get(), Error::::DepositTooLow); - // The function below will try to mutate the `CandidateList` entry for - // the caller to update their deposit to the new value of - // `new_deposit`. The return value is a tuple of the position of - // the entry in the list, used for weight calculation, and the new + // The function below will try to mutate the `CandidateList` entry for the caller to + // update their deposit to the new value of `new_deposit`. The return value is a tuple + // of the position of the entry in the list, used for weight calculation, and the new // bonded amount. let (new_amount, length) = >::try_mutate( |candidates| -> Result<(BalanceOf, usize), DispatchError> { @@ -639,15 +634,13 @@ pub mod pallet { Ok(Some(T::WeightInfo::update_bond(length as u32)).into()) } - /// The caller `origin` replaces a candidate `target` in the collator - /// candidate list by reserving `deposit`. The amount `deposit` - /// reserved by the caller must be greater than the existing bond of - /// the target it is trying to replace. + /// The caller `origin` replaces a candidate `target` in the collator candidate list by + /// reserving `deposit`. The amount `deposit` reserved by the caller must be greater than + /// the existing bond of the target it is trying to replace. /// - /// This call will fail if the caller is already a collator candidate - /// or invulnerable, the caller does not have registered session keys, - /// the target is not a collator candidate, and/or the `deposit` amount - /// cannot be reserved. + /// This call will fail if the caller is already a collator candidate or invulnerable, the + /// caller does not have registered session keys, the target is not a collator candidate, + /// and/or the `deposit` amount cannot be reserved. #[pallet::call_index(8)] #[pallet::weight(T::WeightInfo::take_candidate_slot(T::MaxCandidates::get()))] pub fn take_candidate_slot( @@ -696,8 +689,8 @@ pub mod pallet { who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), ); - // Replace the old candidate in the list with the new candidate and - // then move them up the list to the correct place, if necessary. + // Replace the old candidate in the list with the new candidate and then move them up + // the list to the correct place, if necessary. >::try_mutate(|list| { let mut idx = target_info_idx; list[idx].who = who.clone(); @@ -789,7 +782,7 @@ pub mod pallet { let is_lazy = since_last >= kick_threshold; if is_invulnerable { - // They are invulnerable. No reason for them to be in Candidates also. + // They are invulnerable. No reason for them to be in `CandidateList` also. // We don't even care about the min collators here, because an Account // should not be a collator twice. let _ = Self::try_remove_candidate(&c, false); From c55c8e2aa4d033de1b872bd5be9fcb533f24bae4 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 21 Sep 2023 15:45:50 +0300 Subject: [PATCH 06/33] Fix leftover documentation Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index d196dd69d2fb..ed42e0bbb263 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -593,11 +593,10 @@ pub mod pallet { let who = ensure_signed(origin)?; ensure!(new_deposit >= >::get(), Error::::DepositTooLow); // The function below will try to mutate the `CandidateList` entry for the caller to - // update their deposit to the new value of `new_deposit`. The return value is a tuple - // of the position of the entry in the list, used for weight calculation, and the new - // bonded amount. - let (new_amount, length) = >::try_mutate( - |candidates| -> Result<(BalanceOf, usize), DispatchError> { + // update their deposit to the new value of `new_deposit`. The return value is the + // position of the entry in the list, used for weight calculation. + let length = + >::try_mutate(|candidates| -> Result { let mut idx = candidates .iter() .position(|candidate_info| candidate_info.who == who) @@ -623,13 +622,12 @@ pub mod pallet { } } - Ok((new_deposit, candidate_count)) - }, - )?; + Ok(candidate_count) + })?; Self::deposit_event(Event::CandidateBondUpdated { account_id: who, - deposit: new_amount, + deposit: new_deposit, }); Ok(Some(T::WeightInfo::update_bond(length as u32)).into()) } From 24cae0196f9994c8ac3472f7bd2ac8e06f3420dd Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 21 Sep 2023 16:09:48 +0300 Subject: [PATCH 07/33] Update docs around unchecked arithmetic Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index ed42e0bbb263..4ab8a657ccd4 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -479,6 +479,9 @@ pub mod pallet { })?; Self::deposit_event(Event::CandidateAdded { account_id: who, deposit }); + // Safe to do unchecked add here because we ensure above that `length < + // T::MaxCandidates::get()`, and since `T::MaxCandidates` is `u32` it can be at most + // `u32::MAX`, therefore `length + 1` cannot overflow. Ok(Some(T::WeightInfo::register_as_candidate(length + 1)).into()) } @@ -609,7 +612,15 @@ pub mod pallet { T::Currency::unreserve(&who, old_deposit - new_deposit); } candidates[idx].deposit = new_deposit; + // Since `idx` is at most `T::MaxCandidates - 1`, because it's the position of + // an entry in a list that is at most `T::MaxCandidates` long, it will never be + // an out of bounds index access. Furthermore, the increment will not be an + // overflow since `idx` is `usize` and it can be at most `T::MaxCandidates`, + // which is `u32`. if new_deposit > old_deposit && idx < candidate_count { + // Since the lowest value for `idx` would be `0`, the first element in the + // list`, and the fact we do an increment of `idx` before iterating, we + // ensure the `idx - 1` list access in the loop is not an underflow. idx += 1; while idx < candidate_count && candidates[idx].deposit < new_deposit { candidates.as_mut().swap(idx - 1, idx); @@ -661,15 +672,26 @@ pub mod pallet { ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); let length = >::decode_len().unwrap_or_default(); + // The closure below iterates through all elements of the candidate list to ensure that + // the caller isn't already a candidate and to find the target it's trying to replace in + // the list. The return value is a tuple of the position of the candidate to be replaced + // in the list along with its candidate information. let (target_info_idx, target_info) = >::try_mutate( |candidates| -> Result< (usize, CandidateInfo>), DispatchError, > { + // Find the position in the list of the candidate that is being replaced. let mut target_info_idx = None; for (idx, candidate_info) in candidates.iter().enumerate() { + // While iterating through the candidates trying to find the target, + // also ensure on the same pass that our caller isn't already a + // candidate. ensure!(candidate_info.who != who, Error::::AlreadyCandidate); + // If we find our target, update the position but do not stop the + // iteration since we're also checking that the caller isn't already a + // candidate. if candidate_info.who == target { target_info_idx = Some(idx); } @@ -693,6 +715,13 @@ pub mod pallet { let mut idx = target_info_idx; list[idx].who = who.clone(); list[idx].deposit = deposit; + // Since `idx` is at most `T::MaxCandidates - 1`, because it's the position of an + // entry in a list that is at most `T::MaxCandidates` long, it will never be an out + // of bounds index access. Furthermore, the increment will not be an overflow since + // `idx` is `usize` and it can be at most `T::MaxCandidates`, which is `u32`. Since + // the lowest value for `idx` would be `0`, the first element in the list`, and the + // fact we do an increment of `idx` before iterating, we ensure the `idx - 1` list + // access in the loop is not an underflow. idx += 1; while idx < list.len() && list[idx].deposit < deposit { list.as_mut().swap(idx - 1, idx); From 21988bed7d24e90e5dbc854fa0873f658d1adb4d Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 21 Sep 2023 16:56:15 +0300 Subject: [PATCH 08/33] Add docs and test for unwraps Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 4ab8a657ccd4..34575ac7d998 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -316,6 +316,14 @@ pub mod pallet { impl Hooks> for Pallet { fn integrity_test() { assert!(T::MinEligibleCollators::get() > 0, "chain must require at least one collator"); + assert!( + T::MaxCandidates::get() <= u32::MAX, + "the extrinsics below rely on `MaxCandidates` fitting in a `u32`" + ); + assert!( + T::MinEligibleCollators::get() <= u32::MAX, + "the extrinsics below rely `MinEligibleCollators` fitting in a `u32`" + ); } } @@ -341,9 +349,10 @@ pub mod pallet { // don't wipe out the collator set if new.is_empty() { + // Casting `u32` to `usize` should be safe on all machines running this. ensure!( CandidateList::::decode_len().unwrap_or_default() >= - T::MinEligibleCollators::get().try_into().unwrap(), + T::MinEligibleCollators::get() as usize, Error::::TooFewEligibleCollators ); } @@ -777,8 +786,8 @@ pub mod pallet { /// /// This is done on the fly, as frequent as we are told to do so, as the session manager. pub fn assemble_collators() -> Vec { - let desired_candidates: usize = - >::get().try_into().unwrap_or(usize::MAX); + // Casting `u32` to `usize` should be safe on all machines running this. + let desired_candidates = >::get() as usize; let mut collators = Self::invulnerables().to_vec(); collators.extend( >::get() @@ -865,18 +874,23 @@ pub mod pallet { >::block_number(), ); - let candidates_len_before = >::decode_len().unwrap_or_default(); + // The `expect` below is safe because the list is a `BoundedVec` with a max size of + // `T::MaxCandidates`, which is a `u32`. When `decode_len` returns `Some(len)`, `len` + // must be valid and at most `u32::MAX`, which must always be able to convert to `u32`. + let candidates_len_before: u32 = >::decode_len() + .unwrap_or_default() + .try_into() + .expect("length is at most `T::MaxCandidates`, so it must fit in `u32`; qed"); let active_candidates_count = Self::kick_stale_candidates( >::get() .iter() .map(|candidate_info| candidate_info.who.clone()), ); - let removed = candidates_len_before - .saturating_sub(active_candidates_count.try_into().unwrap_or_default()); + let removed = candidates_len_before.saturating_sub(active_candidates_count); let result = Self::assemble_collators(); frame_system::Pallet::::register_extra_weight_unchecked( - T::WeightInfo::new_session(candidates_len_before as u32, removed as u32), + T::WeightInfo::new_session(candidates_len_before, removed), DispatchClass::Mandatory, ); Some(result) From a95d831016500c6795c9e71f437c5d4bfa5df254 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 21 Sep 2023 17:17:23 +0300 Subject: [PATCH 09/33] Allow clippy absurd comparison in integrity test Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 34575ac7d998..163e4b54a730 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -316,10 +316,12 @@ pub mod pallet { impl Hooks> for Pallet { fn integrity_test() { assert!(T::MinEligibleCollators::get() > 0, "chain must require at least one collator"); + #[allow(clippy::absurd_extreme_comparisons)] assert!( T::MaxCandidates::get() <= u32::MAX, "the extrinsics below rely on `MaxCandidates` fitting in a `u32`" ); + #[allow(clippy::absurd_extreme_comparisons)] assert!( T::MinEligibleCollators::get() <= u32::MAX, "the extrinsics below rely `MinEligibleCollators` fitting in a `u32`" From 06ac94a64854c538861fad088afb2257ee04aa7e Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 21 Sep 2023 17:43:16 +0300 Subject: [PATCH 10/33] Fix placement of clippy allow statement Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 163e4b54a730..fbf027bab8ad 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -314,14 +314,13 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { + #[allow(clippy::absurd_extreme_comparisons)] fn integrity_test() { assert!(T::MinEligibleCollators::get() > 0, "chain must require at least one collator"); - #[allow(clippy::absurd_extreme_comparisons)] assert!( T::MaxCandidates::get() <= u32::MAX, "the extrinsics below rely on `MaxCandidates` fitting in a `u32`" ); - #[allow(clippy::absurd_extreme_comparisons)] assert!( T::MinEligibleCollators::get() <= u32::MAX, "the extrinsics below rely `MinEligibleCollators` fitting in a `u32`" From 8d7a635b8b75ec253cda433b598fc9d40c3793cd Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 12 Oct 2023 13:09:31 +0200 Subject: [PATCH 11/33] Address review comments Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index fbf027bab8ad..dfcc75a4cbed 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -310,6 +310,8 @@ pub mod pallet { InsufficientBond, /// The target account to be replaced in the candidate list is not a candidate. TargetIsNotCandidate, + /// The updated deposit amount is equal to the amount already reserved. + IdenticalDeposit, } #[pallet::hooks] @@ -620,6 +622,8 @@ pub mod pallet { T::Currency::reserve(&who, new_deposit - old_deposit)?; } else if new_deposit < old_deposit { T::Currency::unreserve(&who, old_deposit - new_deposit); + } else { + return Err(Error::::IdenticalDeposit.into()) } candidates[idx].deposit = new_deposit; // Since `idx` is at most `T::MaxCandidates - 1`, because it's the position of @@ -629,7 +633,7 @@ pub mod pallet { // which is `u32`. if new_deposit > old_deposit && idx < candidate_count { // Since the lowest value for `idx` would be `0`, the first element in the - // list`, and the fact we do an increment of `idx` before iterating, we + // list, and the fact we do an increment of `idx` before iterating, we // ensure the `idx - 1` list access in the loop is not an underflow. idx += 1; while idx < candidate_count && candidates[idx].deposit < new_deposit { From ff46963ed9d9a7e430cc7dda4887145dd143bcda Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 12 Oct 2023 13:17:28 +0200 Subject: [PATCH 12/33] Add test for identical deposit update Signed-off-by: georgepisaltu --- .../pallets/collator-selection/src/tests.rs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index 38968e5a8d6f..fa5bee654ead 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -797,6 +797,46 @@ fn decrease_candidacy_bond_works() { }); } +#[test] +fn update_candidacy_bond_with_identical_amount() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::desired_candidates(), 2); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + + assert_eq!(>::get().iter().count(), 0); + assert_eq!(CollatorSelection::invulnerables(), vec![1, 2]); + + // take three endowed, non-invulnerables accounts. + assert_eq!(Balances::free_balance(3), 100); + assert_eq!(Balances::free_balance(4), 100); + assert_eq!(Balances::free_balance(5), 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(5))); + + assert_eq!(Balances::free_balance(3), 90); + assert_eq!(Balances::free_balance(4), 90); + assert_eq!(Balances::free_balance(5), 90); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 20)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 30)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 40)); + + assert_eq!(>::get().iter().count(), 3); + assert_eq!(Balances::free_balance(3), 80); + assert_eq!(Balances::free_balance(4), 70); + assert_eq!(Balances::free_balance(5), 60); + + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(3), 20), + Error::::IdenticalDeposit + ); + assert_eq!(Balances::free_balance(3), 80); + }); +} + #[test] fn candidate_list_works() { new_test_ext().execute_with(|| { From b0747d572066e9a85d74debe6d3e848e0466b287 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Fri, 27 Oct 2023 20:17:38 +0300 Subject: [PATCH 13/33] Disallow lowering bids for top candidates Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 11 +++ .../pallets/collator-selection/src/tests.rs | 76 ++++++++++++++++++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index dfcc75a4cbed..d9ee4414921d 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -312,6 +312,8 @@ pub mod pallet { TargetIsNotCandidate, /// The updated deposit amount is equal to the amount already reserved. IdenticalDeposit, + /// Cannot lower candidacy bond while occupying a future collator slot in the list. + InvalidUnreserve, } #[pallet::hooks] @@ -597,6 +599,9 @@ pub mod pallet { /// Update the candidacy bond of collator candidate `origin` to a new amount `new_deposit`. /// + /// Setting a `new_deposit` that is lower than the current deposit while `origin` is + /// occupying a top-`DesiredCandidates` slot is not allowed. + /// /// This call will fail if `origin` is not a collator candidate, the updated bond is lower /// than the minimum candidacy bond, and/or the amount cannot be reserved. #[pallet::call_index(7)] @@ -621,6 +626,12 @@ pub mod pallet { if new_deposit > old_deposit { T::Currency::reserve(&who, new_deposit - old_deposit)?; } else if new_deposit < old_deposit { + // Casting `u32` to `usize` should be safe on all machines running this. + ensure!( + idx.saturating_add(>::get() as usize) < + candidate_count, + Error::::InvalidUnreserve + ); T::Currency::unreserve(&who, old_deposit - new_deposit); } else { return Err(Error::::IdenticalDeposit.into()) diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index fa5bee654ead..8a61d570415b 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -755,6 +755,67 @@ fn decrease_candidacy_bond_insufficient_funds() { }); } +#[test] +fn decrease_candidacy_bond_occupying_top_slot() { + new_test_ext().execute_with(|| { + assert_eq!(CollatorSelection::desired_candidates(), 2); + // Register 3 candidates. + 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(5))); + // And update their bids. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 30u64.into())); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 30u64.into())); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 60u64.into())); + + // tuple of (id, deposit). + let candidate_3 = CandidateInfo { who: 3, deposit: 30 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 30 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 60 }; + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![candidate_4, candidate_3, candidate_5] + ); + + // Candidates 5 and 3 can't decrease their deposits because they are the 2 top candidates. + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(5), 29), + Error::::InvalidUnreserve, + ); + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(3), 29), + Error::::InvalidUnreserve, + ); + // But candidate 4 should have be able to decrease the deposit up to the minimum. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 29u64.into())); + + // Make candidate 4 outbid candidate 3, taking their spot as the second highest bid. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 35u64.into())); + + // tuple of (id, deposit). + let candidate_3 = CandidateInfo { who: 3, deposit: 30 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 35 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 60 }; + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![candidate_3, candidate_4, candidate_5] + ); + + // Now candidates 5 and 4 are the 2 top candidates, so they can't decrease their deposits. + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(5), 34), + Error::::InvalidUnreserve, + ); + assert_noop!( + CollatorSelection::update_bond(RuntimeOrigin::signed(4), 34), + Error::::InvalidUnreserve, + ); + // Candidate 3 should have be able to decrease the deposit up to the minimum now that + // they've fallen out of the top spots. + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10u64.into())); + }); +} + #[test] fn decrease_candidacy_bond_works() { new_test_ext().execute_with(|| { @@ -788,11 +849,10 @@ fn decrease_candidacy_bond_works() { assert_eq!(Balances::free_balance(5), 60); assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10)); - assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 20)); assert_eq!(>::get().iter().count(), 3); assert_eq!(Balances::free_balance(3), 90); - assert_eq!(Balances::free_balance(4), 80); + assert_eq!(Balances::free_balance(4), 70); assert_eq!(Balances::free_balance(5), 60); }); } @@ -860,7 +920,17 @@ fn candidate_list_works() { assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 30)); - assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 25)); + + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 10)); + + let candidate_3 = CandidateInfo { who: 3, deposit: 30 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 25 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 10 }; + assert_eq!( + >::get().iter().cloned().collect::>(), + vec![candidate_5, candidate_4, candidate_3] + ); }); } From 967429d0880256f4a9d065886b907d6630ad787a Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 30 Oct 2023 14:56:59 +0200 Subject: [PATCH 14/33] Add weights for asset-hub-rococo Signed-off-by: georgepisaltu --- .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs index d98abbbc2d3d..42ddd548e563 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) From dea9465d9cd5c44b201f2b0650d7963683856262 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 6 Nov 2023 16:36:17 +0200 Subject: [PATCH 15/33] Refactor list pivots into separate functions Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 66 +++++----- .../pallets/collator-selection/src/tests.rs | 122 ++++++++++++++++++ 2 files changed, 156 insertions(+), 32 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index d9ee4414921d..ffc2737478e5 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -617,7 +617,7 @@ pub mod pallet { // position of the entry in the list, used for weight calculation. let length = >::try_mutate(|candidates| -> Result { - let mut idx = candidates + let idx = candidates .iter() .position(|candidate_info| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; @@ -637,25 +637,10 @@ pub mod pallet { return Err(Error::::IdenticalDeposit.into()) } candidates[idx].deposit = new_deposit; - // Since `idx` is at most `T::MaxCandidates - 1`, because it's the position of - // an entry in a list that is at most `T::MaxCandidates` long, it will never be - // an out of bounds index access. Furthermore, the increment will not be an - // overflow since `idx` is `usize` and it can be at most `T::MaxCandidates`, - // which is `u32`. - if new_deposit > old_deposit && idx < candidate_count { - // Since the lowest value for `idx` would be `0`, the first element in the - // list, and the fact we do an increment of `idx` before iterating, we - // ensure the `idx - 1` list access in the loop is not an underflow. - idx += 1; - while idx < candidate_count && candidates[idx].deposit < new_deposit { - candidates.as_mut().swap(idx - 1, idx); - idx += 1; - } + if new_deposit > old_deposit { + Self::pivot_up(candidates.as_mut(), idx); } else { - while idx > 0 && candidates[idx].deposit >= new_deposit { - candidates.as_mut().swap(idx - 1, idx); - idx -= 1; - } + Self::pivot_down(candidates.as_mut(), idx); } Ok(candidate_count) @@ -737,21 +722,10 @@ pub mod pallet { // Replace the old candidate in the list with the new candidate and then move them up // the list to the correct place, if necessary. >::try_mutate(|list| { - let mut idx = target_info_idx; + let idx = target_info_idx; list[idx].who = who.clone(); list[idx].deposit = deposit; - // Since `idx` is at most `T::MaxCandidates - 1`, because it's the position of an - // entry in a list that is at most `T::MaxCandidates` long, it will never be an out - // of bounds index access. Furthermore, the increment will not be an overflow since - // `idx` is `usize` and it can be at most `T::MaxCandidates`, which is `u32`. Since - // the lowest value for `idx` would be `0`, the first element in the list`, and the - // fact we do an increment of `idx` before iterating, we ensure the `idx - 1` list - // access in the loop is not an underflow. - idx += 1; - while idx < list.len() && list[idx].deposit < deposit { - list.as_mut().swap(idx - 1, idx); - idx += 1; - } + Self::pivot_up(list.as_mut(), idx); Ok::<(), Error>(()) })?; @@ -855,6 +829,34 @@ pub mod pallet { .try_into() .expect("filter_map operation can't result in a bounded vec larger than its original; qed") } + + /// Given a sorted list of candidates and a potentially out of place candidate at index + /// `start`, pivot the candidate upwards in the list to the correct spot. + pub(super) fn pivot_up( + list: &mut [CandidateInfo>], + start: usize, + ) { + let mut idx = start.saturating_add(1); + while idx < list.len() && list[idx].deposit < list[idx.saturating_sub(1)].deposit { + list.swap(idx.saturating_sub(1), idx); + idx.saturating_inc(); + } + } + + /// Given a sorted list of candidates and a potentially out of place candidate at index + /// `start`, pivot the candidate downwards in the list to the correct spot. + pub(super) fn pivot_down( + list: &mut [CandidateInfo>], + start: usize, + ) { + if start < list.len() { + let mut idx = start; + while idx > 0 && list[idx].deposit <= list[idx.saturating_sub(1)].deposit { + list.swap(idx.saturating_sub(1), idx); + idx.saturating_dec(); + } + } + } } /// Keep track of number of authored blocks per authority, uncles are counted as well since diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index 8a61d570415b..d8a3e939ddda 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -22,6 +22,128 @@ use frame_support::{ use pallet_balances::Error as BalancesError; use sp_runtime::{testing::UintAuthorityId, traits::BadOrigin, BuildStorage}; +#[test] +fn pivot_up_works() { + let mut list: Vec<_> = (0..10).map(|i| CandidateInfo { who: i, deposit: i * 10 }).collect(); + + // Have 4 bid as much as 8. + list[4].deposit = list[8].deposit; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + // Move 4 up the list. + CollatorSelection::pivot_up(list.as_mut(), 4); + assert_eq!(list, expected); + // It should be behind 8 though since 8 has the same bid but is older. + assert_eq!(list[7].who, 4); + assert_eq!(list[8].who, 8); + + let last_idx = list.len() - 1; + let who = list[5].who; + // Make 5 the top bidder. + list[5].deposit = list.iter().map(|info| info.deposit).max().unwrap() + 100; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + CollatorSelection::pivot_up(list.as_mut(), 5); + assert_eq!(list, expected); + // 5 should now be on the top spot. + assert_eq!(list[last_idx].who, who); + // And the last top bidder should have fallen one place. + assert_eq!(list[last_idx - 1].who as usize, last_idx); + + // Make the top bidder increase their bid. + list[last_idx].deposit = list[last_idx].deposit + 100; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + // Nobody should change places. + CollatorSelection::pivot_up(list.as_mut(), last_idx); + assert_eq!(list, expected); + assert_eq!(list[last_idx].who, who); + assert_eq!(list[last_idx - 1].who as usize, last_idx); + + // Out of bounds call is a noop. + CollatorSelection::pivot_up(list.as_mut(), last_idx + 1); + assert_eq!(list, expected); + + // Have the lowest bidder increase their bid to be the top bid. + list[0].deposit = list[last_idx].deposit + 100; + let who = list[0].who; + let prev_top_bidder = list[last_idx].who; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + CollatorSelection::pivot_up(list.as_mut(), 0); + assert_eq!(list, expected); + // 0 should now be on the top spot. + assert_eq!(list[last_idx].who, who); + // The previous top bidder should have fallen one place. + assert_eq!(list[last_idx - 1].who, prev_top_bidder); +} + +#[test] +fn pivot_down_works() { + let mut list: Vec<_> = + (0..10).map(|i| CandidateInfo { who: i, deposit: (i * 10) + 100 }).collect(); + + // Have 8 bid as much as 4. + list[8].deposit = list[4].deposit; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + // The stable sort puts 8 ahead of 4 since that was their original order in the list, but in our + // expected list we want 8 to be ahead of 4 because 4 has the same bid but has the least recent + // change to their bid. + expected.swap(4, 5); + // Move 8 down the list. + CollatorSelection::pivot_down(list.as_mut(), 8); + assert_eq!(list, expected); + // It should be ahead of 4 though since 4 has the same bid but is older. + assert_eq!(list[4].who, 8); + assert_eq!(list[5].who, 4); + + let who = list[6].who; + // Make 6 the lowest bidder. + list[6].deposit = list.iter().map(|info| info.deposit).min().unwrap() - 10; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + // Move 6 down the list. + CollatorSelection::pivot_down(list.as_mut(), 6); + assert_eq!(list, expected); + // 6 should now be on the lowest spot. + assert_eq!(list[0].who, who); + // And the last lowest bidder should have gained one place. + assert_eq!(list[1].who, 0); + + // Make the lowest bidder decrease their bid. + list[0].deposit = list[0].deposit - 10; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + // Nobody should change places. + CollatorSelection::pivot_down(list.as_mut(), 0); + assert_eq!(list, expected); + assert_eq!(list[0].who, who); + assert_eq!(list[1].who, 0); + + // Should be a noop as there were no bid changes. + CollatorSelection::pivot_down(list.as_mut(), 0); + assert_eq!(list, expected); + + let last_idx = list.len() - 1; + // Have the top bidder decrease their bid to be the lowest bid. + list[last_idx].deposit = list[0].deposit - 10; + let who = list[last_idx].who; + let prev_lowest_bidder = list[0].who; + let mut expected = list.clone(); + expected.sort_by_key(|info| info.deposit); + CollatorSelection::pivot_down(list.as_mut(), last_idx); + assert_eq!(list, expected); + // 9 should now be on the lowest spot. + assert_eq!(list[0].who, who); + // The previous lowest bidder should have gained one place. + assert_eq!(list[1].who, prev_lowest_bidder); + + // Out of bounds call should be a noop. + CollatorSelection::pivot_down(list.as_mut(), last_idx + 1); + assert_eq!(list, expected); +} + #[test] fn basic_setup_works() { new_test_ext().execute_with(|| { From fd146da6f8ee2789bdd8f667d7f08ae3a6a68ea0 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 6 Nov 2023 16:41:15 +0200 Subject: [PATCH 16/33] Remove unnecessary integrity test assertions Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index ffc2737478e5..8de67c8c575b 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -318,16 +318,12 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { - #[allow(clippy::absurd_extreme_comparisons)] fn integrity_test() { assert!(T::MinEligibleCollators::get() > 0, "chain must require at least one collator"); assert!( - T::MaxCandidates::get() <= u32::MAX, - "the extrinsics below rely on `MaxCandidates` fitting in a `u32`" - ); - assert!( - T::MinEligibleCollators::get() <= u32::MAX, - "the extrinsics below rely `MinEligibleCollators` fitting in a `u32`" + T::MaxInvulnerables::get().saturating_add(T::MaxCandidates::get()) >= + T::MinEligibleCollators::get(), + "invulnerables and candidates must be able to satisfy collator demand" ); } } From 0fa7fcef42cb3ab53738adc88ec7afe8ba380251 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 6 Nov 2023 16:58:09 +0200 Subject: [PATCH 17/33] Add `try-state` checks Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 8de67c8c575b..bfd2afead020 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -326,6 +326,11 @@ pub mod pallet { "invulnerables and candidates must be able to satisfy collator demand" ); } + + #[cfg(feature = "try-runtime")] + fn try_state(_: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + Self::do_try_state() + } } #[pallet::call] @@ -853,6 +858,35 @@ pub mod pallet { } } } + + /// Ensure the correctness of the state of this pallet. + /// + /// This should be valid before or after each state transition of this pallet. + /// + /// # Invariants + /// + /// ## `DesiredCandidates` + /// + /// * The current desired candidate count should not exceed the candidate list capacity. + /// * The number of selected candidates together with the invulnerables must be greater than + /// or equal to the minimum number of eligible collators. + #[cfg(any(test, feature = "try-runtime"))] + pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { + let desired_candidates = >::get(); + + frame_support::ensure!( + desired_candidates <= T::MaxCandidates::get(), + "Shouldn't demand more candidates than the pallet config allows." + ); + + frame_support::ensure!( + desired_candidates.saturating_add(T::MaxInvulnerables::get()) >= + T::MinEligibleCollators::get(), + "Invulnerable set together with desired candidates should be able to meet the collator quota." + ); + + Ok(()) + } } /// Keep track of number of authored blocks per authority, uncles are counted as well since From 76c02a12b43b54281250f4d73722116cbd9f37a6 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 6 Nov 2023 17:00:49 +0200 Subject: [PATCH 18/33] Add ordering doc comment to candidate list Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index bfd2afead020..4dd4d90aade4 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -188,6 +188,9 @@ pub mod pallet { /// The (community, limited) collation candidates. `Candidates` and `Invulnerables` should be /// mutually exclusive. + /// + /// This list is sorted in ascending order by deposit and when the deposits are equal, the least + /// recently updated is considered greater. #[pallet::storage] #[pallet::getter(fn candidate_list)] pub type CandidateList = StorageValue< From 125435104645b18ee5fbdee4a63385bc3939cbb6 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 6 Nov 2023 18:36:25 +0200 Subject: [PATCH 19/33] Add weights for bridge hub westend Signed-off-by: georgepisaltu --- .../src/weights/pallet_collator_selection.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs index 9cbfa6ce80e3..3fff5980c68c 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs @@ -178,6 +178,30 @@ impl pallet_collator_selection::WeightInfo for WeightIn .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } + fn update_bond(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } + fn take_candidate_slot(c: u32, ) -> Weight { + // Proof Size summary in bytes: + // Measured: `306 + c * (50 ±0)` + // Estimated: `6287` + // Minimum execution time: 34_814_000 picoseconds. + Weight::from_parts(36_371_520, 0) + .saturating_add(Weight::from_parts(0, 6287)) + // Standard Error: 2_391 + .saturating_add(Weight::from_parts(201_700, 0).saturating_mul(c.into())) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } /// Storage: `System::Account` (r:2 w:2) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) /// Storage: `System::BlockWeight` (r:1 w:1) From 8ac84daa2ce48378e6a284c1146627f72d9aab0b Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 14:30:33 +0200 Subject: [PATCH 20/33] WIP set desired candidates Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 4dd4d90aade4..c086028b7a06 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -99,7 +99,7 @@ pub mod pallet { use pallet_session::SessionManager; use sp_runtime::{ traits::{AccountIdConversion, CheckedSub, Convert, Saturating, Zero}, - RuntimeDebug, + DispatchError, RuntimeDebug, }; use sp_staking::SessionIndex; use sp_std::vec::Vec; @@ -449,7 +449,26 @@ pub mod pallet { bond: BalanceOf, ) -> DispatchResultWithPostInfo { T::UpdateOrigin::ensure_origin(origin)?; - >::put(bond); + let bond_increased = >::mutate(|old_bond| -> bool { + let bond_increased = *old_bond < bond; + *old_bond = bond; + bond_increased + }); + let initial_len = >::decode_len().unwrap_or_default(); + if bond_increased && initial_len > 0 { + >::try_mutate(|candidates| -> Result<(), DispatchError> { + let starting_kick_idx = candidates + .iter() + .position(|candidate| candidate.deposit >= bond) + .unwrap_or(initial_len); + let kicked = candidates.drain(..starting_kick_idx); + for candidate in kicked { + T::Currency::unreserve(&candidate.who, candidate.deposit); + >::remove(candidate.who); + } + Ok(()) + })?; + } Self::deposit_event(Event::NewCandidacyBond { bond_amount: bond }); Ok(().into()) } From 2ce5275f3bc56ec5d7c0065e7b6d2e202f64aa4e Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 15:31:43 +0200 Subject: [PATCH 21/33] WIP set candidacy bond v2 Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 11 +++++++-- cumulus/pallets/collator-selection/src/lib.rs | 24 +++++++++++-------- .../pallets/collator-selection/src/weights.rs | 6 ++--- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 5ead458e4473..4669d0ed6605 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -223,10 +223,17 @@ mod benchmarks { } #[benchmark] - fn set_candidacy_bond() -> Result<(), BenchmarkError> { - let bond_amount: BalanceOf = T::Currency::minimum_balance() * 10u32.into(); + fn set_candidacy_bond(k: Linear<0, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + let initial_bond_amount: BalanceOf = T::Currency::minimum_balance() * 10u32.into(); + >::put(initial_bond_amount); let origin = T::UpdateOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + let bond_amount = if k > 0 { + register_candidates::(k); + T::Currency::minimum_balance() * 20u32.into() + } else { + T::Currency::minimum_balance() + }; #[extrinsic_call] _(origin as T::RuntimeOrigin, bond_amount); diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index c086028b7a06..579afbf1f56b 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -99,7 +99,7 @@ pub mod pallet { use pallet_session::SessionManager; use sp_runtime::{ traits::{AccountIdConversion, CheckedSub, Convert, Saturating, Zero}, - DispatchError, RuntimeDebug, + RuntimeDebug, }; use sp_staking::SessionIndex; use sp_std::vec::Vec; @@ -443,7 +443,7 @@ pub mod pallet { /// /// The origin for this call must be the `UpdateOrigin`. #[pallet::call_index(2)] - #[pallet::weight(T::WeightInfo::set_candidacy_bond())] + #[pallet::weight(T::WeightInfo::set_candidacy_bond(T::MaxCandidates::get()))] pub fn set_candidacy_bond( origin: OriginFor, bond: BalanceOf, @@ -455,22 +455,24 @@ pub mod pallet { bond_increased }); let initial_len = >::decode_len().unwrap_or_default(); - if bond_increased && initial_len > 0 { - >::try_mutate(|candidates| -> Result<(), DispatchError> { - let starting_kick_idx = candidates + let kicked = if bond_increased && initial_len > 0 { + >::mutate(|candidates| -> usize { + let first_safe_candidate = candidates .iter() .position(|candidate| candidate.deposit >= bond) .unwrap_or(initial_len); - let kicked = candidates.drain(..starting_kick_idx); + let kicked = candidates.drain(..first_safe_candidate); for candidate in kicked { T::Currency::unreserve(&candidate.who, candidate.deposit); >::remove(candidate.who); } - Ok(()) - })?; - } + first_safe_candidate + }) + } else { + 0 + }; Self::deposit_event(Event::NewCandidacyBond { bond_amount: bond }); - Ok(().into()) + Ok(Some(T::WeightInfo::set_candidacy_bond(kicked as u32)).into()) } /// Register this account as a collator candidate. The account must (a) already have @@ -881,6 +883,8 @@ pub mod pallet { } } + // fn kick_candidates() -> usize {} + /// Ensure the correctness of the state of this pallet. /// /// This should be valid before or after each state transition of this pallet. diff --git a/cumulus/pallets/collator-selection/src/weights.rs b/cumulus/pallets/collator-selection/src/weights.rs index 0e08eda0dbb3..97a3c0e5af0c 100644 --- a/cumulus/pallets/collator-selection/src/weights.rs +++ b/cumulus/pallets/collator-selection/src/weights.rs @@ -30,7 +30,7 @@ pub trait WeightInfo { fn add_invulnerable(_b: u32, _c: u32) -> Weight; fn remove_invulnerable(_b: u32) -> Weight; fn set_desired_candidates() -> Weight; - fn set_candidacy_bond() -> Weight; + fn set_candidacy_bond(_k: u32) -> Weight; fn register_as_candidate(_c: u32) -> Weight; fn leave_intent(_c: u32) -> Weight; fn update_bond(_c: u32) -> Weight; @@ -51,7 +51,7 @@ impl WeightInfo for SubstrateWeight { fn set_desired_candidates() -> Weight { Weight::from_parts(16_363_000_u64, 0).saturating_add(T::DbWeight::get().writes(1_u64)) } - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { Weight::from_parts(16_840_000_u64, 0).saturating_add(T::DbWeight::get().writes(1_u64)) } fn register_as_candidate(c: u32) -> Weight { @@ -152,7 +152,7 @@ impl WeightInfo for () { fn set_desired_candidates() -> Weight { Weight::from_parts(16_363_000_u64, 0).saturating_add(RocksDbWeight::get().writes(1_u64)) } - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { Weight::from_parts(16_840_000_u64, 0).saturating_add(RocksDbWeight::get().writes(1_u64)) } fn register_as_candidate(c: u32) -> Weight { From 49c8fe568d7698b8aaed1c12976a80c3e8e5393b Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 16:12:06 +0200 Subject: [PATCH 22/33] Add tests to new `set_candidacy_bond` Signed-off-by: georgepisaltu --- .../pallets/collator-selection/src/tests.rs | 191 +++++++++++++++++- 1 file changed, 189 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index d8a3e939ddda..007c6cb0ba21 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -390,20 +390,207 @@ fn set_desired_candidates_works() { } #[test] -fn set_candidacy_bond() { +fn set_candidacy_bond_empty_candidate_list() { new_test_ext().execute_with(|| { // given assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert!(>::get().is_empty()); - // can set + // can decrease without candidates assert_ok!(CollatorSelection::set_candidacy_bond( RuntimeOrigin::signed(RootAccount::get()), 7 )); assert_eq!(CollatorSelection::candidacy_bond(), 7); + assert!(>::get().is_empty()); // rejects bad origin. assert_noop!(CollatorSelection::set_candidacy_bond(RuntimeOrigin::signed(1), 8), BadOrigin); + + // can increase without candidates + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 20 + )); + assert!(>::get().is_empty()); + assert_eq!(CollatorSelection::candidacy_bond(), 20); + }); +} + +#[test] +fn set_candidacy_bond_with_one_candidate() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert!(>::get().is_empty()); + + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_eq!(>::get(), vec![candidate_3.clone()]); + + // can decrease with one candidate + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 7 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 7); + assert_eq!(>::get(), vec![candidate_3.clone()]); + + // can increase up to initial deposit + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 10 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!(>::get(), vec![candidate_3.clone()]); + + // can increase past initial deposit, should kick existing candidate + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 20 + )); + assert!(>::get().is_empty()); + }); +} + +#[test] +fn set_candidacy_bond_with_many_candidates_same_deposit() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert!(>::get().is_empty()); + + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 10 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 10 }; + + 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(5))); + assert_eq!( + >::get(), + vec![candidate_5.clone(), candidate_4.clone(), candidate_3.clone()] + ); + + // can decrease with multiple candidates + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 7 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 7); + assert_eq!( + >::get(), + vec![candidate_5.clone(), candidate_4.clone(), candidate_3.clone()] + ); + + // can increase up to initial deposit + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 10 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!( + >::get(), + vec![candidate_5.clone(), candidate_4.clone(), candidate_3.clone()] + ); + + // can increase past initial deposit, should kick existing candidates + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 20 + )); + assert!(>::get().is_empty()); + }); +} + +#[test] +fn set_candidacy_bond_with_many_candidates_different_deposits() { + new_test_ext().execute_with(|| { + // given + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert!(>::get().is_empty()); + + let candidate_3 = CandidateInfo { who: 3, deposit: 10 }; + let candidate_4 = CandidateInfo { who: 4, deposit: 20 }; + let candidate_5 = CandidateInfo { who: 5, deposit: 30 }; + + 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(5))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(5), 30)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 20)); + assert_eq!( + >::get(), + vec![candidate_3.clone(), candidate_4.clone(), candidate_5.clone()] + ); + + // can decrease with multiple candidates + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 7 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 7); + assert_eq!( + >::get(), + vec![candidate_3.clone(), candidate_4.clone(), candidate_5.clone()] + ); + // can increase up to initial deposit + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 10 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 10); + assert_eq!( + >::get(), + vec![candidate_3.clone(), candidate_4.clone(), candidate_5.clone()] + ); + + // can increase to 4's deposit, should kick 3 + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 20 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 20); + assert_eq!( + >::get(), + vec![candidate_4.clone(), candidate_5.clone()] + ); + + // can increase past 4's deposit, should kick 4 + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 25 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 25); + assert_eq!(>::get(), vec![candidate_5.clone()]); + + // lowering the minimum deposit should have no effect + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 5 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 5); + assert_eq!(>::get(), vec![candidate_5.clone()]); + + // add 3 and 4 back but with higher deposits than minimum + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); + assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 10)); + assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(4), 20)); + assert_eq!( + >::get(), + vec![candidate_3.clone(), candidate_4.clone(), candidate_5.clone()] + ); + + // can increase the deposit above the current max in the list, all candidates should be + // kicked + assert_ok!(CollatorSelection::set_candidacy_bond( + RuntimeOrigin::signed(RootAccount::get()), + 40 + )); + assert_eq!(CollatorSelection::candidacy_bond(), 40); + assert!(>::get().is_empty()); }); } From 27788ac9da32ab58bff0bd26acaabfdd5b00409b Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 16:19:51 +0200 Subject: [PATCH 23/33] Small refactor in `set_candidacy_bond` Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 579afbf1f56b..5408314da213 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -455,22 +455,22 @@ pub mod pallet { bond_increased }); let initial_len = >::decode_len().unwrap_or_default(); - let kicked = if bond_increased && initial_len > 0 { - >::mutate(|candidates| -> usize { - let first_safe_candidate = candidates - .iter() - .position(|candidate| candidate.deposit >= bond) - .unwrap_or(initial_len); - let kicked = candidates.drain(..first_safe_candidate); - for candidate in kicked { - T::Currency::unreserve(&candidate.who, candidate.deposit); - >::remove(candidate.who); - } - first_safe_candidate + let kicked = (bond_increased && initial_len > 0) + .then(|| { + >::mutate(|candidates| -> usize { + let first_safe_candidate = candidates + .iter() + .position(|candidate| candidate.deposit >= bond) + .unwrap_or(initial_len); + let kicked_candidates = candidates.drain(..first_safe_candidate); + for candidate in kicked_candidates { + T::Currency::unreserve(&candidate.who, candidate.deposit); + >::remove(candidate.who); + } + first_safe_candidate + }) }) - } else { - 0 - }; + .unwrap_or_default(); Self::deposit_event(Event::NewCandidacyBond { bond_amount: bond }); Ok(Some(T::WeightInfo::set_candidacy_bond(kicked as u32)).into()) } From 43d82516e92f34a034e7f823ea58655e90f9a67b Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 16:27:50 +0200 Subject: [PATCH 24/33] Update `set_candidacy_bond` documentation Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 5408314da213..32ca890c8a91 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -441,6 +441,10 @@ pub mod pallet { /// Set the candidacy bond amount. /// + /// If the candidacy bond is increased by this call, all current candidates which have a + /// deposit lower than the new bond will be kicked from the list and get their deposits + /// back. + /// /// The origin for this call must be the `UpdateOrigin`. #[pallet::call_index(2)] #[pallet::weight(T::WeightInfo::set_candidacy_bond(T::MaxCandidates::get()))] @@ -457,6 +461,8 @@ pub mod pallet { let initial_len = >::decode_len().unwrap_or_default(); let kicked = (bond_increased && initial_len > 0) .then(|| { + // Closure below returns the number of candidates which were kicked because + // their deposits were lower than the new candidacy bond. >::mutate(|candidates| -> usize { let first_safe_candidate = candidates .iter() From 0ecc4807528b03911cefeecdc6b8ace3ca1d4eeb Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 16:29:20 +0200 Subject: [PATCH 25/33] Fixup kick candidates comment Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 32ca890c8a91..6fc977de23af 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -889,8 +889,6 @@ pub mod pallet { } } - // fn kick_candidates() -> usize {} - /// Ensure the correctness of the state of this pallet. /// /// This should be valid before or after each state transition of this pallet. From bb686f9ed69cc13f7b29f4077d62b017a364a5ee Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 18:01:53 +0200 Subject: [PATCH 26/33] Refactor out pivot functions Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 124 +++++++--------- .../pallets/collator-selection/src/tests.rs | 138 ++---------------- 2 files changed, 71 insertions(+), 191 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 6fc977de23af..3b9c5cda074d 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -653,7 +653,9 @@ pub mod pallet { .position(|candidate_info| candidate_info.who == who) .ok_or_else(|| Error::::NotCandidate)?; let candidate_count = candidates.len(); - let old_deposit = candidates[idx].deposit; + // Remove the candidate from the list. + let mut info = candidates.remove(idx); + let old_deposit = info.deposit; if new_deposit > old_deposit { T::Currency::reserve(&who, new_deposit - old_deposit)?; } else if new_deposit < old_deposit { @@ -667,12 +669,16 @@ pub mod pallet { } else { return Err(Error::::IdenticalDeposit.into()) } - candidates[idx].deposit = new_deposit; - if new_deposit > old_deposit { - Self::pivot_up(candidates.as_mut(), idx); - } else { - Self::pivot_down(candidates.as_mut(), idx); - } + + // Update the deposit and insert the candidate in the correct spot in the list. + info.deposit = new_deposit; + let new_pos = candidates + .iter() + .position(|candidate| candidate.deposit >= new_deposit) + .unwrap_or_else(|| candidates.len()); + candidates + .try_insert(new_pos, info) + .expect("candidate count previously decremented; qed"); Ok(candidate_count) })?; @@ -717,32 +723,49 @@ pub mod pallet { // the caller isn't already a candidate and to find the target it's trying to replace in // the list. The return value is a tuple of the position of the candidate to be replaced // in the list along with its candidate information. - let (target_info_idx, target_info) = - >::try_mutate( - |candidates| -> Result< - (usize, CandidateInfo>), - DispatchError, - > { - // Find the position in the list of the candidate that is being replaced. - let mut target_info_idx = None; - for (idx, candidate_info) in candidates.iter().enumerate() { - // While iterating through the candidates trying to find the target, - // also ensure on the same pass that our caller isn't already a - // candidate. - ensure!(candidate_info.who != who, Error::::AlreadyCandidate); - // If we find our target, update the position but do not stop the - // iteration since we're also checking that the caller isn't already a - // candidate. - if candidate_info.who == target { - target_info_idx = Some(idx); - } + let target_info = >::try_mutate( + |candidates| -> Result>, DispatchError> { + // Find the position in the list of the candidate that is being replaced. + let mut target_info_idx = None; + let mut new_info_idx = None; + for (idx, candidate_info) in candidates.iter().enumerate() { + // While iterating through the candidates trying to find the target, + // also ensure on the same pass that our caller isn't already a + // candidate. + ensure!(candidate_info.who != who, Error::::AlreadyCandidate); + // If we find our target, update the position but do not stop the + // iteration since we're also checking that the caller isn't already a + // candidate. + if candidate_info.who == target { + target_info_idx = Some(idx); } - let target_info_idx = - target_info_idx.ok_or(Error::::TargetIsNotCandidate)?; - Ok((target_info_idx, candidates[target_info_idx].clone())) - }, - )?; - ensure!(deposit > target_info.deposit, Error::::InsufficientBond); + // Find the spot where the new candidate would be inserted in the current + // version of the list. + if new_info_idx.is_none() && candidate_info.deposit >= deposit { + new_info_idx = Some(idx); + } + } + let target_info_idx = + target_info_idx.ok_or(Error::::TargetIsNotCandidate)?; + + // Remove the old candidate from the list. + let target_info = candidates.remove(target_info_idx); + ensure!(deposit > target_info.deposit, Error::::InsufficientBond); + + // We have removed one element before `new_info_idx`, so the position we have to + // insert to is reduced by 1. + let new_pos = new_info_idx + .map(|i| i.saturating_sub(1)) + .unwrap_or_else(|| candidates.len()); + let new_info = CandidateInfo { who: who.clone(), deposit }; + // Insert the new candidate in the correct spot in the list. + candidates + .try_insert(new_pos, new_info) + .expect("candidate count previously decremented; qed"); + + Ok(target_info) + }, + )?; T::Currency::reserve(&who, deposit)?; T::Currency::unreserve(&target_info.who, target_info.deposit); >::remove(target_info.who.clone()); @@ -750,15 +773,6 @@ pub mod pallet { who.clone(), frame_system::Pallet::::block_number() + T::KickThreshold::get(), ); - // Replace the old candidate in the list with the new candidate and then move them up - // the list to the correct place, if necessary. - >::try_mutate(|list| { - let idx = target_info_idx; - list[idx].who = who.clone(); - list[idx].deposit = deposit; - Self::pivot_up(list.as_mut(), idx); - Ok::<(), Error>(()) - })?; Self::deposit_event(Event::CandidateReplaced { old: target, new: who, deposit }); Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into()) @@ -861,34 +875,6 @@ pub mod pallet { .expect("filter_map operation can't result in a bounded vec larger than its original; qed") } - /// Given a sorted list of candidates and a potentially out of place candidate at index - /// `start`, pivot the candidate upwards in the list to the correct spot. - pub(super) fn pivot_up( - list: &mut [CandidateInfo>], - start: usize, - ) { - let mut idx = start.saturating_add(1); - while idx < list.len() && list[idx].deposit < list[idx.saturating_sub(1)].deposit { - list.swap(idx.saturating_sub(1), idx); - idx.saturating_inc(); - } - } - - /// Given a sorted list of candidates and a potentially out of place candidate at index - /// `start`, pivot the candidate downwards in the list to the correct spot. - pub(super) fn pivot_down( - list: &mut [CandidateInfo>], - start: usize, - ) { - if start < list.len() { - let mut idx = start; - while idx > 0 && list[idx].deposit <= list[idx.saturating_sub(1)].deposit { - list.swap(idx.saturating_sub(1), idx); - idx.saturating_dec(); - } - } - } - /// Ensure the correctness of the state of this pallet. /// /// This should be valid before or after each state transition of this pallet. diff --git a/cumulus/pallets/collator-selection/src/tests.rs b/cumulus/pallets/collator-selection/src/tests.rs index 007c6cb0ba21..ed2044ccdfad 100644 --- a/cumulus/pallets/collator-selection/src/tests.rs +++ b/cumulus/pallets/collator-selection/src/tests.rs @@ -22,128 +22,6 @@ use frame_support::{ use pallet_balances::Error as BalancesError; use sp_runtime::{testing::UintAuthorityId, traits::BadOrigin, BuildStorage}; -#[test] -fn pivot_up_works() { - let mut list: Vec<_> = (0..10).map(|i| CandidateInfo { who: i, deposit: i * 10 }).collect(); - - // Have 4 bid as much as 8. - list[4].deposit = list[8].deposit; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - // Move 4 up the list. - CollatorSelection::pivot_up(list.as_mut(), 4); - assert_eq!(list, expected); - // It should be behind 8 though since 8 has the same bid but is older. - assert_eq!(list[7].who, 4); - assert_eq!(list[8].who, 8); - - let last_idx = list.len() - 1; - let who = list[5].who; - // Make 5 the top bidder. - list[5].deposit = list.iter().map(|info| info.deposit).max().unwrap() + 100; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - CollatorSelection::pivot_up(list.as_mut(), 5); - assert_eq!(list, expected); - // 5 should now be on the top spot. - assert_eq!(list[last_idx].who, who); - // And the last top bidder should have fallen one place. - assert_eq!(list[last_idx - 1].who as usize, last_idx); - - // Make the top bidder increase their bid. - list[last_idx].deposit = list[last_idx].deposit + 100; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - // Nobody should change places. - CollatorSelection::pivot_up(list.as_mut(), last_idx); - assert_eq!(list, expected); - assert_eq!(list[last_idx].who, who); - assert_eq!(list[last_idx - 1].who as usize, last_idx); - - // Out of bounds call is a noop. - CollatorSelection::pivot_up(list.as_mut(), last_idx + 1); - assert_eq!(list, expected); - - // Have the lowest bidder increase their bid to be the top bid. - list[0].deposit = list[last_idx].deposit + 100; - let who = list[0].who; - let prev_top_bidder = list[last_idx].who; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - CollatorSelection::pivot_up(list.as_mut(), 0); - assert_eq!(list, expected); - // 0 should now be on the top spot. - assert_eq!(list[last_idx].who, who); - // The previous top bidder should have fallen one place. - assert_eq!(list[last_idx - 1].who, prev_top_bidder); -} - -#[test] -fn pivot_down_works() { - let mut list: Vec<_> = - (0..10).map(|i| CandidateInfo { who: i, deposit: (i * 10) + 100 }).collect(); - - // Have 8 bid as much as 4. - list[8].deposit = list[4].deposit; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - // The stable sort puts 8 ahead of 4 since that was their original order in the list, but in our - // expected list we want 8 to be ahead of 4 because 4 has the same bid but has the least recent - // change to their bid. - expected.swap(4, 5); - // Move 8 down the list. - CollatorSelection::pivot_down(list.as_mut(), 8); - assert_eq!(list, expected); - // It should be ahead of 4 though since 4 has the same bid but is older. - assert_eq!(list[4].who, 8); - assert_eq!(list[5].who, 4); - - let who = list[6].who; - // Make 6 the lowest bidder. - list[6].deposit = list.iter().map(|info| info.deposit).min().unwrap() - 10; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - // Move 6 down the list. - CollatorSelection::pivot_down(list.as_mut(), 6); - assert_eq!(list, expected); - // 6 should now be on the lowest spot. - assert_eq!(list[0].who, who); - // And the last lowest bidder should have gained one place. - assert_eq!(list[1].who, 0); - - // Make the lowest bidder decrease their bid. - list[0].deposit = list[0].deposit - 10; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - // Nobody should change places. - CollatorSelection::pivot_down(list.as_mut(), 0); - assert_eq!(list, expected); - assert_eq!(list[0].who, who); - assert_eq!(list[1].who, 0); - - // Should be a noop as there were no bid changes. - CollatorSelection::pivot_down(list.as_mut(), 0); - assert_eq!(list, expected); - - let last_idx = list.len() - 1; - // Have the top bidder decrease their bid to be the lowest bid. - list[last_idx].deposit = list[0].deposit - 10; - let who = list[last_idx].who; - let prev_lowest_bidder = list[0].who; - let mut expected = list.clone(); - expected.sort_by_key(|info| info.deposit); - CollatorSelection::pivot_down(list.as_mut(), last_idx); - assert_eq!(list, expected); - // 9 should now be on the lowest spot. - assert_eq!(list[0].who, who); - // The previous lowest bidder should have gained one place. - assert_eq!(list[1].who, prev_lowest_bidder); - - // Out of bounds call should be a noop. - CollatorSelection::pivot_down(list.as_mut(), last_idx + 1); - assert_eq!(list, expected); -} - #[test] fn basic_setup_works() { new_test_ext().execute_with(|| { @@ -835,10 +713,14 @@ fn cannot_take_candidate_slot_if_insufficient_deposit() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 60u64.into())); + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 100); assert_noop!( CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 5u64.into(), 3), Error::::InsufficientBond, ); + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 100); }); } @@ -847,10 +729,14 @@ fn cannot_take_candidate_slot_if_deposit_less_than_target() { new_test_ext().execute_with(|| { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::update_bond(RuntimeOrigin::signed(3), 60u64.into())); + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 100); assert_noop!( CollatorSelection::take_candidate_slot(RuntimeOrigin::signed(4), 20u64.into(), 3), Error::::InsufficientBond, ); + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 100); }); } @@ -955,6 +841,8 @@ fn increase_candidacy_bond_insufficient_balance() { CollatorSelection::update_bond(RuntimeOrigin::signed(3), 110), BalancesError::::InsufficientBalance ); + + assert_eq!(Balances::free_balance(3), 90); }); } @@ -1013,10 +901,12 @@ fn decrease_candidacy_bond_non_candidate_account() { assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(3))); assert_ok!(CollatorSelection::register_as_candidate(RuntimeOrigin::signed(4))); + assert_eq!(Balances::free_balance(5), 100); assert_noop!( CollatorSelection::update_bond(RuntimeOrigin::signed(5), 10), Error::::NotCandidate ); + assert_eq!(Balances::free_balance(5), 100); }); } @@ -1061,6 +951,10 @@ fn decrease_candidacy_bond_insufficient_funds() { CollatorSelection::update_bond(RuntimeOrigin::signed(5), 9), Error::::DepositTooLow ); + + assert_eq!(Balances::free_balance(3), 40); + assert_eq!(Balances::free_balance(4), 40); + assert_eq!(Balances::free_balance(5), 40); }); } From 58653afd7d0129c313d7d213ad5528fb3789a28b Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 18:31:39 +0200 Subject: [PATCH 27/33] Fix weights for new `set_candidacy_bond` Signed-off-by: georgepisaltu --- .../asset-hub-kusama/src/weights/pallet_collator_selection.rs | 2 +- .../asset-hub-polkadot/src/weights/pallet_collator_selection.rs | 2 +- .../asset-hub-rococo/src/weights/pallet_collator_selection.rs | 2 +- .../asset-hub-westend/src/weights/pallet_collator_selection.rs | 2 +- .../bridge-hub-kusama/src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../bridge-hub-rococo/src/weights/pallet_collator_selection.rs | 2 +- .../bridge-hub-westend/src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs index 4f612946427f..3162ee7d5a3b 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs index f966ca118098..078c867bcadf 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -121,7 +121,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs index 42ddd548e563..249fb6ae8b03 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs @@ -124,7 +124,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs index 0552a4c0f31d..5bd646884bd5 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs index ad0bba15cb52..fba1e28412b7 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs index 74ec646dc6f0..5e17dd706655 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs index 0c52d69c4e61..0a1f1c03de70 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs index 3fff5980c68c..f03795b8e087 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs @@ -124,7 +124,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs index f683b553dfcc..66e34d79f5cb 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs @@ -121,7 +121,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond() -> Weight { + fn set_candidacy_bond(_k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` From 1a54df2cbd4ec4af715a5711bbef00b1dcab0978 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Thu, 9 Nov 2023 20:46:30 +0200 Subject: [PATCH 28/33] Make `set_candidacy_bond` bench work Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/benchmarking.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 4669d0ed6605..9bf3a708a4e4 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -224,13 +224,14 @@ mod benchmarks { #[benchmark] fn set_candidacy_bond(k: Linear<0, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { - let initial_bond_amount: BalanceOf = T::Currency::minimum_balance() * 10u32.into(); + let initial_bond_amount: BalanceOf = T::Currency::minimum_balance() * 2u32.into(); >::put(initial_bond_amount); + register_validators::(k); + register_candidates::(k); let origin = T::UpdateOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let bond_amount = if k > 0 { - register_candidates::(k); - T::Currency::minimum_balance() * 20u32.into() + T::Currency::minimum_balance() * 3u32.into() } else { T::Currency::minimum_balance() }; From 4a7bebac94f5179f2cd5e3f2468d449108c7c5c3 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Fri, 10 Nov 2023 00:21:53 +0200 Subject: [PATCH 29/33] Refine `set_candidacy_bond` benchmark Signed-off-by: georgepisaltu --- .../collator-selection/src/benchmarking.rs | 15 ++++++++++++--- cumulus/pallets/collator-selection/src/lib.rs | 7 +++++-- cumulus/pallets/collator-selection/src/weights.rs | 6 +++--- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- .../src/weights/pallet_collator_selection.rs | 2 +- 12 files changed, 29 insertions(+), 17 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 9bf3a708a4e4..7443048d8e0f 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -223,14 +223,23 @@ mod benchmarks { } #[benchmark] - fn set_candidacy_bond(k: Linear<0, { T::MaxCandidates::get() }>) -> Result<(), BenchmarkError> { + fn set_candidacy_bond( + c: Linear<0, { T::MaxCandidates::get() }>, + k: Linear<0, { T::MaxCandidates::get() }>, + ) -> Result<(), BenchmarkError> { let initial_bond_amount: BalanceOf = T::Currency::minimum_balance() * 2u32.into(); >::put(initial_bond_amount); - register_validators::(k); - register_candidates::(k); + register_validators::(c); + register_candidates::(c); + let kicked = std::cmp::min(k, c); let origin = T::UpdateOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let bond_amount = if k > 0 { + >::mutate(|candidates| { + for info in candidates.iter_mut().skip(kicked as usize) { + info.deposit = T::Currency::minimum_balance() * 3u32.into(); + } + }); T::Currency::minimum_balance() * 3u32.into() } else { T::Currency::minimum_balance() diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 3b9c5cda074d..a0e8128a3ff6 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -447,7 +447,10 @@ pub mod pallet { /// /// The origin for this call must be the `UpdateOrigin`. #[pallet::call_index(2)] - #[pallet::weight(T::WeightInfo::set_candidacy_bond(T::MaxCandidates::get()))] + #[pallet::weight(T::WeightInfo::set_candidacy_bond( + T::MaxCandidates::get(), + T::MaxCandidates::get() + ))] pub fn set_candidacy_bond( origin: OriginFor, bond: BalanceOf, @@ -478,7 +481,7 @@ pub mod pallet { }) .unwrap_or_default(); Self::deposit_event(Event::NewCandidacyBond { bond_amount: bond }); - Ok(Some(T::WeightInfo::set_candidacy_bond(kicked as u32)).into()) + Ok(Some(T::WeightInfo::set_candidacy_bond(initial_len as u32, kicked as u32)).into()) } /// Register this account as a collator candidate. The account must (a) already have diff --git a/cumulus/pallets/collator-selection/src/weights.rs b/cumulus/pallets/collator-selection/src/weights.rs index 97a3c0e5af0c..1c01ad6cd6fe 100644 --- a/cumulus/pallets/collator-selection/src/weights.rs +++ b/cumulus/pallets/collator-selection/src/weights.rs @@ -30,7 +30,7 @@ pub trait WeightInfo { fn add_invulnerable(_b: u32, _c: u32) -> Weight; fn remove_invulnerable(_b: u32) -> Weight; fn set_desired_candidates() -> Weight; - fn set_candidacy_bond(_k: u32) -> Weight; + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight; fn register_as_candidate(_c: u32) -> Weight; fn leave_intent(_c: u32) -> Weight; fn update_bond(_c: u32) -> Weight; @@ -51,7 +51,7 @@ impl WeightInfo for SubstrateWeight { fn set_desired_candidates() -> Weight { Weight::from_parts(16_363_000_u64, 0).saturating_add(T::DbWeight::get().writes(1_u64)) } - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { Weight::from_parts(16_840_000_u64, 0).saturating_add(T::DbWeight::get().writes(1_u64)) } fn register_as_candidate(c: u32) -> Weight { @@ -152,7 +152,7 @@ impl WeightInfo for () { fn set_desired_candidates() -> Weight { Weight::from_parts(16_363_000_u64, 0).saturating_add(RocksDbWeight::get().writes(1_u64)) } - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { Weight::from_parts(16_840_000_u64, 0).saturating_add(RocksDbWeight::get().writes(1_u64)) } fn register_as_candidate(c: u32) -> Weight { diff --git a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs index 3162ee7d5a3b..c686bd6134a7 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs index 078c867bcadf..b3062984baf0 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -121,7 +121,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs index 249fb6ae8b03..aeda7bbbb6a7 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_collator_selection.rs @@ -124,7 +124,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs index 5bd646884bd5..1fac2d59ab96 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs index fba1e28412b7..72d8ba4045a9 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs index 5e17dd706655..f7c78f7db82a 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs index 0a1f1c03de70..f7e233189abb 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/pallet_collator_selection.rs @@ -123,7 +123,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs index f03795b8e087..9dcee77082b9 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/pallet_collator_selection.rs @@ -124,7 +124,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` diff --git a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs index 66e34d79f5cb..03f3ff602a5b 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_collator_selection.rs @@ -121,7 +121,7 @@ impl pallet_collator_selection::WeightInfo for WeightIn } /// Storage: `CollatorSelection::CandidacyBond` (r:0 w:1) /// Proof: `CollatorSelection::CandidacyBond` (`max_values`: Some(1), `max_size`: Some(16), added: 511, mode: `MaxEncodedLen`) - fn set_candidacy_bond(_k: u32) -> Weight { + fn set_candidacy_bond(_c: u32, _k: u32) -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` From bba85730e1ee3827514233f37a577bd5660262eb Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Fri, 10 Nov 2023 11:42:40 +0200 Subject: [PATCH 30/33] Fix clippy warning in bench Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/benchmarking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/benchmarking.rs b/cumulus/pallets/collator-selection/src/benchmarking.rs index 7443048d8e0f..fa95303495dd 100644 --- a/cumulus/pallets/collator-selection/src/benchmarking.rs +++ b/cumulus/pallets/collator-selection/src/benchmarking.rs @@ -29,7 +29,7 @@ use frame_support::traits::{Currency, EnsureOrigin, Get, ReservableCurrency}; use frame_system::{pallet_prelude::BlockNumberFor, EventRecord, RawOrigin}; use pallet_authorship::EventHandler; use pallet_session::{self as session, SessionManager}; -use sp_std::prelude::*; +use sp_std::{cmp, prelude::*}; pub type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; @@ -231,7 +231,7 @@ mod benchmarks { >::put(initial_bond_amount); register_validators::(c); register_candidates::(c); - let kicked = std::cmp::min(k, c); + let kicked = cmp::min(k, c); let origin = T::UpdateOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let bond_amount = if k > 0 { From 4c14458d61fe684a1f54a6c46322c960feddc892 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 13 Nov 2023 10:42:01 +0200 Subject: [PATCH 31/33] Remove duplicated `ensure` Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index a0e8128a3ff6..f7732d43a32d 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -719,8 +719,6 @@ pub mod pallet { Error::::ValidatorNotRegistered ); - ensure!(deposit >= Self::candidacy_bond(), Error::::InsufficientBond); - let length = >::decode_len().unwrap_or_default(); // The closure below iterates through all elements of the candidate list to ensure that // the caller isn't already a candidate and to find the target it's trying to replace in From 35b4b0261e64b5f2afa68bf2f48d3bf397da0285 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 13 Nov 2023 14:31:46 +0200 Subject: [PATCH 32/33] Graceful error instead of panic on `try_insert` Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index f7732d43a32d..5f1a48991165 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -681,7 +681,7 @@ pub mod pallet { .unwrap_or_else(|| candidates.len()); candidates .try_insert(new_pos, info) - .expect("candidate count previously decremented; qed"); + .map_err(|_| Error::::InsertToCandidateListFailed)?; Ok(candidate_count) })?; From 12dccf402e5063c7d4dd9e9de37a01a762e99e84 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Mon, 13 Nov 2023 19:27:07 +0200 Subject: [PATCH 33/33] Update pallet documentation Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/lib.rs | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 5f1a48991165..7449f4d68c7e 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -38,10 +38,24 @@ //! 2. [`CandidateList`]: these are *candidates to the collation task* and may or may not be elected //! as a final collator. //! -//! The current implementation resolves congestion of [`CandidateList`] through a simple election -//! mechanism. Initially candidates register by placing the minimum bond, up until the maximum -//! number of allowed candidates is reached. Then, if an account wants to register as a candidate, -//! they have to replace an existing one by placing a greater deposit. +//! The current implementation resolves congestion of [`CandidateList`] through a simple auction +//! mechanism. Candidates bid for the collator slots and at the end of the session, the auction ends +//! and the top candidates are selected to become collators. The number of selected candidates is +//! determined by the value of `DesiredCandidates`. +//! +//! Before the list reaches full capacity, candidates can register by placing the minimum bond +//! through `register_as_candidate`. Then, if an account wants to participate in the collator slot +//! auction, they have to replace an existing candidate by placing a greater deposit through +//! `take_candidate_slot`. Existing candidates can increase their bids through `update_bond`. +//! +//! At any point, an account can take the place of another account in the candidate list if they put +//! up a greater deposit than the target. While new joiners would like to deposit as little as +//! possible to participate in the auction, the replacement threat incentivizes candidates to bid as +//! close to their budget as possible in order to avoid being replaced. +//! +//! Candidates which are not on "winning" slots in the list can also decrease their deposits through +//! `update_bond`, but candidates who are on top slots and try to decrease their deposits will fail +//! in order to enforce auction mechanics and have meaningful bids. //! //! Candidates will not be allowed to get kicked or `leave_intent` if the total number of collators //! would fall below `MinEligibleCollators`. This is to ensure that some collators will always @@ -62,8 +76,8 @@ //! //! To initiate rewards, an ED needs to be transferred to the pot address. //! -//! Note: Eventually the Pot distribution may be modified as discussed in -//! [this issue](https://github.com/paritytech/statemint/issues/21#issuecomment-810481073). +//! Note: Eventually the Pot distribution may be modified as discussed in [this +//! issue](https://github.com/paritytech/statemint/issues/21#issuecomment-810481073). #![cfg_attr(not(feature = "std"), no_std)]