From 5a990c4673899f7704372bce98ee7b21cf891504 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 12:02:08 +0000 Subject: [PATCH 01/21] Use proper bounded vector type for nominations --- bin/node/cli/src/chain_spec.rs | 6 +- bin/node/runtime/src/lib.rs | 8 +- frame/babe/src/mock.rs | 2 +- .../src/benchmarking.rs | 6 +- .../election-provider-multi-phase/src/lib.rs | 2 +- .../election-provider-multi-phase/src/mock.rs | 5 +- frame/election-provider-support/src/lib.rs | 8 +- .../election-provider-support/src/onchain.rs | 4 +- frame/grandpa/src/mock.rs | 2 +- frame/offences/benchmarking/src/lib.rs | 6 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/session/benchmarking/src/lib.rs | 8 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/staking/src/benchmarking.rs | 28 +++--- frame/staking/src/lib.rs | 9 +- frame/staking/src/mock.rs | 5 +- frame/staking/src/pallet/impls.rs | 12 ++- frame/staking/src/pallet/mod.rs | 24 ++--- frame/staking/src/tests.rs | 93 ++++++++++++++++++- .../support/src/storage/types/counted_map.rs | 7 ++ frame/system/src/lib.rs | 5 - 21 files changed, 174 insertions(+), 70 deletions(-) diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs index 747bc71c5007c..6fd57e31e466e 100644 --- a/bin/node/cli/src/chain_spec.rs +++ b/bin/node/cli/src/chain_spec.rs @@ -23,8 +23,8 @@ use hex_literal::hex; use node_runtime::{ constants::currency::*, wasm_binary_unwrap, AuthorityDiscoveryConfig, BabeConfig, BalancesConfig, Block, CouncilConfig, DemocracyConfig, ElectionsConfig, GrandpaConfig, - ImOnlineConfig, IndicesConfig, SessionConfig, SessionKeys, SocietyConfig, StakerStatus, - StakingConfig, SudoConfig, SystemConfig, TechnicalCommitteeConfig, MAX_NOMINATIONS, + ImOnlineConfig, IndicesConfig, MaxNominations, SessionConfig, SessionKeys, SocietyConfig, + StakerStatus, StakingConfig, SudoConfig, SystemConfig, TechnicalCommitteeConfig, }; use pallet_im_online::sr25519::AuthorityId as ImOnlineId; use sc_chain_spec::ChainSpecExtension; @@ -278,7 +278,7 @@ pub fn testnet_genesis( .map(|x| (x.0.clone(), x.1.clone(), STASH, StakerStatus::Validator)) .chain(initial_nominators.iter().map(|x| { use rand::{seq::SliceRandom, Rng}; - let limit = (MAX_NOMINATIONS as usize).min(initial_authorities.len()); + let limit = (MaxNominations::get() as usize).min(initial_authorities.len()); let count = rng.gen::() % limit; let nominations = initial_authorities .as_slice() diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index aab813f29e0aa..b824cda4b8fec 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -541,7 +541,7 @@ impl pallet_staking::BenchmarkingConfig for StakingBenchmarkingConfig { } impl pallet_staking::Config for Runtime { - const MAX_NOMINATIONS: u32 = MAX_NOMINATIONS; + type MaxNominations = MaxNominations; type Currency = Balances; type UnixTime = Timestamp; type CurrencyToVote = U128CurrencyToVote; @@ -605,7 +605,9 @@ sp_npos_elections::generate_solution_type!( >(16) ); -pub const MAX_NOMINATIONS: u32 = ::LIMIT as u32; +parameter_types! { + pub MaxNominations: u32 = ::LIMIT as u32; +} /// The numbers configured here could always be more than the the maximum limits of staking pallet /// to ensure election snapshot will not run out of memory. For now, we set them to smaller values @@ -1816,7 +1818,7 @@ mod tests { #[test] fn perbill_as_onchain_accuracy() { type OnChainAccuracy = ::Accuracy; - let maximum_chain_accuracy: Vec> = (0..MAX_NOMINATIONS) + let maximum_chain_accuracy: Vec> = (0..MaxNominations::get()) .map(|_| >::from(OnChainAccuracy::one().deconstruct())) .collect(); let _: UpperOf = diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index f0faa55e5b333..22411f5e81ddc 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -180,7 +180,7 @@ impl onchain::Config for Test { } impl pallet_staking::Config for Test { - const MAX_NOMINATIONS: u32 = 16; + type MaxNominations = ConstU32<16>; type RewardRemainder = (); type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = Event; diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index d605c5131cf77..c405afa383222 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -152,7 +152,7 @@ fn set_up_data_provider(v: u32, t: u32) { info, "setting up with voters = {} [degree = {}], targets = {}", v, - T::DataProvider::MAXIMUM_VOTES_PER_VOTER, + T::DataProvider::MaxVotesPerVoter::get(), t ); @@ -165,8 +165,8 @@ fn set_up_data_provider(v: u32, t: u32) { }) .collect::>(); // we should always have enough voters to fill. - assert!(targets.len() > T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize); - targets.truncate(T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize); + assert!(targets.len() > T::DataProvider::MaxVotesPerVoter::get() as usize); + targets.truncate(T::DataProvider::MaxVotesPerVoter::get() as usize); // fill voters. (0..v).for_each(|i| { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 8087d22cffe69..9433862fdaafb 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -820,7 +820,7 @@ pub mod pallet { // NOTE that this pallet does not really need to enforce this in runtime. The // solution cannot represent any voters more than `LIMIT` anyhow. assert_eq!( - ::MAXIMUM_VOTES_PER_VOTER, + ::MaxVotesPerVoter::get(), as NposSolution>::LIMIT as u32, ); } diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 409ebd3f1e10d..6c4088507ad1d 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -436,6 +436,9 @@ where pub type Extrinsic = sp_runtime::testing::TestXt; +parameter_types! { + pub MaxNominations: u32 = ::LIMIT as u32 +} #[derive(Default)] pub struct ExtBuilder {} @@ -443,7 +446,7 @@ pub struct StakingMock; impl ElectionDataProvider for StakingMock { type AccountId = AccountId; type BlockNumber = u64; - const MAXIMUM_VOTES_PER_VOTER: u32 = ::LIMIT as u32; + type MaxVotesPerVoter = MaxNominations; fn targets(maybe_max_len: Option) -> data_provider::Result> { let targets = Targets::get(); diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index d10504c88cc67..654a80496dbea 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -80,6 +80,7 @@ //! ```rust //! # use frame_election_provider_support::{*, data_provider}; //! # use sp_npos_elections::{Support, Assignment}; +//! # use frame_support::traits::ConstU32; //! //! type AccountId = u64; //! type Balance = u64; @@ -101,7 +102,7 @@ //! impl ElectionDataProvider for Pallet { //! type AccountId = AccountId; //! type BlockNumber = BlockNumber; -//! const MAXIMUM_VOTES_PER_VOTER: u32 = 1; +//! type MaxVotesPerVoter = ConstU32<1>; //! //! fn desired_targets() -> data_provider::Result { //! Ok(1) @@ -191,7 +192,7 @@ pub trait ElectionDataProvider { type BlockNumber; /// Maximum number of votes per voter that this data provider is providing. - const MAXIMUM_VOTES_PER_VOTER: u32; + type MaxVotesPerVoter: Get; /// All possible targets for the election, i.e. the candidates. /// @@ -266,8 +267,7 @@ pub struct TestDataProvider(sp_std::marker::PhantomData); impl ElectionDataProvider for TestDataProvider<(AccountId, BlockNumber)> { type AccountId = AccountId; type BlockNumber = BlockNumber; - - const MAXIMUM_VOTES_PER_VOTER: u32 = 0; + type MaxVotesPerVoter = (); fn targets(_maybe_max_len: Option) -> data_provider::Result> { Ok(Default::default()) } diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index d325daf514757..4c812c13c06be 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -161,6 +161,8 @@ mod tests { type OnChainPhragmen = OnChainSequentialPhragmen; mod mock_data_provider { + use frame_support::traits::ConstU32; + use super::*; use crate::data_provider; @@ -168,7 +170,7 @@ mod tests { impl ElectionDataProvider for DataProvider { type AccountId = AccountId; type BlockNumber = BlockNumber; - const MAXIMUM_VOTES_PER_VOTER: u32 = 2; + type MaxVotesPerVoter = ConstU32<2>; fn voters( _: Option, ) -> data_provider::Result)>> { diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index ddfae3d7ea75b..c389a5fb5b880 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -187,7 +187,7 @@ impl onchain::Config for Test { } impl pallet_staking::Config for Test { - const MAX_NOMINATIONS: u32 = 16; + type MaxNominations = ConstU32<16>; type RewardRemainder = (); type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = Event; diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index 4d6a182034c9e..8a594c8aef525 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -275,7 +275,7 @@ benchmarks! { let r in 1 .. MAX_REPORTERS; // we skip 1 offender, because in such case there is no slashing let o in 2 .. MAX_OFFENDERS; - let n in 0 .. MAX_NOMINATORS.min(::MAX_NOMINATIONS); + let n in 0 .. MAX_NOMINATORS.min(::MaxNominations::get()); // Make r reporters let mut reporters = vec![]; @@ -381,7 +381,7 @@ benchmarks! { } report_offence_grandpa { - let n in 0 .. MAX_NOMINATORS.min(::MAX_NOMINATIONS); + let n in 0 .. MAX_NOMINATORS.min(::MaxNominations::get()); // for grandpa equivocation reports the number of reporters // and offenders is always 1 @@ -416,7 +416,7 @@ benchmarks! { } report_offence_babe { - let n in 0 .. MAX_NOMINATORS.min(::MAX_NOMINATIONS); + let n in 0 .. MAX_NOMINATORS.min(::MaxNominations::get()); // for babe equivocation reports the number of reporters // and offenders is always 1 diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 598ba650dfffb..3b5e640867c5a 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -156,7 +156,7 @@ impl onchain::Config for Test { } impl pallet_staking::Config for Test { - const MAX_NOMINATIONS: u32 = 16; + type MaxNominations = ConstU32<16>; type Currency = Balances; type UnixTime = pallet_timestamp::Pallet; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; diff --git a/frame/session/benchmarking/src/lib.rs b/frame/session/benchmarking/src/lib.rs index 7beb4631e0518..5cadcf194a027 100644 --- a/frame/session/benchmarking/src/lib.rs +++ b/frame/session/benchmarking/src/lib.rs @@ -53,10 +53,10 @@ impl OnInitialize for Pallet { benchmarks! { set_keys { - let n = ::MAX_NOMINATIONS; + let n = ::MaxNominations::get(); let (v_stash, _) = create_validator_with_nominators::( n, - ::MAX_NOMINATIONS, + ::MaxNominations::get(), false, RewardDestination::Staked, )?; @@ -70,10 +70,10 @@ benchmarks! { }: _(RawOrigin::Signed(v_controller), keys, proof) purge_keys { - let n = ::MAX_NOMINATIONS; + let n = ::MaxNominations::get(); let (v_stash, _) = create_validator_with_nominators::( n, - ::MAX_NOMINATIONS, + ::MaxNominations::get(), false, RewardDestination::Staked )?; diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 1f00236605130..37305437ca095 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -162,7 +162,7 @@ impl onchain::Config for Test { } impl pallet_staking::Config for Test { - const MAX_NOMINATIONS: u32 = 16; + type MaxNominations = ConstU32<16>; type Currency = Balances; type UnixTime = pallet_timestamp::Pallet; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 8328adc00a978..564172d912413 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -355,17 +355,17 @@ benchmarks! { kick { // scenario: we want to kick `k` nominators from nominating us (we are a validator). // we'll assume that `k` is under 128 for the purposes of determining the slope. - // each nominator should have `T::MAX_NOMINATIONS` validators nominated, and our validator + // each nominator should have `T::MaxNominations::get()` validators nominated, and our validator // should be somewhere in there. let k in 1 .. 128; - // these are the other validators; there are `T::MAX_NOMINATIONS - 1` of them, so - // there are a total of `T::MAX_NOMINATIONS` validators in the system. - let rest_of_validators = create_validators_with_seed::(T::MAX_NOMINATIONS - 1, 100, 415)?; + // these are the other validators; there are `T::MaxNominations::get() - 1` of them, so + // there are a total of `T::MaxNominations::get()` validators in the system. + let rest_of_validators = create_validators_with_seed::(T::MaxNominations::get() - 1, 100, 415)?; // this is the validator that will be kicking. let (stash, controller) = create_stash_controller::( - T::MAX_NOMINATIONS - 1, + T::MaxNominations::get() - 1, 100, Default::default(), )?; @@ -380,7 +380,7 @@ benchmarks! { for i in 0 .. k { // create a nominator stash. let (n_stash, n_controller) = create_stash_controller::( - T::MAX_NOMINATIONS + i, + T::MaxNominations::get() + i, 100, Default::default(), )?; @@ -415,9 +415,9 @@ benchmarks! { } } - // Worst case scenario, T::MAX_NOMINATIONS + // Worst case scenario, T::MaxNominations::get() nominate { - let n in 1 .. T::MAX_NOMINATIONS; + let n in 1 .. T::MaxNominations::get(); // clean up any existing state. clear_validators_and_nominators::(); @@ -428,7 +428,7 @@ benchmarks! { // we are just doing an insert into the origin position. let scenario = ListScenario::::new(origin_weight, true)?; let (stash, controller) = create_stash_controller_with_balance::( - SEED + T::MAX_NOMINATIONS + 1, // make sure the account does not conflict with others + SEED + T::MaxNominations::get() + 1, // make sure the account does not conflict with others origin_weight, Default::default(), ).unwrap(); @@ -724,7 +724,7 @@ benchmarks! { create_validators_with_nominators_for_era::( v, n, - ::MAX_NOMINATIONS as usize, + ::MaxNominations::get() as usize, false, None, )?; @@ -742,7 +742,7 @@ benchmarks! { create_validators_with_nominators_for_era::( v, n, - ::MAX_NOMINATIONS as usize, + ::MaxNominations::get() as usize, false, None, )?; @@ -822,7 +822,7 @@ benchmarks! { let s in 1 .. 20; let validators = create_validators_with_nominators_for_era::( - v, n, T::MAX_NOMINATIONS as usize, false, None + v, n, T::MaxNominations::get() as usize, false, None )? .into_iter() .map(|v| T::Lookup::lookup(v).unwrap()) @@ -845,7 +845,7 @@ benchmarks! { let n = MaxNominators::::get(); let _ = create_validators_with_nominators_for_era::( - v, n, T::MAX_NOMINATIONS as usize, false, None + v, n, T::MaxNominations::get() as usize, false, None )?; }: { let targets = >::get_npos_targets(); @@ -923,7 +923,7 @@ mod tests { create_validators_with_nominators_for_era::( v, n, - ::MAX_NOMINATIONS as usize, + ::MaxNominations::get() as usize, false, None, ) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 268618fb5f44f..0bce31cbbe61b 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -303,6 +303,7 @@ use codec::{Decode, Encode, HasCompact}; use frame_support::{ traits::{ConstU32, Currency, Get}, weights::Weight, + BoundedVec, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, }; use scale_info::TypeInfo; use sp_runtime::{ @@ -577,10 +578,12 @@ where } /// A record of the nominations made by a specific account. -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo)] -pub struct Nominations { +#[derive(PartialEqNoBound, EqNoBound, Clone, Encode, Decode, RuntimeDebugNoBound, TypeInfo)] +#[codec(mel_bound(T: Config))] +#[scale_info(skip_type_params(T))] +pub struct Nominations { /// The targets of nomination. - pub targets: Vec, + pub targets: BoundedVec, /// The era the nominations were submitted. /// /// Except for initial nominations which are considered submitted at era 0. diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3bf46588044a6..95f305dfdd22a 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -234,6 +234,7 @@ const THRESHOLDS: [sp_npos_elections::VoteWeight; 9] = parameter_types! { pub static BagThresholds: &'static [sp_npos_elections::VoteWeight] = &THRESHOLDS; + pub static MaxNominations: u32 = 16; } impl pallet_bags_list::Config for Test { @@ -249,7 +250,7 @@ impl onchain::Config for Test { } impl crate::pallet::pallet::Config for Test { - const MAX_NOMINATIONS: u32 = 16; + type MaxNominations = MaxNominations; type Currency = Balances; type UnixTime = Timestamp; type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; @@ -533,7 +534,7 @@ fn post_conditions() { } fn check_count() { - let nominator_count = Nominators::::iter().count() as u32; + let nominator_count = Nominators::::iter_keys().count() as u32; let validator_count = Validators::::iter().count() as u32; assert_eq!(nominator_count, Nominators::::count()); assert_eq!(validator_count, Validators::::count()); diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index c97541de81961..541ee80588a6b 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -720,11 +720,15 @@ impl Pallet { .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(( + nominator.clone(), + weight_of(&nominator), + targets.into_inner(), + )); nominators_taken.saturating_inc(); } } else { - log!(error, "DEFENSIVE: invalid item in `SortedListProvider`: {:?}", nominator) + log!(warn, "DEFENSIVE: invalid item in `SortedListProvider`: {:?}, this nominator probably has too many nominations now", nominator) } } @@ -772,7 +776,7 @@ impl Pallet { /// NOTE: you must ALWAYS use this function to add nominator or update their targets. Any access /// to `Nominators` or `VoterList` outside of this function is almost certainly /// wrong. - pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations) { + 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. if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() { @@ -847,7 +851,7 @@ impl Pallet { impl ElectionDataProvider for Pallet { type AccountId = T::AccountId; type BlockNumber = BlockNumberFor; - const MAXIMUM_VOTES_PER_VOTER: u32 = T::MAX_NOMINATIONS; + type MaxVotesPerVoter = T::MaxNominations; fn desired_targets() -> data_provider::Result { Self::register_weight(T::DbWeight::get().reads(1)); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index e45a21ab25036..dcd3cc8dd53a7 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -93,7 +93,7 @@ pub mod pallet { >; /// Maximum number of nominations per nominator. - const MAX_NOMINATIONS: u32; + type MaxNominations: Get; /// Tokens have been minted and are unused for validator-reward. /// See [Era payout](./index.html#era-payout). @@ -160,15 +160,6 @@ pub mod pallet { type WeightInfo: WeightInfo; } - #[pallet::extra_constants] - impl Pallet { - // TODO: rename to snake case after https://github.com/paritytech/substrate/issues/8826 fixed. - #[allow(non_snake_case)] - fn MaxNominations() -> u32 { - T::MAX_NOMINATIONS - } - } - #[pallet::type_value] pub(crate) fn HistoryDepthOnEmpty() -> u32 { 84u32 @@ -249,7 +240,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn nominators)] pub type Nominators = - CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations>; + CountedStorageMap<_, Twox64Concat, T::AccountId, Nominations>; /// The maximum nominator count before we stop allowing new validators to join. /// @@ -994,7 +985,7 @@ pub mod pallet { /// /// # /// - The transaction's complexity is proportional to the size of `targets` (N) - /// which is capped at CompactAssignments::LIMIT (MAX_NOMINATIONS). + /// which is capped at CompactAssignments::LIMIT (T::MaxNominations). /// - Both the reads and writes follow a similar pattern. /// # #[pallet::weight(T::WeightInfo::nominate(targets.len() as u32))] @@ -1022,9 +1013,9 @@ pub mod pallet { } ensure!(!targets.is_empty(), Error::::EmptyTargets); - ensure!(targets.len() <= T::MAX_NOMINATIONS as usize, Error::::TooManyTargets); + ensure!(targets.len() <= T::MaxNominations::get() as usize, Error::::TooManyTargets); - let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets); + let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets.into_inner()); let targets = targets .into_iter() @@ -1040,6 +1031,11 @@ pub mod pallet { }) .collect::, _>>()?; + // TODO: might be able to simplify it with + // https://github.com/paritytech/substrate/pull/10590, directly collect into a bounded + // vec after altering the vec one last time. + let targets = targets.try_into().expect("bound checked in previous line; qed"); + let nominations = Nominations { targets, // Initial nominations are considered submitted at era 0. See `Nominations` doc diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 8d465c8c93dc4..57c072d91ac16 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4248,7 +4248,11 @@ fn count_check_works() { Validators::::insert(987654321, ValidatorPrefs::default()); Nominators::::insert( 987654321, - Nominations { targets: vec![], submitted_in: Default::default(), suppressed: false }, + Nominations { + targets: Default::default(), + submitted_in: Default::default(), + suppressed: false, + }, ); }) } @@ -4589,6 +4593,93 @@ fn min_commission_works() { }) } +#[test] +fn change_of_max_nominations() { + use frame_election_provider_support::ElectionDataProvider; + ExtBuilder::default() + .add_staker(60, 61, 10, StakerStatus::Nominator(vec![1])) + .add_staker(70, 71, 10, StakerStatus::Nominator(vec![1, 2, 3])) + .balance_factor(10) + .build_and_execute(|| { + // pre-condition + assert_eq!(MaxNominations::get(), 16); + + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(70, 3), (101, 2), (60, 1)] + ); + // 3 validators and 3 nominators + assert_eq!(Staking::voters(None).unwrap().len(), 3 + 3); + + // abrupt change from 16 to 4, everyone should be fine. + MaxNominations::set(4); + + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(70, 3), (101, 2), (60, 1)] + ); + assert_eq!(Staking::voters(None).unwrap().len(), 3 + 3); + + // abrupt change from 4 to 3, everyone should be fine. + MaxNominations::set(3); + + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(70, 3), (101, 2), (60, 1)] + ); + assert_eq!(Staking::voters(None).unwrap().len(), 3 + 3); + + // abrupt change from 3 to 2, this should cause some nominators to be non-decodable, and + // thus non-existent unless if they update. + MaxNominations::set(2); + + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(101, 2), (60, 1)] + ); + // 70 is still in storage.. + assert!(Nominators::::contains_key(70)); + // but its value cannot be decoded and default is returned. + assert!(Nominators::::get(70).is_none()); + + assert_eq!(Staking::voters(None).unwrap().len(), 3 + 2); + assert!(Nominators::::contains_key(101)); + + // abrupt change from 2 to 1, this should cause some nominators to be non-decodable, and + // thus non-existent unless if they update. + MaxNominations::set(1); + + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(60, 1)] + ); + assert!(Nominators::::contains_key(70)); + assert!(Nominators::::contains_key(60)); + assert!(Nominators::::get(70).is_none()); + assert!(Nominators::::get(60).is_some()); + assert_eq!(Staking::voters(None).unwrap().len(), 3 + 1); + + // now one of them can revive themselves by re-nominating to a proper value + assert_ok!(Staking::nominate(Origin::signed(71), vec![1])); + assert_eq!( + Nominators::::iter() + .map(|(k, n)| (k, n.targets.len())) + .collect::>(), + vec![(70, 1), (60, 1)] + ); + }) +} + mod sorted_list_provider { use super::*; use frame_election_provider_support::SortedListProvider; diff --git a/frame/support/src/storage/types/counted_map.rs b/frame/support/src/storage/types/counted_map.rs index 99d645fba3298..258a3cbeff127 100644 --- a/frame/support/src/storage/types/counted_map.rs +++ b/frame/support/src/storage/types/counted_map.rs @@ -429,6 +429,13 @@ where phantom: Default::default(), } } + + /// Enumerate all keys in the counted map. + /// + /// If you alter the map while doing this, you'll get undefined results. + pub fn iter_keys() -> crate::storage::KeyPrefixIterator { + ::Map::iter_keys() + } } impl StorageEntryMetadataBuilder diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index cec45a8aa1cb1..513480a77ca3d 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -478,11 +478,6 @@ pub mod pallet { } /// Make some on-chain remark and emit event. - /// - /// # - /// - `O(b)` where b is the length of the remark. - /// - 1 event. - /// # #[pallet::weight(T::SystemWeightInfo::remark_with_event(remark.len() as u32))] pub fn remark_with_event( origin: OriginFor, From fb60d305961303bde327a8e6ec67e22445f30498 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 12:26:30 +0000 Subject: [PATCH 02/21] add docs and tweak chill_other for cleanup purposes --- .../election-provider-multi-phase/src/mock.rs | 1 + frame/election-provider-support/src/lib.rs | 4 ++ frame/staking/src/pallet/mod.rs | 55 ++++++++++--------- frame/staking/src/tests.rs | 9 ++- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 6c4088507ad1d..3b5c71be52eee 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -439,6 +439,7 @@ pub type Extrinsic = sp_runtime::testing::TestXt; parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32 } + #[derive(Default)] pub struct ExtBuilder {} diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 654a80496dbea..1f9bf3c6e2904 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -268,17 +268,21 @@ impl ElectionDataProvider for TestDataProvider<(AccountI type AccountId = AccountId; type BlockNumber = BlockNumber; type MaxVotesPerVoter = (); + fn targets(_maybe_max_len: Option) -> data_provider::Result> { Ok(Default::default()) } + fn voters( _maybe_max_len: Option, ) -> data_provider::Result)>> { Ok(Default::default()) } + fn desired_targets() -> data_provider::Result { Ok(Default::default()) } + fn next_election_prediction(now: BlockNumber) -> BlockNumber { now } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index dcd3cc8dd53a7..1ef44e942706e 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -236,7 +236,19 @@ pub mod pallet { #[pallet::storage] pub type MaxValidatorsCount = StorageValue<_, u32, OptionQuery>; - /// The map from nominator stash key to the set of stash keys of all validators to nominate. + /// The map from nominator stash key to their nomination preferences, namely the validators that + /// they wish to support. + /// + /// Note that the keys of this storage map might become non-decodable in case the + /// [`Config::MaxNominations`] configuration is decreased. In this rare case, these nominators + /// are still existent in storage, their key is correct and retrievable (i.e. `contains_key` + /// indicates that they exist), but their value cannot be decoded. Therefore, the non-decodable + /// nominators will effectively not-exist, until they re-submit their preferences such that it + /// is within the bounds of the newly set `Config::MaxNominations`. + /// + /// This implies that `::iter_keys().count()` and `::iter().count()` might return different + /// values for this map. Moreover, the main `::count()` is aligned with the later, namely the + /// number of keys that exist. #[pallet::storage] #[pallet::getter(fn nominators)] pub type Nominators = @@ -1228,11 +1240,6 @@ pub mod pallet { /// Set the validators who cannot be slashed (if any). /// /// The dispatch origin must be Root. - /// - /// # - /// - O(V) - /// - Write: Invulnerables - /// # #[pallet::weight(T::WeightInfo::set_invulnerables(invulnerables.len() as u32))] pub fn set_invulnerables( origin: OriginFor, @@ -1246,13 +1253,6 @@ pub mod pallet { /// Force a current staker to become completely unstaked, immediately. /// /// The dispatch origin must be Root. - /// - /// # - /// O(S) where S is the number of slashing spans to be removed - /// Reads: Bonded, Slashing Spans, Account, Locks - /// Writes: Bonded, Slashing Spans (if S > 0), Ledger, Payee, Validators, Nominators, - /// Account, Locks Writes Each: SpanSlash * S - /// # #[pallet::weight(T::WeightInfo::force_unstake(*num_slashing_spans))] pub fn force_unstake( origin: OriginFor, @@ -1278,11 +1278,6 @@ pub mod pallet { /// The election process starts multiple blocks before the end of the era. /// If this is called just before a new era is triggered, the election process may not /// have enough blocks to get a result. - /// - /// # - /// - Weight: O(1) - /// - Write: ForceEra - /// # #[pallet::weight(T::WeightInfo::force_new_era_always())] pub fn force_new_era_always(origin: OriginFor) -> DispatchResult { ensure_root(origin)?; @@ -1295,14 +1290,6 @@ pub mod pallet { /// Can be called by the `T::SlashCancelOrigin`. /// /// Parameters: era and indices of the slashes for that era to kill. - /// - /// # - /// Complexity: O(U + S) - /// with U unapplied slashes weighted with U=1000 - /// and S is the number of slash indices to be canceled. - /// - Read: Unapplied Slashes - /// - Write: Unapplied Slashes - /// # #[pallet::weight(T::WeightInfo::cancel_deferred_slash(slash_indices.len() as u32))] pub fn cancel_deferred_slash( origin: OriginFor, @@ -1562,6 +1549,11 @@ pub mod pallet { /// /// If the caller is different than the controller being targeted, the following conditions /// must be met: + /// + /// * `controller` must belong to a nominator who has become non-decodable, + /// + /// Or: + /// /// * A `ChillThreshold` must be set and checked which defines how close to the max /// nominators or validators we must reach before users can start chilling one-another. /// * A `MaxNominatorCount` and `MaxValidatorCount` must be set which is used to determine @@ -1580,6 +1572,11 @@ pub mod pallet { let stash = ledger.stash; // In order for one user to chill another user, the following conditions must be met: + // + // * `controller` belongs to a nominator who has become non-decodable, + // + // Or + // // * A `ChillThreshold` is set which defines how close to the max nominators or // validators we must reach before users can start chilling one-another. // * A `MaxNominatorCount` and `MaxValidatorCount` which is used to determine how close @@ -1589,6 +1586,12 @@ pub mod pallet { // threshold bond required. // // Otherwise, if caller is the same as the controller, this is just like `chill`. + + if Nominators::::contains_key(&stash) || Nominators::::get(&stash).is_none() { + Self::chill_stash(&stash); + return Ok(()) + } + if caller != controller { let threshold = ChillThreshold::::get().ok_or(Error::::CannotChillOther)?; let min_active_bond = if Nominators::::contains_key(&stash) { diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 57c072d91ac16..538b75ead340b 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -4669,7 +4669,7 @@ fn change_of_max_nominations() { assert!(Nominators::::get(60).is_some()); assert_eq!(Staking::voters(None).unwrap().len(), 3 + 1); - // now one of them can revive themselves by re-nominating to a proper value + // now one of them can revive themselves by re-nominating to a proper value. assert_ok!(Staking::nominate(Origin::signed(71), vec![1])); assert_eq!( Nominators::::iter() @@ -4677,6 +4677,13 @@ fn change_of_max_nominations() { .collect::>(), vec![(70, 1), (60, 1)] ); + + // or they can be chilled by any account. + assert!(Nominators::::contains_key(101)); + assert!(Nominators::::get(101).is_none()); + assert_ok!(Staking::chill_other(Origin::signed(70), 100)); + assert!(!Nominators::::contains_key(101)); + assert!(Nominators::::get(101).is_none()); }) } From b96f3af109cba1692be9abcc85175c49c4e8d445 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 12:39:44 +0000 Subject: [PATCH 03/21] Fix the build --- frame/election-provider-multi-phase/src/benchmarking.rs | 8 +++++--- frame/election-provider-multi-phase/src/mock.rs | 2 +- frame/offences/benchmarking/src/lib.rs | 2 +- frame/session/benchmarking/src/lib.rs | 2 +- frame/staking/src/pallet/impls.rs | 4 +++- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index c405afa383222..036af70808243 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -152,7 +152,7 @@ fn set_up_data_provider(v: u32, t: u32) { info, "setting up with voters = {} [degree = {}], targets = {}", v, - T::DataProvider::MaxVotesPerVoter::get(), + ::MaxVotesPerVoter::get(), t ); @@ -165,8 +165,10 @@ fn set_up_data_provider(v: u32, t: u32) { }) .collect::>(); // we should always have enough voters to fill. - assert!(targets.len() > T::DataProvider::MaxVotesPerVoter::get() as usize); - targets.truncate(T::DataProvider::MaxVotesPerVoter::get() as usize); + assert!( + targets.len() > ::MaxVotesPerVoter::get() as usize + ); + targets.truncate(::MaxVotesPerVoter::get() as usize); // fill voters. (0..v).for_each(|i| { diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 3b5c71be52eee..e2258af88d4de 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -437,7 +437,7 @@ where pub type Extrinsic = sp_runtime::testing::TestXt; parameter_types! { - pub MaxNominations: u32 = ::LIMIT as u32 + pub MaxNominations: u32 = ::LIMIT as u32; } #[derive(Default)] diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index 8a594c8aef525..c2e29f0d0fdd3 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -24,7 +24,7 @@ mod mock; use sp_std::{prelude::*, vec}; use frame_benchmarking::{account, benchmarks}; -use frame_support::traits::{Currency, ValidatorSet, ValidatorSetWithIdentification}; +use frame_support::traits::{Currency, Get, ValidatorSet, ValidatorSetWithIdentification}; use frame_system::{Config as SystemConfig, Pallet as System, RawOrigin}; use sp_runtime::{ diff --git a/frame/session/benchmarking/src/lib.rs b/frame/session/benchmarking/src/lib.rs index 5cadcf194a027..265c35cbe4908 100644 --- a/frame/session/benchmarking/src/lib.rs +++ b/frame/session/benchmarking/src/lib.rs @@ -28,7 +28,7 @@ use sp_std::{prelude::*, vec}; use frame_benchmarking::benchmarks; use frame_support::{ codec::Decode, - traits::{KeyOwnerProofSystem, OnInitialize}, + traits::{Get, KeyOwnerProofSystem, OnInitialize}, }; use frame_system::RawOrigin; use pallet_session::{historical::Pallet as Historical, Pallet as Session, *}; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 541ee80588a6b..923ed75728c90 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -917,6 +917,7 @@ impl ElectionDataProvider for Pallet { let stake = >::try_from(weight).unwrap_or_else(|_| { panic!("cannot convert a VoteWeight into BalanceOf, benchmark needs reconfiguring.") }); + let targets = targets.try_into().unwrap(); >::insert(voter.clone(), voter.clone()); >::insert( voter.clone(), @@ -928,6 +929,7 @@ impl ElectionDataProvider for Pallet { claimed_rewards: vec![], }, ); + Self::do_add_nominator(&voter, Nominations { targets, submitted_in: 0, suppressed: false }); } @@ -1005,7 +1007,7 @@ impl ElectionDataProvider for Pallet { ); Self::do_add_nominator( &v, - Nominations { targets: t, submitted_in: 0, suppressed: false }, + Nominations { targets: t.try_into().unwrap(), submitted_in: 0, suppressed: false }, ); }); } From 1b8376db1e8fffe13bd0012a32c3a53279f9ac33 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 12:41:43 +0000 Subject: [PATCH 04/21] remove TODO --- frame/staking/src/pallet/mod.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 1ef44e942706e..8416e52807ea3 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1041,12 +1041,9 @@ pub mod pallet { } }) }) - .collect::, _>>()?; - - // TODO: might be able to simplify it with - // https://github.com/paritytech/substrate/pull/10590, directly collect into a bounded - // vec after altering the vec one last time. - let targets = targets.try_into().expect("bound checked in previous line; qed"); + .collect::, _>>()? + .try_into() + .expect("bound checked in previous line; qed"); let nominations = Nominations { targets, From b93f109d369e0cd4177bcdc14f583e3ee2aba4b5 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 13:03:45 +0000 Subject: [PATCH 05/21] add a bit more doc --- frame/staking/src/pallet/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 8416e52807ea3..6004dd9250fec 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -249,6 +249,9 @@ pub mod pallet { /// This implies that `::iter_keys().count()` and `::iter().count()` might return different /// values for this map. Moreover, the main `::count()` is aligned with the later, namely the /// number of keys that exist. + /// + /// Lastly, if any of the nominators become non-decodable, they can be chilled immediately via + /// [`Call::chill_other`] dispatchable by anyone. #[pallet::storage] #[pallet::getter(fn nominators)] pub type Nominators = From d3cb9308bc46e21ffe18389ec18efb03b00e436d Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 6 Jan 2022 13:28:10 +0000 Subject: [PATCH 06/21] even more docs gushc --- frame/staking/src/pallet/impls.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 923ed75728c90..a993ab0d4b48e 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -728,6 +728,11 @@ impl Pallet { nominators_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) } } From b55f9e2f48da578c5955b591aaff8d1a73c6d1cf Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Fri, 7 Jan 2022 07:52:42 +0100 Subject: [PATCH 07/21] Update frame/staking/src/pallet/mod.rs Co-authored-by: Zeke Mostov --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 6004dd9250fec..8e8dd63a881a9 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1046,7 +1046,7 @@ pub mod pallet { }) .collect::, _>>()? .try_into() - .expect("bound checked in previous line; qed"); + .expect("bound checked above; qed"); let nominations = Nominations { targets, From 85ff60f4fb459ae7e52496a7d312d5b30453f645 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Fri, 7 Jan 2022 07:56:20 +0100 Subject: [PATCH 08/21] Update frame/staking/src/pallet/mod.rs Co-authored-by: Zeke Mostov --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 8e8dd63a881a9..734cb3e610451 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -247,7 +247,7 @@ pub mod pallet { /// is within the bounds of the newly set `Config::MaxNominations`. /// /// This implies that `::iter_keys().count()` and `::iter().count()` might return different - /// values for this map. Moreover, the main `::count()` is aligned with the later, namely the + /// values for this map. Moreover, the main `::count()` is aligned with the former, namely the /// number of keys that exist. /// /// Lastly, if any of the nominators become non-decodable, they can be chilled immediately via From ef51ec4de7ffc84faa14afb74e1ff5f4eca2f846 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 7 Jan 2022 09:49:16 +0000 Subject: [PATCH 09/21] Fix the nasty bug --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 6004dd9250fec..ad418e4a584e9 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1587,7 +1587,7 @@ pub mod pallet { // // Otherwise, if caller is the same as the controller, this is just like `chill`. - if Nominators::::contains_key(&stash) || Nominators::::get(&stash).is_none() { + if Nominators::::contains_key(&stash) && Nominators::::get(&stash).is_none() { Self::chill_stash(&stash); return Ok(()) } From 477079c722b46b9f615e2ed96c30ea103a2a1c9c Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 7 Jan 2022 14:25:24 +0000 Subject: [PATCH 10/21] also bound the Snapshot type --- .../src/benchmarking.rs | 17 ++++--- .../src/helpers.rs | 12 ++--- .../election-provider-multi-phase/src/lib.rs | 36 ++++++++++--- .../election-provider-multi-phase/src/mock.rs | 50 +++++++++++-------- .../src/unsigned.rs | 18 +++---- frame/election-provider-support/src/lib.rs | 40 +++++++++++---- .../election-provider-support/src/onchain.rs | 26 ++++++---- frame/staking/src/pallet/impls.rs | 33 ++++++------ frame/staking/src/pallet/mod.rs | 10 ++++ frame/support/src/lib.rs | 7 +++ 10 files changed, 164 insertions(+), 85 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 036af70808243..3bea7c9541ae6 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -20,7 +20,10 @@ use super::*; use crate::{unsigned::IndexAssignmentOf, Pallet as MultiPhase}; use frame_benchmarking::account; -use frame_support::{assert_ok, traits::Hooks}; +use frame_support::{ + assert_ok, + traits::{Hooks, TryCollect}, +}; use frame_system::RawOrigin; use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng}; use sp_arithmetic::{per_things::Percent, traits::One}; @@ -69,11 +72,12 @@ fn solution_with_size( let active_voters = (0..active_voters_count) .map(|i| { // chose a random subset of winners. - let winner_votes = winners + let winner_votes: BoundedVec<_, _> = winners .as_slice() .choose_multiple(&mut rng, >::LIMIT) .cloned() - .collect::>(); + .try_collect() + .expect(">::LIMIT is the correct bound; qed."); let voter = frame_benchmarking::account::("Voter", i, SEED); (voter, stake, winner_votes) }) @@ -87,10 +91,11 @@ fn solution_with_size( .collect::>(); let rest_voters = (active_voters_count..size.voters) .map(|i| { - let votes = (&non_winners) + let votes: BoundedVec<_, _> = (&non_winners) .choose_multiple(&mut rng, >::LIMIT) .cloned() - .collect::>(); + .try_collect() + .expect(">::LIMIT is the correct bound; qed."); let voter = frame_benchmarking::account::("Voter", i, SEED); (voter, stake, votes) }) @@ -174,7 +179,7 @@ fn set_up_data_provider(v: u32, t: u32) { (0..v).for_each(|i| { let voter = frame_benchmarking::account::("Voter", i, SEED); let weight = T::Currency::minimum_balance().saturated_into::() * 1000; - T::DataProvider::add_voter(voter, weight, targets.clone()); + T::DataProvider::add_voter(voter, weight, targets.clone().try_into().unwrap()); }); } diff --git a/frame/election-provider-multi-phase/src/helpers.rs b/frame/election-provider-multi-phase/src/helpers.rs index 9bd5b5dbabd25..48da194cc65d9 100644 --- a/frame/election-provider-multi-phase/src/helpers.rs +++ b/frame/election-provider-multi-phase/src/helpers.rs @@ -17,7 +17,7 @@ //! Some helper functions/macros for this crate. -use super::{Config, SolutionTargetIndexOf, SolutionVoterIndexOf, VoteWeight}; +use crate::{unsigned::VoterOf, Config, SolutionTargetIndexOf, SolutionVoterIndexOf, VoteWeight}; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; #[macro_export] @@ -34,7 +34,7 @@ macro_rules! log { /// /// This can be used to efficiently build index getter closures. pub fn generate_voter_cache( - snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, + snapshot: &Vec>, ) -> BTreeMap { let mut cache: BTreeMap = BTreeMap::new(); snapshot.iter().enumerate().for_each(|(i, (x, _, _))| { @@ -97,7 +97,7 @@ pub fn voter_index_fn_usize( /// Not meant to be used in production. #[cfg(test)] pub fn voter_index_fn_linear( - snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, + snapshot: &Vec>, ) -> impl Fn(&T::AccountId) -> Option> + '_ { move |who| { snapshot @@ -148,7 +148,7 @@ pub fn target_index_fn_linear( /// Create a function that can map a voter index ([`SolutionVoterIndexOf`]) to the actual voter /// account using a linearly indexible snapshot. pub fn voter_at_fn( - snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, + snapshot: &Vec>, ) -> impl Fn(SolutionVoterIndexOf) -> Option + '_ { move |i| { as TryInto>::try_into(i) @@ -174,7 +174,7 @@ pub fn target_at_fn( /// This is not optimized and uses a linear search. #[cfg(test)] pub fn stake_of_fn_linear( - snapshot: &Vec<(T::AccountId, VoteWeight, Vec)>, + snapshot: &Vec>, ) -> impl Fn(&T::AccountId) -> VoteWeight + '_ { move |who| { snapshot @@ -192,7 +192,7 @@ pub fn stake_of_fn_linear( /// The cache need must be derived from the same snapshot. Zero is returned if a voter is /// non-existent. pub fn stake_of_fn<'a, T: Config>( - snapshot: &'a Vec<(T::AccountId, VoteWeight, Vec)>, + snapshot: &'a Vec>, cache: &'a BTreeMap, ) -> impl Fn(&T::AccountId) -> VoteWeight + 'a { move |who| { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9433862fdaafb..345fd18c54f84 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -236,6 +236,7 @@ use frame_support::{ ensure, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, weights::{DispatchClass, Weight}, + BoundedVec, }; use frame_system::{ensure_none, offchain::SendTransactionTypes}; use scale_info::TypeInfo; @@ -269,6 +270,7 @@ const LOG_TARGET: &'static str = "runtime::election-provider"; pub mod signed; pub mod unsigned; pub mod weights; +use unsigned::VoterOf; pub use weights::WeightInfo; pub use signed::{ @@ -448,11 +450,17 @@ pub struct ReadySolution { /// /// These are stored together because they are often accessed together. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] -pub struct RoundSnapshot { +#[codec(mel_bound(T: Config))] +#[scale_info(skip_type_params(T))] +pub struct RoundSnapshot { /// All of the voters. - pub voters: Vec<(A, VoteWeight, Vec)>, + pub voters: Vec<( + T::AccountId, + VoteWeight, + BoundedVec::MaxVotesPerVoter>, + )>, /// All of the targets. - pub targets: Vec, + pub targets: Vec, } /// Encodes the length of a solution or a snapshot. @@ -1140,7 +1148,7 @@ pub mod pallet { /// This is created at the beginning of the signed phase and cleared upon calling `elect`. #[pallet::storage] #[pallet::getter(fn snapshot)] - pub type Snapshot = StorageValue<_, RoundSnapshot>; + pub type Snapshot = StorageValue<_, RoundSnapshot>; /// Desired number of targets to elect for this round. /// @@ -1209,6 +1217,20 @@ pub mod pallet { } impl Pallet { + /// Same as the getter of the [`Snapshot`] storage item, but it returns the voters converted + /// into an unbounded vector. + /// + /// Note that this incurs an extra allocation of the entire snapshot, unfortunately. + pub fn snapshot_unbounded( + ) -> Option<(Vec, Vec<(T::AccountId, VoteWeight, Vec)>)> { + use frame_election_provider_support::IntoUnboundedVoters; + Self::snapshot().map(|snapshot| { + let voters = snapshot.voters.into_unbounded_voters(); + let targets = snapshot.targets; + (targets, voters) + }) + } + /// Internal logic of the offchain worker, to be executed only when the offchain lock is /// acquired with success. fn do_synchronized_offchain_worker(now: T::BlockNumber) { @@ -1256,7 +1278,7 @@ impl Pallet { /// Extracted for easier weight calculation. fn create_snapshot_internal( targets: Vec, - voters: Vec>, + voters: Vec>, desired_targets: u32, ) { let metadata = @@ -1269,7 +1291,7 @@ impl Pallet { // instead of using storage APIs, we do a manual encoding into a fixed-size buffer. // `encoded_size` encodes it without storing it anywhere, this should not cause any // allocation. - let snapshot = RoundSnapshot { voters, targets }; + let snapshot = RoundSnapshot:: { voters, targets }; let size = snapshot.encoded_size(); log!(debug, "snapshot pre-calculated size {:?}", size); let mut buffer = Vec::with_capacity(size); @@ -1287,7 +1309,7 @@ impl Pallet { /// /// Extracted for easier weight calculation. fn create_snapshot_external( - ) -> Result<(Vec, Vec>, u32), ElectionError> { + ) -> Result<(Vec, Vec>, u32), ElectionError> { let target_limit = >::max_value().saturated_into::(); // for now we have just a single block snapshot. let voter_limit = T::VoterSnapshotPerBlock::get().saturated_into::(); diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index e2258af88d4de..087c473e54659 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -18,15 +18,16 @@ use super::*; use crate as multi_phase; use frame_election_provider_support::{ - data_provider, onchain, ElectionDataProvider, SequentialPhragmen, + data_provider, onchain, ElectionDataProvider, IntoUnboundedVoters, SequentialPhragmen, }; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ parameter_types, traits::{ConstU32, Hooks}, + try_bounded_vec, weights::Weight, }; -use multi_phase::unsigned::{IndexAssignmentOf, Voter}; +use multi_phase::unsigned::{IndexAssignmentOf, VoterOf}; use parking_lot::RwLock; use sp_core::{ offchain::{ @@ -100,7 +101,7 @@ pub fn roll_to_with_ocw(n: BlockNumber) { } pub struct TrimHelpers { - pub voters: Vec>, + pub voters: Vec>, pub assignments: Vec>, pub encoded_size_of: Box]) -> Result>, @@ -134,7 +135,7 @@ pub fn trim_helpers() -> TrimHelpers { let ElectionResult { mut assignments, .. } = seq_phragmen::<_, SolutionAccuracyOf>( desired_targets as usize, targets.clone(), - voters.clone(), + voters.clone().into_unbounded_voters(), None, ) .unwrap(); @@ -167,7 +168,7 @@ pub fn raw_solution() -> RawSolution> { seq_phragmen::<_, SolutionAccuracyOf>( desired_targets as usize, targets.clone(), - voters.clone(), + voters.clone().into_unbounded_voters(), None, ) .unwrap(); @@ -246,16 +247,16 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub static Targets: Vec = vec![10, 20, 30, 40]; - pub static Voters: Vec<(AccountId, VoteWeight, Vec)> = vec![ - (1, 10, vec![10, 20]), - (2, 10, vec![30, 40]), - (3, 10, vec![40]), - (4, 10, vec![10, 20, 30, 40]), + pub static Voters: Vec> = vec![ + (1, 10, try_bounded_vec![10, 20]), + (2, 10, try_bounded_vec![30, 40]), + (3, 10, try_bounded_vec![40]), + (4, 10, try_bounded_vec![10, 20, 30, 40]), // self votes. - (10, 10, vec![10]), - (20, 20, vec![20]), - (30, 30, vec![30]), - (40, 40, vec![40]), + (10, 10, try_bounded_vec![10]), + (20, 20, try_bounded_vec![20]), + (30, 30, try_bounded_vec![30]), + (40, 40, try_bounded_vec![40]), ]; pub static DesiredTargets: u32 = 2; @@ -458,9 +459,7 @@ impl ElectionDataProvider for StakingMock { Ok(targets) } - fn voters( - maybe_max_len: Option, - ) -> data_provider::Result)>> { + fn voters(maybe_max_len: Option) -> data_provider::Result>> { let mut voters = Voters::get(); if let Some(max_len) = maybe_max_len { voters.truncate(max_len) @@ -479,7 +478,7 @@ impl ElectionDataProvider for StakingMock { #[cfg(feature = "runtime-benchmarks")] fn put_snapshot( - voters: Vec<(AccountId, VoteWeight, Vec)>, + voters: Vec>, targets: Vec, _target_stake: Option, ) { @@ -494,7 +493,11 @@ impl ElectionDataProvider for StakingMock { } #[cfg(feature = "runtime-benchmarks")] - fn add_voter(voter: AccountId, weight: VoteWeight, targets: Vec) { + fn add_voter( + voter: AccountId, + weight: VoteWeight, + targets: BoundedVec, + ) { let mut current = Voters::get(); current.push((voter, weight, targets)); Voters::set(current); @@ -509,7 +512,7 @@ impl ElectionDataProvider for StakingMock { // to be on-par with staking, we add a self vote as well. the stake is really not that // important. let mut current = Voters::get(); - current.push((target, ExistentialDeposit::get() as u64, vec![target])); + current.push((target, ExistentialDeposit::get() as u64, try_bounded_vec![target])); Voters::set(current); } } @@ -544,7 +547,12 @@ impl ExtBuilder { ::set(t); self } - pub fn add_voter(self, who: AccountId, stake: Balance, targets: Vec) -> Self { + pub fn add_voter( + self, + who: AccountId, + stake: Balance, + targets: BoundedVec, + ) -> Self { VOTERS.with(|v| v.borrow_mut().push((who, stake, targets))); self } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index da56dd4d073df..82e7e50d8d6a8 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -47,11 +47,7 @@ pub(crate) const OFFCHAIN_CACHED_CALL: &[u8] = b"parity/multi-phase-unsigned-ele /// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they /// voted. -pub type Voter = ( - ::AccountId, - sp_npos_elections::VoteWeight, - Vec<::AccountId>, -); +pub type VoterOf = frame_election_provider_support::VoterOf<::DataProvider>; /// The relative distribution of a voter's stake among the winning targets. pub type Assignment = @@ -277,8 +273,8 @@ impl Pallet { where S: NposSolver>, { - let RoundSnapshot { voters, targets } = - Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; + let (targets, voters) = + Self::snapshot_unbounded().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; S::solve(desired_targets as usize, targets, voters) @@ -749,7 +745,9 @@ mod tests { }; use codec::Decode; use frame_benchmarking::Zero; - use frame_support::{assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker}; + use frame_support::{ + assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker, try_bounded_vec, + }; use sp_npos_elections::IndexAssignment; use sp_runtime::{ offchain::storage_lock::{BlockAndTime, StorageLock}, @@ -1048,8 +1046,8 @@ mod tests { fn unsigned_per_dispatch_checks_can_only_submit_threshold_better() { ExtBuilder::default() .desired_targets(1) - .add_voter(7, 2, vec![10]) - .add_voter(8, 5, vec![10]) + .add_voter(7, 2, try_bounded_vec![10]) + .add_voter(8, 5, try_bounded_vec![10]) .solution_improvement_threshold(Perbill::from_percent(50)) .build_and_execute(|| { roll_to(25); diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 1f9bf3c6e2904..0873c4ea26310 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -167,7 +167,7 @@ #![cfg_attr(not(feature = "std"), no_std)] pub mod onchain; -use frame_support::traits::Get; +use frame_support::{traits::Get, BoundedVec}; use sp_std::{fmt::Debug, prelude::*}; /// Re-export some type as they are used in the interface. @@ -212,9 +212,7 @@ pub trait ElectionDataProvider { /// /// This should be implemented as a self-weighing function. The implementor should register its /// appropriate weight at the end of execution with the system pallet directly. - fn voters( - maybe_max_len: Option, - ) -> data_provider::Result)>>; + fn voters(maybe_max_len: Option) -> data_provider::Result>>; /// The number of targets to elect. /// @@ -234,7 +232,7 @@ pub trait ElectionDataProvider { /// else a noop. #[cfg(any(feature = "runtime-benchmarks", test))] fn put_snapshot( - _voters: Vec<(Self::AccountId, VoteWeight, Vec)>, + _voters: Vec>, _targets: Vec, _target_stake: Option, ) { @@ -245,7 +243,12 @@ pub trait ElectionDataProvider { /// /// Same as `put_snapshot`, but can add a single voter one by one. #[cfg(any(feature = "runtime-benchmarks", test))] - fn add_voter(_voter: Self::AccountId, _weight: VoteWeight, _targets: Vec) {} + fn add_voter( + _voter: Self::AccountId, + _weight: VoteWeight, + _targets: BoundedVec, + ) { + } /// Utility function only to be used in benchmarking scenarios, to be implemented optionally, /// else a noop. @@ -273,9 +276,7 @@ impl ElectionDataProvider for TestDataProvider<(AccountI Ok(Default::default()) } - fn voters( - _maybe_max_len: Option, - ) -> data_provider::Result)>> { + fn voters(_maybe_max_len: Option) -> data_provider::Result>> { Ok(Default::default()) } @@ -476,3 +477,24 @@ impl< sp_npos_elections::phragmms(winners, targets, voters, Balancing::get()) } } + +/// A voter, at the level of abstraction of this crate. +pub type Voter = (AccountId, VoteWeight, BoundedVec); + +/// Same as [`Voter`], but parameterized by an [`ElectionDataProvider`]. +pub type VoterOf = + Voter<::AccountId, ::MaxVotesPerVoter>; + +/// Extension trait to easily convert from bounded voters to unbounded ones. +/// +/// Undesirably, this cannot be done (in safe rust) without re-allocating the entire vector, so +/// don't use it recklessly. +pub trait IntoUnboundedVoters { + fn into_unbounded_voters(self) -> Vec<(AccountId, VoteWeight, Vec)>; +} + +impl> IntoUnboundedVoters for Vec> { + fn into_unbounded_voters(self) -> Vec<(AccountId, VoteWeight, Vec)> { + self.into_iter().map(|(x, y, z)| (x, y, z.into_inner())).collect::>() + } +} diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 4c812c13c06be..c85cc2c759aad 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -17,7 +17,7 @@ //! An implementation of [`ElectionProvider`] that does an on-chain sequential phragmen. -use crate::{ElectionDataProvider, ElectionProvider}; +use crate::{ElectionDataProvider, ElectionProvider, IntoUnboundedVoters}; use frame_support::{traits::Get, weights::DispatchClass}; use sp_npos_elections::*; use sp_std::{collections::btree_map::BTreeMap, marker::PhantomData, prelude::*}; @@ -87,9 +87,13 @@ impl ElectionProvider for OnChainSequentialPhragmen { let stake_of = |w: &T::AccountId| -> VoteWeight { stake_map.get(w).cloned().unwrap_or_default() }; - let ElectionResult { winners: _, assignments } = - seq_phragmen::<_, T::Accuracy>(desired_targets as usize, targets, voters, None) - .map_err(Error::from)?; + let ElectionResult { winners: _, assignments } = seq_phragmen::<_, T::Accuracy>( + desired_targets as usize, + targets, + voters.into_unbounded_voters(), + None, + ) + .map_err(Error::from)?; let staked = assignment_ratio_to_staked_normalized(assignments, &stake_of)?; @@ -161,20 +165,22 @@ mod tests { type OnChainPhragmen = OnChainSequentialPhragmen; mod mock_data_provider { - use frame_support::traits::ConstU32; + use frame_support::{traits::ConstU32, try_bounded_vec}; use super::*; - use crate::data_provider; + use crate::{data_provider, VoterOf}; pub struct DataProvider; impl ElectionDataProvider for DataProvider { type AccountId = AccountId; type BlockNumber = BlockNumber; type MaxVotesPerVoter = ConstU32<2>; - fn voters( - _: Option, - ) -> data_provider::Result)>> { - Ok(vec![(1, 10, vec![10, 20]), (2, 20, vec![30, 20]), (3, 30, vec![10, 30])]) + fn voters(_: Option) -> data_provider::Result>> { + Ok(vec![ + (1, 10, try_bounded_vec![10, 20]), + (2, 20, try_bounded_vec![30, 20]), + (3, 30, try_bounded_vec![10, 30]), + ]) } fn targets(_: Option) -> data_provider::Result> { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index a993ab0d4b48e..24dd073f6021d 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -19,7 +19,7 @@ use frame_election_provider_support::{ data_provider, ElectionDataProvider, ElectionProvider, SortedListProvider, Supports, - VoteWeight, VoteWeightProvider, + VoteWeight, VoteWeightProvider, VoterOf, }; use frame_support::{ pallet_prelude::*, @@ -661,9 +661,7 @@ impl Pallet { /// /// 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`. - pub fn get_npos_voters( - maybe_max_len: Option, - ) -> Vec<(T::AccountId, VoteWeight, Vec)> { + 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; @@ -677,8 +675,13 @@ impl Pallet { 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()]); + 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(); } @@ -720,11 +723,7 @@ impl Pallet { .map_or(true, |spans| submitted_in >= spans.last_nonzero_slash()) }); if !targets.len().is_zero() { - all_voters.push(( - nominator.clone(), - weight_of(&nominator), - targets.into_inner(), - )); + all_voters.push((nominator.clone(), weight_of(&nominator), targets)); nominators_taken.saturating_inc(); } } else { @@ -863,9 +862,7 @@ impl ElectionDataProvider for Pallet { Ok(Self::validator_count()) } - fn voters( - maybe_max_len: Option, - ) -> data_provider::Result)>> { + fn voters(maybe_max_len: Option) -> data_provider::Result>> { // This can never fail -- if `maybe_max_len` is `Some(_)` we handle it. let voters = Self::get_npos_voters(maybe_max_len); debug_assert!(maybe_max_len.map_or(true, |max| voters.len() <= max)); @@ -918,7 +915,11 @@ impl ElectionDataProvider for Pallet { } #[cfg(feature = "runtime-benchmarks")] - fn add_voter(voter: T::AccountId, weight: VoteWeight, targets: Vec) { + fn add_voter( + voter: T::AccountId, + weight: VoteWeight, + targets: BoundedVec, + ) { let stake = >::try_from(weight).unwrap_or_else(|_| { panic!("cannot convert a VoteWeight into BalanceOf, benchmark needs reconfiguring.") }); @@ -970,7 +971,7 @@ impl ElectionDataProvider for Pallet { #[cfg(feature = "runtime-benchmarks")] fn put_snapshot( - voters: Vec<(T::AccountId, VoteWeight, Vec)>, + voters: Vec>, targets: Vec, target_stake: Option, ) { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 105fe7d1206a4..8264f0343b69c 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -50,6 +50,8 @@ const STAKING_ID: LockIdentifier = *b"staking "; #[frame_support::pallet] pub mod pallet { + use frame_election_provider_support::ElectionDataProvider; + use crate::BenchmarkingConfig; use super::*; @@ -703,6 +705,14 @@ pub mod pallet { } fn integrity_test() { + // ensure that we funnel the correct value to the `DataProvider::MaxVotesPerVoter`; + assert_eq!( + T::MaxNominations::get(), + ::MaxVotesPerVoter::get() + ); + // and that MaxNominations is always greater than 1, since we count on this. + assert!(!T::MaxNominations::get().is_zero()); + sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| assert!( diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 64bca7262829e..c27cf2037b3f4 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -114,6 +114,13 @@ impl TypeId for PalletId { const TYPE_ID: [u8; 4] = *b"modl"; } +#[macro_export] +macro_rules! try_bounded_vec { + ($ ($values:expr),* ) => { + $crate::sp_std::vec![$($values),*].try_into().unwrap() + } +} + /// Generate a new type alias for [`storage::types::StorageValue`], /// [`storage::types::StorageMap`], [`storage::types::StorageDoubleMap`] /// and [`storage::types::StorageNMap`]. From 419354c668b4c1e2da1267d9e5d42f54976f9da4 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 7 Jan 2022 14:30:34 +0000 Subject: [PATCH 11/21] fix doc test --- frame/election-provider-support/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 0873c4ea26310..cb67740f5dd2c 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -108,7 +108,7 @@ //! Ok(1) //! } //! fn voters(maybe_max_len: Option) -//! -> data_provider::Result)>> +//! -> data_provider::Result>> //! { //! Ok(Default::default()) //! } From ec6b11b48a710da3f793c4d7deb6c4f0e8ddb97b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 7 Jan 2022 14:34:06 +0000 Subject: [PATCH 12/21] document bounded_vec --- .../election-provider-multi-phase/src/mock.rs | 20 +++++++++---------- .../src/unsigned.rs | 6 +++--- .../election-provider-support/src/onchain.rs | 8 ++++---- frame/support/src/lib.rs | 9 ++++++++- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 087c473e54659..dd2941997c8de 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -24,7 +24,7 @@ pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ parameter_types, traits::{ConstU32, Hooks}, - try_bounded_vec, + bounded_vec, weights::Weight, }; use multi_phase::unsigned::{IndexAssignmentOf, VoterOf}; @@ -248,15 +248,15 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub static Targets: Vec = vec![10, 20, 30, 40]; pub static Voters: Vec> = vec![ - (1, 10, try_bounded_vec![10, 20]), - (2, 10, try_bounded_vec![30, 40]), - (3, 10, try_bounded_vec![40]), - (4, 10, try_bounded_vec![10, 20, 30, 40]), + (1, 10, bounded_vec![10, 20]), + (2, 10, bounded_vec![30, 40]), + (3, 10, bounded_vec![40]), + (4, 10, bounded_vec![10, 20, 30, 40]), // self votes. - (10, 10, try_bounded_vec![10]), - (20, 20, try_bounded_vec![20]), - (30, 30, try_bounded_vec![30]), - (40, 40, try_bounded_vec![40]), + (10, 10, bounded_vec![10]), + (20, 20, bounded_vec![20]), + (30, 30, bounded_vec![30]), + (40, 40, bounded_vec![40]), ]; pub static DesiredTargets: u32 = 2; @@ -512,7 +512,7 @@ impl ElectionDataProvider for StakingMock { // to be on-par with staking, we add a self vote as well. the stake is really not that // important. let mut current = Voters::get(); - current.push((target, ExistentialDeposit::get() as u64, try_bounded_vec![target])); + current.push((target, ExistentialDeposit::get() as u64, bounded_vec![target])); Voters::set(current); } } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 82e7e50d8d6a8..21b51c6c092f4 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -746,7 +746,7 @@ mod tests { use codec::Decode; use frame_benchmarking::Zero; use frame_support::{ - assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker, try_bounded_vec, + assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker, bounded_vec, }; use sp_npos_elections::IndexAssignment; use sp_runtime::{ @@ -1046,8 +1046,8 @@ mod tests { fn unsigned_per_dispatch_checks_can_only_submit_threshold_better() { ExtBuilder::default() .desired_targets(1) - .add_voter(7, 2, try_bounded_vec![10]) - .add_voter(8, 5, try_bounded_vec![10]) + .add_voter(7, 2, bounded_vec![10]) + .add_voter(8, 5, bounded_vec![10]) .solution_improvement_threshold(Perbill::from_percent(50)) .build_and_execute(|| { roll_to(25); diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index c85cc2c759aad..aeb3fbf3c95bf 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -165,7 +165,7 @@ mod tests { type OnChainPhragmen = OnChainSequentialPhragmen; mod mock_data_provider { - use frame_support::{traits::ConstU32, try_bounded_vec}; + use frame_support::{bounded_vec, traits::ConstU32}; use super::*; use crate::{data_provider, VoterOf}; @@ -177,9 +177,9 @@ mod tests { type MaxVotesPerVoter = ConstU32<2>; fn voters(_: Option) -> data_provider::Result>> { Ok(vec![ - (1, 10, try_bounded_vec![10, 20]), - (2, 20, try_bounded_vec![30, 20]), - (3, 30, try_bounded_vec![10, 30]), + (1, 10, bounded_vec![10, 20]), + (2, 20, bounded_vec![30, 20]), + (3, 30, bounded_vec![10, 30]), ]) } diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index c27cf2037b3f4..2a85e9a59789d 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -114,8 +114,15 @@ impl TypeId for PalletId { const TYPE_ID: [u8; 4] = *b"modl"; } +/// Build a bounded vec from the given literals. +/// +/// The type of the outcome must be known. +/// +/// Will not handle any errors and just panic if the given literals cannot fit in the corresponding +/// bounded vec type. Thus, this is only suitable for testing and non-consensus code. #[macro_export] -macro_rules! try_bounded_vec { +#[cfg(feature = "std")] +macro_rules! bounded_vec { ($ ($values:expr),* ) => { $crate::sp_std::vec![$($values),*].try_into().unwrap() } From 2864d22a0e4ff3847ac270d05edd47775d8f73a0 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 7 Jan 2022 14:43:42 +0000 Subject: [PATCH 13/21] self-review --- .../election-provider-multi-phase/src/lib.rs | 20 +------------------ .../src/unsigned.rs | 10 +++++----- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 345fd18c54f84..f1b5da3402060 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -454,11 +454,7 @@ pub struct ReadySolution { #[scale_info(skip_type_params(T))] pub struct RoundSnapshot { /// All of the voters. - pub voters: Vec<( - T::AccountId, - VoteWeight, - BoundedVec::MaxVotesPerVoter>, - )>, + pub voters: Vec>, /// All of the targets. pub targets: Vec, } @@ -1217,20 +1213,6 @@ pub mod pallet { } impl Pallet { - /// Same as the getter of the [`Snapshot`] storage item, but it returns the voters converted - /// into an unbounded vector. - /// - /// Note that this incurs an extra allocation of the entire snapshot, unfortunately. - pub fn snapshot_unbounded( - ) -> Option<(Vec, Vec<(T::AccountId, VoteWeight, Vec)>)> { - use frame_election_provider_support::IntoUnboundedVoters; - Self::snapshot().map(|snapshot| { - let voters = snapshot.voters.into_unbounded_voters(); - let targets = snapshot.targets; - (targets, voters) - }) - } - /// Internal logic of the offchain worker, to be executed only when the offchain lock is /// acquired with success. fn do_synchronized_offchain_worker(now: T::BlockNumber) { diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 21b51c6c092f4..fc8160c1caf7c 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -23,7 +23,7 @@ use crate::{ WeightInfo, }; use codec::Encode; -use frame_election_provider_support::{NposSolver, PerThing128}; +use frame_election_provider_support::{IntoUnboundedVoters, NposSolver, PerThing128}; use frame_support::{dispatch::DispatchResult, ensure, traits::Get}; use frame_system::offchain::SubmitTransaction; use sp_arithmetic::Perbill; @@ -273,11 +273,11 @@ impl Pallet { where S: NposSolver>, { - let (targets, voters) = - Self::snapshot_unbounded().ok_or(MinerError::SnapshotUnAvailable)?; + let RoundSnapshot { voters, targets } = + Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; - S::solve(desired_targets as usize, targets, voters) + S::solve(desired_targets as usize, targets, voters.into_unbounded_voters()) .map_err(|e| MinerError::Solver::(e)) .and_then(|e| Self::prepare_election_result::(e)) } @@ -746,7 +746,7 @@ mod tests { use codec::Decode; use frame_benchmarking::Zero; use frame_support::{ - assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker, bounded_vec, + assert_noop, assert_ok, bounded_vec, dispatch::Dispatchable, traits::OffchainWorker, }; use sp_npos_elections::IndexAssignment; use sp_runtime::{ From 7ca67ee8d120963d6666a13c3ce2a19e1670e17b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sat, 8 Jan 2022 08:27:26 +0000 Subject: [PATCH 14/21] remove unused --- frame/election-provider-multi-phase/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index f1b5da3402060..5a3e4aacc86e4 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -236,7 +236,6 @@ use frame_support::{ ensure, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, weights::{DispatchClass, Weight}, - BoundedVec, }; use frame_system::{ensure_none, offchain::SendTransactionTypes}; use scale_info::TypeInfo; From c868c414d895db024701eeea75cf55efbee29fb2 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sat, 8 Jan 2022 08:56:17 +0000 Subject: [PATCH 15/21] Fix build --- frame/election-provider-multi-phase/src/benchmarking.rs | 1 + frame/election-provider-multi-phase/src/mock.rs | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 3bea7c9541ae6..d37b9451b770f 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -23,6 +23,7 @@ use frame_benchmarking::account; use frame_support::{ assert_ok, traits::{Hooks, TryCollect}, + BoundedVec, }; use frame_system::RawOrigin; use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng}; diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index dd2941997c8de..ea9841ce45528 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -22,10 +22,10 @@ use frame_election_provider_support::{ }; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ - parameter_types, + bounded_vec, parameter_types, traits::{ConstU32, Hooks}, - bounded_vec, weights::Weight, + BoundedVec, }; use multi_phase::unsigned::{IndexAssignmentOf, VoterOf}; use parking_lot::RwLock; @@ -496,7 +496,7 @@ impl ElectionDataProvider for StakingMock { fn add_voter( voter: AccountId, weight: VoteWeight, - targets: BoundedVec, + targets: frame_support::BoundedVec, ) { let mut current = Voters::get(); current.push((voter, weight, targets)); From 104ba4a74090e9cd246a2270f39fe29bcee8dd1c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 12 Jan 2022 12:21:34 +0100 Subject: [PATCH 16/21] frame-support: repetition overload for bounded_vec Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/lib.rs | 3 +++ frame/support/src/storage/bounded_vec.rs | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 2a85e9a59789d..16591b7fb4baf 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -125,6 +125,9 @@ impl TypeId for PalletId { macro_rules! bounded_vec { ($ ($values:expr),* ) => { $crate::sp_std::vec![$($values),*].try_into().unwrap() + }; + ( $value:expr ; $repetition:expr ) => { + $crate::sp_std::vec![$value ; $repetition].try_into().unwrap() } } diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index bdac8f23d7c96..74eeeffe0751e 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -437,6 +437,19 @@ pub mod test { assert_eq!(*bounded, vec![1, 0, 2, 3]); } + #[test] + fn constructor_macro_works() { + use frame_support::bounded_vec; + + // With values. Use some brackets to make sure the macro doesn't expand. + let bv: BoundedVec<(u32, u32), ConstU32<3>> = bounded_vec![(1, 2), (1, 2), (1, 2)]; + assert_eq!(bv, vec![(1, 2), (1, 2), (1, 2)]); + + // With repetition. + let bv: BoundedVec<(u32, u32), ConstU32<3>> = bounded_vec![(1, 2); 3]; + assert_eq!(bv, vec![(1, 2), (1, 2), (1, 2)]); + } + #[test] #[should_panic(expected = "insertion index (is 9) should be <= len (is 3)")] fn try_inert_panics_if_oob() { From eb8904e5051f12444d71460cb6baafa1128ee352 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 19 Jan 2022 06:58:59 +0000 Subject: [PATCH 17/21] fix --- frame/staking/src/pallet/impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 24dd073f6021d..a24faf8a3c3d6 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -923,7 +923,6 @@ impl ElectionDataProvider for Pallet { let stake = >::try_from(weight).unwrap_or_else(|_| { panic!("cannot convert a VoteWeight into BalanceOf, benchmark needs reconfiguring.") }); - let targets = targets.try_into().unwrap(); >::insert(voter.clone(), voter.clone()); >::insert( voter.clone(), From 587f7154ec43195ccd3c1bd08664de59fbb85b03 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 19 Jan 2022 08:42:09 +0000 Subject: [PATCH 18/21] remove the need to allocate into unbounded voters etc etc --- .../election-provider-multi-phase/src/mock.rs | 21 ++++---------- .../src/unsigned.rs | 4 +-- frame/election-provider-support/src/lib.rs | 20 ++----------- .../election-provider-support/src/onchain.rs | 11 ++------ frame/elections-phragmen/src/lib.rs | 4 +-- .../npos-elections/fuzzer/src/common.rs | 18 +++--------- .../fuzzer/src/phragmen_balancing.rs | 11 ++------ .../fuzzer/src/phragmms_balancing.rs | 13 +++------ primitives/npos-elections/src/lib.rs | 4 +-- primitives/npos-elections/src/mock.rs | 2 +- primitives/npos-elections/src/phragmen.rs | 2 +- primitives/npos-elections/src/phragmms.rs | 14 +++++----- primitives/npos-elections/src/tests.rs | 28 +++++++++---------- 13 files changed, 51 insertions(+), 101 deletions(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index ea9841ce45528..1be93c363e321 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -18,7 +18,7 @@ use super::*; use crate as multi_phase; use frame_election_provider_support::{ - data_provider, onchain, ElectionDataProvider, IntoUnboundedVoters, SequentialPhragmen, + data_provider, onchain, ElectionDataProvider, SequentialPhragmen, }; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ @@ -132,13 +132,8 @@ pub fn trim_helpers() -> TrimHelpers { let desired_targets = MultiPhase::desired_targets().unwrap(); - let ElectionResult { mut assignments, .. } = seq_phragmen::<_, SolutionAccuracyOf>( - desired_targets as usize, - targets.clone(), - voters.clone().into_unbounded_voters(), - None, - ) - .unwrap(); + let ElectionResult::<_, SolutionAccuracyOf> { mut assignments, .. } = + seq_phragmen(desired_targets as usize, targets.clone(), voters.clone(), None).unwrap(); // sort by decreasing order of stake assignments.sort_unstable_by_key(|assignment| { @@ -164,14 +159,8 @@ pub fn raw_solution() -> RawSolution> { let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); let desired_targets = MultiPhase::desired_targets().unwrap(); - let ElectionResult { winners: _, assignments } = - seq_phragmen::<_, SolutionAccuracyOf>( - desired_targets as usize, - targets.clone(), - voters.clone().into_unbounded_voters(), - None, - ) - .unwrap(); + let ElectionResult::<_, SolutionAccuracyOf> { winners: _, assignments } = + seq_phragmen(desired_targets as usize, targets.clone(), voters.clone(), None).unwrap(); // closures let cache = helpers::generate_voter_cache::(&voters); diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index fc8160c1caf7c..936993b41fb6c 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -23,7 +23,7 @@ use crate::{ WeightInfo, }; use codec::Encode; -use frame_election_provider_support::{IntoUnboundedVoters, NposSolver, PerThing128}; +use frame_election_provider_support::{NposSolver, PerThing128}; use frame_support::{dispatch::DispatchResult, ensure, traits::Get}; use frame_system::offchain::SubmitTransaction; use sp_arithmetic::Perbill; @@ -277,7 +277,7 @@ impl Pallet { Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; - S::solve(desired_targets as usize, targets, voters.into_unbounded_voters()) + S::solve(desired_targets as usize, targets, voters) .map_err(|e| MinerError::Solver::(e)) .and_then(|e| Self::prepare_election_result::(e)) } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index cb67740f5dd2c..43b2e3d25084a 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -426,7 +426,7 @@ pub trait NposSolver { fn solve( to_elect: usize, targets: Vec, - voters: Vec<(Self::AccountId, VoteWeight, Vec)>, + voters: Vec<(Self::AccountId, VoteWeight, impl IntoIterator)>, ) -> Result, Self::Error>; } @@ -448,7 +448,7 @@ impl< fn solve( winners: usize, targets: Vec, - voters: Vec<(Self::AccountId, VoteWeight, Vec)>, + voters: Vec<(Self::AccountId, VoteWeight, impl IntoIterator)>, ) -> Result, Self::Error> { sp_npos_elections::seq_phragmen(winners, targets, voters, Balancing::get()) } @@ -472,7 +472,7 @@ impl< fn solve( winners: usize, targets: Vec, - voters: Vec<(Self::AccountId, VoteWeight, Vec)>, + voters: Vec<(Self::AccountId, VoteWeight, impl IntoIterator)>, ) -> Result, Self::Error> { sp_npos_elections::phragmms(winners, targets, voters, Balancing::get()) } @@ -484,17 +484,3 @@ pub type Voter = (AccountId, VoteWeight, BoundedVec = Voter<::AccountId, ::MaxVotesPerVoter>; - -/// Extension trait to easily convert from bounded voters to unbounded ones. -/// -/// Undesirably, this cannot be done (in safe rust) without re-allocating the entire vector, so -/// don't use it recklessly. -pub trait IntoUnboundedVoters { - fn into_unbounded_voters(self) -> Vec<(AccountId, VoteWeight, Vec)>; -} - -impl> IntoUnboundedVoters for Vec> { - fn into_unbounded_voters(self) -> Vec<(AccountId, VoteWeight, Vec)> { - self.into_iter().map(|(x, y, z)| (x, y, z.into_inner())).collect::>() - } -} diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index aeb3fbf3c95bf..808b49ba6234d 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -17,7 +17,7 @@ //! An implementation of [`ElectionProvider`] that does an on-chain sequential phragmen. -use crate::{ElectionDataProvider, ElectionProvider, IntoUnboundedVoters}; +use crate::{ElectionDataProvider, ElectionProvider}; use frame_support::{traits::Get, weights::DispatchClass}; use sp_npos_elections::*; use sp_std::{collections::btree_map::BTreeMap, marker::PhantomData, prelude::*}; @@ -87,13 +87,8 @@ impl ElectionProvider for OnChainSequentialPhragmen { let stake_of = |w: &T::AccountId| -> VoteWeight { stake_map.get(w).cloned().unwrap_or_default() }; - let ElectionResult { winners: _, assignments } = seq_phragmen::<_, T::Accuracy>( - desired_targets as usize, - targets, - voters.into_unbounded_voters(), - None, - ) - .map_err(Error::from)?; + let ElectionResult::<_, T::Accuracy> { winners: _, assignments } = + seq_phragmen(desired_targets as usize, targets, voters, None).map_err(Error::from)?; let staked = assignment_ratio_to_staked_normalized(assignments, &stake_of)?; diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index b0e0a6fb984e8..748f38bb6b33c 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -925,13 +925,13 @@ impl Pallet { let weight_candidates = candidates_and_deposit.len() as u32; let weight_voters = voters_and_votes.len() as u32; let weight_edges = num_edges; - let _ = sp_npos_elections::seq_phragmen::( + let _ = sp_npos_elections::seq_phragmen( num_to_elect, candidate_ids, voters_and_votes.clone(), None, ) - .map(|ElectionResult { winners, assignments: _ }| { + .map(|ElectionResult:: { winners, assignments: _ }| { // this is already sorted by id. let old_members_ids_sorted = >::take().into_iter().map(|m| m.who).collect::>(); diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index 7ca66f72dd92b..89c635c4b4f68 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -158,20 +158,10 @@ pub fn generate_random_npos_result( ( match election_type { - ElectionType::Phragmen(conf) => seq_phragmen::( - to_elect, - candidates.clone(), - voters.clone(), - conf, - ) - .unwrap(), - ElectionType::Phragmms(conf) => phragmms::( - to_elect, - candidates.clone(), - voters.clone(), - conf, - ) - .unwrap(), + ElectionType::Phragmen(conf) => + seq_phragmen(to_elect, candidates.clone(), voters.clone(), conf).unwrap(), + ElectionType::Phragmms(conf) => + phragmms(to_elect, candidates.clone(), voters.clone(), conf).unwrap(), }, candidates, voters, diff --git a/primitives/npos-elections/fuzzer/src/phragmen_balancing.rs b/primitives/npos-elections/fuzzer/src/phragmen_balancing.rs index 2af6a4c0f8151..8f782405df527 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_balancing.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_balancing.rs @@ -24,7 +24,7 @@ use honggfuzz::fuzz; use rand::{self, SeedableRng}; use sp_npos_elections::{ assignment_ratio_to_staked_normalized, is_score_better, seq_phragmen, to_supports, - EvaluateSupport, VoteWeight, + ElectionResult, EvaluateSupport, VoteWeight, }; use sp_runtime::Perbill; @@ -68,13 +68,8 @@ fn main() { }; if iterations > 0 { - let balanced = seq_phragmen::( - to_elect, - candidates, - voters, - Some((iterations, 0)), - ) - .unwrap(); + let balanced: ElectionResult = + seq_phragmen(to_elect, candidates, voters, Some((iterations, 0))).unwrap(); let balanced_score = { let staked = assignment_ratio_to_staked_normalized( diff --git a/primitives/npos-elections/fuzzer/src/phragmms_balancing.rs b/primitives/npos-elections/fuzzer/src/phragmms_balancing.rs index 0cd49c3f80442..f2b12b137883c 100644 --- a/primitives/npos-elections/fuzzer/src/phragmms_balancing.rs +++ b/primitives/npos-elections/fuzzer/src/phragmms_balancing.rs @@ -23,8 +23,8 @@ use common::*; use honggfuzz::fuzz; use rand::{self, SeedableRng}; use sp_npos_elections::{ - assignment_ratio_to_staked_normalized, is_score_better, phragmms, to_supports, EvaluateSupport, - VoteWeight, + assignment_ratio_to_staked_normalized, is_score_better, phragmms, to_supports, ElectionResult, + EvaluateSupport, VoteWeight, }; use sp_runtime::Perbill; @@ -67,13 +67,8 @@ fn main() { score }; - let balanced = phragmms::( - to_elect, - candidates, - voters, - Some((iterations, 0)), - ) - .unwrap(); + let balanced: ElectionResult = + phragmms(to_elect, candidates, voters, Some((iterations, 0))).unwrap(); let balanced_score = { let staked = diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index bb1c38d3077c4..7b3b09a4c7346 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -471,7 +471,7 @@ pub fn is_score_better(this: ElectionScore, that: ElectionScore, ep /// - It drops duplicate targets within a voter. pub fn setup_inputs( initial_candidates: Vec, - initial_voters: Vec<(AccountId, VoteWeight, Vec)>, + initial_voters: Vec<(AccountId, VoteWeight, impl IntoIterator)>, ) -> (Vec>, Vec>) { // used to cache and access candidates index. let mut c_idx_cache = BTreeMap::::new(); @@ -496,7 +496,7 @@ pub fn setup_inputs( let voters = initial_voters .into_iter() .filter_map(|(who, voter_stake, votes)| { - let mut edges: Vec> = Vec::with_capacity(votes.len()); + let mut edges: Vec> = Vec::new(); for v in votes { if edges.iter().any(|e| e.who == v) { // duplicate edge. diff --git a/primitives/npos-elections/src/mock.rs b/primitives/npos-elections/src/mock.rs index 8e8e7ebc1c0c6..85c970d7b418f 100644 --- a/primitives/npos-elections/src/mock.rs +++ b/primitives/npos-elections/src/mock.rs @@ -344,7 +344,7 @@ pub(crate) fn run_and_compare( FS: Fn(&AccountId) -> VoteWeight, { // run fixed point code. - let ElectionResult { winners, assignments } = seq_phragmen::<_, Output>( + let ElectionResult::<_, Output> { winners, assignments } = seq_phragmen( to_elect, candidates.clone(), voters diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index c582c5910d69a..e8e925935f774 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -70,7 +70,7 @@ const DEN: ExtendedBalance = ExtendedBalance::max_value(); pub fn seq_phragmen( to_elect: usize, candidates: Vec, - voters: Vec<(AccountId, VoteWeight, Vec)>, + voters: Vec<(AccountId, VoteWeight, impl IntoIterator)>, balancing: Option<(usize, ExtendedBalance)>, ) -> Result, crate::Error> { let (candidates, voters) = setup_inputs(candidates, voters); diff --git a/primitives/npos-elections/src/phragmms.rs b/primitives/npos-elections/src/phragmms.rs index 7c51da9ee92e0..6220cacd157b2 100644 --- a/primitives/npos-elections/src/phragmms.rs +++ b/primitives/npos-elections/src/phragmms.rs @@ -44,7 +44,7 @@ use sp_std::{prelude::*, rc::Rc}; pub fn phragmms( to_elect: usize, candidates: Vec, - voters: Vec<(AccountId, VoteWeight, Vec)>, + voters: Vec<(AccountId, VoteWeight, impl IntoIterator)>, balancing: Option<(usize, ExtendedBalance)>, ) -> Result, crate::Error> { let (candidates, mut voters) = setup_inputs(candidates, voters); @@ -351,8 +351,8 @@ mod tests { let candidates = vec![1, 2, 3]; let voters = vec![(10, 10, vec![1, 2]), (20, 20, vec![1, 3]), (30, 30, vec![2, 3])]; - let ElectionResult { winners, assignments } = - phragmms::<_, Perbill>(2, candidates, voters, Some((2, 0))).unwrap(); + let ElectionResult::<_, Perbill> { winners, assignments } = + phragmms(2, candidates, voters, Some((2, 0))).unwrap(); assert_eq!(winners, vec![(3, 30), (2, 30)]); assert_eq!( assignments, @@ -383,8 +383,8 @@ mod tests { (130, 1000, vec![61, 71]), ]; - let ElectionResult { winners, assignments: _ } = - phragmms::<_, Perbill>(4, candidates, voters, Some((2, 0))).unwrap(); + let ElectionResult::<_, Perbill> { winners, assignments: _ } = + phragmms(4, candidates, voters, Some((2, 0))).unwrap(); assert_eq!(winners, vec![(11, 3000), (31, 2000), (51, 1500), (61, 1500),]); } @@ -396,8 +396,8 @@ mod tests { // give a bit more to 1 and 3. voters.push((2, u64::MAX, vec![1, 3])); - let ElectionResult { winners, assignments: _ } = - phragmms::<_, Perbill>(2, candidates, voters, Some((2, 0))).unwrap(); + let ElectionResult::<_, Perbill> { winners, assignments: _ } = + phragmms(2, candidates, voters, Some((2, 0))).unwrap(); assert_eq!(winners.into_iter().map(|(w, _)| w).collect::>(), vec![1u32, 3]); } } diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index e7d0078b1fbe0..c6748b29e9851 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -230,7 +230,7 @@ fn phragmen_poc_works() { let voters = vec![(10, vec![1, 2]), (20, vec![1, 3]), (30, vec![2, 3])]; let stake_of = create_stake_of(&[(10, 10), (20, 20), (30, 30)]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates, voters @@ -285,7 +285,7 @@ fn phragmen_poc_works_with_balancing() { let voters = vec![(10, vec![1, 2]), (20, vec![1, 3]), (30, vec![2, 3])]; let stake_of = create_stake_of(&[(10, 10), (20, 20), (30, 30)]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates, voters @@ -372,7 +372,7 @@ fn phragmen_accuracy_on_large_scale_only_candidates() { (5, (u64::MAX - 2).into()), ]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates.clone(), auto_generate_self_voters(&candidates) @@ -403,7 +403,7 @@ fn phragmen_accuracy_on_large_scale_voters_and_candidates() { (14, u64::MAX.into()), ]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates, voters @@ -435,7 +435,7 @@ fn phragmen_accuracy_on_small_scale_self_vote() { let voters = auto_generate_self_voters(&candidates); let stake_of = create_stake_of(&[(40, 0), (10, 1), (20, 2), (30, 1)]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 3, candidates, voters @@ -465,7 +465,7 @@ fn phragmen_accuracy_on_small_scale_no_self_vote() { (3, 1), ]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 3, candidates, voters @@ -501,7 +501,7 @@ fn phragmen_large_scale_test() { (50, 990000000000000000), ]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates, voters @@ -528,7 +528,7 @@ fn phragmen_large_scale_test_2() { let stake_of = create_stake_of(&[(2, c_budget.into()), (4, c_budget.into()), (50, nom_budget.into())]); - let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments } = seq_phragmen( 2, candidates, voters @@ -597,7 +597,7 @@ fn elect_has_no_entry_barrier() { let voters = vec![(1, vec![10]), (2, vec![20])]; let stake_of = create_stake_of(&[(1, 10), (2, 10)]); - let ElectionResult { winners, assignments: _ } = seq_phragmen::<_, Perbill>( + let ElectionResult::<_, Perbill> { winners, assignments: _ } = seq_phragmen( 3, candidates, voters @@ -618,7 +618,7 @@ fn phragmen_self_votes_should_be_kept() { let voters = vec![(5, vec![5]), (10, vec![10]), (20, vec![20]), (1, vec![10, 20])]; let stake_of = create_stake_of(&[(5, 5), (10, 10), (20, 20), (1, 8)]); - let result = seq_phragmen::<_, Perbill>( + let result: ElectionResult<_, Perbill> = seq_phragmen( 2, candidates, voters @@ -664,8 +664,8 @@ fn duplicate_target_is_ignored() { let candidates = vec![1, 2, 3]; let voters = vec![(10, 100, vec![1, 1, 2, 3]), (20, 100, vec![2, 3]), (30, 50, vec![1, 1, 2])]; - let ElectionResult { winners, assignments } = - seq_phragmen::<_, Perbill>(2, candidates, voters, None).unwrap(); + let ElectionResult::<_, Perbill> { winners, assignments } = + seq_phragmen(2, candidates, voters, None).unwrap(); assert_eq!(winners, vec![(2, 140), (3, 110)]); assert_eq!( @@ -682,8 +682,8 @@ fn duplicate_target_is_ignored_when_winner() { let candidates = vec![1, 2, 3]; let voters = vec![(10, 100, vec![1, 1, 2, 3]), (20, 100, vec![1, 2])]; - let ElectionResult { winners, assignments } = - seq_phragmen::<_, Perbill>(2, candidates, voters, None).unwrap(); + let ElectionResult::<_, Perbill> { winners, assignments } = + seq_phragmen(2, candidates, voters, None).unwrap(); assert_eq!(winners, vec![(1, 100), (2, 100)]); assert_eq!( From 88cb4a6649ebdb1f825d7ac39378ce03e3ce53e9 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 19 Jan 2022 14:49:19 +0000 Subject: [PATCH 19/21] Don't expect --- frame/staking/src/pallet/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 8264f0343b69c..7b1e28c9e2960 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -32,7 +32,7 @@ use sp_runtime::{ DispatchError, Perbill, Percent, }; use sp_staking::SessionIndex; -use sp_std::{convert::From, prelude::*, result}; +use sp_std::{convert::From, prelude::*}; mod impls; @@ -1042,7 +1042,7 @@ pub mod pallet { let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets.into_inner()); - let targets = targets + let targets: BoundedVec<_, _> = targets .into_iter() .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) .map(|n| { @@ -1054,13 +1054,13 @@ pub mod pallet { } }) }) - .collect::, _>>()? + .collect::, _>>()? .try_into() - .expect("bound checked above; qed"); + .map_err(|_| Error::::TooManyNominators)?; let nominations = Nominations { targets, - // Initial nominations are considered submitted at era 0. See `Nominations` doc + // Initial nominations are considered submitted at era 0. See `Nominations` doc. submitted_in: Self::current_era().unwrap_or(0), suppressed: false, }; From a831b081122ca04082455c93d41f9d1add40dd41 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 19 Jan 2022 16:17:11 +0000 Subject: [PATCH 20/21] unbreal the build again --- frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 26b8f697b6d7f..0b7fec70787c1 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -31,7 +31,7 @@ use sp_runtime::{ traits::{CheckedSub, SaturatedConversion, StaticLookup, Zero}, DispatchError, Perbill, Percent, }; -use sp_staking::SessionIndex; +use sp_staking::{EraIndex, SessionIndex}; use sp_std::{convert::From, prelude::*}; mod impls; From 5e1b8f0c26d5fa48433bc0ef65d9082a0903b12b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 20 Jan 2022 14:57:21 +0000 Subject: [PATCH 21/21] handle macro a bit better --- frame/support/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index d22fd1934af19..ef60729f6d861 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -124,10 +124,16 @@ impl TypeId for PalletId { #[cfg(feature = "std")] macro_rules! bounded_vec { ($ ($values:expr),* ) => { - $crate::sp_std::vec![$($values),*].try_into().unwrap() + { + use $crate::sp_std::convert::TryInto as _; + $crate::sp_std::vec![$($values),*].try_into().unwrap() + } }; ( $value:expr ; $repetition:expr ) => { - $crate::sp_std::vec![$value ; $repetition].try_into().unwrap() + { + use $crate::sp_std::convert::TryInto as _; + $crate::sp_std::vec![$value ; $repetition].try_into().unwrap() + } } }