From c9a0ef284442c7c012c5da45680bf2a6368594c6 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Wed, 23 Mar 2022 14:17:26 +0000 Subject: [PATCH] Store validator self-vote in bags-list, and allow them to be trimmed for election (#10821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Implement the new validator-in-bags-list scenario + migration * Apply suggestions from code review Co-authored-by: Zeke Mostov * some review comments * guard the migration * some review comments * Fix tests 🤦‍♂️ * Fix build * fix weight_of_fn * reformat line width * make const * use weight of fn cached * SortedListProvider -> VoterList * Fix all build and docs * check post migration Co-authored-by: Zeke Mostov --- bin/node/runtime/src/lib.rs | 4 +- frame/babe/src/mock.rs | 2 +- frame/bags-list/remote-tests/src/lib.rs | 2 +- frame/bags-list/remote-tests/src/migration.rs | 11 +- frame/bags-list/remote-tests/src/snapshot.rs | 5 +- frame/grandpa/src/mock.rs | 2 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/session/src/migrations/v1.rs | 4 +- frame/staking/src/benchmarking.rs | 50 +++-- frame/staking/src/lib.rs | 3 +- frame/staking/src/migrations.rs | 83 ++++++++- frame/staking/src/mock.rs | 10 +- frame/staking/src/pallet/impls.rs | 176 ++++++++++-------- frame/staking/src/pallet/mod.rs | 35 ++-- frame/staking/src/testing_utils.rs | 4 +- frame/staking/src/tests.rs | 114 ++++++------ 17 files changed, 305 insertions(+), 204 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index b7137de48fb15..a6d97cb299b24 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -558,9 +558,7 @@ impl pallet_staking::Config for Runtime { type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = ElectionProviderMultiPhase; type GenesisElectionProvider = onchain::OnChainSequentialPhragmen; - // Alternatively, use pallet_staking::UseNominatorsMap to just use the nominators map. - // Note that the aforementioned does not scale to a very large number of nominators. - type SortedListProvider = BagsList; + type VoterList = BagsList; type MaxUnlockingChunks = ConstU32<32>; type WeightInfo = pallet_staking::weights::SubstrateWeight; type BenchmarkingConfig = StakingBenchmarkingConfig; diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 152ec5ab206e7..e74288577c9ef 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -197,7 +197,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; - type SortedListProvider = pallet_staking::UseNominatorsMap; + type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type MaxUnlockingChunks = ConstU32<32>; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); diff --git a/frame/bags-list/remote-tests/src/lib.rs b/frame/bags-list/remote-tests/src/lib.rs index 83c322f93134e..caf7a2a547e09 100644 --- a/frame/bags-list/remote-tests/src/lib.rs +++ b/frame/bags-list/remote-tests/src/lib.rs @@ -48,7 +48,7 @@ pub fn display_and_check_bags(currency_unit: u64, currency_na let min_nominator_bond = >::get(); log::info!(target: LOG_TARGET, "min nominator bond is {:?}", min_nominator_bond); - let voter_list_count = ::SortedListProvider::count(); + let voter_list_count = ::VoterList::count(); // go through every bag to track the total number of voters within bags and log some info about // how voters are distributed within the bags. diff --git a/frame/bags-list/remote-tests/src/migration.rs b/frame/bags-list/remote-tests/src/migration.rs index 4d5169fcc6dfa..c4cd73c45d377 100644 --- a/frame/bags-list/remote-tests/src/migration.rs +++ b/frame/bags-list/remote-tests/src/migration.rs @@ -17,7 +17,6 @@ //! Test to check the migration of the voter bag. use crate::{RuntimeT, LOG_TARGET}; -use frame_election_provider_support::SortedListProvider; use frame_support::traits::PalletInfoAccess; use pallet_staking::Nominators; use remote_externalities::{Builder, Mode, OnlineConfig}; @@ -45,16 +44,16 @@ pub async fn execute( let pre_migrate_nominator_count = >::iter().count() as u32; log::info!(target: LOG_TARGET, "Nominator count: {}", pre_migrate_nominator_count); - // run the actual migration, - let moved = ::SortedListProvider::unsafe_regenerate( + use frame_election_provider_support::SortedListProvider; + // run the actual migration + let moved = ::VoterList::unsafe_regenerate( pallet_staking::Nominators::::iter().map(|(n, _)| n), pallet_staking::Pallet::::weight_of_fn(), ); log::info!(target: LOG_TARGET, "Moved {} nominators", moved); - let voter_list_len = - ::SortedListProvider::iter().count() as u32; - let voter_list_count = ::SortedListProvider::count(); + let voter_list_len = ::VoterList::iter().count() as u32; + let voter_list_count = ::VoterList::count(); // and confirm it is equal to the length of the `VoterList`. assert_eq!(pre_migrate_nominator_count, voter_list_len); assert_eq!(pre_migrate_nominator_count, voter_list_count); diff --git a/frame/bags-list/remote-tests/src/snapshot.rs b/frame/bags-list/remote-tests/src/snapshot.rs index 2d996746a29cc..408f5f2bd8aa2 100644 --- a/frame/bags-list/remote-tests/src/snapshot.rs +++ b/frame/bags-list/remote-tests/src/snapshot.rs @@ -16,6 +16,7 @@ //! Test to execute the snapshot using the voter bag. +use frame_election_provider_support::SortedListProvider; use frame_support::traits::PalletInfoAccess; use remote_externalities::{Builder, Mode, OnlineConfig}; use sp_runtime::{traits::Block as BlockT, DeserializeOwned}; @@ -48,11 +49,11 @@ pub async fn execute .unwrap(); ext.execute_with(|| { - use frame_election_provider_support::{ElectionDataProvider, SortedListProvider}; + use frame_election_provider_support::ElectionDataProvider; log::info!( target: crate::LOG_TARGET, "{} nodes in bags list.", - ::SortedListProvider::count(), + ::VoterList::count(), ); let voters = diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 9dac33f979841..6490a2b6992bf 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -205,7 +205,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; - type SortedListProvider = pallet_staking::UseNominatorsMap; + type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type MaxUnlockingChunks = ConstU32<32>; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 22c9af0f4c3cd..4359b7745ddd6 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -175,7 +175,7 @@ impl pallet_staking::Config for Test { type OffendingValidatorsThreshold = (); type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; - type SortedListProvider = pallet_staking::UseNominatorsMap; + type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type MaxUnlockingChunks = ConstU32<32>; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index a9328b6546c91..5ebc75245630c 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -182,7 +182,7 @@ impl pallet_staking::Config for Test { type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; type MaxUnlockingChunks = ConstU32<32>; - type SortedListProvider = pallet_staking::UseNominatorsMap; + type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; type WeightInfo = (); } diff --git a/frame/session/src/migrations/v1.rs b/frame/session/src/migrations/v1.rs index 2a69cd6d6a550..3c687ea7d9d66 100644 --- a/frame/session/src/migrations/v1.rs +++ b/frame/session/src/migrations/v1.rs @@ -87,7 +87,7 @@ pub fn migrate ListScenario { /// - the destination bag has at least one node, which will need its next pointer updated. /// /// NOTE: while this scenario specifically targets a worst case for the bags-list, it should - /// also elicit a worst case for other known `SortedListProvider` implementations; although - /// this may not be true against unknown `SortedListProvider` implementations. + /// also elicit a worst case for other known `VoterList` implementations; although + /// this may not be true against unknown `VoterList` implementations. fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); @@ -189,7 +189,7 @@ impl ListScenario { // find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = - T::SortedListProvider::score_update_worst_case(&origin_stash1, is_increase); + T::VoterList::score_update_worst_case(&origin_stash1, is_increase); let total_issuance = T::Currency::total_issuance(); @@ -316,7 +316,7 @@ benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); - assert!(T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); let ed = T::Currency::minimum_balance(); let mut ledger = Ledger::::get(&controller).unwrap(); @@ -328,28 +328,24 @@ benchmarks! { }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { assert!(!Ledger::::contains_key(controller)); - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); } validate { - // clean up any existing state. - clear_validators_and_nominators::(); - - let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); - - // setup a worst case scenario where the user calling validate was formerly a nominator so - // they must be removed from the list. - let scenario = ListScenario::::new(origin_weight, true)?; - let controller = scenario.origin_controller1.clone(); - let stash = scenario.origin_stash1.clone(); - assert!(T::SortedListProvider::contains(&stash)); + let (stash, controller) = create_stash_controller::( + T::MaxNominations::get() - 1, + 100, + Default::default(), + )?; + // because it is chilled. + assert!(!T::VoterList::contains(&stash)); let prefs = ValidatorPrefs::default(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), prefs) verify { assert!(Validators::::contains_key(&stash)); - assert!(!T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); } kick { @@ -434,14 +430,14 @@ benchmarks! { ).unwrap(); assert!(!Nominators::::contains_key(&stash)); - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); let validators = create_validators::(n, 100).unwrap(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), validators) verify { assert!(Nominators::::contains_key(&stash)); - assert!(T::SortedListProvider::contains(&stash)) + assert!(T::VoterList::contains(&stash)) } chill { @@ -455,12 +451,12 @@ benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); - assert!(T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); } set_payee { @@ -523,13 +519,13 @@ benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); - assert!(T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); add_slashing_spans::(&stash, s); }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); } cancel_deferred_slash { @@ -708,13 +704,13 @@ benchmarks! { Ledger::::insert(&controller, l); assert!(Bonded::::contains_key(&stash)); - assert!(T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { assert!(!Bonded::::contains_key(&stash)); - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); } new_era { @@ -899,7 +895,7 @@ benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); - assert!(T::SortedListProvider::contains(&stash)); + assert!(T::VoterList::contains(&stash)); Staking::::set_staking_configs( RawOrigin::Root.into(), @@ -914,7 +910,7 @@ benchmarks! { let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { - assert!(!T::SortedListProvider::contains(&stash)); + assert!(!T::VoterList::contains(&stash)); } force_apply_min_commission { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 4c1bb438457e5..2a0716721dd51 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -780,7 +780,8 @@ enum Releases { V5_0_0, // blockable validators. V6_0_0, // removal of all storage associated with offchain phragmen. V7_0_0, // keep track of number of nominators / validators in map - V8_0_0, // populate `SortedListProvider`. + V8_0_0, // populate `VoterList`. + V9_0_0, // inject validators into `VoterList` as well. } impl Default for Releases { diff --git a/frame/staking/src/migrations.rs b/frame/staking/src/migrations.rs index 3991c4a66076f..96c905f4e5942 100644 --- a/frame/staking/src/migrations.rs +++ b/frame/staking/src/migrations.rs @@ -17,13 +17,83 @@ //! Storage migrations for the Staking pallet. use super::*; +use frame_election_provider_support::SortedListProvider; +use frame_support::traits::OnRuntimeUpgrade; + +pub mod v9 { + use super::*; + + /// Migration implementation that injects all validators into sorted list. + /// + /// This is only useful for chains that started their `VoterList` just based on nominators. + pub struct InjectValidatorsIntoVoterList(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for InjectValidatorsIntoVoterList { + fn on_runtime_upgrade() -> Weight { + if StorageVersion::::get() == Releases::V8_0_0 { + let prev_count = T::VoterList::count(); + let weight_of_cached = Pallet::::weight_of_fn(); + for (v, _) in Validators::::iter() { + let weight = weight_of_cached(&v); + let _ = T::VoterList::on_insert(v.clone(), weight).map_err(|err| { + log!(warn, "failed to insert {:?} into VoterList: {:?}", v, err) + }); + } + + log!( + info, + "injected a total of {} new voters, prev count: {} next count: {}, updating to version 9", + Validators::::count(), + prev_count, + T::VoterList::count(), + ); + + StorageVersion::::put(crate::Releases::V9_0_0); + T::BlockWeights::get().max_block + } else { + log!( + warn, + "InjectValidatorsIntoVoterList being executed on the wrong storage \ + version, expected Releases::V8_0_0" + ); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), &'static str> { + use frame_support::traits::OnRuntimeUpgradeHelpersExt; + frame_support::ensure!( + StorageVersion::::get() == crate::Releases::V8_0_0, + "must upgrade linearly" + ); + + let prev_count = T::VoterList::count(); + Self::set_temp_storage(prev_count, "prev"); + Ok(()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade() -> Result<(), &'static str> { + use frame_support::traits::OnRuntimeUpgradeHelpersExt; + let post_count = T::VoterList::count(); + let prev_count = Self::get_temp_storage::("prev").unwrap(); + let validators = Validators::::count(); + assert!(post_count == prev_count + validators); + + frame_support::ensure!( + StorageVersion::::get() == crate::Releases::V9_0_0, + "must upgrade " + ); + Ok(()) + } + } +} pub mod v8 { + use crate::{Config, Nominators, Pallet, StorageVersion, Weight}; use frame_election_provider_support::SortedListProvider; use frame_support::traits::Get; - use crate::{Config, Nominators, Pallet, StorageVersion, Weight}; - #[cfg(feature = "try-runtime")] pub fn pre_migrate() -> Result<(), &'static str> { frame_support::ensure!( @@ -35,16 +105,16 @@ pub mod v8 { Ok(()) } - /// Migration to sorted [`SortedListProvider`]. + /// Migration to sorted `VoterList`. pub fn migrate() -> Weight { if StorageVersion::::get() == crate::Releases::V7_0_0 { crate::log!(info, "migrating staking to Releases::V8_0_0"); - let migrated = T::SortedListProvider::unsafe_regenerate( + let migrated = T::VoterList::unsafe_regenerate( Nominators::::iter().map(|(id, _)| id), Pallet::::weight_of_fn(), ); - debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(())); + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); StorageVersion::::put(crate::Releases::V8_0_0); crate::log!( @@ -61,8 +131,7 @@ pub mod v8 { #[cfg(feature = "try-runtime")] pub fn post_migrate() -> Result<(), &'static str> { - T::SortedListProvider::sanity_check() - .map_err(|_| "SortedListProvider is not in a sane state.")?; + T::VoterList::sanity_check().map_err(|_| "VoterList is not in a sane state.")?; crate::log!(info, "👜 staking bags-list migration passes POST migrate checks ✅",); Ok(()) } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 843791a46ade2..bb71232c34673 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -270,8 +270,8 @@ impl crate::pallet::pallet::Config for Test { type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = onchain::OnChainSequentialPhragmen; type GenesisElectionProvider = Self::ElectionProvider; - // NOTE: consider a macro and use `UseNominatorsMap` as well. - type SortedListProvider = BagsList; + // NOTE: consider a macro and use `UseNominatorsAndValidatorsMap` as well. + type VoterList = BagsList; type MaxUnlockingChunks = ConstU32<32>; type BenchmarkingConfig = TestBenchmarkingConfig; type WeightInfo = (); @@ -541,9 +541,9 @@ fn check_count() { assert_eq!(nominator_count, Nominators::::count()); assert_eq!(validator_count, Validators::::count()); - // the voters that the `SortedListProvider` list is storing for us. - let external_voters = ::SortedListProvider::count(); - assert_eq!(external_voters, nominator_count); + // the voters that the `VoterList` list is storing for us. + let external_voters = ::VoterList::count(); + assert_eq!(external_voters, nominator_count + validator_count); } fn check_ledgers() { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index cb024ba2bc524..9d5a3ed484184 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -49,6 +49,14 @@ use crate::{ use super::{pallet::*, STAKING_ID}; +/// The maximum number of iterations that we do whilst iterating over `T::VoterList` in +/// `get_npos_voters`. +/// +/// In most cases, if we want n items, we iterate exactly n times. In rare cases, if a voter is +/// invalid (for any reason) the iteration continues. With this constant, we iterate at most 2 * n +/// times and then give up. +const NPOS_MAX_ITERATIONS_COEFFICIENT: u32 = 2; + impl Pallet { /// The total balance that can be slashed from a stash account as of right now. pub fn slashable_balance_of(stash: &T::AccountId) -> BalanceOf { @@ -649,90 +657,77 @@ impl Pallet { /// Get all of the voters that are eligible for the npos election. /// - /// `maybe_max_len` can imposes a cap on the number of voters returned; First all the validator - /// are included in no particular order, then remainder is taken from the nominators, as - /// returned by [`Config::SortedListProvider`]. - /// - /// This will use nominators, and all the validators will inject a self vote. + /// `maybe_max_len` can imposes a cap on the number of voters returned; /// /// This function is self-weighing as [`DispatchClass::Mandatory`]. /// /// ### Slashing /// - /// All nominations that have been submitted before the last non-zero slash of the validator are - /// auto-chilled, but still count towards the limit imposed by `maybe_max_len`. + /// All votes that have been submitted before the last non-zero slash of the corresponding + /// target are *auto-chilled*, but still count towards the limit imposed by `maybe_max_len`. pub fn get_npos_voters(maybe_max_len: Option) -> Vec> { let max_allowed_len = { - let nominator_count = Nominators::::count() as usize; - let validator_count = Validators::::count() as usize; - let all_voter_count = validator_count.saturating_add(nominator_count); + let all_voter_count = T::VoterList::count() as usize; maybe_max_len.unwrap_or(all_voter_count).min(all_voter_count) }; let mut all_voters = Vec::<_>::with_capacity(max_allowed_len); - // first, grab all validators in no particular order, capped by the maximum allowed length. - let mut validators_taken = 0u32; - for (validator, _) in >::iter().take(max_allowed_len) { - // Append self vote. - let self_vote = ( - validator.clone(), - Self::weight_of(&validator), - vec![validator.clone()] - .try_into() - .expect("`MaxVotesPerVoter` must be greater than or equal to 1"), - ); - all_voters.push(self_vote); - validators_taken.saturating_inc(); - } - - // .. and grab whatever we have left from nominators. - let nominators_quota = (max_allowed_len as u32).saturating_sub(validators_taken); + // cache a few things. + let weight_of = Self::weight_of_fn(); let slashing_spans = >::iter().collect::>(); - // track the count of nominators added to `all_voters + let mut voters_seen = 0u32; + let mut validators_taken = 0u32; let mut nominators_taken = 0u32; - // track every nominator iterated over, but not necessarily added to `all_voters` - let mut nominators_seen = 0u32; - // cache the total-issuance once in this function - let weight_of = Self::weight_of_fn(); - - let mut nominators_iter = T::SortedListProvider::iter(); - while nominators_taken < nominators_quota && nominators_seen < nominators_quota * 2 { - let nominator = match nominators_iter.next() { - Some(nominator) => { - nominators_seen.saturating_inc(); - nominator + let mut sorted_voters = T::VoterList::iter(); + while all_voters.len() < max_allowed_len && + voters_seen < (NPOS_MAX_ITERATIONS_COEFFICIENT * max_allowed_len as u32) + { + let voter = match sorted_voters.next() { + Some(voter) => { + voters_seen.saturating_inc(); + voter }, None => break, }; if let Some(Nominations { submitted_in, mut targets, suppressed: _ }) = - >::get(&nominator) + >::get(&voter) { - log!( - trace, - "fetched nominator {:?} with weight {:?}", - nominator, - weight_of(&nominator) - ); + // if this voter is a nominator: targets.retain(|stash| { slashing_spans .get(stash) .map_or(true, |spans| submitted_in >= spans.last_nonzero_slash()) }); if !targets.len().is_zero() { - all_voters.push((nominator.clone(), weight_of(&nominator), targets)); + all_voters.push((voter.clone(), weight_of(&voter), targets)); nominators_taken.saturating_inc(); } + } else if Validators::::contains_key(&voter) { + // if this voter is a validator: + let self_vote = ( + voter.clone(), + weight_of(&voter), + vec![voter.clone()] + .try_into() + .expect("`MaxVotesPerVoter` must be greater than or equal to 1"), + ); + all_voters.push(self_vote); + validators_taken.saturating_inc(); } else { - // this can only happen if: 1. there a pretty bad bug in the bags-list (or whatever - // is the sorted list) logic and the state of the two pallets is no longer - // compatible, or because the nominators is not decodable since they have more - // nomination than `T::MaxNominations`. This can rarely happen, and is not really an - // emergency or bug if it does. - log!(warn, "DEFENSIVE: invalid item in `SortedListProvider`: {:?}, this nominator probably has too many nominations now", nominator) + // this can only happen if: 1. there a bug in the bags-list (or whatever is the + // sorted list) logic and the state of the two pallets is no longer compatible, or + // because the nominators is not decodable since they have more nomination than + // `T::MaxNominations`. The latter can rarely happen, and is not really an emergency + // or bug if it does. + log!( + warn, + "DEFENSIVE: invalid item in `VoterList`: {:?}, this nominator probably has too many nominations now", + voter + ) } } @@ -752,6 +747,7 @@ impl Pallet { validators_taken, nominators_taken ); + all_voters } @@ -773,7 +769,7 @@ impl Pallet { } /// This function will add a nominator to the `Nominators` storage map, - /// and [`SortedListProvider`]. + /// and `VoterList`. /// /// If the nominator already exists, their nominations will be updated. /// @@ -782,18 +778,21 @@ impl Pallet { /// wrong. pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations) { if !Nominators::::contains_key(who) { - // maybe update sorted list. Error checking is defensive-only - this should never fail. - let _ = T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)) + // maybe update sorted list. + let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who)) .defensive_unwrap_or_default(); - - debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(())); } - Nominators::::insert(who, nominations); + + debug_assert_eq!( + Nominators::::count() + Validators::::count(), + T::VoterList::count() + ); + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); } /// This function will remove a nominator from the `Nominators` storage map, - /// and [`SortedListProvider`]. + /// and `VoterList`. /// /// Returns true if `who` was removed from `Nominators`, otherwise false. /// @@ -801,15 +800,21 @@ impl Pallet { /// `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_nominator(who: &T::AccountId) -> bool { - if Nominators::::contains_key(who) { + let outcome = if Nominators::::contains_key(who) { Nominators::::remove(who); - T::SortedListProvider::on_remove(who); - debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(())); - debug_assert_eq!(Nominators::::count(), T::SortedListProvider::count()); + T::VoterList::on_remove(who); true } else { false - } + }; + + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); + debug_assert_eq!( + Nominators::::count() + Validators::::count(), + T::VoterList::count() + ); + + outcome } /// This function will add a validator to the `Validators` storage map. @@ -820,7 +825,18 @@ impl Pallet { /// `Validators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_add_validator(who: &T::AccountId, prefs: ValidatorPrefs) { + if !Validators::::contains_key(who) { + // maybe update sorted list. + let _ = T::VoterList::on_insert(who.clone(), Self::weight_of(who)) + .defensive_unwrap_or_default(); + } Validators::::insert(who, prefs); + + debug_assert_eq!( + Nominators::::count() + Validators::::count(), + T::VoterList::count() + ); + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); } /// This function will remove a validator from the `Validators` storage map. @@ -831,12 +847,21 @@ impl Pallet { /// `Validators` or `VoterList` outside of this function is almost certainly /// wrong. pub fn do_remove_validator(who: &T::AccountId) -> bool { - if Validators::::contains_key(who) { + let outcome = if Validators::::contains_key(who) { Validators::::remove(who); + T::VoterList::on_remove(who); true } else { false - } + }; + + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); + debug_assert_eq!( + Nominators::::count() + Validators::::count(), + T::VoterList::count() + ); + + outcome } /// Register some amount of weight directly with the system pallet. @@ -963,7 +988,7 @@ impl ElectionDataProvider for Pallet { >::remove_all(); >::remove_all(); - T::SortedListProvider::unsafe_clear(); + T::VoterList::unsafe_clear(); } #[cfg(feature = "runtime-benchmarks")] @@ -1278,20 +1303,24 @@ impl ScoreProvider for Pallet { /// A simple voter list implementation that does not require any additional pallets. Note, this /// does not provided nominators in sorted ordered. If you desire nominators in a sorted order take /// a look at [`pallet-bags-list]. -pub struct UseNominatorsMap(sp_std::marker::PhantomData); -impl SortedListProvider for UseNominatorsMap { +pub struct UseNominatorsAndValidatorsMap(sp_std::marker::PhantomData); +impl SortedListProvider for UseNominatorsAndValidatorsMap { type Error = (); type Score = VoteWeight; /// Returns iterator over voter list, which can have `take` called on it. fn iter() -> Box> { - Box::new(Nominators::::iter().map(|(n, _)| n)) + Box::new( + Validators::::iter() + .map(|(v, _)| v) + .chain(Nominators::::iter().map(|(n, _)| n)), + ) } fn count() -> u32 { - Nominators::::count() + Nominators::::count().saturating_add(Validators::::count()) } fn contains(id: &T::AccountId) -> bool { - Nominators::::contains_key(id) + Nominators::::contains_key(id) || Validators::::contains_key(id) } fn on_insert(_: T::AccountId, _weight: Self::Score) -> Result<(), Self::Error> { // nothing to do on insert. @@ -1318,5 +1347,6 @@ impl SortedListProvider for UseNominatorsMap { // NOTE: Caller must ensure this doesn't lead to too many storage accesses. This is a // condition of SortedListProvider::unsafe_clear. Nominators::::remove_all(); + Validators::::remove_all(); } } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 39173bf61c833..306bd34390d82 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -17,7 +17,7 @@ //! Staking FRAME Pallet. -use frame_election_provider_support::SortedListProvider; +use frame_election_provider_support::{SortedListProvider, VoteWeight}; use frame_support::{ dispatch::Codec, pallet_prelude::*, @@ -163,13 +163,12 @@ pub mod pallet { /// After the threshold is reached a new era will be forced. type OffendingValidatorsThreshold: Get; - /// Something that can provide a sorted list of voters in a somewhat sorted way. The - /// original use case for this was designed with `pallet_bags_list::Pallet` in mind. If - /// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option. - type SortedListProvider: SortedListProvider< - Self::AccountId, - Score = frame_election_provider_support::VoteWeight, - >; + /// Something that provides a best-effort sorted list of voters aka electing nominators, + /// used for NPoS election. + /// + /// The changes to nominators are reported to this. Moreover, each validator's self-vote is + /// also reported as one independent vote. + type VoterList: SortedListProvider; /// The maximum number of `unlocking` chunks a [`StakingLedger`] can have. Effectively /// determines how many unique eras a staker may be unbonding in. @@ -584,10 +583,10 @@ pub mod pallet { }); } - // all voters are reported to the `SortedListProvider`. + // all voters are reported to the `VoterList`. assert_eq!( - T::SortedListProvider::count(), - Nominators::::count(), + T::VoterList::count(), + Nominators::::count() + Validators::::count(), "not all genesis stakers were inserted into sorted list provider, something is wrong." ); } @@ -837,9 +836,9 @@ pub mod pallet { // NOTE: ledger must be updated prior to calling `Self::weight_of`. Self::update_ledger(&controller, &ledger); // update this staker in the sorted list, if they exist in it. - if T::SortedListProvider::contains(&stash) { - T::SortedListProvider::on_update(&stash, Self::weight_of(&ledger.stash)); - debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(())); + if T::VoterList::contains(&stash) { + T::VoterList::on_update(&stash, Self::weight_of(&ledger.stash)); + debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); } Self::deposit_event(Event::::Bonded(stash.clone(), extra)); @@ -920,8 +919,8 @@ pub mod pallet { Self::update_ledger(&controller, &ledger); // update this staker in the sorted list, if they exist in it. - if T::SortedListProvider::contains(&ledger.stash) { - T::SortedListProvider::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); + if T::VoterList::contains(&ledger.stash) { + T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); } Self::deposit_event(Event::::Unbonded(ledger.stash, value)); @@ -1403,8 +1402,8 @@ pub mod pallet { // NOTE: ledger must be updated prior to calling `Self::weight_of`. Self::update_ledger(&controller, &ledger); - if T::SortedListProvider::contains(&ledger.stash) { - T::SortedListProvider::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); + if T::VoterList::contains(&ledger.stash) { + T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); } let removed_chunks = 1u32 // for the case where the last iterated chunk is not removed diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 8e6bd88468930..5f9f378b10619 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -38,11 +38,11 @@ const SEED: u32 = 0; pub fn clear_validators_and_nominators() { Validators::::remove_all(); - // whenever we touch nominators counter we should update `T::SortedListProvider` as well. + // whenever we touch nominators counter we should update `T::VoterList` as well. Nominators::::remove_all(); // NOTE: safe to call outside block production - T::SortedListProvider::unsafe_clear(); + T::VoterList::unsafe_clear(); } /// Grab a funded user. diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 2d4145242a45c..11dfe3e4777f8 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4113,11 +4113,7 @@ mod election_data_provider { .set_status(41, StakerStatus::Validator) .build_and_execute(|| { // sum of all nominators who'd be voters (1), plus the self-votes (4). - assert_eq!( - ::SortedListProvider::count() + - >::iter().count() as u32, - 5 - ); + assert_eq!(::VoterList::count(), 5); // if limits is less.. assert_eq!(Staking::electing_voters(Some(1)).unwrap().len(), 1); @@ -4140,43 +4136,43 @@ mod election_data_provider { }); } + // Tests the criteria that in `ElectionDataProvider::voters` function, we try to get at most + // `maybe_max_len` voters, and if some of them end up being skipped, we iterate at most `2 * + // maybe_max_len`. #[test] - fn only_iterates_max_2_times_nominators_quota() { + fn only_iterates_max_2_times_max_allowed_len() { ExtBuilder::default() - .nominate(true) // add nominator 101, who nominates [11, 21] + .nominate(false) // the other nominators only nominate 21 .add_staker(61, 60, 2_000, StakerStatus::::Nominator(vec![21])) .add_staker(71, 70, 2_000, StakerStatus::::Nominator(vec![21])) .add_staker(81, 80, 2_000, StakerStatus::::Nominator(vec![21])) .build_and_execute(|| { - // given our nominators ordered by stake, - assert_eq!( - ::SortedListProvider::iter().collect::>(), - vec![61, 71, 81, 101] - ); - - // and total voters + // all voters ordered by stake, assert_eq!( - ::SortedListProvider::count() + - >::iter().count() as u32, - 7 + ::VoterList::iter().collect::>(), + vec![61, 71, 81, 11, 21, 31] ); - // roll to session 5 run_to_block(25); // slash 21, the only validator nominated by our first 3 nominators add_slash(&21); - // we take 4 voters: 2 validators and 2 nominators (so nominators quota = 2) + // we want 2 voters now, and in maximum we allow 4 iterations. This is what happens: + // 61 is pruned; + // 71 is pruned; + // 81 is pruned; + // 11 is taken; + // we finish since the 2x limit is reached. assert_eq!( - Staking::electing_voters(Some(3)) + Staking::electing_voters(Some(2)) .unwrap() .iter() .map(|(stash, _, _)| stash) .copied() .collect::>(), - vec![31, 11], // 2 validators, but no nominators because we hit the quota + vec![11], ); }); } @@ -4189,46 +4185,35 @@ mod election_data_provider { #[test] fn get_max_len_voters_even_if_some_nominators_are_slashed() { ExtBuilder::default() - .nominate(true) // add nominator 101, who nominates [11, 21] + .nominate(false) .add_staker(61, 60, 20, StakerStatus::::Nominator(vec![21])) - // 61 only nominates validator 21 ^^ .add_staker(71, 70, 10, StakerStatus::::Nominator(vec![11, 21])) + .add_staker(81, 80, 10, StakerStatus::::Nominator(vec![11, 21])) .build_and_execute(|| { - // given our nominators ordered by stake, + // given our voters ordered by stake, assert_eq!( - ::SortedListProvider::iter().collect::>(), - vec![101, 61, 71] + ::VoterList::iter().collect::>(), + vec![11, 21, 31, 61, 71, 81] ); - // and total voters - assert_eq!( - ::SortedListProvider::count() + - >::iter().count() as u32, - 6 - ); - - // we take 5 voters + // we take 4 voters assert_eq!( - Staking::electing_voters(Some(5)) + Staking::electing_voters(Some(4)) .unwrap() .iter() .map(|(stash, _, _)| stash) .copied() .collect::>(), - // then - vec![ - 31, 21, 11, // 3 nominators - 101, 61 // 2 validators, and 71 is excluded - ], + vec![11, 21, 31, 61], ); // roll to session 5 run_to_block(25); - // slash 21, the only validator nominated by 61 + // slash 21, the only validator nominated by 61. add_slash(&21); - // we take 4 voters + // we take 4 voters; 71 and 81 are replacing the ejected ones. assert_eq!( Staking::electing_voters(Some(4)) .unwrap() @@ -4236,10 +4221,7 @@ mod election_data_provider { .map(|(stash, _, _)| stash) .copied() .collect::>(), - vec![ - 31, 11, // 2 validators (21 was slashed) - 101, 71 // 2 nominators, excluding 61 - ], + vec![11, 31, 71, 81], ); }); } @@ -4755,19 +4737,45 @@ mod sorted_list_provider { fn re_nominate_does_not_change_counters_or_list() { ExtBuilder::default().nominate(true).build_and_execute(|| { // given - let pre_insert_nominator_count = Nominators::::iter().count() as u32; - assert_eq!(::SortedListProvider::count(), pre_insert_nominator_count); - assert!(Nominators::::contains_key(101)); - assert_eq!(::SortedListProvider::iter().collect::>(), vec![101]); + let pre_insert_voter_count = + (Nominators::::count() + Validators::::count()) as u32; + assert_eq!(::VoterList::count(), pre_insert_voter_count); + + assert_eq!( + ::VoterList::iter().collect::>(), + vec![11, 21, 31, 101] + ); // when account 101 renominates assert_ok!(Staking::nominate(Origin::signed(100), vec![41])); // then counts don't change - assert_eq!(::SortedListProvider::count(), pre_insert_nominator_count); - assert_eq!(Nominators::::iter().count() as u32, pre_insert_nominator_count); + assert_eq!(::VoterList::count(), pre_insert_voter_count); + // and the list is the same + assert_eq!( + ::VoterList::iter().collect::>(), + vec![11, 21, 31, 101] + ); + }); + } + + #[test] + fn re_validate_does_not_change_counters_or_list() { + ExtBuilder::default().nominate(false).build_and_execute(|| { + // given + let pre_insert_voter_count = + (Nominators::::count() + Validators::::count()) as u32; + assert_eq!(::VoterList::count(), pre_insert_voter_count); + + assert_eq!(::VoterList::iter().collect::>(), vec![11, 21, 31]); + + // when account 11 re-validates + assert_ok!(Staking::validate(Origin::signed(10), Default::default())); + + // then counts don't change + assert_eq!(::VoterList::count(), pre_insert_voter_count); // and the list is the same - assert_eq!(::SortedListProvider::iter().collect::>(), vec![101]); + assert_eq!(::VoterList::iter().collect::>(), vec![11, 21, 31]); }); } }