From 6f10773d0e93093f3bd2853feb0964fcb6aa75b1 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 29 Sep 2022 15:25:55 +0200 Subject: [PATCH 01/66] bounding election provider with kian --- .../election-provider-multi-phase/src/lib.rs | 56 +++++++++++++++---- frame/election-provider-support/src/lib.rs | 8 ++- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 3dc6161bb202a..85bd694871806 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -231,7 +231,7 @@ use codec::{Decode, Encode}; use frame_election_provider_support::{ - ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, + ElectionDataProvider, ElectionProvider, ElectionProviderBase, BoundedElectionProvider, BoundedSupportsOf, InstantElectionProvider, NposSolution, }; use frame_support::{ @@ -247,7 +247,8 @@ use sp_arithmetic::{ UpperOf, }; use sp_npos_elections::{ - assignment_ratio_to_staked_normalized, ElectionScore, EvaluateSupport, Supports, VoteWeight, + assignment_ratio_to_staked_normalized, BoundedSupports, ElectionScore, EvaluateSupport, + Supports, VoteWeight, }; use sp_runtime::{ transaction_validity::{ @@ -445,12 +446,12 @@ impl Default for RawSolution { /// A checked solution, ready to be enacted. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] -pub struct ReadySolution { +pub struct ReadySolution { /// The final supports of the solution. /// /// This is target-major vector, storing each winners, total backing, and each individual /// backer. - pub supports: Supports, + pub supports: BoundedSupports, /// The score of the solution. /// /// This is needed to potentially challenge the solution. @@ -560,6 +561,8 @@ pub enum FeasibilityError { InvalidRound, /// Comparison against `MinimumUntrustedScore` failed. UntrustedScoreTooLow, + /// A bound was not satisfied. + BoundNotMet, } impl From for FeasibilityError { @@ -672,6 +675,10 @@ pub mod pallet { /// The maximum number of electable targets to put in the snapshot. #[pallet::constant] type MaxElectableTargets: Get>; + + /// The maximum number of winners that can be elected from the electable targets. + #[pallet::constant] + type MaxWinners: Get; /// Handler for the slashed deposits. type SlashHandler: OnUnbalanced>; @@ -971,7 +978,7 @@ pub mod pallet { // `ElectionProvider::elect` will succeed and take care of that. let solution = ReadySolution { - supports, + supports: supports.try_into().map_err(|_| "BoundNotMet")?, score: Default::default(), compute: ElectionCompute::Emergency, }; @@ -1084,8 +1091,9 @@ pub mod pallet { Error::::FallbackFailed })?; + // TODO: sort and truncate supports with MaxWinners let solution = ReadySolution { - supports, + supports: supports.try_into().unwrap(), score: Default::default(), compute: ElectionCompute::Fallback, }; @@ -1226,7 +1234,7 @@ pub mod pallet { /// Current best solution, signed or unsigned, queued to be returned upon `elect`. #[pallet::storage] #[pallet::getter(fn queued_solution)] - pub type QueuedSolution = StorageValue<_, ReadySolution>; + pub type QueuedSolution = StorageValue<_, ReadySolution>; /// Snapshot data of the round. /// @@ -1238,10 +1246,13 @@ pub mod pallet { /// Desired number of targets to elect for this round. /// /// Only exists when [`Snapshot`] is present. + // WIP: call desired winners #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; + // WIP: desired_targets < MaxWinners + /// The metadata of the [`RoundSnapshot`] /// /// Only exists when [`Snapshot`] is present. @@ -1460,7 +1471,7 @@ impl Pallet { pub fn feasibility_check( raw_solution: RawSolution>, compute: ElectionCompute, - ) -> Result, FeasibilityError> { + ) -> Result, FeasibilityError> { let RawSolution { solution, score, round } = raw_solution; // First, check round. @@ -1533,6 +1544,7 @@ impl Pallet { let known_score = supports.evaluate(); ensure!(known_score == score, FeasibilityError::InvalidScore); + let supports = supports.try_into().map_err(|_| FeasibilityError::BoundNotMet); Ok(ReadySolution { supports, compute, score }) } @@ -1552,7 +1564,7 @@ impl Pallet { Self::kill_snapshot(); } - fn do_elect() -> Result, ElectionError> { + fn do_elect() -> Result, ElectionError> { // We have to unconditionally try finalizing the signed phase here. There are only two // possibilities: // @@ -1562,14 +1574,15 @@ impl Pallet { // inexpensive (1 read of an empty vector). let _ = Self::finalize_signed_phase(); >::take() + // TODO: fix to one statement .ok_or(ElectionError::::NothingQueued) .or_else(|_| { ::elect() - .map(|supports| ReadySolution { - supports, + .and_then(|supports| Ok(ReadySolution { + supports: supports.try_into().map_err(|_| ElectionError::Feasibility(FeasibilityError::BoundNotMet))?, score: Default::default(), compute: ElectionCompute::Fallback, - }) + })) .map_err(|fe| ElectionError::Fallback(fe)) }) .map(|ReadySolution { compute, score, supports }| { @@ -1630,6 +1643,25 @@ impl ElectionProvider for Pallet { } } } + +impl BoundedElectionProvider for Pallet { + fn elect() -> Result, Self::Error> { + match Self::do_elect() { + Ok(supports) => { + // All went okay, record the weight, put sign to be Off, clean snapshot, etc. + Self::weigh_supports(&supports); + Self::rotate_round(); + // WIP: Fix unwrap + Ok(supports) + }, + Err(why) => { + log!(error, "Entering emergency mode: {:?}", why); + >::put(Phase::Emergency); + Err(why) + }, + } + } +} /// convert a DispatchError to a custom InvalidTransaction with the inner code being the error /// number. pub fn dispatch_error_to_invalid(error: DispatchError) -> InvalidTransaction { diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 5ee65e102bd06..d3455a43a81b8 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -379,6 +379,12 @@ pub trait ElectionProviderBase { fn ongoing() -> bool; } +/// Same as `BoundedSupports` but parameterized by a `BoundedElectionProvider`. +pub type BoundedSupportsOf = BoundedSupports< + ::AccountId, + ::MaxWinners, +>; + /// Elect a new set of winners, bounded by `MaxWinners`. /// /// Returns a result in bounded, target major format, namely as @@ -389,7 +395,7 @@ pub trait BoundedElectionProvider: ElectionProviderBase { /// Performs the election. 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 elect() -> Result, Self::Error>; + fn elect() -> Result, Self::Error>; } /// Same a [`BoundedElectionProvider`], but no bounds are imposed on the number From 0afb48c0dfe83d83fbfe5236b47e5e670090b4bd Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 30 Sep 2022 21:43:06 +0200 Subject: [PATCH 02/66] multi phase implement bounded election provider --- .../election-provider-multi-phase/src/lib.rs | 40 ++++++------------- .../election-provider-multi-phase/src/mock.rs | 2 + .../src/signed.rs | 2 +- .../src/unsigned.rs | 2 +- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 85bd694871806..19368f98d35a5 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -445,8 +445,9 @@ impl Default for RawSolution { } /// A checked solution, ready to be enacted. -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] -pub struct ReadySolution { +#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, scale_info::TypeInfo)] +#[scale_info(skip_type_params(B))] +pub struct ReadySolution> { /// The final supports of the solution. /// /// This is target-major vector, storing each winners, total backing, and each individual @@ -465,7 +466,6 @@ pub struct ReadySolution { /// /// These are stored together because they are often accessed together. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] -#[codec(mel_bound())] #[scale_info(skip_type_params(T))] pub struct RoundSnapshot { /// All of the voters. @@ -1544,7 +1544,7 @@ impl Pallet { let known_score = supports.evaluate(); ensure!(known_score == score, FeasibilityError::InvalidScore); - let supports = supports.try_into().map_err(|_| FeasibilityError::BoundNotMet); + let supports = supports.try_into().map_err(|_| FeasibilityError::BoundNotMet)?; Ok(ReadySolution { supports, compute, score }) } @@ -1578,12 +1578,14 @@ impl Pallet { .ok_or(ElectionError::::NothingQueued) .or_else(|_| { ::elect() - .and_then(|supports| Ok(ReadySolution { - supports: supports.try_into().map_err(|_| ElectionError::Feasibility(FeasibilityError::BoundNotMet))?, - score: Default::default(), - compute: ElectionCompute::Fallback, - })) .map_err(|fe| ElectionError::Fallback(fe)) + .and_then(|supports| { + Ok(ReadySolution { + supports: supports.try_into().map_err(|_| FeasibilityError::BoundNotMet)?, + score: Default::default(), + compute: ElectionCompute::Fallback, + }) + }) }) .map(|ReadySolution { compute, score, supports }| { Self::deposit_event(Event::ElectionFinalized { compute, score }); @@ -1626,25 +1628,9 @@ impl ElectionProviderBase for Pallet { } } -impl ElectionProvider for Pallet { - fn elect() -> Result, Self::Error> { - match Self::do_elect() { - Ok(supports) => { - // All went okay, record the weight, put sign to be Off, clean snapshot, etc. - Self::weigh_supports(&supports); - Self::rotate_round(); - Ok(supports) - }, - Err(why) => { - log!(error, "Entering emergency mode: {:?}", why); - >::put(Phase::Emergency); - Err(why) - }, - } - } -} - impl BoundedElectionProvider for Pallet { + type MaxWinners = T::MaxWinners; + fn elect() -> Result, Self::Error> { match Self::do_elect() { Ok(supports) => { diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 2615d863c91e0..4d0487dc6124f 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -297,6 +297,7 @@ parameter_types! { pub static MockWeightInfo: MockedWeightInfo = MockedWeightInfo::Real; pub static MaxElectingVoters: VoterIndex = u32::max_value(); pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); + pub static MaxWinners: u32 = 100; pub static EpochLength: u64 = 30; pub static OnChainFallback: bool = true; @@ -408,6 +409,7 @@ impl crate::Config for Runtime { type ForceOrigin = frame_system::EnsureRoot; type MaxElectingVoters = MaxElectingVoters; type MaxElectableTargets = MaxElectableTargets; + type MaxWinners = MaxWinners; type MinerConfig = Self; type Solver = SequentialPhragmen, Balancing>; } diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 175c92757f35e..2067b71e3adf9 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -451,7 +451,7 @@ impl Pallet { /// /// Infallible pub fn finalize_signed_phase_accept_solution( - ready_solution: ReadySolution, + ready_solution: ReadySolution, who: &T::AccountId, deposit: BalanceOf, call_fee: BalanceOf, diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 7340605dfe621..f514f659d373a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -351,7 +351,7 @@ impl Pallet { // ensure score is being improved. Panic henceforth. ensure!( - Self::queued_solution().map_or(true, |q: ReadySolution<_>| raw_solution + Self::queued_solution().map_or(true, |q: ReadySolution<_, _>| raw_solution .score .strict_threshold_better(q.score, T::BetterUnsignedThreshold::get())), Error::::PreDispatchWeakSubmission, From ce7aff80e19f94ad8dcee6b1e0d23981500407e2 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 30 Sep 2022 21:44:03 +0200 Subject: [PATCH 03/66] election provider blanket implementation --- frame/election-provider-support/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index d3455a43a81b8..4d5213d3b4d8f 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -409,6 +409,12 @@ pub trait ElectionProvider: ElectionProviderBase { fn elect() -> Result, Self::Error>; } +impl ElectionProvider for E { + fn elect() -> Result, Self::Error> { + Self::elect().map(|supports| supports.into_inner()) + } +} + /// A sub-trait of the [`ElectionProvider`] for cases where we need to be sure /// an election needs to happen instantly, not asynchronously. /// From 72fdf0047dd256ea33f91e66ec1f66a3615e9093 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 2 Oct 2022 15:35:28 +0200 Subject: [PATCH 04/66] staking compiles --- frame/staking/src/lib.rs | 10 +++++ frame/staking/src/pallet/impls.rs | 62 ++++++++++++++++++------------- frame/staking/src/pallet/mod.rs | 4 +- 3 files changed, 49 insertions(+), 27 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index a0144463540be..6436a115b163b 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -333,6 +333,16 @@ macro_rules! log { }; } +/// Maximum number of winners (aka. active validators), as defined in the election provider of this +/// pallet. +pub type MaxWinnersOf = <::ElectionProvider as frame_election_provider_support::BoundedElectionProvider>::MaxWinners; + +// impl, S2: Get> TryFrom> for BoundedVec { +// type Error = (); +// fn try_from(x: BoundedVec) -> Result, Self::Error> { +// if x.len() <= Self::bound +// } +// } /// Counter for the number of "reward" points earned by a given validator. pub type RewardPoint = u32; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 6da27da362b53..e0820e4dfa30f 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, ElectionDataProvider, ElectionProvider, ScoreProvider, SortedListProvider, - Supports, VoteWeight, VoterOf, + data_provider, ElectionDataProvider, ElectionProvider, BoundedElectionProvider, ScoreProvider, SortedListProvider, + BoundedSupportsOf, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, @@ -45,7 +45,7 @@ use sp_std::{collections::btree_map::BTreeMap, prelude::*}; use crate::{ log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, Exposure, ExposureOf, Forcing, IndividualExposure, Nominations, PositiveImbalanceOf, RewardDestination, - SessionInterface, StakingLedger, ValidatorPrefs, + SessionInterface, StakingLedger, ValidatorPrefs, MaxWinnersOf, }; use super::{pallet::*, STAKING_ID}; @@ -267,7 +267,7 @@ impl Pallet { } /// Plan a new session potentially trigger a new era. - fn new_session(session_index: SessionIndex, is_genesis: bool) -> Option> { + fn new_session(session_index: SessionIndex, is_genesis: bool) -> Option>> { if let Some(current_era) = Self::current_era() { // Initial era has been set. let current_era_start_session_index = Self::eras_start_session_index(current_era) @@ -426,8 +426,8 @@ impl Pallet { /// Returns the new validator set. pub fn trigger_new_era( start_session_index: SessionIndex, - exposures: Vec<(T::AccountId, Exposure>)>, - ) -> Vec { + exposures: BoundedVec<(T::AccountId, Exposure>), MaxWinnersOf>, + ) -> BoundedVec> { // Increment or set current era. let new_planned_era = CurrentEra::::mutate(|s| { *s = Some(s.map(|s| s + 1).unwrap_or(0)); @@ -453,19 +453,27 @@ impl Pallet { pub(crate) fn try_trigger_new_era( start_session_index: SessionIndex, is_genesis: bool, - ) -> Option> { - let election_result = if is_genesis { - T::GenesisElectionProvider::elect().map_err(|e| { + ) -> Option>> { + + let election_result: BoundedVec<_, MaxWinnersOf> = if is_genesis { + let result = ::elect() + .map_err(|e| { + log!(warn, "genesis election provider failed due to {:?}", e); + Self::deposit_event(Event::StakingElectionFailed); + }); + result.ok()?.into_inner().try_into().map_err(|e| { + // AKON: Bounds not met if genesis_max_winners > election_provider_max_winners log!(warn, "genesis election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); - }) + }).ok()? } else { - T::ElectionProvider::elect().map_err(|e| { - log!(warn, "election provider failed due to {:?}", e); - Self::deposit_event(Event::StakingElectionFailed); - }) - } - .ok()?; + let result = ::elect() + .map_err(|e| { + log!(warn, "election provider failed due to {:?}", e); + Self::deposit_event(Event::StakingElectionFailed); + }); + result.ok()? + }; let exposures = Self::collect_exposures(election_result); if (exposures.len() as u32) < Self::minimum_validator_count().max(1) { @@ -502,9 +510,9 @@ impl Pallet { /// /// Store staking information for the new planned era pub fn store_stakers_info( - exposures: Vec<(T::AccountId, Exposure>)>, + exposures: BoundedVec<(T::AccountId, Exposure>), MaxWinnersOf>, new_planned_era: EraIndex, - ) -> Vec { + ) -> BoundedVec> { let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); // Populate stakers, exposures, and the snapshot of validator prefs. @@ -540,14 +548,15 @@ impl Pallet { ); } - elected_stashes + // AKON: return result here? + elected_stashes.try_into().unwrap() } - /// Consume a set of [`Supports`] from [`sp_npos_elections`] and collect them into a + /// Consume a set of [`BoundedSupports`] from [`sp_npos_elections`] and collect them into a /// [`Exposure`]. fn collect_exposures( - supports: Supports, - ) -> Vec<(T::AccountId, Exposure>)> { + supports: BoundedSupportsOf, + ) -> BoundedVec<(T::AccountId, Exposure>), MaxWinnersOf> { let total_issuance = T::Currency::total_issuance(); let to_currency = |e: frame_election_provider_support::ExtendedBalance| { T::CurrencyToVote::to_currency(e, total_issuance) @@ -576,7 +585,8 @@ impl Pallet { let exposure = Exposure { own, others, total }; (validator, exposure) }) - .collect::)>>() + // AKON: return result here? + .collect::)>>().try_into().unwrap() } /// Remove all associated data of a stash account from the staking system. @@ -1082,15 +1092,17 @@ impl ElectionDataProvider for Pallet { /// Once the first new_session is planned, all session must start and then end in order, though /// some session can lag in between the newest session planned and the latest session started. impl pallet_session::SessionManager for Pallet { + // AKON: session functions, dont change their signatures fn new_session(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {}", new_index); CurrentPlannedSession::::put(new_index); - Self::new_session(new_index, false) + let validators: Option> = Self::new_session(new_index, false); + validators.map(|v| v.into_inner()) } fn new_session_genesis(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {} at genesis", new_index); CurrentPlannedSession::::put(new_index); - Self::new_session(new_index, true) + Self::new_session(new_index, true).map(|v| v.into_inner()) } fn start_session(start_index: SessionIndex) { log!(trace, "starting session {}", start_index); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 560c3b6ed830c..8a147c19be1f8 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -107,7 +107,7 @@ pub mod pallet { type CurrencyToVote: CurrencyToVote>; /// Something that provides the election functionality. - type ElectionProvider: frame_election_provider_support::ElectionProvider< + type ElectionProvider: frame_election_provider_support::BoundedElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, // we only accept an election provider that has staking as data provider. @@ -115,7 +115,7 @@ pub mod pallet { >; /// Something that provides the election functionality at genesis. - type GenesisElectionProvider: frame_election_provider_support::ElectionProvider< + type GenesisElectionProvider: frame_election_provider_support::BoundedElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Pallet, From 65f5e0059ced7fe6b170b202e95dbeba58310df7 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 2 Oct 2022 23:22:08 +0200 Subject: [PATCH 05/66] fix test for election provider support --- frame/election-provider-support/src/lib.rs | 4 +- .../election-provider-support/src/onchain.rs | 43 +++++++++++++------ frame/staking/src/mock.rs | 1 + 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 4d5213d3b4d8f..795870d9606a6 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -422,7 +422,7 @@ impl ElectionProvider for E { /// /// Consequently, allows for control over the amount of data that is being /// fetched from the [`ElectionProviderBase::DataProvider`]. -pub trait InstantElectionProvider: ElectionProvider { +pub trait InstantElectionProvider: BoundedElectionProvider { /// Elect a new set of winners, but unlike [`ElectionProvider::elect`] which cannot enforce /// bounds, this trait method can enforce bounds on the amount of data provided by the /// `DataProvider`. @@ -434,7 +434,7 @@ pub trait InstantElectionProvider: ElectionProvider { fn elect_with_bounds( max_voters: usize, max_targets: usize, - ) -> Result, Self::Error>; + ) -> Result, Self::Error>; } /// An election provider to be used only for testing. diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 88aa6ca7267a0..d6e39e994b523 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -20,8 +20,8 @@ //! careful when using it onchain. use crate::{ - Debug, ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, - NposSolver, WeightInfo, + Debug, ElectionDataProvider, ElectionProviderBase, BoundedElectionProvider, InstantElectionProvider, + NposSolver, WeightInfo, BoundedSupportsOf, }; use frame_support::{dispatch::DispatchClass, traits::Get}; use sp_npos_elections::*; @@ -85,6 +85,9 @@ pub trait Config { >; /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + + /// Upper bound on maximum winners from electable targets + type MaxWinners: Get; } pub trait BoundedConfig: Config { @@ -94,10 +97,16 @@ pub trait BoundedConfig: Config { type TargetsBound: Get; } +/// Same as `BoundedSupportsOf` but for `onchain::Config`. +pub type OnChainBoundedSupportsOf = BoundedSupports< + <::System as frame_system::Config>::AccountId, + ::MaxWinners, +>; + fn elect_with( maybe_max_voters: Option, maybe_max_targets: Option, -) -> Result::AccountId>, Error> { +) -> Result, Error> { let voters = T::DataProvider::electing_voters(maybe_max_voters).map_err(Error::DataProvider)?; let targets = T::DataProvider::electable_targets(maybe_max_targets).map_err(Error::DataProvider)?; @@ -129,12 +138,15 @@ fn elect_with( weight, DispatchClass::Mandatory, ); - - Ok(to_supports(&staked)) + + let supports = to_supports(&staked).try_into().map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; + Ok(supports) } -impl ElectionProvider for UnboundedExecution { - fn elect() -> Result, Self::Error> { +impl BoundedElectionProvider for UnboundedExecution { + type MaxWinners = T::MaxWinners; + + fn elect() -> Result, Self::Error> { // This should not be called if not in `std` mode (and therefore neither in genesis nor in // testing) if cfg!(not(feature = "std")) { @@ -163,7 +175,7 @@ impl InstantElectionProvider for UnboundedExecution { fn elect_with_bounds( max_voters: usize, max_targets: usize, - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { elect_with::(Some(max_voters), Some(max_targets)) } } @@ -179,8 +191,9 @@ impl ElectionProviderBase for BoundedExecution { } } -impl ElectionProvider for BoundedExecution { - fn elect() -> Result, Self::Error> { +impl BoundedElectionProvider for BoundedExecution { + type MaxWinners = T::MaxWinners; + fn elect() -> Result, Self::Error> { elect_with::(Some(T::VotersBound::get() as usize), Some(T::TargetsBound::get() as usize)) } } @@ -189,7 +202,7 @@ impl InstantElectionProvider for BoundedExecution { fn elect_with_bounds( max_voters: usize, max_targets: usize, - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { elect_with::( Some(max_voters.min(T::VotersBound::get() as usize)), Some(max_targets.min(T::TargetsBound::get() as usize)), @@ -200,7 +213,7 @@ impl InstantElectionProvider for BoundedExecution { #[cfg(test)] mod tests { use super::*; - use crate::{PhragMMS, SequentialPhragmen}; + use crate::{PhragMMS, SequentialPhragmen, ElectionProvider}; use frame_support::traits::ConstU32; use sp_npos_elections::Support; use sp_runtime::Perbill; @@ -256,6 +269,7 @@ mod tests { type Solver = SequentialPhragmen; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); + type MaxWinners = ConstU32<50>; } impl BoundedConfig for PhragmenParams { @@ -268,6 +282,7 @@ mod tests { type Solver = PhragMMS; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); + type MaxWinners = ConstU32<50>; } impl BoundedConfig for PhragMMSParams { @@ -312,7 +327,7 @@ mod tests { fn onchain_seq_phragmen_works() { sp_io::TestExternalities::new_empty().execute_with(|| { assert_eq!( - BoundedExecution::::elect().unwrap(), + as ElectionProvider>::elect().unwrap(), vec![ (10, Support { total: 25, voters: vec![(1, 10), (3, 15)] }), (30, Support { total: 35, voters: vec![(2, 20), (3, 15)] }) @@ -325,7 +340,7 @@ mod tests { fn onchain_phragmms_works() { sp_io::TestExternalities::new_empty().execute_with(|| { assert_eq!( - BoundedExecution::::elect().unwrap(), + as ElectionProvider>::elect().unwrap(), vec![ (10, Support { total: 25, voters: vec![(1, 10), (3, 15)] }), (30, Support { total: 35, voters: vec![(2, 20), (3, 15)] }) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3a9351ef4a271..be2a7961be610 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -297,6 +297,7 @@ impl crate::pallet::pallet::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; + // AKON: Implement BoundedElectionProvider type ElectionProvider = onchain::UnboundedExecution; type GenesisElectionProvider = Self::ElectionProvider; // NOTE: consider a macro and use `UseNominatorsAndValidatorsMap` as well. From 49feb6655139cf3ea7e4c939f8ca77e1fc122aa9 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 2 Oct 2022 23:22:20 +0200 Subject: [PATCH 06/66] fmt --- .../election-provider-multi-phase/src/lib.rs | 21 ++++---- .../election-provider-support/src/onchain.rs | 12 +++-- frame/staking/src/pallet/impls.rs | 54 ++++++++++++------- 3 files changed, 53 insertions(+), 34 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 19368f98d35a5..6224d9900d968 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -231,8 +231,8 @@ use codec::{Decode, Encode}; use frame_election_provider_support::{ - ElectionDataProvider, ElectionProvider, ElectionProviderBase, BoundedElectionProvider, BoundedSupportsOf, InstantElectionProvider, - NposSolution, + BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, + ElectionProviderBase, InstantElectionProvider, NposSolution, }; use frame_support::{ dispatch::DispatchClass, @@ -675,8 +675,8 @@ pub mod pallet { /// The maximum number of electable targets to put in the snapshot. #[pallet::constant] type MaxElectableTargets: Get>; - - /// The maximum number of winners that can be elected from the electable targets. + + /// The maximum number of winners that can be elected from the electable targets. #[pallet::constant] type MaxWinners: Get; @@ -1091,7 +1091,7 @@ pub mod pallet { Error::::FallbackFailed })?; - // TODO: sort and truncate supports with MaxWinners + // TODO: sort and truncate supports with MaxWinners let solution = ReadySolution { supports: supports.try_into().unwrap(), score: Default::default(), @@ -1234,7 +1234,8 @@ pub mod pallet { /// Current best solution, signed or unsigned, queued to be returned upon `elect`. #[pallet::storage] #[pallet::getter(fn queued_solution)] - pub type QueuedSolution = StorageValue<_, ReadySolution>; + pub type QueuedSolution = + StorageValue<_, ReadySolution>; /// Snapshot data of the round. /// @@ -1574,18 +1575,20 @@ impl Pallet { // inexpensive (1 read of an empty vector). let _ = Self::finalize_signed_phase(); >::take() - // TODO: fix to one statement + // TODO: fix to one statement .ok_or(ElectionError::::NothingQueued) .or_else(|_| { ::elect() .map_err(|fe| ElectionError::Fallback(fe)) .and_then(|supports| { Ok(ReadySolution { - supports: supports.try_into().map_err(|_| FeasibilityError::BoundNotMet)?, + supports: supports + .try_into() + .map_err(|_| FeasibilityError::BoundNotMet)?, score: Default::default(), compute: ElectionCompute::Fallback, }) - }) + }) }) .map(|ReadySolution { compute, score, supports }| { Self::deposit_event(Event::ElectionFinalized { compute, score }); diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index d6e39e994b523..d1ea67dbad0b5 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -20,8 +20,8 @@ //! careful when using it onchain. use crate::{ - Debug, ElectionDataProvider, ElectionProviderBase, BoundedElectionProvider, InstantElectionProvider, - NposSolver, WeightInfo, BoundedSupportsOf, + BoundedElectionProvider, BoundedSupportsOf, Debug, ElectionDataProvider, ElectionProviderBase, + InstantElectionProvider, NposSolver, WeightInfo, }; use frame_support::{dispatch::DispatchClass, traits::Get}; use sp_npos_elections::*; @@ -138,8 +138,10 @@ fn elect_with( weight, DispatchClass::Mandatory, ); - - let supports = to_supports(&staked).try_into().map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; + + let supports = to_supports(&staked) + .try_into() + .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; Ok(supports) } @@ -213,7 +215,7 @@ impl InstantElectionProvider for BoundedExecution { #[cfg(test)] mod tests { use super::*; - use crate::{PhragMMS, SequentialPhragmen, ElectionProvider}; + use crate::{ElectionProvider, PhragMMS, SequentialPhragmen}; use frame_support::traits::ConstU32; use sp_npos_elections::Support; use sp_runtime::Perbill; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index e0820e4dfa30f..2d3b3c10092da 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, ElectionDataProvider, ElectionProvider, BoundedElectionProvider, ScoreProvider, SortedListProvider, - BoundedSupportsOf, VoteWeight, VoterOf, + data_provider, BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, + ElectionProvider, ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, @@ -44,8 +44,8 @@ use sp_std::{collections::btree_map::BTreeMap, prelude::*}; use crate::{ log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, Exposure, ExposureOf, - Forcing, IndividualExposure, Nominations, PositiveImbalanceOf, RewardDestination, - SessionInterface, StakingLedger, ValidatorPrefs, MaxWinnersOf, + Forcing, IndividualExposure, MaxWinnersOf, Nominations, PositiveImbalanceOf, RewardDestination, + SessionInterface, StakingLedger, ValidatorPrefs, }; use super::{pallet::*, STAKING_ID}; @@ -267,7 +267,10 @@ impl Pallet { } /// Plan a new session potentially trigger a new era. - fn new_session(session_index: SessionIndex, is_genesis: bool) -> Option>> { + fn new_session( + session_index: SessionIndex, + is_genesis: bool, + ) -> Option>> { if let Some(current_era) = Self::current_era() { // Initial era has been set. let current_era_start_session_index = Self::eras_start_session_index(current_era) @@ -426,7 +429,10 @@ impl Pallet { /// Returns the new validator set. pub fn trigger_new_era( start_session_index: SessionIndex, - exposures: BoundedVec<(T::AccountId, Exposure>), MaxWinnersOf>, + exposures: BoundedVec< + (T::AccountId, Exposure>), + MaxWinnersOf, + >, ) -> BoundedVec> { // Increment or set current era. let new_planned_era = CurrentEra::::mutate(|s| { @@ -454,23 +460,26 @@ impl Pallet { start_session_index: SessionIndex, is_genesis: bool, ) -> Option>> { - let election_result: BoundedVec<_, MaxWinnersOf> = if is_genesis { - let result = ::elect() - .map_err(|e| { + let result = + ::elect().map_err(|e| { log!(warn, "genesis election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); - }); - result.ok()?.into_inner().try_into().map_err(|e| { - // AKON: Bounds not met if genesis_max_winners > election_provider_max_winners - log!(warn, "genesis election provider failed due to {:?}", e); - Self::deposit_event(Event::StakingElectionFailed); - }).ok()? - } else { - let result = ::elect() + }); + result + .ok()? + .into_inner() + .try_into() .map_err(|e| { - log!(warn, "election provider failed due to {:?}", e); + // AKON: Bounds not met if genesis_max_winners > election_provider_max_winners + log!(warn, "genesis election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); + }) + .ok()? + } else { + let result = ::elect().map_err(|e| { + log!(warn, "election provider failed due to {:?}", e); + Self::deposit_event(Event::StakingElectionFailed); }); result.ok()? }; @@ -510,7 +519,10 @@ impl Pallet { /// /// Store staking information for the new planned era pub fn store_stakers_info( - exposures: BoundedVec<(T::AccountId, Exposure>), MaxWinnersOf>, + exposures: BoundedVec< + (T::AccountId, Exposure>), + MaxWinnersOf, + >, new_planned_era: EraIndex, ) -> BoundedVec> { let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); @@ -586,7 +598,9 @@ impl Pallet { (validator, exposure) }) // AKON: return result here? - .collect::)>>().try_into().unwrap() + .collect::)>>() + .try_into() + .unwrap() } /// Remove all associated data of a stash account from the staking system. From 129ce839e540e715026811c6b90623925985cd27 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 3 Oct 2022 15:06:26 +0200 Subject: [PATCH 07/66] fixing epmp tests, does not compile yet --- frame/election-provider-multi-phase/src/lib.rs | 15 ++++++++++----- frame/election-provider-multi-phase/src/mock.rs | 9 ++++++--- .../election-provider-multi-phase/src/unsigned.rs | 3 ++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 6224d9900d968..2c2c089e7a853 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -325,15 +325,17 @@ impl ElectionProviderBase for NoFallback { } } -impl ElectionProvider for NoFallback { - fn elect() -> Result, Self::Error> { +impl BoundedElectionProvider for NoFallback { + // AKON: Can probably be 0 but verify later + type MaxWinners = T::MaxWinners; + fn elect() -> Result, Self::Error> { // Do nothing, this will enable the emergency phase. Err("NoFallback.") } } impl InstantElectionProvider for NoFallback { - fn elect_with_bounds(_: usize, _: usize) -> Result, Self::Error> { + fn elect_with_bounds(_: usize, _: usize) -> Result::MaxWinners>, Self::Error> { Err("NoFallback.") } } @@ -1091,9 +1093,12 @@ pub mod pallet { Error::::FallbackFailed })?; - // TODO: sort and truncate supports with MaxWinners + // AKON: This is a hack to convert a BoundedVec to + // BoundedVec. May be there is a more elegant solution. + let supports: BoundedVec<_, T::MaxWinners> = supports.into_inner().try_into().unwrap(); + // AKON: sort and truncate supports with MaxWinners let solution = ReadySolution { - supports: supports.try_into().unwrap(), + supports, score: Default::default(), compute: ElectionCompute::Fallback, }; diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 4d0487dc6124f..8e76842134fe9 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -309,6 +309,7 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen, Balancing>; type DataProvider = StakingMock; type WeightInfo = (); + type MaxWinners = MaxWinners; } pub struct MockFallback; @@ -322,8 +323,10 @@ impl ElectionProviderBase for MockFallback { false } } -impl ElectionProvider for MockFallback { - fn elect() -> Result, Self::Error> { + +impl BoundedElectionProvider for MockFallback { + type MaxWinners = MaxWinners; + fn elect() -> Result, Self::Error> { Self::elect_with_bounds(Bounded::max_value(), Bounded::max_value()) } } @@ -332,7 +335,7 @@ impl InstantElectionProvider for MockFallback { fn elect_with_bounds( max_voters: usize, max_targets: usize, - ) -> Result, Self::Error> { + ) -> Result::MaxWinners>, Self::Error> { if OnChainFallback::get() { onchain::UnboundedExecution::::elect_with_bounds( max_voters, diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index f514f659d373a..36793268813bd 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1171,7 +1171,8 @@ mod tests { // set a better score let ready = ReadySolution { score: ElectionScore { minimal_stake: 10, ..Default::default() }, - ..Default::default() + supports: bounded_vec![], + compute: Default::default() }; >::put(ready); From c6d13604a3d189ba053e4c3c2d26c9f37ab084c4 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 09:55:41 +0200 Subject: [PATCH 08/66] fix epmp tests --- .../election-provider-multi-phase/src/lib.rs | 43 ++++++++++++------- .../election-provider-multi-phase/src/mock.rs | 18 +++++++- .../src/unsigned.rs | 2 +- primitives/npos-elections/src/lib.rs | 4 +- 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 2c2c089e7a853..324e6610550cb 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -335,7 +335,13 @@ impl BoundedElectionProvider for NoFallback { } impl InstantElectionProvider for NoFallback { - fn elect_with_bounds(_: usize, _: usize) -> Result::MaxWinners>, Self::Error> { + fn elect_with_bounds( + _: usize, + _: usize, + ) -> Result< + BoundedSupports::MaxWinners>, + Self::Error, + > { Err("NoFallback.") } } @@ -1874,7 +1880,6 @@ mod tests { }, Phase, }; - use frame_election_provider_support::ElectionProvider; use frame_support::{assert_noop, assert_ok}; use sp_npos_elections::{BalancingConfig, Support}; @@ -1929,7 +1934,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); assert!(MultiPhase::snapshot().is_some()); - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -1980,7 +1985,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_unsigned_open_at(20)); - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -2018,7 +2023,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_signed()); - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -2056,7 +2061,7 @@ mod tests { assert!(MultiPhase::current_phase().is_off()); // This module is now only capable of doing on-chain backup. - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert!(MultiPhase::current_phase().is_off()); @@ -2082,7 +2087,7 @@ mod tests { assert_eq!(MultiPhase::round(), 1); // An unexpected call to elect. - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); // We surely can't have any feasible solutions. This will cause an on-chain election. assert_eq!( @@ -2129,7 +2134,7 @@ mod tests { } // an unexpected call to elect. - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); // all storage items must be cleared. assert_eq!(MultiPhase::round(), 2); @@ -2179,7 +2184,7 @@ mod tests { )); roll_to(30); - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert_eq!( multi_phase_events(), @@ -2223,7 +2228,7 @@ mod tests { )); assert!(MultiPhase::queued_solution().is_some()); - assert_ok!(MultiPhase::elect()); + assert_ok!(::elect()); assert_eq!( multi_phase_events(), @@ -2255,7 +2260,7 @@ mod tests { // Zilch solutions thus far, but we get a result. assert!(MultiPhase::queued_solution().is_none()); - let supports = MultiPhase::elect().unwrap(); + let supports = ::elect().unwrap(); assert_eq!( supports, @@ -2288,7 +2293,10 @@ mod tests { // Zilch solutions thus far. assert!(MultiPhase::queued_solution().is_none()); - assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback.")); + assert_eq!( + ::elect().unwrap_err(), + ElectionError::Fallback("NoFallback.") + ); // phase is now emergency. assert_eq!(MultiPhase::current_phase(), Phase::Emergency); @@ -2311,7 +2319,10 @@ mod tests { // Zilch solutions thus far. assert!(MultiPhase::queued_solution().is_none()); - assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback.")); + assert_eq!( + ::elect().unwrap_err(), + ElectionError::Fallback("NoFallback.") + ); // phase is now emergency. assert_eq!(MultiPhase::current_phase(), Phase::Emergency); @@ -2328,7 +2339,7 @@ mod tests { // something is queued now assert!(MultiPhase::queued_solution().is_some()); // next election call with fix everything.; - assert!(MultiPhase::elect().is_ok()); + assert!(::elect().is_ok()); assert_eq!(MultiPhase::current_phase(), Phase::Off); assert_eq!( @@ -2365,7 +2376,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Off); // On-chain backup works though. - let supports = MultiPhase::elect().unwrap(); + let supports = ::elect().unwrap(); assert!(supports.len() > 0); assert_eq!( @@ -2395,7 +2406,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Off); roll_to(29); - let err = MultiPhase::elect().unwrap_err(); + let err = ::elect().unwrap_err(); assert_eq!(err, ElectionError::Fallback("NoFallback.")); assert_eq!(MultiPhase::current_phase(), Phase::Emergency); diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 8e76842134fe9..dc8f323898b97 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -335,7 +335,10 @@ impl InstantElectionProvider for MockFallback { fn elect_with_bounds( max_voters: usize, max_targets: usize, - ) -> Result::MaxWinners>, Self::Error> { + ) -> Result< + BoundedSupports::MaxWinners>, + Self::Error, + > { if OnChainFallback::get() { onchain::UnboundedExecution::::elect_with_bounds( max_voters, @@ -348,6 +351,19 @@ impl InstantElectionProvider for MockFallback { } } +impl PartialEq for ReadySolution { + fn eq(&self, other: &ReadySolution) -> bool { + self.score == other.score && self.compute == other.compute + } +} + +// AKON: Is there a better way? +impl std::fmt::Debug for MaxWinners { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("MaxWinners").field(&MaxWinners::get()).finish() + } +} + parameter_types! { pub static Balancing: Option = Some( BalancingConfig { iterations: 0, tolerance: 0 } ); } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 36793268813bd..876db44ab5a3d 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1172,7 +1172,7 @@ mod tests { let ready = ReadySolution { score: ElectionScore { minimal_stake: 10, ..Default::default() }, supports: bounded_vec![], - compute: Default::default() + compute: Default::default(), }; >::put(ready); diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 514ded67ad38b..d0c9ed18caddc 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -450,10 +450,10 @@ impl Default for Support { /// The main advantage of this is that it is encodable. pub type Supports = Vec<(A, Support)>; -/// Same as `Supports` bounded by `MaxWinners`. +/// Same as `Supports` but bounded by `B`. /// /// To note, the inner `Support` is still unbounded. -pub type BoundedSupports = BoundedVec<(A, Support), MaxWinners>; +pub type BoundedSupports = BoundedVec<(A, Support), B>; /// Linkage from a winner to their [`Support`]. /// From e8012d2e5dbbc0e919485231d6ccb0096c8cd2b8 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 10:07:29 +0200 Subject: [PATCH 09/66] fix staking tests --- frame/staking/src/mock.rs | 2 ++ frame/staking/src/pallet/impls.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index be2a7961be610..f5c10af15455b 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -240,6 +240,7 @@ parameter_types! { pub static MaxUnlockingChunks: u32 = 32; pub static RewardOnUnbalanceWasCalled: bool = false; pub static LedgerSlashPerEra: (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); + pub static MaxWinners: u32 = 100; } type VoterBagsListInstance = pallet_bags_list::Instance1; @@ -258,6 +259,7 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = MaxWinners; } pub struct MockReward {} diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 2d3b3c10092da..292cf42d597dd 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, BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, - ElectionProvider, ScoreProvider, SortedListProvider, VoteWeight, VoterOf, + ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, From eb5753054d517bc78cd8cebd91f9589a70cfd195 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 10:07:50 +0200 Subject: [PATCH 10/66] fmt --- frame/staking/src/pallet/impls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 292cf42d597dd..5778a7b72f4fa 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, - ScoreProvider, SortedListProvider, VoteWeight, VoterOf, + data_provider, BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, ScoreProvider, + SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, From defea25113195e8589bbe09b619a60e6373f0c39 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 10:52:21 +0200 Subject: [PATCH 11/66] fix runtime tests --- bin/node/runtime/src/lib.rs | 3 +++ frame/babe/src/mock.rs | 1 + frame/election-provider-multi-phase/src/mock.rs | 3 ++- frame/election-provider-support/src/lib.rs | 7 ++++--- frame/fast-unstake/src/mock.rs | 6 ++++-- frame/grandpa/src/mock.rs | 1 + frame/root-offences/src/mock.rs | 1 + 7 files changed, 16 insertions(+), 6 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index c2d29731ea2e6..604912b8384e0 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -625,6 +625,7 @@ frame_election_provider_support::generate_solution_type!( parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; pub MaxElectingVoters: u32 = 10_000; + pub MaxWinners: u32 = 1000; } /// The numbers configured here could always be more than the the maximum limits of staking pallet @@ -675,6 +676,7 @@ impl onchain::Config for OnChainSeqPhragmen { >; type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; + type MaxWinners = ::MaxWinners; } impl onchain::BoundedConfig for OnChainSeqPhragmen { @@ -727,6 +729,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type Solver = SequentialPhragmen, OffchainRandomBalancing>; type ForceOrigin = EnsureRootOrHalfCouncil; type MaxElectableTargets = ConstU16<{ u16::MAX }>; + type MaxWinners = MaxWinners; type MaxElectingVoters = MaxElectingVoters; type BenchmarkingConfig = ElectionProviderBenchmarkConfig; type WeightInfo = pallet_election_provider_multi_phase::weights::SubstrateWeight; diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 46aeabe743fe2..d23b43035d36a 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -178,6 +178,7 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = ConstU32<100>; } impl pallet_staking::Config for Test { diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index dc8f323898b97..030f6da6c745f 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -354,10 +354,11 @@ impl InstantElectionProvider for MockFallback { impl PartialEq for ReadySolution { fn eq(&self, other: &ReadySolution) -> bool { self.score == other.score && self.compute == other.compute + && self.supports.clone().into_inner() == other.supports.clone().into_inner() } } -// AKON: Is there a better way? +// AKON: Is there another way? impl std::fmt::Debug for MaxWinners { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_tuple("MaxWinners").field(&MaxWinners::get()).finish() diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 795870d9606a6..1f47914cc2c13 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -458,13 +458,14 @@ where } #[cfg(feature = "std")] -impl ElectionProvider +impl BoundedElectionProvider for NoElection<(AccountId, BlockNumber, DataProvider)> where DataProvider: ElectionDataProvider, { - fn elect() -> Result, Self::Error> { - Err(" cannot do anything.") + type MaxWinners = frame_support::traits::ConstU32<0>; + fn elect() -> Result, Self::Error> { + Err(" cannot do anything.") } } diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index 71fc2d4ba905a..5bd0cfc2aadee 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -103,6 +103,7 @@ parameter_types! { pub static BondingDuration: u32 = 3; pub static CurrentEra: u32 = 0; pub static Ongoing: bool = false; + pub static MaxWinners: u32 = 100; } pub struct MockElection; @@ -117,8 +118,9 @@ impl frame_election_provider_support::ElectionProviderBase for MockElection { } } -impl frame_election_provider_support::ElectionProvider for MockElection { - fn elect() -> Result, Self::Error> { +impl frame_election_provider_support::BoundedElectionProvider for MockElection { + type MaxWinners = MaxWinners; + fn elect() -> Result, Self::Error> { Err(()) } } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 573a74d2bfae8..00e53c55ecb68 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -182,6 +182,7 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = ConstU32<100>; } impl pallet_staking::Config for Test { diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index 3f0a26afc1358..cef7c2c854700 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -145,6 +145,7 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = ConstU32<100>; } pub struct OnStakerSlashMock(core::marker::PhantomData); From f4dc053250bd988020a47ca97578a5e160860817 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 10:52:45 +0200 Subject: [PATCH 12/66] fmt --- frame/election-provider-multi-phase/src/mock.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 030f6da6c745f..2dea52c329e62 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -353,8 +353,9 @@ impl InstantElectionProvider for MockFallback { impl PartialEq for ReadySolution { fn eq(&self, other: &ReadySolution) -> bool { - self.score == other.score && self.compute == other.compute - && self.supports.clone().into_inner() == other.supports.clone().into_inner() + self.score == other.score && + self.compute == other.compute && + self.supports.clone().into_inner() == other.supports.clone().into_inner() } } From 40765c82b8f9c59e65a670c43eff7c9993b976dc Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 11:25:54 +0200 Subject: [PATCH 13/66] remove outdated wip tags --- frame/election-provider-multi-phase/src/lib.rs | 9 +++------ frame/staking/src/mock.rs | 1 - frame/staking/src/pallet/impls.rs | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 324e6610550cb..028b92a56a27e 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -326,7 +326,6 @@ impl ElectionProviderBase for NoFallback { } impl BoundedElectionProvider for NoFallback { - // AKON: Can probably be 0 but verify later type MaxWinners = T::MaxWinners; fn elect() -> Result, Self::Error> { // Do nothing, this will enable the emergency phase. @@ -1258,13 +1257,13 @@ pub mod pallet { /// Desired number of targets to elect for this round. /// /// Only exists when [`Snapshot`] is present. - // WIP: call desired winners + // AKON: call desired winners? + // AKON: ensure desired_targets < MaxWinners when changing this value #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; - // WIP: desired_targets < MaxWinners - + /// The metadata of the [`RoundSnapshot`] /// /// Only exists when [`Snapshot`] is present. @@ -1586,7 +1585,6 @@ impl Pallet { // inexpensive (1 read of an empty vector). let _ = Self::finalize_signed_phase(); >::take() - // TODO: fix to one statement .ok_or(ElectionError::::NothingQueued) .or_else(|_| { ::elect() @@ -1651,7 +1649,6 @@ impl BoundedElectionProvider for Pallet { // All went okay, record the weight, put sign to be Off, clean snapshot, etc. Self::weigh_supports(&supports); Self::rotate_round(); - // WIP: Fix unwrap Ok(supports) }, Err(why) => { diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index f5c10af15455b..b07c526d557ce 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -299,7 +299,6 @@ impl crate::pallet::pallet::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; - // AKON: Implement BoundedElectionProvider type ElectionProvider = onchain::UnboundedExecution; type GenesisElectionProvider = Self::ElectionProvider; // NOTE: consider a macro and use `UseNominatorsAndValidatorsMap` as well. diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 5778a7b72f4fa..df062acab9c8b 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1106,7 +1106,6 @@ impl ElectionDataProvider for Pallet { /// Once the first new_session is planned, all session must start and then end in order, though /// some session can lag in between the newest session planned and the latest session started. impl pallet_session::SessionManager for Pallet { - // AKON: session functions, dont change their signatures fn new_session(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {}", new_index); CurrentPlannedSession::::put(new_index); From 4084fa8d0f8be2d6b80a6fc5b8be301b4b722f39 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 11:56:57 +0200 Subject: [PATCH 14/66] add enum error --- frame/election-provider-multi-phase/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 028b92a56a27e..55a67f10b56c1 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -985,7 +985,7 @@ pub mod pallet { // `ElectionProvider::elect` will succeed and take care of that. let solution = ReadySolution { - supports: supports.try_into().map_err(|_| "BoundNotMet")?, + supports: supports.try_into().map_err(|_| >::BoundNotMet)?, score: Default::default(), compute: ElectionCompute::Emergency, }; @@ -1171,6 +1171,8 @@ pub mod pallet { CallNotAllowed, /// The fallback failed FallbackFailed, + /// Some bound not met + BoundNotMet, } #[pallet::validate_unsigned] From 3a4058b9eb86a840143597fd93c26d11fe447188 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 12:24:13 +0200 Subject: [PATCH 15/66] sort and truncate supports --- frame/election-provider-multi-phase/src/lib.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 55a67f10b56c1..d71103686e86a 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1089,7 +1089,7 @@ pub mod pallet { let maybe_max_voters = maybe_max_voters.map(|x| x as usize); let maybe_max_targets = maybe_max_targets.map(|x| x as usize); - let supports = T::GovernanceFallback::elect_with_bounds( + let mut supports = T::GovernanceFallback::elect_with_bounds( maybe_max_voters.unwrap_or(Bounded::max_value()), maybe_max_targets.unwrap_or(Bounded::max_value()), ) @@ -1098,10 +1098,17 @@ pub mod pallet { Error::::FallbackFailed })?; + // AKON: verify sort and truncate in a test + + // sort by total balance of the `supports` + supports.sort_by(|a, b| a.1.total.partial_cmp(&b.1.total).unwrap()); + // truncate GovernanceFallback::MaxWinners by pallet::Config::MaxWinners + supports.truncate(T::MaxWinners::get().try_into().unwrap()); + // AKON: This is a hack to convert a BoundedVec to // BoundedVec. May be there is a more elegant solution. let supports: BoundedVec<_, T::MaxWinners> = supports.into_inner().try_into().unwrap(); - // AKON: sort and truncate supports with MaxWinners + let solution = ReadySolution { supports, score: Default::default(), From f47d8c859974566d2b5f0d7a13900e2871a6ef3e Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 12:30:56 +0200 Subject: [PATCH 16/66] comment --- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index d71103686e86a..0bdb0b061cb37 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1105,8 +1105,8 @@ pub mod pallet { // truncate GovernanceFallback::MaxWinners by pallet::Config::MaxWinners supports.truncate(T::MaxWinners::get().try_into().unwrap()); - // AKON: This is a hack to convert a BoundedVec to - // BoundedVec. May be there is a more elegant solution. + // AKON: This is an ugly hack to convert a BoundedVec to + // BoundedVec. May be impl try_from to do this conversion? let supports: BoundedVec<_, T::MaxWinners> = supports.into_inner().try_into().unwrap(); let solution = ReadySolution { From a96fb7202a37015effe1657b602df4c6c35628c7 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 5 Oct 2022 13:58:18 +0200 Subject: [PATCH 17/66] error when unsupported number of election winners --- frame/staking/src/pallet/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 8a147c19be1f8..0cf824f5a9f3b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -750,6 +750,10 @@ pub mod pallet { CommissionTooLow, /// Some bound is not met. BoundNotMet, + /// Validator count exceeded the maximum supported by the `ElectionProvider`. + // AKON: sounds like too many validators but should probably + // differentiate from it. + TooManyElectionWinners, } #[pallet::hooks] @@ -1264,6 +1268,10 @@ pub mod pallet { #[pallet::compact] new: u32, ) -> DispatchResult { ensure_root(origin)?; + // AKON:: test new number is less than maxwinners + // max winners supported by election provider + let max_winners = ::MaxWinners::get(); + ensure!(new > max_winners, Error::::TooManyElectionWinners); ValidatorCount::::put(new); Ok(()) } From c32c53429c47ba1bf5b69bb20d669b3a3bab3422 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 12:26:18 +0200 Subject: [PATCH 18/66] compiling wip after kian's suggestions --- .../election-provider-multi-phase/src/lib.rs | 39 +++++++++++++++---- .../election-provider-multi-phase/src/mock.rs | 16 +------- .../election-provider-support/src/onchain.rs | 26 +++++++++++-- frame/staking/src/pallet/impls.rs | 4 +- frame/staking/src/pallet/mod.rs | 5 ++- 5 files changed, 59 insertions(+), 31 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 0bdb0b061cb37..e1f5fa55b6c5c 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -716,6 +716,33 @@ pub mod pallet { DataProvider = Self::DataProvider, >; + // // approach 1 + // #[pallet::hooks] + // impl Pallet { + // fn integrity_check() { + // // construct-runtime check -- can panic here + // assert!(T::MaxWinners::get() == T::GovernanceFallback::MaxWinners::get()); + // } + // } + + // fn governance_fallback() -> { + // let support: Supports: BoundedVec<_, T::GovernanceFallback::MaxWinners> = T::GovernanceFallback::elect(); + // let other_supports: Supports: BoundedVec<_, T::MaxWinners> = support + // .into_inner() + // .try_into() + // .expect("bounds are checked to be same in integrity_check; conversion work; qed"); + // } + + + + // // approach #2 + // parmeter_type! { + // // Config::MaxWinners + // type Foo: u32 = 10; + // // GovernanceFallback::MaxWinners + // type Bar: u32 = 10; + // } + /// OCW election solution miner algorithm implementation. type Solver: NposSolver; @@ -887,6 +914,10 @@ pub mod pallet { // `SignedMaxSubmissions` is a red flag that the developer does not understand how to // configure this pallet. assert!(T::SignedMaxSubmissions::get() >= T::SignedMaxRefunds::get()); + // We expect the same bounds on election results provided by the + // `ElectionProvider` implementations of the pallet and + // `GovernanceFallback`. + assert!(T::MaxWinners::get() == <::GovernanceFallback as BoundedElectionProvider>::MaxWinners::get()); } } @@ -1098,13 +1129,6 @@ pub mod pallet { Error::::FallbackFailed })?; - // AKON: verify sort and truncate in a test - - // sort by total balance of the `supports` - supports.sort_by(|a, b| a.1.total.partial_cmp(&b.1.total).unwrap()); - // truncate GovernanceFallback::MaxWinners by pallet::Config::MaxWinners - supports.truncate(T::MaxWinners::get().try_into().unwrap()); - // AKON: This is an ugly hack to convert a BoundedVec to // BoundedVec. May be impl try_from to do this conversion? let supports: BoundedVec<_, T::MaxWinners> = supports.into_inner().try_into().unwrap(); @@ -1267,7 +1291,6 @@ pub mod pallet { /// /// Only exists when [`Snapshot`] is present. // AKON: call desired winners? - // AKON: ensure desired_targets < MaxWinners when changing this value #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 2dea52c329e62..f558724c4a303 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -297,6 +297,7 @@ parameter_types! { pub static MockWeightInfo: MockedWeightInfo = MockedWeightInfo::Real; pub static MaxElectingVoters: VoterIndex = u32::max_value(); pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); + #[derive(Eq, PartialEq, Debug)] pub static MaxWinners: u32 = 100; pub static EpochLength: u64 = 30; @@ -351,21 +352,6 @@ impl InstantElectionProvider for MockFallback { } } -impl PartialEq for ReadySolution { - fn eq(&self, other: &ReadySolution) -> bool { - self.score == other.score && - self.compute == other.compute && - self.supports.clone().into_inner() == other.supports.clone().into_inner() - } -} - -// AKON: Is there another way? -impl std::fmt::Debug for MaxWinners { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("MaxWinners").field(&MaxWinners::get()).finish() - } -} - parameter_types! { pub static Balancing: Option = Some( BalancingConfig { iterations: 0, tolerance: 0 } ); } diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index d1ea67dbad0b5..f59acb91606d2 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -69,6 +69,12 @@ pub struct BoundedExecution(PhantomData); /// This can be very expensive to run frequently on-chain. Use with care. pub struct UnboundedExecution(PhantomData); +pub enum TooManyWinners { + Error, + Truncate, + SortAndTruncate, +} + /// Configuration trait for an onchain election execution. pub trait Config { /// Needed for weight registration. @@ -86,8 +92,11 @@ pub trait Config { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// Upper bound on maximum winners from electable targets + /// Upper bound on maximum winners from electable targets, and how to deal with it if we have + /// Too many. type MaxWinners: Get; + + // type TooManyWinners: Get; } pub trait BoundedConfig: Config { @@ -139,9 +148,18 @@ fn elect_with( DispatchClass::Mandatory, ); - let supports = to_supports(&staked) - .try_into() - .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; + let supports = + // match T::TooManyWinners::get() { + // TooManyWinners::Error => { + to_supports(&staked) + .try_into() + .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; + // }, + // _ => { + // todo!(); + // } + // }; + Ok(supports) } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index df062acab9c8b..53f634222af85 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -561,7 +561,7 @@ impl Pallet { } // AKON: return result here? - elected_stashes.try_into().unwrap() + elected_stashes.try_into().expect("reasons why; qed;") } /// Consume a set of [`BoundedSupports`] from [`sp_npos_elections`] and collect them into a @@ -597,7 +597,7 @@ impl Pallet { let exposure = Exposure { own, others, total }; (validator, exposure) }) - // AKON: return result here? + // AKON: return result here? try_collect .collect::)>>() .try_into() .unwrap() diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 0cf824f5a9f3b..6a9af49ab823b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -743,7 +743,7 @@ pub mod pallet { /// There are too many nominators in the system. Governance needs to adjust the staking /// settings to keep things safe for the runtime. TooManyNominators, - /// There are too many validators in the system. Governance needs to adjust the staking + /// There are too many validator candidates in the system. Governance needs to adjust the staking /// settings to keep things safe for the runtime. TooManyValidators, /// Commission is too low. Must be at least `MinCommission`. @@ -1262,6 +1262,7 @@ pub mod pallet { /// Weight: O(1) /// Write: Validator Count /// # + // AKON: Probably change name? #[pallet::weight(T::WeightInfo::set_validator_count())] pub fn set_validator_count( origin: OriginFor, @@ -1271,7 +1272,7 @@ pub mod pallet { // AKON:: test new number is less than maxwinners // max winners supported by election provider let max_winners = ::MaxWinners::get(); - ensure!(new > max_winners, Error::::TooManyElectionWinners); + ensure!(new > max_winners, Error::::TooManyValidators); ValidatorCount::::put(new); Ok(()) } From 53aba74a3c2331d54871fbe0a05edd04b52dc0e8 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 14:11:46 +0200 Subject: [PATCH 19/66] fix TODOs --- .../election-provider-multi-phase/src/lib.rs | 21 ++++++++-------- .../election-provider-support/src/onchain.rs | 2 +- frame/staking/src/pallet/impls.rs | 25 +++++++------------ frame/staking/src/pallet/mod.rs | 19 ++++++++------ 4 files changed, 31 insertions(+), 36 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index e1f5fa55b6c5c..885018217c640 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -726,15 +726,13 @@ pub mod pallet { // } // fn governance_fallback() -> { - // let support: Supports: BoundedVec<_, T::GovernanceFallback::MaxWinners> = T::GovernanceFallback::elect(); - // let other_supports: Supports: BoundedVec<_, T::MaxWinners> = support - // .into_inner() + // let support: Supports: BoundedVec<_, T::GovernanceFallback::MaxWinners> = + // T::GovernanceFallback::elect(); let other_supports: Supports: BoundedVec<_, + // T::MaxWinners> = support .into_inner() // .try_into() // .expect("bounds are checked to be same in integrity_check; conversion work; qed"); // } - - // // approach #2 // parmeter_type! { // // Config::MaxWinners @@ -916,7 +914,7 @@ pub mod pallet { assert!(T::SignedMaxSubmissions::get() >= T::SignedMaxRefunds::get()); // We expect the same bounds on election results provided by the // `ElectionProvider` implementations of the pallet and - // `GovernanceFallback`. + // `GovernanceFallback`. assert!(T::MaxWinners::get() == <::GovernanceFallback as BoundedElectionProvider>::MaxWinners::get()); } } @@ -1129,9 +1127,12 @@ pub mod pallet { Error::::FallbackFailed })?; - // AKON: This is an ugly hack to convert a BoundedVec to - // BoundedVec. May be impl try_from to do this conversion? - let supports: BoundedVec<_, T::MaxWinners> = supports.into_inner().try_into().unwrap(); + // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into `BoundedVec<_, + // T::MaxWinners>` + let supports: BoundedVec<_, T::MaxWinners> = supports + .into_inner() + .try_into() + .expect("integrity test guarantees both bounds are always equal; qed"); let solution = ReadySolution { supports, @@ -1290,12 +1291,10 @@ pub mod pallet { /// Desired number of targets to elect for this round. /// /// Only exists when [`Snapshot`] is present. - // AKON: call desired winners? #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; - /// The metadata of the [`RoundSnapshot`] /// /// Only exists when [`Snapshot`] is present. diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index f59acb91606d2..5fb5c837b37c1 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -154,7 +154,7 @@ fn elect_with( to_supports(&staked) .try_into() .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; - // }, + // }, // _ => { // todo!(); // } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 53f634222af85..80117d4038967 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -26,7 +26,7 @@ use frame_support::{ pallet_prelude::*, traits::{ Currency, CurrencyToVote, Defensive, DefensiveResult, EstimateNextNewSession, Get, - Imbalance, LockableCurrency, OnUnbalanced, UnixTime, WithdrawReasons, + Imbalance, LockableCurrency, OnUnbalanced, TryCollect, UnixTime, WithdrawReasons, }, weights::Weight, }; @@ -466,16 +466,12 @@ impl Pallet { log!(warn, "genesis election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); }); + result - .ok()? - .into_inner() - .try_into() - .map_err(|e| { - // AKON: Bounds not met if genesis_max_winners > election_provider_max_winners - log!(warn, "genesis election provider failed due to {:?}", e); - Self::deposit_event(Event::StakingElectionFailed); - }) - .ok()? + .ok()? + .into_inner() + .try_into() + .expect("integrity test guarantees bounds for genesis and election provider are always equal; qed") } else { let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); @@ -560,8 +556,7 @@ impl Pallet { ); } - // AKON: return result here? - elected_stashes.try_into().expect("reasons why; qed;") + elected_stashes.try_into().expect("since we only map through exposures, size of elected_stashes is always same as exposures; qed") } /// Consume a set of [`BoundedSupports`] from [`sp_npos_elections`] and collect them into a @@ -597,10 +592,8 @@ impl Pallet { let exposure = Exposure { own, others, total }; (validator, exposure) }) - // AKON: return result here? try_collect - .collect::)>>() - .try_into() - .unwrap() + .try_collect() + .expect("we only map through support vector which cannot change the size; qed") } /// Remove all associated data of a stash account from the staking system. diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 6a9af49ab823b..3711c29c5f160 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -17,7 +17,7 @@ //! Staking FRAME Pallet. -use frame_election_provider_support::{SortedListProvider, VoteWeight}; +use frame_election_provider_support::{BoundedElectionProvider, SortedListProvider, VoteWeight}; use frame_support::{ dispatch::Codec, pallet_prelude::*, @@ -107,7 +107,7 @@ pub mod pallet { type CurrencyToVote: CurrencyToVote>; /// Something that provides the election functionality. - type ElectionProvider: frame_election_provider_support::BoundedElectionProvider< + type ElectionProvider: BoundedElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, // we only accept an election provider that has staking as data provider. @@ -115,7 +115,7 @@ pub mod pallet { >; /// Something that provides the election functionality at genesis. - type GenesisElectionProvider: frame_election_provider_support::BoundedElectionProvider< + type GenesisElectionProvider: BoundedElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Pallet, @@ -743,16 +743,14 @@ pub mod pallet { /// There are too many nominators in the system. Governance needs to adjust the staking /// settings to keep things safe for the runtime. TooManyNominators, - /// There are too many validator candidates in the system. Governance needs to adjust the staking - /// settings to keep things safe for the runtime. + /// There are too many validator candidates in the system. Governance needs to adjust the + /// staking settings to keep things safe for the runtime. TooManyValidators, /// Commission is too low. Must be at least `MinCommission`. CommissionTooLow, /// Some bound is not met. BoundNotMet, /// Validator count exceeded the maximum supported by the `ElectionProvider`. - // AKON: sounds like too many validators but should probably - // differentiate from it. TooManyElectionWinners, } @@ -786,6 +784,11 @@ pub mod pallet { // and that MaxNominations is always greater than 1, since we count on this. assert!(!T::MaxNominations::get().is_zero()); + // ensure election results are always bounded with the same value + assert!( + ::MaxWinners::get() == + ::MaxWinners::get() + ); sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| assert!( @@ -1271,7 +1274,7 @@ pub mod pallet { ensure_root(origin)?; // AKON:: test new number is less than maxwinners // max winners supported by election provider - let max_winners = ::MaxWinners::get(); + let max_winners = ::MaxWinners::get(); ensure!(new > max_winners, Error::::TooManyValidators); ValidatorCount::::put(new); Ok(()) From d4e536749105a441e2a8768811a97184b92cb22b Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 14:21:16 +0200 Subject: [PATCH 20/66] remove,fix tags --- frame/staking/src/pallet/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 3711c29c5f160..826af37093672 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1265,14 +1265,13 @@ pub mod pallet { /// Weight: O(1) /// Write: Validator Count /// # - // AKON: Probably change name? #[pallet::weight(T::WeightInfo::set_validator_count())] pub fn set_validator_count( origin: OriginFor, #[pallet::compact] new: u32, ) -> DispatchResult { ensure_root(origin)?; - // AKON:: test new number is less than maxwinners + // TODO:: test for new number is less than maxwinners // max winners supported by election provider let max_winners = ::MaxWinners::get(); ensure!(new > max_winners, Error::::TooManyValidators); From 7a7f1af2d9a00ca785255e73b2815c9e9d49eb90 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 14:36:22 +0200 Subject: [PATCH 21/66] ensure validator count does not exceed maxwinners --- frame/staking/src/pallet/mod.rs | 3 +-- frame/staking/src/tests.rs | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 826af37093672..2b38de54d6a70 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1271,10 +1271,9 @@ pub mod pallet { #[pallet::compact] new: u32, ) -> DispatchResult { ensure_root(origin)?; - // TODO:: test for new number is less than maxwinners // max winners supported by election provider let max_winners = ::MaxWinners::get(); - ensure!(new > max_winners, Error::::TooManyValidators); + ensure!(new <= max_winners, Error::::TooManyElectionWinners); ValidatorCount::::put(new); Ok(()) } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 4812c105c0d80..bc845f780dc54 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5624,3 +5624,18 @@ fn reducing_max_unlocking_chunks_abrupt() { MaxUnlockingChunks::set(2); }) } + +#[test] +fn cannot_set_unsupported_validator_count() { + ExtBuilder::default().build_and_execute(|| { + MaxWinners::set(50); + // set validator count works + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 30)); + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 50)); + // setting validator count above 100 does not work + assert_noop!( + Staking::set_validator_count(RuntimeOrigin::root(), 51), + Error::::TooManyElectionWinners, + ); + }) +} From 74485d37b13c91a933ef4769a51207c323fd8172 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 14:39:35 +0200 Subject: [PATCH 22/66] clean up --- .../election-provider-multi-phase/src/lib.rs | 27 +------------------ frame/staking/src/pallet/impls.rs | 2 +- 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 885018217c640..027ec60068929 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -716,31 +716,6 @@ pub mod pallet { DataProvider = Self::DataProvider, >; - // // approach 1 - // #[pallet::hooks] - // impl Pallet { - // fn integrity_check() { - // // construct-runtime check -- can panic here - // assert!(T::MaxWinners::get() == T::GovernanceFallback::MaxWinners::get()); - // } - // } - - // fn governance_fallback() -> { - // let support: Supports: BoundedVec<_, T::GovernanceFallback::MaxWinners> = - // T::GovernanceFallback::elect(); let other_supports: Supports: BoundedVec<_, - // T::MaxWinners> = support .into_inner() - // .try_into() - // .expect("bounds are checked to be same in integrity_check; conversion work; qed"); - // } - - // // approach #2 - // parmeter_type! { - // // Config::MaxWinners - // type Foo: u32 = 10; - // // GovernanceFallback::MaxWinners - // type Bar: u32 = 10; - // } - /// OCW election solution miner algorithm implementation. type Solver: NposSolver; @@ -1132,7 +1107,7 @@ pub mod pallet { let supports: BoundedVec<_, T::MaxWinners> = supports .into_inner() .try_into() - .expect("integrity test guarantees both bounds are always equal; qed"); + .expect("integrity test guarantees both bounds are equal; qed"); let solution = ReadySolution { supports, diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 80117d4038967..ff06632c67bce 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -471,7 +471,7 @@ impl Pallet { .ok()? .into_inner() .try_into() - .expect("integrity test guarantees bounds for genesis and election provider are always equal; qed") + .expect("both bounds checked in integrity test to be equal; qed") } else { let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); From 476a78f6eb009ebcc07bf61d166f7d8823f7f164 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 15:03:42 +0200 Subject: [PATCH 23/66] some more clean up and todos --- frame/election-provider-multi-phase/src/lib.rs | 8 ++++---- frame/election-provider-support/src/onchain.rs | 5 +++-- frame/staking/src/lib.rs | 6 ------ 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 027ec60068929..b5b4da81765c2 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -987,7 +987,7 @@ pub mod pallet { // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. - + // TODO: Do not error here let solution = ReadySolution { supports: supports.try_into().map_err(|_| >::BoundNotMet)?, score: Default::default(), @@ -1093,7 +1093,7 @@ pub mod pallet { let maybe_max_voters = maybe_max_voters.map(|x| x as usize); let maybe_max_targets = maybe_max_targets.map(|x| x as usize); - let mut supports = T::GovernanceFallback::elect_with_bounds( + let supports = T::GovernanceFallback::elect_with_bounds( maybe_max_voters.unwrap_or(Bounded::max_value()), maybe_max_targets.unwrap_or(Bounded::max_value()), ) @@ -1102,8 +1102,8 @@ pub mod pallet { Error::::FallbackFailed })?; - // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into `BoundedVec<_, - // T::MaxWinners>` + // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into + // `BoundedVec<_, T::MaxWinners>` let supports: BoundedVec<_, T::MaxWinners> = supports .into_inner() .try_into() diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 5fb5c837b37c1..ac4567971ddd6 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -148,6 +148,7 @@ fn elect_with( DispatchClass::Mandatory, ); + // TODO: Handle TooManyWinners error let supports = // match T::TooManyWinners::get() { // TooManyWinners::Error => { @@ -289,7 +290,7 @@ mod tests { type Solver = SequentialPhragmen; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - type MaxWinners = ConstU32<50>; + type MaxWinners = ConstU32<100>; } impl BoundedConfig for PhragmenParams { @@ -302,7 +303,7 @@ mod tests { type Solver = PhragMMS; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - type MaxWinners = ConstU32<50>; + type MaxWinners = ConstU32<100>; } impl BoundedConfig for PhragMMSParams { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 6436a115b163b..b7015df581b56 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -337,12 +337,6 @@ macro_rules! log { /// pallet. pub type MaxWinnersOf = <::ElectionProvider as frame_election_provider_support::BoundedElectionProvider>::MaxWinners; -// impl, S2: Get> TryFrom> for BoundedVec { -// type Error = (); -// fn try_from(x: BoundedVec) -> Result, Self::Error> { -// if x.len() <= Self::bound -// } -// } /// Counter for the number of "reward" points earned by a given validator. pub type RewardPoint = u32; From 8db0f9e56a0b895b1d66a636bfcb38acb2c1b04c Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 20:45:42 +0200 Subject: [PATCH 24/66] handle too many winners --- .../election-provider-support/src/onchain.rs | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index ac4567971ddd6..cc3925c7230ee 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -69,7 +69,8 @@ pub struct BoundedExecution(PhantomData); /// This can be very expensive to run frequently on-chain. Use with care. pub struct UnboundedExecution(PhantomData); -pub enum TooManyWinners { +#[derive(Copy, Clone, PartialEq, Eq)] +pub enum TooManyWinnersResolution { Error, Truncate, SortAndTruncate, @@ -96,7 +97,8 @@ pub trait Config { /// Too many. type MaxWinners: Get; - // type TooManyWinners: Get; + /// Action to take if there are too many winners + type TooManyWinnersResolution: Get; } pub trait BoundedConfig: Config { @@ -148,18 +150,25 @@ fn elect_with( DispatchClass::Mandatory, ); - // TODO: Handle TooManyWinners error - let supports = - // match T::TooManyWinners::get() { - // TooManyWinners::Error => { - to_supports(&staked) + // TODO: Add test for TooManyWinnersResolution variants + let mut supports = to_supports(&staked); + let supports: OnChainBoundedSupportsOf = + match T::TooManyWinnersResolution::get() { + TooManyWinnersResolution::Error => { + supports .try_into() - .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?; - // }, - // _ => { - // todo!(); - // } - // }; + .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))? + }, + TooManyWinnersResolution::Truncate => { + supports.truncate(T::MaxWinners::get() as usize); + supports.try_into().expect("we truncated to the bound so this always works; qed") + }, + TooManyWinnersResolution::SortAndTruncate => { + supports.sort_by(|a, b| a.1.total.partial_cmp(&b.1.total).unwrap()); + supports.truncate(T::MaxWinners::get() as usize); + supports.try_into().expect("we truncated to the bound so this always works; qed") + } + }; Ok(supports) } @@ -285,12 +294,17 @@ mod tests { struct PhragmenParams; struct PhragMMSParams; + frame_support::parameter_types! { + pub static Tmw: TooManyWinnersResolution = TooManyWinnersResolution::Error; + } + impl Config for PhragmenParams { type System = Runtime; type Solver = SequentialPhragmen; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type TooManyWinnersResolution = Tmw; } impl BoundedConfig for PhragmenParams { @@ -298,12 +312,14 @@ mod tests { type TargetsBound = ConstU32<400>; } + // TODO: test TooManyErrors impl Config for PhragMMSParams { type System = Runtime; type Solver = PhragMMS; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type TooManyWinnersResolution = Tmw; } impl BoundedConfig for PhragMMSParams { From ac9d7c2554f5ccba147c5bd30a68c7c9ecbc936a Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 6 Oct 2022 20:47:44 +0200 Subject: [PATCH 25/66] rename parameter for mock --- frame/election-provider-support/src/onchain.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index cc3925c7230ee..bab72c34543bd 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -295,7 +295,7 @@ mod tests { struct PhragMMSParams; frame_support::parameter_types! { - pub static Tmw: TooManyWinnersResolution = TooManyWinnersResolution::Error; + pub static ErrorResolution: TooManyWinnersResolution = TooManyWinnersResolution::Error; } impl Config for PhragmenParams { @@ -304,7 +304,7 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = ConstU32<100>; - type TooManyWinnersResolution = Tmw; + type TooManyWinnersResolution = ErrorResolution; } impl BoundedConfig for PhragmenParams { @@ -319,7 +319,7 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = ConstU32<100>; - type TooManyWinnersResolution = Tmw; + type TooManyWinnersResolution = ErrorResolution; } impl BoundedConfig for PhragMMSParams { From c555de53fff957b08cbc514d2074108e8bb13707 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 7 Oct 2022 15:07:23 +0200 Subject: [PATCH 26/66] todo --- frame/election-provider-multi-phase/src/lib.rs | 9 +++++++-- frame/election-provider-multi-phase/src/mock.rs | 2 ++ frame/election-provider-support/src/onchain.rs | 1 - 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index b5b4da81765c2..9d2f8737aeb91 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -985,11 +985,16 @@ pub mod pallet { T::ForceOrigin::ensure_origin(origin)?; ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); + // Truncate supports upto MaxWinners bound + let mut supports = supports; + supports.truncate(T::MaxWinners::get() as usize); + // bound supports with T::MaxWinners + let supports = supports.try_into().expect("since we truncated, its guaranteed to satisfy bounds; qed"); + // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. - // TODO: Do not error here let solution = ReadySolution { - supports: supports.try_into().map_err(|_| >::BoundNotMet)?, + supports: supports, score: Default::default(), compute: ElectionCompute::Emergency, }; diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index f558724c4a303..d4d5e860d445a 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -299,6 +299,7 @@ parameter_types! { pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); #[derive(Eq, PartialEq, Debug)] pub static MaxWinners: u32 = 100; + pub static ErrorResolution: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::Error; pub static EpochLength: u64 = 30; pub static OnChainFallback: bool = true; @@ -311,6 +312,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = StakingMock; type WeightInfo = (); type MaxWinners = MaxWinners; + type TooManyWinnersResolution = ErrorResolution; } pub struct MockFallback; diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index bab72c34543bd..5284c7fa10419 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -312,7 +312,6 @@ mod tests { type TargetsBound = ConstU32<400>; } - // TODO: test TooManyErrors impl Config for PhragMMSParams { type System = Runtime; type Solver = PhragMMS; From a77c874b326c50d304f5ca4563016bbe9940066c Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 7 Oct 2022 15:27:06 +0200 Subject: [PATCH 27/66] add sort and truncate rule if there are too many winners --- bin/node/runtime/src/lib.rs | 2 ++ frame/babe/src/mock.rs | 2 ++ frame/grandpa/src/mock.rs | 2 ++ frame/root-offences/src/mock.rs | 2 ++ frame/staking/src/mock.rs | 2 ++ 5 files changed, 10 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 604912b8384e0..c74609dc6e49a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -626,6 +626,7 @@ parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; pub MaxElectingVoters: u32 = 10_000; pub MaxWinners: u32 = 1000; + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } /// The numbers configured here could always be more than the the maximum limits of staking pallet @@ -677,6 +678,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; type MaxWinners = ::MaxWinners; + type TooManyWinnersResolution = SortAndTruncate; } impl onchain::BoundedConfig for OnChainSeqPhragmen { diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index d23b43035d36a..6739fb2777f9b 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -170,6 +170,7 @@ parameter_types! { pub const SlashDeferDuration: EraIndex = 0; pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(16); + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } pub struct OnChainSeqPhragmen; @@ -179,6 +180,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type TooManyWinnersResolution = SortAndTruncate; } impl pallet_staking::Config for Test { diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 00e53c55ecb68..bd1ddbdb70693 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -174,6 +174,7 @@ parameter_types! { pub const BondingDuration: EraIndex = 3; pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17); + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } pub struct OnChainSeqPhragmen; @@ -183,6 +184,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type TooManyWinnersResolution = SortAndTruncate; } impl pallet_staking::Config for Test { diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index cef7c2c854700..e325e8e0f05dd 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -87,6 +87,7 @@ impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler { parameter_types! { pub BlockWeights: frame_system::limits::BlockWeights = frame_system::limits::BlockWeights::simple_max(frame_support::weights::Weight::from_ref_time(1024)); + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } impl frame_system::Config for Test { @@ -146,6 +147,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type TooManyWinnersResolution = SortAndTruncate; } pub struct OnStakerSlashMock(core::marker::PhantomData); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index b07c526d557ce..a6ad034d56b45 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -241,6 +241,7 @@ parameter_types! { pub static RewardOnUnbalanceWasCalled: bool = false; pub static LedgerSlashPerEra: (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); pub static MaxWinners: u32 = 100; + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } type VoterBagsListInstance = pallet_bags_list::Instance1; @@ -260,6 +261,7 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = MaxWinners; + type TooManyWinnersResolution = SortAndTruncate; } pub struct MockReward {} From d162039f4e9d263da07e6c8d90b95df8bbedfb32 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 7 Oct 2022 15:35:25 +0200 Subject: [PATCH 28/66] fmt --- .../election-provider-multi-phase/src/lib.rs | 8 ++++--- .../election-provider-support/src/onchain.rs | 23 ++++++++++--------- frame/root-offences/src/mock.rs | 2 +- frame/staking/src/pallet/impls.rs | 8 +++---- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9d2f8737aeb91..e44d5f7975138 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -989,12 +989,14 @@ pub mod pallet { let mut supports = supports; supports.truncate(T::MaxWinners::get() as usize); // bound supports with T::MaxWinners - let supports = supports.try_into().expect("since we truncated, its guaranteed to satisfy bounds; qed"); + let supports = supports + .try_into() + .expect("since we truncated, its guaranteed to satisfy bounds; qed"); // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. let solution = ReadySolution { - supports: supports, + supports, score: Default::default(), compute: ElectionCompute::Emergency, }; @@ -1107,7 +1109,7 @@ pub mod pallet { Error::::FallbackFailed })?; - // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into + // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into // `BoundedVec<_, T::MaxWinners>` let supports: BoundedVec<_, T::MaxWinners> = supports .into_inner() diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 5284c7fa10419..ed87e8316c7c4 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -152,22 +152,23 @@ fn elect_with( // TODO: Add test for TooManyWinnersResolution variants let mut supports = to_supports(&staked); - let supports: OnChainBoundedSupportsOf = - match T::TooManyWinnersResolution::get() { - TooManyWinnersResolution::Error => { - supports - .try_into() - .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))? - }, + let supports: OnChainBoundedSupportsOf = match T::TooManyWinnersResolution::get() { + TooManyWinnersResolution::Error => supports + .try_into() + .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?, TooManyWinnersResolution::Truncate => { supports.truncate(T::MaxWinners::get() as usize); - supports.try_into().expect("we truncated to the bound so this always works; qed") - }, + supports + .try_into() + .expect("we truncated to the bound so this always works; qed") + }, TooManyWinnersResolution::SortAndTruncate => { supports.sort_by(|a, b| a.1.total.partial_cmp(&b.1.total).unwrap()); supports.truncate(T::MaxWinners::get() as usize); - supports.try_into().expect("we truncated to the bound so this always works; qed") - } + supports + .try_into() + .expect("we truncated to the bound so this always works; qed") + }, }; Ok(supports) diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index e325e8e0f05dd..201a1b405c0b3 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -87,7 +87,7 @@ impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler { parameter_types! { pub BlockWeights: frame_system::limits::BlockWeights = frame_system::limits::BlockWeights::simple_max(frame_support::weights::Weight::from_ref_time(1024)); - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; + pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } impl frame_system::Config for Test { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index ff06632c67bce..5b6b2d9cbf67c 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -468,10 +468,10 @@ impl Pallet { }); result - .ok()? - .into_inner() - .try_into() - .expect("both bounds checked in integrity test to be equal; qed") + .ok()? + .into_inner() + .try_into() + .expect("both bounds checked in integrity test to be equal; qed") } else { let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); From e2fd857f42be8eb8a68951e1b106b8d16e98294b Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 9 Oct 2022 00:45:28 +0200 Subject: [PATCH 29/66] fail, not swallow emergency result bound not met --- frame/election-provider-multi-phase/src/lib.rs | 5 +---- frame/election-provider-support/src/onchain.rs | 14 ++++++++------ 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index e44d5f7975138..93a6da6a0256d 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -985,13 +985,10 @@ pub mod pallet { T::ForceOrigin::ensure_origin(origin)?; ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); - // Truncate supports upto MaxWinners bound - let mut supports = supports; - supports.truncate(T::MaxWinners::get() as usize); // bound supports with T::MaxWinners let supports = supports .try_into() - .expect("since we truncated, its guaranteed to satisfy bounds; qed"); + .map_err(|_| Error::::BoundNotMet)?; // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index ed87e8316c7c4..22ed7d8af1e4d 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -46,7 +46,7 @@ impl From for Error { /// /// This will accept voting data on the fly and produce the results immediately. /// -/// The [`ElectionProvider`] implementation of this type does not impose any dynamic limits on the +/// The [`BoundedElectionProvider`] implementation of this type does not impose any dynamic limits on the /// number of voters and targets that are fetched. This could potentially make this unsuitable for /// execution onchain. One could, however, impose bounds on it by using `BoundedExecution` using the /// `MaxVoters` and `MaxTargets` bonds in the `BoundedConfig` trait. @@ -160,14 +160,15 @@ fn elect_with( supports.truncate(T::MaxWinners::get() as usize); supports .try_into() - .expect("we truncated to the bound so this always works; qed") + .expect("truncated to the bound so this always works; qed") }, TooManyWinnersResolution::SortAndTruncate => { - supports.sort_by(|a, b| a.1.total.partial_cmp(&b.1.total).unwrap()); + // sort by total support balance, highest first + supports.sort_by(|b, a| a.1.total.partial_cmp(&b.1.total).unwrap()); supports.truncate(T::MaxWinners::get() as usize); supports .try_into() - .expect("we truncated to the bound so this always works; qed") + .expect("truncated to the bound so this always works; qed") }, }; @@ -297,6 +298,7 @@ mod tests { frame_support::parameter_types! { pub static ErrorResolution: TooManyWinnersResolution = TooManyWinnersResolution::Error; + pub static MaxWinners: u32 = 10; } impl Config for PhragmenParams { @@ -304,7 +306,7 @@ mod tests { type Solver = SequentialPhragmen; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - type MaxWinners = ConstU32<100>; + type MaxWinners = MaxWinners; type TooManyWinnersResolution = ErrorResolution; } @@ -318,7 +320,7 @@ mod tests { type Solver = PhragMMS; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - type MaxWinners = ConstU32<100>; + type MaxWinners = MaxWinners; type TooManyWinnersResolution = ErrorResolution; } From 3a0f9efecb5e68d310d4330a04d59914bdb2ca5d Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 9 Oct 2022 01:14:03 +0200 Subject: [PATCH 30/66] remove too many winners resolution as it can be guaranteed to be bounded --- bin/node/runtime/src/lib.rs | 2 - frame/babe/src/mock.rs | 2 - .../election-provider-multi-phase/src/lib.rs | 4 +- .../election-provider-multi-phase/src/mock.rs | 2 - .../election-provider-support/src/onchain.rs | 52 +++++-------------- frame/grandpa/src/mock.rs | 2 - frame/root-offences/src/mock.rs | 2 - frame/staking/src/mock.rs | 2 - frame/staking/src/pallet/mod.rs | 6 +++ 9 files changed, 19 insertions(+), 55 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 7986cb85efa2e..f4b675545f6c2 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -628,7 +628,6 @@ parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; pub MaxElectingVoters: u32 = 10_000; pub MaxWinners: u32 = 1000; - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } /// The numbers configured here could always be more than the the maximum limits of staking pallet @@ -680,7 +679,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; type MaxWinners = ::MaxWinners; - type TooManyWinnersResolution = SortAndTruncate; } impl onchain::BoundedConfig for OnChainSeqPhragmen { diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 6739fb2777f9b..d23b43035d36a 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -170,7 +170,6 @@ parameter_types! { pub const SlashDeferDuration: EraIndex = 0; pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(16); - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } pub struct OnChainSeqPhragmen; @@ -180,7 +179,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; - type TooManyWinnersResolution = SortAndTruncate; } impl pallet_staking::Config for Test { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 93a6da6a0256d..9f92e35441fc7 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -986,9 +986,7 @@ pub mod pallet { ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); // bound supports with T::MaxWinners - let supports = supports - .try_into() - .map_err(|_| Error::::BoundNotMet)?; + let supports = supports.try_into().map_err(|_| Error::::BoundNotMet)?; // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index d4d5e860d445a..f558724c4a303 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -299,7 +299,6 @@ parameter_types! { pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); #[derive(Eq, PartialEq, Debug)] pub static MaxWinners: u32 = 100; - pub static ErrorResolution: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::Error; pub static EpochLength: u64 = 30; pub static OnChainFallback: bool = true; @@ -312,7 +311,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = StakingMock; type WeightInfo = (); type MaxWinners = MaxWinners; - type TooManyWinnersResolution = ErrorResolution; } pub struct MockFallback; diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 22ed7d8af1e4d..a21d40e88596a 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -34,6 +34,9 @@ pub enum Error { NposElections(sp_npos_elections::Error), /// Errors from the data provider. DataProvider(&'static str), + /// Configurational error caused by `desired_targets` requested by data + /// provider exceeding `MaxWinners`. + TooManyWinners, } impl From for Error { @@ -46,10 +49,10 @@ impl From for Error { /// /// This will accept voting data on the fly and produce the results immediately. /// -/// The [`BoundedElectionProvider`] implementation of this type does not impose any dynamic limits on the -/// number of voters and targets that are fetched. This could potentially make this unsuitable for -/// execution onchain. One could, however, impose bounds on it by using `BoundedExecution` using the -/// `MaxVoters` and `MaxTargets` bonds in the `BoundedConfig` trait. +/// The [`BoundedElectionProvider`] implementation of this type does not impose any dynamic limits +/// on the number of voters and targets that are fetched. This could potentially make this +/// unsuitable for execution onchain. One could, however, impose bounds on it by using +/// `BoundedExecution` using the `MaxVoters` and `MaxTargets` bonds in the `BoundedConfig` trait. /// /// On the other hand, the [`InstantElectionProvider`] implementation does limit these inputs /// dynamically. If you use `elect_with_bounds` along with `InstantElectionProvider`, the bound that @@ -69,13 +72,6 @@ pub struct BoundedExecution(PhantomData); /// This can be very expensive to run frequently on-chain. Use with care. pub struct UnboundedExecution(PhantomData); -#[derive(Copy, Clone, PartialEq, Eq)] -pub enum TooManyWinnersResolution { - Error, - Truncate, - SortAndTruncate, -} - /// Configuration trait for an onchain election execution. pub trait Config { /// Needed for weight registration. @@ -93,12 +89,10 @@ pub trait Config { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// Upper bound on maximum winners from electable targets, and how to deal with it if we have - /// Too many. + /// Upper bound on maximum winners from electable targets. This should be + /// strictly higher than `DataProvider::desired_targets()`. Its recommended + /// to add this check to integrity test of data provider. type MaxWinners: Get; - - /// Action to take if there are too many winners - type TooManyWinnersResolution: Get; } pub trait BoundedConfig: Config { @@ -150,27 +144,8 @@ fn elect_with( DispatchClass::Mandatory, ); - // TODO: Add test for TooManyWinnersResolution variants - let mut supports = to_supports(&staked); - let supports: OnChainBoundedSupportsOf = match T::TooManyWinnersResolution::get() { - TooManyWinnersResolution::Error => supports - .try_into() - .map_err(|_| Error::NposElections(sp_npos_elections::Error::SolutionTargetOverflow))?, - TooManyWinnersResolution::Truncate => { - supports.truncate(T::MaxWinners::get() as usize); - supports - .try_into() - .expect("truncated to the bound so this always works; qed") - }, - TooManyWinnersResolution::SortAndTruncate => { - // sort by total support balance, highest first - supports.sort_by(|b, a| a.1.total.partial_cmp(&b.1.total).unwrap()); - supports.truncate(T::MaxWinners::get() as usize); - supports - .try_into() - .expect("truncated to the bound so this always works; qed") - }, - }; + let supports: OnChainBoundedSupportsOf = + to_supports(&staked).try_into().map_err(|_| Error::TooManyWinners)?; Ok(supports) } @@ -297,7 +272,6 @@ mod tests { struct PhragMMSParams; frame_support::parameter_types! { - pub static ErrorResolution: TooManyWinnersResolution = TooManyWinnersResolution::Error; pub static MaxWinners: u32 = 10; } @@ -307,7 +281,6 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = MaxWinners; - type TooManyWinnersResolution = ErrorResolution; } impl BoundedConfig for PhragmenParams { @@ -321,7 +294,6 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = MaxWinners; - type TooManyWinnersResolution = ErrorResolution; } impl BoundedConfig for PhragMMSParams { diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index bd1ddbdb70693..00e53c55ecb68 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -174,7 +174,6 @@ parameter_types! { pub const BondingDuration: EraIndex = 3; pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17); - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } pub struct OnChainSeqPhragmen; @@ -184,7 +183,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; - type TooManyWinnersResolution = SortAndTruncate; } impl pallet_staking::Config for Test { diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index 201a1b405c0b3..cef7c2c854700 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -87,7 +87,6 @@ impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler { parameter_types! { pub BlockWeights: frame_system::limits::BlockWeights = frame_system::limits::BlockWeights::simple_max(frame_support::weights::Weight::from_ref_time(1024)); - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } impl frame_system::Config for Test { @@ -147,7 +146,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; - type TooManyWinnersResolution = SortAndTruncate; } pub struct OnStakerSlashMock(core::marker::PhantomData); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index a6ad034d56b45..b07c526d557ce 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -241,7 +241,6 @@ parameter_types! { pub static RewardOnUnbalanceWasCalled: bool = false; pub static LedgerSlashPerEra: (BalanceOf, BTreeMap>) = (Zero::zero(), BTreeMap::new()); pub static MaxWinners: u32 = 100; - pub SortAndTruncate: onchain::TooManyWinnersResolution = onchain::TooManyWinnersResolution::SortAndTruncate; } type VoterBagsListInstance = pallet_bags_list::Instance1; @@ -261,7 +260,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = MaxWinners; - type TooManyWinnersResolution = SortAndTruncate; } pub struct MockReward {} diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 2b38de54d6a70..4f6214ede3f2b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -789,6 +789,12 @@ pub mod pallet { ::MaxWinners::get() == ::MaxWinners::get() ); + // ensure desired targets requested is always lower than max winners + // supported by the election provider. + assert!( + ValidatorCount::::get() == + ::MaxWinners::get() + ); sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| assert!( From 7c7190c0c23be24d5ba8b3b8665c063310ee8f24 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Sun, 9 Oct 2022 02:46:06 +0200 Subject: [PATCH 31/66] fix benchmark --- frame/election-provider-multi-phase/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index a8195df7305ff..449b3d28ba2c2 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -221,7 +221,7 @@ frame_benchmarking::benchmarks! { let initial_balance = T::Currency::minimum_balance() * 10u32.into(); T::Currency::make_free_balance_be(&receiver, initial_balance); let ready = ReadySolution { - supports: vec![], + supports: vec![].try_into().unwrap(), score: Default::default(), compute: Default::default() }; From 8b067254794c880efc2272cd2c1cd2e432287bf7 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 10 Oct 2022 11:22:40 +0200 Subject: [PATCH 32/66] give MaxWinners more contextual name --- bin/node/runtime/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index f4b675545f6c2..dff8bc4586a15 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -627,7 +627,7 @@ frame_election_provider_support::generate_solution_type!( parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; pub MaxElectingVoters: u32 = 10_000; - pub MaxWinners: u32 = 1000; + pub MaxActiveValidators: u32 = 1000; } /// The numbers configured here could always be more than the the maximum limits of staking pallet @@ -731,7 +731,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type Solver = SequentialPhragmen, OffchainRandomBalancing>; type ForceOrigin = EnsureRootOrHalfCouncil; type MaxElectableTargets = ConstU16<{ u16::MAX }>; - type MaxWinners = MaxWinners; + type MaxWinners = MaxActiveValidators; type MaxElectingVoters = MaxElectingVoters; type BenchmarkingConfig = ElectionProviderBenchmarkConfig; type WeightInfo = pallet_election_provider_multi_phase::weights::SubstrateWeight; From c3a973e2017f110d865211f8543e9db47f8676d5 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 10 Oct 2022 11:42:33 +0200 Subject: [PATCH 33/66] make ready solution generic over T --- frame/election-provider-multi-phase/src/lib.rs | 10 +++++----- frame/election-provider-multi-phase/src/signed.rs | 2 +- frame/election-provider-multi-phase/src/unsigned.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9f92e35441fc7..141707792f980 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -453,13 +453,13 @@ impl Default for RawSolution { /// A checked solution, ready to be enacted. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, scale_info::TypeInfo)] -#[scale_info(skip_type_params(B))] -pub struct ReadySolution> { +#[scale_info(skip_type_params(T))] +pub struct ReadySolution { /// The final supports of the solution. /// /// This is target-major vector, storing each winners, total backing, and each individual /// backer. - pub supports: BoundedSupports, + pub supports: BoundedSupports, /// The score of the solution. /// /// This is needed to potentially challenge the solution. @@ -1256,7 +1256,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn queued_solution)] pub type QueuedSolution = - StorageValue<_, ReadySolution>; + StorageValue<_, ReadySolution>; /// Snapshot data of the round. /// @@ -1490,7 +1490,7 @@ impl Pallet { pub fn feasibility_check( raw_solution: RawSolution>, compute: ElectionCompute, - ) -> Result, FeasibilityError> { + ) -> Result, FeasibilityError> { let RawSolution { solution, score, round } = raw_solution; // First, check round. diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 2067b71e3adf9..6ab4d5a74b8f2 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -451,7 +451,7 @@ impl Pallet { /// /// Infallible pub fn finalize_signed_phase_accept_solution( - ready_solution: ReadySolution, + ready_solution: ReadySolution, who: &T::AccountId, deposit: BalanceOf, call_fee: BalanceOf, diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 876db44ab5a3d..71f118355518a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -351,7 +351,7 @@ impl Pallet { // ensure score is being improved. Panic henceforth. ensure!( - Self::queued_solution().map_or(true, |q: ReadySolution<_, _>| raw_solution + Self::queued_solution().map_or(true, |q: ReadySolution<_>| raw_solution .score .strict_threshold_better(q.score, T::BetterUnsignedThreshold::get())), Error::::PreDispatchWeakSubmission, From 1a58b93dd803ec511792e8e03fc30d4372d9b6e4 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 10 Oct 2022 12:02:11 +0200 Subject: [PATCH 34/66] kian feedback --- frame/election-provider-support/src/onchain.rs | 3 +++ frame/staking/src/pallet/mod.rs | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index a21d40e88596a..b1d70b4d75ca9 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -144,6 +144,9 @@ fn elect_with( DispatchClass::Mandatory, ); + // Since npos solver returns a result always bounded by `desired_targets`, + // this is never expected to happen as long as npos solver does what is + // expected for it to do. let supports: OnChainBoundedSupportsOf = to_supports(&staked).try_into().map_err(|_| Error::TooManyWinners)?; diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 4f6214ede3f2b..361242bc7069a 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -792,7 +792,7 @@ pub mod pallet { // ensure desired targets requested is always lower than max winners // supported by the election provider. assert!( - ValidatorCount::::get() == + ValidatorCount::::get() <= ::MaxWinners::get() ); sp_std::if_std! { @@ -1277,9 +1277,9 @@ pub mod pallet { #[pallet::compact] new: u32, ) -> DispatchResult { ensure_root(origin)?; - // max winners supported by election provider - let max_winners = ::MaxWinners::get(); - ensure!(new <= max_winners, Error::::TooManyElectionWinners); + // ensure new validator count does not exceed maximum winners + // support by election provider. + ensure!(new <= ::MaxWinners::get(), Error::::TooManyElectionWinners); ValidatorCount::::put(new); Ok(()) } From 6fa5b0eedbf0ec1d5d275bdff217b9fed8b5d218 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 10 Oct 2022 16:47:28 +0200 Subject: [PATCH 35/66] fix stuff --- frame/election-provider-multi-phase/src/benchmarking.rs | 2 +- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- frame/election-provider-multi-phase/src/mock.rs | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 449b3d28ba2c2..110f2d91f8ed3 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -404,7 +404,7 @@ frame_benchmarking::benchmarks! { assert_eq!(raw_solution.solution.voter_count() as u32, a); assert_eq!(raw_solution.solution.unique_targets().len() as u32, d); }: { - assert_ok!(>::feasibility_check(raw_solution, ElectionCompute::Unsigned)); + assert!(>::feasibility_check(raw_solution, ElectionCompute::Unsigned).is_ok()); } // NOTE: this weight is not used anywhere, but the fact that it should succeed when execution in diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 141707792f980..ec8ea8f24107f 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -238,7 +238,7 @@ use frame_support::{ dispatch::DispatchClass, ensure, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, - weights::Weight, + weights::Weight, EqNoBound, PartialEqNoBound, DefaultNoBound, }; use frame_system::{ensure_none, offchain::SendTransactionTypes}; use scale_info::TypeInfo; @@ -452,7 +452,7 @@ impl Default for RawSolution { } /// A checked solution, ready to be enacted. -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, scale_info::TypeInfo)] +#[derive(PartialEqNoBound, EqNoBound, Clone, Encode, Decode, RuntimeDebug, DefaultNoBound, scale_info::TypeInfo)] #[scale_info(skip_type_params(T))] pub struct ReadySolution { /// The final supports of the solution. diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index f558724c4a303..b1c82cc30d198 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -297,7 +297,6 @@ parameter_types! { pub static MockWeightInfo: MockedWeightInfo = MockedWeightInfo::Real; pub static MaxElectingVoters: VoterIndex = u32::max_value(); pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); - #[derive(Eq, PartialEq, Debug)] pub static MaxWinners: u32 = 100; pub static EpochLength: u64 = 30; From ffce12cc0c58506a4ff29467b1f1d43f582c7cee Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 11 Oct 2022 09:15:35 +0200 Subject: [PATCH 36/66] Kian's way of solvign this --- .../election-provider-multi-phase/src/lib.rs | 143 +++++++----------- .../election-provider-multi-phase/src/mock.rs | 37 ++--- frame/election-provider-support/src/lib.rs | 117 ++++++-------- .../election-provider-support/src/onchain.rs | 139 ++++++----------- frame/staking/src/pallet/mod.rs | 5 +- 5 files changed, 164 insertions(+), 277 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index ec8ea8f24107f..81517bc57bc5b 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -231,19 +231,20 @@ use codec::{Decode, Encode}; use frame_election_provider_support::{ - BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, - ElectionProviderBase, InstantElectionProvider, NposSolution, + BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + InstantElectionProvider, NposSolution, }; use frame_support::{ dispatch::DispatchClass, ensure, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, - weights::Weight, EqNoBound, PartialEqNoBound, DefaultNoBound, + weights::Weight, + DefaultNoBound, EqNoBound, PartialEqNoBound, }; use frame_system::{ensure_none, offchain::SendTransactionTypes}; use scale_info::TypeInfo; use sp_arithmetic::{ - traits::{Bounded, CheckedAdd, Zero}, + traits::{CheckedAdd, Zero}, UpperOf, }; use sp_npos_elections::{ @@ -311,40 +312,6 @@ pub trait BenchmarkingConfig { const MAXIMUM_TARGETS: u32; } -/// A fallback implementation that transitions the pallet to the emergency phase. -pub struct NoFallback(sp_std::marker::PhantomData); - -impl ElectionProviderBase for NoFallback { - type AccountId = T::AccountId; - type BlockNumber = T::BlockNumber; - type DataProvider = T::DataProvider; - type Error = &'static str; - - fn ongoing() -> bool { - false - } -} - -impl BoundedElectionProvider for NoFallback { - type MaxWinners = T::MaxWinners; - fn elect() -> Result, Self::Error> { - // Do nothing, this will enable the emergency phase. - Err("NoFallback.") - } -} - -impl InstantElectionProvider for NoFallback { - fn elect_with_bounds( - _: usize, - _: usize, - ) -> Result< - BoundedSupports::MaxWinners>, - Self::Error, - > { - Err("NoFallback.") - } -} - /// Current phase of the pallet. #[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)] pub enum Phase { @@ -452,7 +419,16 @@ impl Default for RawSolution { } /// A checked solution, ready to be enacted. -#[derive(PartialEqNoBound, EqNoBound, Clone, Encode, Decode, RuntimeDebug, DefaultNoBound, scale_info::TypeInfo)] +#[derive( + PartialEqNoBound, + EqNoBound, + Clone, + Encode, + Decode, + RuntimeDebug, + DefaultNoBound, + scale_info::TypeInfo, +)] #[scale_info(skip_type_params(T))] pub struct ReadySolution { /// The final supports of the solution. @@ -704,6 +680,7 @@ pub mod pallet { AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Self::DataProvider, + MaxWinners = Self::MaxWinners, >; /// Configuration of the governance-only fallback. @@ -714,6 +691,7 @@ pub mod pallet { AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Self::DataProvider, + MaxWinners = Self::MaxWinners, >; /// OCW election solution miner algorithm implementation. @@ -887,10 +865,6 @@ pub mod pallet { // `SignedMaxSubmissions` is a red flag that the developer does not understand how to // configure this pallet. assert!(T::SignedMaxSubmissions::get() >= T::SignedMaxRefunds::get()); - // We expect the same bounds on election results provided by the - // `ElectionProvider` implementations of the pallet and - // `GovernanceFallback`. - assert!(T::MaxWinners::get() == <::GovernanceFallback as BoundedElectionProvider>::MaxWinners::get()); } } @@ -1092,17 +1066,13 @@ pub mod pallet { T::ForceOrigin::ensure_origin(origin)?; ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); - let maybe_max_voters = maybe_max_voters.map(|x| x as usize); - let maybe_max_targets = maybe_max_targets.map(|x| x as usize); - - let supports = T::GovernanceFallback::elect_with_bounds( - maybe_max_voters.unwrap_or(Bounded::max_value()), - maybe_max_targets.unwrap_or(Bounded::max_value()), - ) - .map_err(|e| { - log!(error, "GovernanceFallback failed: {:?}", e); - Error::::FallbackFailed - })?; + let supports = + T::GovernanceFallback::instant_elect(maybe_max_voters, maybe_max_targets).map_err( + |e| { + log!(error, "GovernanceFallback failed: {:?}", e); + Error::::FallbackFailed + }, + )?; // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into // `BoundedVec<_, T::MaxWinners>` @@ -1255,8 +1225,7 @@ pub mod pallet { /// Current best solution, signed or unsigned, queued to be returned upon `elect`. #[pallet::storage] #[pallet::getter(fn queued_solution)] - pub type QueuedSolution = - StorageValue<_, ReadySolution>; + pub type QueuedSolution = StorageValue<_, ReadySolution>; /// Snapshot data of the round. /// @@ -1417,31 +1386,31 @@ impl Pallet { let targets = T::DataProvider::electable_targets(Some(target_limit)) .map_err(ElectionError::DataProvider)?; + let voters = T::DataProvider::electing_voters(Some(voter_limit)) .map_err(ElectionError::DataProvider)?; - let mut desired_targets = - T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; + // TODO: make sure a test for this exist/is written. // Defensive-only. if targets.len() > target_limit || voters.len() > voter_limit { debug_assert!(false, "Snapshot limit has not been respected."); return Err(ElectionError::DataProvider("Snapshot too big for submission.")) } + let mut desired_targets = + T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; + // If `desired_targets` > `targets.len()`, cap `desired_targets` to that level and emit a // warning - let max_len = targets - .len() - .try_into() - .map_err(|_| ElectionError::DataProvider("Failed to convert usize"))?; - if desired_targets > max_len { + let max_desired_targets: u32 = (targets.len() as u32).min(T::MaxWinners::get()); + if desired_targets > max_desired_targets { log!( warn, "desired_targets: {} > targets.len(): {}, capping desired_targets", desired_targets, - max_len + max_desired_targets ); - desired_targets = max_len; + desired_targets = max_desired_targets; } Ok((targets, voters, desired_targets)) @@ -1595,7 +1564,7 @@ impl Pallet { >::take() .ok_or(ElectionError::::NothingQueued) .or_else(|_| { - ::elect() + T::Fallback::instant_elect(None, None) .map_err(|fe| ElectionError::Fallback(fe)) .and_then(|supports| { Ok(ReadySolution { @@ -1639,17 +1608,16 @@ impl ElectionProviderBase for Pallet { type BlockNumber = T::BlockNumber; type Error = ElectionError; type DataProvider = T::DataProvider; + type MaxWinners = T::MaxWinners; +} +impl ElectionProvider for Pallet { fn ongoing() -> bool { match Self::current_phase() { Phase::Off => false, _ => true, } } -} - -impl BoundedElectionProvider for Pallet { - type MaxWinners = T::MaxWinners; fn elect() -> Result, Self::Error> { match Self::do_elect() { @@ -1667,6 +1635,7 @@ impl BoundedElectionProvider for Pallet { } } } + /// convert a DispatchError to a custom InvalidTransaction with the inner code being the error /// number. pub fn dispatch_error_to_invalid(error: DispatchError) -> InvalidTransaction { @@ -1939,7 +1908,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); assert!(MultiPhase::snapshot().is_some()); - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -1990,7 +1959,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_unsigned_open_at(20)); - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -2028,7 +1997,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_signed()); - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); assert!(MultiPhase::snapshot().is_none()); @@ -2066,7 +2035,7 @@ mod tests { assert!(MultiPhase::current_phase().is_off()); // This module is now only capable of doing on-chain backup. - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); @@ -2092,7 +2061,7 @@ mod tests { assert_eq!(MultiPhase::round(), 1); // An unexpected call to elect. - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); // We surely can't have any feasible solutions. This will cause an on-chain election. assert_eq!( @@ -2139,7 +2108,7 @@ mod tests { } // an unexpected call to elect. - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); // all storage items must be cleared. assert_eq!(MultiPhase::round(), 2); @@ -2189,7 +2158,7 @@ mod tests { )); roll_to(30); - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert_eq!( multi_phase_events(), @@ -2233,7 +2202,7 @@ mod tests { )); assert!(MultiPhase::queued_solution().is_some()); - assert_ok!(::elect()); + assert_ok!(MultiPhase::elect()); assert_eq!( multi_phase_events(), @@ -2265,7 +2234,7 @@ mod tests { // Zilch solutions thus far, but we get a result. assert!(MultiPhase::queued_solution().is_none()); - let supports = ::elect().unwrap(); + let supports = MultiPhase::elect().unwrap(); assert_eq!( supports, @@ -2298,10 +2267,7 @@ mod tests { // Zilch solutions thus far. assert!(MultiPhase::queued_solution().is_none()); - assert_eq!( - ::elect().unwrap_err(), - ElectionError::Fallback("NoFallback.") - ); + assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback.")); // phase is now emergency. assert_eq!(MultiPhase::current_phase(), Phase::Emergency); @@ -2324,10 +2290,7 @@ mod tests { // Zilch solutions thus far. assert!(MultiPhase::queued_solution().is_none()); - assert_eq!( - ::elect().unwrap_err(), - ElectionError::Fallback("NoFallback.") - ); + assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback.")); // phase is now emergency. assert_eq!(MultiPhase::current_phase(), Phase::Emergency); @@ -2344,7 +2307,7 @@ mod tests { // something is queued now assert!(MultiPhase::queued_solution().is_some()); // next election call with fix everything.; - assert!(::elect().is_ok()); + assert!(MultiPhase::elect().is_ok()); assert_eq!(MultiPhase::current_phase(), Phase::Off); assert_eq!( @@ -2381,7 +2344,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Off); // On-chain backup works though. - let supports = ::elect().unwrap(); + let supports = MultiPhase::elect().unwrap(); assert!(supports.len() > 0); assert_eq!( @@ -2411,7 +2374,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Off); roll_to(29); - let err = ::elect().unwrap_err(); + let err = MultiPhase::elect().unwrap_err(); assert_eq!(err, ElectionError::Fallback("NoFallback.")); assert_eq!(MultiPhase::current_phase(), Phase::Emergency); diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index b1c82cc30d198..7306fb19ca6b7 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -19,7 +19,7 @@ use super::*; use crate::{self as multi_phase, unsigned::MinerConfig}; use frame_election_provider_support::{ data_provider, - onchain::{self, UnboundedExecution}, + onchain::{self}, ElectionDataProvider, NposSolution, SequentialPhragmen, }; pub use frame_support::{assert_noop, assert_ok, pallet_prelude::GetDefault}; @@ -310,6 +310,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = StakingMock; type WeightInfo = (); type MaxWinners = MaxWinners; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } pub struct MockFallback; @@ -318,35 +320,19 @@ impl ElectionProviderBase for MockFallback { type BlockNumber = u64; type Error = &'static str; type DataProvider = StakingMock; - - fn ongoing() -> bool { - false - } -} - -impl BoundedElectionProvider for MockFallback { type MaxWinners = MaxWinners; - fn elect() -> Result, Self::Error> { - Self::elect_with_bounds(Bounded::max_value(), Bounded::max_value()) - } } impl InstantElectionProvider for MockFallback { - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, - ) -> Result< - BoundedSupports::MaxWinners>, - Self::Error, - > { + fn instant_elect( + max_voters: Option, + max_targets: Option, + ) -> Result, Self::Error> { if OnChainFallback::get() { - onchain::UnboundedExecution::::elect_with_bounds( - max_voters, - max_targets, - ) - .map_err(|_| "onchain::UnboundedExecution failed.") + onchain::OnChainExecution::::instant_elect(max_voters, max_targets) + .map_err(|_| "onchain::OnChainExecution failed.") } else { - super::NoFallback::::elect_with_bounds(max_voters, max_targets) + Err("NoFallback.") } } } @@ -411,7 +397,8 @@ impl crate::Config for Runtime { type WeightInfo = (); type BenchmarkingConfig = TestBenchmarkingConfig; type Fallback = MockFallback; - type GovernanceFallback = UnboundedExecution; + type GovernanceFallback = + frame_election_provider_support::onchain::OnChainExecution; type ForceOrigin = frame_system::EnsureRoot; type MaxElectingVoters = MaxElectingVoters; type MaxElectableTargets = MaxElectableTargets; diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 1f47914cc2c13..0bb4e5a9c4c0c 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -82,6 +82,7 @@ //! # use frame_election_provider_support::{*, data_provider}; //! # use sp_npos_elections::{Support, Assignment}; //! # use frame_support::traits::ConstU32; +//! # use frame_support::bounded_vec; //! //! type AccountId = u64; //! type Balance = u64; @@ -137,15 +138,16 @@ //! type BlockNumber = BlockNumber; //! type Error = &'static str; //! type DataProvider = T::DataProvider; -//! fn ongoing() -> bool { false } -//! +//! type MaxWinners = ConstU32<{ u32::MAX }>; +//! //! } //! //! impl ElectionProvider for GenericElectionProvider { -//! fn elect() -> Result, Self::Error> { +//! fn ongoing() -> bool { false } +//! fn elect() -> Result, Self::Error> { //! Self::DataProvider::electable_targets(None) //! .map_err(|_| "failed to elect") -//! .map(|t| vec![(t[0], Support::default())]) +//! .map(|t| bounded_vec![(t[0], Support::default())]) //! } //! } //! } @@ -354,11 +356,7 @@ pub trait ElectionDataProvider { fn clear() {} } -/// Base trait for [`ElectionProvider`] and [`BoundedElectionProvider`]. It is -/// meant to be used only with an extension trait that adds an election -/// functionality. -/// -/// Data can be bounded or unbounded and is fetched from [`Self::DataProvider`]. +/// Base trait for types that can provide election pub trait ElectionProviderBase { /// The account identifier type. type AccountId; @@ -369,79 +367,55 @@ pub trait ElectionProviderBase { /// The error type that is returned by the provider. type Error: Debug; + /// The upper bound on election winners that cab be returned. + /// + /// # WARNING + /// + /// when communicating with the data provider, one must ensure that + /// `DataProvider::desired_targets` returns a value less than this bound. An implementation can + /// chose to either return an error and/or sort and truncate the output to meet this bound. + type MaxWinners: Get; + /// The data provider of the election. type DataProvider: ElectionDataProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, >; - - /// Indicate if this election provider is currently ongoing an asynchronous election or not. - fn ongoing() -> bool; } -/// Same as `BoundedSupports` but parameterized by a `BoundedElectionProvider`. -pub type BoundedSupportsOf = BoundedSupports< - ::AccountId, - ::MaxWinners, ->; - /// Elect a new set of winners, bounded by `MaxWinners`. /// -/// Returns a result in bounded, target major format, namely as -/// *BoundedVec<(AccountId, Vec), MaxWinners>*. -pub trait BoundedElectionProvider: ElectionProviderBase { - /// The upper bound on election winners. - type MaxWinners: Get; +/// It must always use `Config::DataProvider` to fetch the data it needs. +/// +/// This election provider that could function asynchronously. This implies that this election might +/// needs data ahead of time (ergo, receives no arguments to `elect`), and might be `ongoing` at +/// times. +pub trait ElectionProvider: ElectionProviderBase { + /// Indicate if this election provider is currently ongoing an asynchronous election or not. + fn ongoing() -> bool; + /// Performs the election. 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 elect() -> Result, Self::Error>; } -/// Same a [`BoundedElectionProvider`], but no bounds are imposed on the number -/// of winners. -/// -/// The result is returned in a target major format, namely as -///*Vec<(AccountId, Vec)>*. -pub trait ElectionProvider: ElectionProviderBase { - /// Performs the election. This should be implemented as a self-weighing - /// function, similar to [`BoundedElectionProvider::elect()`]. - fn elect() -> Result, Self::Error>; -} - -impl ElectionProvider for E { - fn elect() -> Result, Self::Error> { - Self::elect().map(|supports| supports.into_inner()) - } -} - -/// A sub-trait of the [`ElectionProvider`] for cases where we need to be sure -/// an election needs to happen instantly, not asynchronously. -/// -/// The same `DataProvider` is assumed to be used. +/// A (almost) marker trait that signifies an election provider as working synchronously. i.e. being +/// *instant*. /// -/// Consequently, allows for control over the amount of data that is being -/// fetched from the [`ElectionProviderBase::DataProvider`]. -pub trait InstantElectionProvider: BoundedElectionProvider { - /// Elect a new set of winners, but unlike [`ElectionProvider::elect`] which cannot enforce - /// bounds, this trait method can enforce bounds on the amount of data provided by the - /// `DataProvider`. - /// - /// An implementing type, if itself bounded, should choose the minimum of the two bounds to - /// choose the final value of `max_voters` and `max_targets`. In other words, an implementation - /// should guarantee that `max_voter` and `max_targets` provided to this method are absolutely - /// respected. - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, +/// This must still use the same data provider as with [`Config::DataProvider`]. However, it can +/// optionally overwrite the amount of voters and targets that are fetched from the data provider at +/// runtime via `forced_input_voters_bound` and `forced_input_target_bound`. +pub trait InstantElectionProvider: ElectionProviderBase { + fn instant_elect( + forced_input_voters_bound: Option, + forced_input_target_bound: Option, ) -> Result, Self::Error>; } -/// An election provider to be used only for testing. -#[cfg(feature = "std")] +/// An election provider that does nothing whatsoever. pub struct NoElection(sp_std::marker::PhantomData); -#[cfg(feature = "std")] impl ElectionProviderBase for NoElection<(AccountId, BlockNumber, DataProvider)> where @@ -450,22 +424,21 @@ where type AccountId = AccountId; type BlockNumber = BlockNumber; type Error = &'static str; + type MaxWinners = frame_support::traits::ConstU32<0>; type DataProvider = DataProvider; - - fn ongoing() -> bool { - false - } } -#[cfg(feature = "std")] -impl BoundedElectionProvider +impl ElectionProvider for NoElection<(AccountId, BlockNumber, DataProvider)> where DataProvider: ElectionDataProvider, { - type MaxWinners = frame_support::traits::ConstU32<0>; + fn ongoing() -> bool { + false + } + fn elect() -> Result, Self::Error> { - Err(" cannot do anything.") + Err("`NoElection` cannot do anything.") } } @@ -663,3 +636,9 @@ pub type Voter = (AccountId, VoteWeight, BoundedVec = Voter<::AccountId, ::MaxVotesPerVoter>; + +/// Same as `BoundedSupports` but parameterized by a `BoundedElectionProvider`. +pub type BoundedSupportsOf = BoundedSupports< + ::AccountId, + ::MaxWinners, +>; diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index b1d70b4d75ca9..db862f4bd6759 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -20,7 +20,7 @@ //! careful when using it onchain. use crate::{ - BoundedElectionProvider, BoundedSupportsOf, Debug, ElectionDataProvider, ElectionProviderBase, + BoundedSupportsOf, Debug, ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, NposSolver, WeightInfo, }; use frame_support::{dispatch::DispatchClass, traits::Get}; @@ -34,8 +34,8 @@ pub enum Error { NposElections(sp_npos_elections::Error), /// Errors from the data provider. DataProvider(&'static str), - /// Configurational error caused by `desired_targets` requested by data - /// provider exceeding `MaxWinners`. + /// Configurational error caused by `desired_targets` requested by data provider exceeding + /// `MaxWinners`. TooManyWinners, } @@ -47,58 +47,48 @@ impl From for Error { /// A simple on-chain implementation of the election provider trait. /// -/// This will accept voting data on the fly and produce the results immediately. +/// This implements both `ElectionProvider` and `InstantElectionProvider`. /// -/// The [`BoundedElectionProvider`] implementation of this type does not impose any dynamic limits -/// on the number of voters and targets that are fetched. This could potentially make this -/// unsuitable for execution onchain. One could, however, impose bounds on it by using -/// `BoundedExecution` using the `MaxVoters` and `MaxTargets` bonds in the `BoundedConfig` trait. -/// -/// On the other hand, the [`InstantElectionProvider`] implementation does limit these inputs -/// dynamically. If you use `elect_with_bounds` along with `InstantElectionProvider`, the bound that -/// would be used is the minimum of the dynamic bounds given as arguments to `elect_with_bounds` and -/// the trait bounds (`MaxVoters` and `MaxTargets`). -/// -/// Please use `BoundedExecution` at all times except at genesis or for testing, with thoughtful -/// bounds in order to bound the potential execution time. Limit the use `UnboundedExecution` at -/// genesis or for testing, as it does not bound the inputs. However, this can be used with -/// `[InstantElectionProvider::elect_with_bounds`] that dynamically imposes limits. -pub struct BoundedExecution(PhantomData); +/// This type has some utilities to make it safe. Nonetheless, it should be used with utmost care. A +/// thoughtful value must be set as [`Config::VotersBound`] and [`Config::TargetsBound`] to ensure +/// the size of the input is sensible. +pub struct OnChainExecution(PhantomData); -/// An unbounded variant of [`BoundedExecution`]. -/// -/// ### Warning -/// -/// This can be very expensive to run frequently on-chain. Use with care. -pub struct UnboundedExecution(PhantomData); +#[deprecated(note = "use OnChainExecution, which is bounded by default")] +pub type BoundedExecution = OnChainExecution; /// Configuration trait for an onchain election execution. pub trait Config { /// Needed for weight registration. type System: frame_system::Config; + /// `NposSolver` that should be used, an example would be `PhragMMS`. type Solver: NposSolver< AccountId = ::AccountId, Error = sp_npos_elections::Error, >; + /// Something that provides the data for election. type DataProvider: ElectionDataProvider< AccountId = ::AccountId, BlockNumber = ::BlockNumber, >; + /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// Upper bound on maximum winners from electable targets. This should be - /// strictly higher than `DataProvider::desired_targets()`. Its recommended - /// to add this check to integrity test of data provider. + /// Upper bound on maximum winners from electable targets. + /// + /// As as noted in the documentation of [`ElectionProviderBase::MaxWinners`], this value should + /// always be more than `DataProvider::desired_target`. type MaxWinners: Get; -} -pub trait BoundedConfig: Config { - /// Bounds the number of voters. + /// Bounds the number of voters, when calling into [`Config::DataProvider`]. It might be + /// overwritten in the `InstantElectionProvider` impl. type VotersBound: Get; - /// Bounds the number of targets. + + /// Bounds the number of targets, when calling into [`Config::DataProvider`]. It might be + /// overwritten in the `InstantElectionProvider` impl. type TargetsBound: Get; } @@ -108,7 +98,7 @@ pub type OnChainBoundedSupportsOf = BoundedSupports< ::MaxWinners, >; -fn elect_with( +fn elect_with_input_bounds( maybe_max_voters: Option, maybe_max_targets: Option, ) -> Result, Error> { @@ -117,6 +107,12 @@ fn elect_with( T::DataProvider::electable_targets(maybe_max_targets).map_err(Error::DataProvider)?; let desired_targets = T::DataProvider::desired_targets().map_err(Error::DataProvider)?; + if desired_targets > T::MaxWinners::get() { + // TODO: make sure a test for this exists in this crate. + // early exist + return Err(Error::TooManyWinners) + } + let voters_len = voters.len() as u32; let targets_len = targets.len() as u32; @@ -144,78 +140,43 @@ fn elect_with( DispatchClass::Mandatory, ); - // Since npos solver returns a result always bounded by `desired_targets`, - // this is never expected to happen as long as npos solver does what is - // expected for it to do. + // defensive: Since npos solver returns a result always bounded by `desired_targets`, this is + // never expected to happen as long as npos solver does what is expected for it to do. let supports: OnChainBoundedSupportsOf = to_supports(&staked).try_into().map_err(|_| Error::TooManyWinners)?; Ok(supports) } -impl BoundedElectionProvider for UnboundedExecution { - type MaxWinners = T::MaxWinners; - - fn elect() -> Result, Self::Error> { - // This should not be called if not in `std` mode (and therefore neither in genesis nor in - // testing) - if cfg!(not(feature = "std")) { - frame_support::log::error!( - "Please use `InstantElectionProvider` instead to provide bounds on election if not in \ - genesis or testing mode" - ); - } - - elect_with::(None, None) - } -} - -impl ElectionProviderBase for UnboundedExecution { +impl ElectionProviderBase for OnChainExecution { type AccountId = ::AccountId; type BlockNumber = ::BlockNumber; type Error = Error; + type MaxWinners = T::MaxWinners; type DataProvider = T::DataProvider; - - fn ongoing() -> bool { - false - } } -impl InstantElectionProvider for UnboundedExecution { - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, +impl InstantElectionProvider for OnChainExecution { + fn instant_elect( + forced_input_voters_bound: Option, + forced_input_target_bound: Option, ) -> Result, Self::Error> { - elect_with::(Some(max_voters), Some(max_targets)) + elect_with_input_bounds::( + Some(T::VotersBound::get().min(forced_input_voters_bound.unwrap_or(u32::MAX)) as usize), + Some(T::TargetsBound::get().min(forced_input_target_bound.unwrap_or(u32::MAX)) as usize), + ) } } -impl ElectionProviderBase for BoundedExecution { - type AccountId = ::AccountId; - type BlockNumber = ::BlockNumber; - type Error = Error; - type DataProvider = T::DataProvider; - +impl ElectionProvider for OnChainExecution { fn ongoing() -> bool { false } -} -impl BoundedElectionProvider for BoundedExecution { - type MaxWinners = T::MaxWinners; fn elect() -> Result, Self::Error> { - elect_with::(Some(T::VotersBound::get() as usize), Some(T::TargetsBound::get() as usize)) - } -} - -impl InstantElectionProvider for BoundedExecution { - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, - ) -> Result, Self::Error> { - elect_with::( - Some(max_voters.min(T::VotersBound::get() as usize)), - Some(max_targets.min(T::TargetsBound::get() as usize)), + elect_with_input_bounds::( + Some(T::VotersBound::get() as usize), + Some(T::TargetsBound::get() as usize), ) } } @@ -284,9 +245,6 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = MaxWinners; - } - - impl BoundedConfig for PhragmenParams { type VotersBound = ConstU32<600>; type TargetsBound = ConstU32<400>; } @@ -297,9 +255,6 @@ mod tests { type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); type MaxWinners = MaxWinners; - } - - impl BoundedConfig for PhragMMSParams { type VotersBound = ConstU32<600>; type TargetsBound = ConstU32<400>; } @@ -341,7 +296,7 @@ mod tests { fn onchain_seq_phragmen_works() { sp_io::TestExternalities::new_empty().execute_with(|| { assert_eq!( - as ElectionProvider>::elect().unwrap(), + as ElectionProvider>::elect().unwrap(), vec![ (10, Support { total: 25, voters: vec![(1, 10), (3, 15)] }), (30, Support { total: 35, voters: vec![(2, 20), (3, 15)] }) @@ -354,7 +309,7 @@ mod tests { fn onchain_phragmms_works() { sp_io::TestExternalities::new_empty().execute_with(|| { assert_eq!( - as ElectionProvider>::elect().unwrap(), + as ElectionProvider>::elect().unwrap(), vec![ (10, Support { total: 25, voters: vec![(1, 10), (3, 15)] }), (30, Support { total: 35, voters: vec![(2, 20), (3, 15)] }) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 361242bc7069a..eb9c3e0beadd8 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1279,7 +1279,10 @@ pub mod pallet { ensure_root(origin)?; // ensure new validator count does not exceed maximum winners // support by election provider. - ensure!(new <= ::MaxWinners::get(), Error::::TooManyElectionWinners); + ensure!( + new <= ::MaxWinners::get(), + Error::::TooManyElectionWinners + ); ValidatorCount::::put(new); Ok(()) } From 548ebfa1b30df11ffa14a6be774565afd7d49e6e Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 11 Oct 2022 10:27:26 +0200 Subject: [PATCH 37/66] comment fix --- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- frame/election-provider-support/src/lib.rs | 7 ++++--- frame/election-provider-support/src/onchain.rs | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 81517bc57bc5b..d3aae6356cfb9 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1400,8 +1400,8 @@ impl Pallet { let mut desired_targets = T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; - // If `desired_targets` > `targets.len()`, cap `desired_targets` to that level and emit a - // warning + // If `desired_targets` > `targets.len()`, cap `desired_targets` to that + // level and emit a warning let max_desired_targets: u32 = (targets.len() as u32).min(T::MaxWinners::get()); if desired_targets > max_desired_targets { log!( diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 0bb4e5a9c4c0c..9a511d6da2791 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -367,13 +367,14 @@ pub trait ElectionProviderBase { /// The error type that is returned by the provider. type Error: Debug; - /// The upper bound on election winners that cab be returned. + /// The upper bound on election winners that can be returned. /// /// # WARNING /// /// when communicating with the data provider, one must ensure that - /// `DataProvider::desired_targets` returns a value less than this bound. An implementation can - /// chose to either return an error and/or sort and truncate the output to meet this bound. + /// `DataProvider::desired_targets` returns a value less than this bound. An + /// implementation can chose to either return an error and/or sort and + /// truncate the output to meet this bound. type MaxWinners: Get; /// The data provider of the election. diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index db862f4bd6759..7b50951495806 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -109,7 +109,7 @@ fn elect_with_input_bounds( if desired_targets > T::MaxWinners::get() { // TODO: make sure a test for this exists in this crate. - // early exist + // early exit return Err(Error::TooManyWinners) } From bcfd80bbdf86278bead33bcedaad424562ae7e8d Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 11 Oct 2022 13:20:55 +0200 Subject: [PATCH 38/66] fix compile --- bin/node/runtime/src/lib.rs | 5 +---- frame/babe/src/mock.rs | 4 +++- frame/election-provider-multi-phase/src/lib.rs | 2 +- frame/election-provider-support/src/lib.rs | 2 +- frame/fast-unstake/src/lib.rs | 4 ++-- frame/fast-unstake/src/mock.rs | 7 +++---- frame/grandpa/src/mock.rs | 4 +++- frame/offences/benchmarking/src/mock.rs | 5 ++++- frame/root-offences/src/mock.rs | 4 +++- frame/session/benchmarking/src/mock.rs | 5 ++++- frame/staking/src/lib.rs | 2 +- frame/staking/src/mock.rs | 4 +++- frame/staking/src/pallet/impls.rs | 6 +++--- frame/staking/src/pallet/mod.rs | 17 +++++++---------- 14 files changed, 39 insertions(+), 32 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index dff8bc4586a15..aaa59eb770b28 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -570,7 +570,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = ElectionProviderMultiPhase; - type GenesisElectionProvider = onchain::UnboundedExecution; + type GenesisElectionProvider = onchain::OnChainExecution::; type VoterList = VoterList; // This a placeholder, to be introduced in the next PR as an instance of bags-list type TargetList = pallet_staking::UseValidatorsMap; @@ -679,9 +679,6 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; type MaxWinners = ::MaxWinners; -} - -impl onchain::BoundedConfig for OnChainSeqPhragmen { type VotersBound = MaxElectingVoters; type TargetsBound = ConstU32<2_000>; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index d23b43035d36a..a10cc69ae2b7e 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -179,6 +179,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } impl pallet_staking::Config for Test { @@ -200,7 +202,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution::; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index d3aae6356cfb9..d7eb7faa346b1 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1607,8 +1607,8 @@ impl ElectionProviderBase for Pallet { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; type Error = ElectionError; - type DataProvider = T::DataProvider; type MaxWinners = T::MaxWinners; + type DataProvider = T::DataProvider; } impl ElectionProvider for Pallet { diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 9a511d6da2791..45ac159124254 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -638,7 +638,7 @@ pub type Voter = (AccountId, VoteWeight, BoundedVec = Voter<::AccountId, ::MaxVotesPerVoter>; -/// Same as `BoundedSupports` but parameterized by a `BoundedElectionProvider`. +/// Same as `BoundedSupports` but parameterized by a `ElectionProviderBase`. pub type BoundedSupportsOf = BoundedSupports< ::AccountId, ::MaxWinners, diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 8fdb7a79dd537..55c3aa1c9b58b 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -80,7 +80,7 @@ macro_rules! log { pub mod pallet { use super::*; use crate::types::*; - use frame_election_provider_support::ElectionProviderBase; + use frame_election_provider_support::ElectionProvider; use frame_support::{ pallet_prelude::*, traits::{Defensive, ReservableCurrency}, @@ -330,7 +330,7 @@ pub mod pallet { } } - if <::ElectionProvider as ElectionProviderBase>::ongoing() + if ::ElectionProvider::ongoing() { // NOTE: we assume `ongoing` does not consume any weight. // there is an ongoing election -- we better not do anything. Imagine someone is not diff --git a/frame/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index 5bd0cfc2aadee..2635c814b33dd 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -110,16 +110,15 @@ pub struct MockElection; impl frame_election_provider_support::ElectionProviderBase for MockElection { type AccountId = AccountId; type BlockNumber = BlockNumber; + type MaxWinners = MaxWinners; type DataProvider = Staking; type Error = (); +} +impl frame_election_provider_support::ElectionProvider for MockElection { fn ongoing() -> bool { Ongoing::get() } -} - -impl frame_election_provider_support::BoundedElectionProvider for MockElection { - type MaxWinners = MaxWinners; fn elect() -> Result, Self::Error> { Err(()) } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 00e53c55ecb68..065136830d66e 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -183,6 +183,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } impl pallet_staking::Config for Test { @@ -204,7 +206,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution::; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index c4c1d2aca8d07..057ac4e29d5ee 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -156,6 +156,9 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = ConstU32<100>; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } impl pallet_staking::Config for Test { @@ -177,7 +180,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution::; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index cef7c2c854700..d5c8611c8826d 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -146,6 +146,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = ConstU32<100>; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } pub struct OnStakerSlashMock(core::marker::PhantomData); @@ -189,7 +191,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution::; type GenesisElectionProvider = Self::ElectionProvider; type TargetList = pallet_staking::UseValidatorsMap; type MaxUnlockingChunks = ConstU32<32>; diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index f86ddfeedc08b..b7c9bcf6ef5b6 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -150,6 +150,9 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = ConstU32<100>; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } impl pallet_staking::Config for Test { @@ -171,7 +174,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution::; type GenesisElectionProvider = Self::ElectionProvider; type MaxUnlockingChunks = ConstU32<32>; type HistoryDepth = ConstU32<84>; diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index b7015df581b56..0f5b8e0123ab6 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -335,7 +335,7 @@ macro_rules! log { /// Maximum number of winners (aka. active validators), as defined in the election provider of this /// pallet. -pub type MaxWinnersOf = <::ElectionProvider as frame_election_provider_support::BoundedElectionProvider>::MaxWinners; +pub type MaxWinnersOf = <::ElectionProvider as frame_election_provider_support::ElectionProviderBase>::MaxWinners; /// Counter for the number of "reward" points earned by a given validator. pub type RewardPoint = u32; diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index b07c526d557ce..faae214bd6664 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -260,6 +260,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = Staking; type WeightInfo = (); type MaxWinners = MaxWinners; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } pub struct MockReward {} @@ -299,7 +301,7 @@ impl crate::pallet::pallet::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; - type ElectionProvider = onchain::UnboundedExecution; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; // NOTE: consider a macro and use `UseNominatorsAndValidatorsMap` as well. type VoterList = VoterBagsList; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 5b6b2d9cbf67c..77351fbebe5f3 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,7 +18,7 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, BoundedElectionProvider, BoundedSupportsOf, ElectionDataProvider, ScoreProvider, + data_provider, ElectionProvider, BoundedSupportsOf, ElectionDataProvider, ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ @@ -462,7 +462,7 @@ impl Pallet { ) -> Option>> { let election_result: BoundedVec<_, MaxWinnersOf> = if is_genesis { let result = - ::elect().map_err(|e| { + ::elect().map_err(|e| { log!(warn, "genesis election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); }); @@ -473,7 +473,7 @@ impl Pallet { .try_into() .expect("both bounds checked in integrity test to be equal; qed") } else { - let result = ::elect().map_err(|e| { + let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); }); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index eb9c3e0beadd8..a32a7ce55664d 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -17,7 +17,7 @@ //! Staking FRAME Pallet. -use frame_election_provider_support::{BoundedElectionProvider, SortedListProvider, VoteWeight}; +use frame_election_provider_support::{ElectionProvider, ElectionProviderBase, SortedListProvider, VoteWeight}; use frame_support::{ dispatch::Codec, pallet_prelude::*, @@ -107,7 +107,7 @@ pub mod pallet { type CurrencyToVote: CurrencyToVote>; /// Something that provides the election functionality. - type ElectionProvider: BoundedElectionProvider< + type ElectionProvider: ElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, // we only accept an election provider that has staking as data provider. @@ -115,7 +115,7 @@ pub mod pallet { >; /// Something that provides the election functionality at genesis. - type GenesisElectionProvider: BoundedElectionProvider< + type GenesisElectionProvider: ElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Pallet, @@ -786,14 +786,14 @@ pub mod pallet { // ensure election results are always bounded with the same value assert!( - ::MaxWinners::get() == - ::MaxWinners::get() + ::MaxWinners::get() == + ::MaxWinners::get() ); // ensure desired targets requested is always lower than max winners // supported by the election provider. assert!( ValidatorCount::::get() <= - ::MaxWinners::get() + ::MaxWinners::get() ); sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| @@ -1279,10 +1279,7 @@ pub mod pallet { ensure_root(origin)?; // ensure new validator count does not exceed maximum winners // support by election provider. - ensure!( - new <= ::MaxWinners::get(), - Error::::TooManyElectionWinners - ); + ensure!(new <= ::MaxWinners::get(), Error::::TooManyElectionWinners); ValidatorCount::::put(new); Ok(()) } From 2673de670791ea5d9e91f5499d2a660bb20ebfee Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 11 Oct 2022 13:25:22 +0200 Subject: [PATCH 39/66] remove use of BoundedExecution --- bin/node/runtime/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a53eef5d52642..7a7f06ec250ff 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -723,8 +723,8 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type SlashHandler = (); // burn slashes type RewardHandler = (); // nothing to do upon rewards type DataProvider = Staking; - type Fallback = onchain::BoundedExecution; - type GovernanceFallback = onchain::BoundedExecution; + type Fallback = onchain::OnChainExecution; + type GovernanceFallback = onchain::OnChainExecution; type Solver = SequentialPhragmen, OffchainRandomBalancing>; type ForceOrigin = EnsureRootOrHalfCouncil; type MaxElectableTargets = ConstU16<{ u16::MAX }>; From d4c5c1b377295d6d257321987470c26e95692a7d Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 11 Oct 2022 13:48:15 +0200 Subject: [PATCH 40/66] fmt --- bin/node/runtime/src/lib.rs | 2 +- frame/babe/src/mock.rs | 2 +- frame/election-provider-support/src/lib.rs | 4 ++-- frame/fast-unstake/src/lib.rs | 3 +-- frame/grandpa/src/mock.rs | 2 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/root-offences/src/mock.rs | 2 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/staking/src/pallet/impls.rs | 11 +++++------ frame/staking/src/pallet/mod.rs | 9 +++++++-- 10 files changed, 21 insertions(+), 18 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 7a7f06ec250ff..123f6a9707813 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -570,7 +570,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type ElectionProvider = ElectionProviderMultiPhase; - type GenesisElectionProvider = onchain::OnChainExecution::; + type GenesisElectionProvider = onchain::OnChainExecution; type VoterList = VoterList; // This a placeholder, to be introduced in the next PR as an instance of bags-list type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index a10cc69ae2b7e..204de8aae172e 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -202,7 +202,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; - type ElectionProvider = onchain::OnChainExecution::; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 45ac159124254..e6a1920c9f658 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -386,7 +386,7 @@ pub trait ElectionProviderBase { /// Elect a new set of winners, bounded by `MaxWinners`. /// -/// It must always use `Config::DataProvider` to fetch the data it needs. +/// It must always use [`ElectionProviderBase::DataProvider`] to fetch the data it needs. /// /// This election provider that could function asynchronously. This implies that this election might /// needs data ahead of time (ergo, receives no arguments to `elect`), and might be `ongoing` at @@ -404,7 +404,7 @@ pub trait ElectionProvider: ElectionProviderBase { /// A (almost) marker trait that signifies an election provider as working synchronously. i.e. being /// *instant*. /// -/// This must still use the same data provider as with [`Config::DataProvider`]. However, it can +/// This must still use the same data provider as with [`ElectionProviderBase::DataProvider`]. However, it can /// optionally overwrite the amount of voters and targets that are fetched from the data provider at /// runtime via `forced_input_voters_bound` and `forced_input_target_bound`. pub trait InstantElectionProvider: ElectionProviderBase { diff --git a/frame/fast-unstake/src/lib.rs b/frame/fast-unstake/src/lib.rs index 55c3aa1c9b58b..ed26d6b436e1d 100644 --- a/frame/fast-unstake/src/lib.rs +++ b/frame/fast-unstake/src/lib.rs @@ -330,8 +330,7 @@ pub mod pallet { } } - if ::ElectionProvider::ongoing() - { + if ::ElectionProvider::ongoing() { // NOTE: we assume `ongoing` does not consume any weight. // there is an ongoing election -- we better not do anything. Imagine someone is not // exposed anywhere in the last era, and the snapshot for the election is already diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 065136830d66e..1a97b1345fe5d 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -206,7 +206,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; type NextNewSession = Session; - type ElectionProvider = onchain::OnChainExecution::; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 057ac4e29d5ee..e022d81c5b5bd 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -180,7 +180,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); - type ElectionProvider = onchain::OnChainExecution::; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = pallet_staking::UseNominatorsAndValidatorsMap; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/root-offences/src/mock.rs b/frame/root-offences/src/mock.rs index d5c8611c8826d..65bfcad4b26fc 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -191,7 +191,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = OffendingValidatorsThreshold; - type ElectionProvider = onchain::OnChainExecution::; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; type TargetList = pallet_staking::UseValidatorsMap; type MaxUnlockingChunks = ConstU32<32>; diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index b7c9bcf6ef5b6..2db7eb385111c 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -174,7 +174,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); - type ElectionProvider = onchain::OnChainExecution::; + type ElectionProvider = onchain::OnChainExecution; type GenesisElectionProvider = Self::ElectionProvider; type MaxUnlockingChunks = ConstU32<32>; type HistoryDepth = ConstU32<84>; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 77351fbebe5f3..03fd0c361837c 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,7 +18,7 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, ElectionProvider, BoundedSupportsOf, ElectionDataProvider, ScoreProvider, + data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ @@ -461,11 +461,10 @@ impl Pallet { is_genesis: bool, ) -> Option>> { let election_result: BoundedVec<_, MaxWinnersOf> = if is_genesis { - let result = - ::elect().map_err(|e| { - log!(warn, "genesis election provider failed due to {:?}", e); - Self::deposit_event(Event::StakingElectionFailed); - }); + let result = ::elect().map_err(|e| { + log!(warn, "genesis election provider failed due to {:?}", e); + Self::deposit_event(Event::StakingElectionFailed); + }); result .ok()? diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index a32a7ce55664d..50082b7ccc179 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -17,7 +17,9 @@ //! Staking FRAME Pallet. -use frame_election_provider_support::{ElectionProvider, ElectionProviderBase, SortedListProvider, VoteWeight}; +use frame_election_provider_support::{ + ElectionProvider, ElectionProviderBase, SortedListProvider, VoteWeight, +}; use frame_support::{ dispatch::Codec, pallet_prelude::*, @@ -1279,7 +1281,10 @@ pub mod pallet { ensure_root(origin)?; // ensure new validator count does not exceed maximum winners // support by election provider. - ensure!(new <= ::MaxWinners::get(), Error::::TooManyElectionWinners); + ensure!( + new <= ::MaxWinners::get(), + Error::::TooManyElectionWinners + ); ValidatorCount::::put(new); Ok(()) } From de0b4d022128d79c9b21fcd037aaa40f4d7d65f1 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 12 Oct 2022 00:36:00 +0200 Subject: [PATCH 41/66] comment out failing integrity test --- frame/staking/src/pallet/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 50082b7ccc179..964529e323e6d 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -793,10 +793,10 @@ pub mod pallet { ); // ensure desired targets requested is always lower than max winners // supported by the election provider. - assert!( - ValidatorCount::::get() <= - ::MaxWinners::get() - ); + // assert!( + // ValidatorCount::::get() <= + // ::MaxWinners::get() + // ); sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| assert!( From 8c7ec50c98f29a5a973097016eb2a3f46d4c6aa4 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 12 Oct 2022 01:41:27 +0200 Subject: [PATCH 42/66] cap validator count increment to max winners --- frame/election-provider-support/src/lib.rs | 6 ++-- frame/staking/src/pallet/mod.rs | 16 ++++++--- frame/staking/src/tests.rs | 40 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index e6a1920c9f658..9c104b5a5b5e5 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -404,9 +404,9 @@ pub trait ElectionProvider: ElectionProviderBase { /// A (almost) marker trait that signifies an election provider as working synchronously. i.e. being /// *instant*. /// -/// This must still use the same data provider as with [`ElectionProviderBase::DataProvider`]. However, it can -/// optionally overwrite the amount of voters and targets that are fetched from the data provider at -/// runtime via `forced_input_voters_bound` and `forced_input_target_bound`. +/// This must still use the same data provider as with [`ElectionProviderBase::DataProvider`]. +/// However, it can optionally overwrite the amount of voters and targets that are fetched from the +/// data provider at runtime via `forced_input_voters_bound` and `forced_input_target_bound`. pub trait InstantElectionProvider: ElectionProviderBase { fn instant_elect( forced_input_voters_bound: Option, diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 964529e323e6d..4b79b384ff03c 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1289,7 +1289,8 @@ pub mod pallet { Ok(()) } - /// Increments the ideal number of validators. + /// Increments the ideal number of validators upto maximum of + /// `ElectionProviderBase::MaxWinners`. /// /// The dispatch origin must be Root. /// @@ -1302,11 +1303,15 @@ pub mod pallet { #[pallet::compact] additional: u32, ) -> DispatchResult { ensure_root(origin)?; - ValidatorCount::::mutate(|n| *n += additional); + ValidatorCount::::mutate(|n| { + *n = ::MaxWinners::get() + .min(*n + additional); + }); Ok(()) } - /// Scale up the ideal number of validators by a factor. + /// Scale up the ideal number of validators by a factor upto maximum of + /// `ElectionProviderBase::MaxWinners`. /// /// The dispatch origin must be Root. /// @@ -1316,7 +1321,10 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_validator_count())] pub fn scale_validator_count(origin: OriginFor, factor: Percent) -> DispatchResult { ensure_root(origin)?; - ValidatorCount::::mutate(|n| *n += factor * *n); + ValidatorCount::::mutate(|n| { + *n = ::MaxWinners::get() + .min(*n + factor * *n) + }); Ok(()) } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index bc845f780dc54..988b46a39583d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5639,3 +5639,43 @@ fn cannot_set_unsupported_validator_count() { ); }) } + +#[test] +fn increase_validator_count_is_capped() { + ExtBuilder::default().build_and_execute(|| { + MaxWinners::set(50); + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 40)); + + // increased by full value + assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 6)); + assert_eq!(ValidatorCount::::get(), 46); + + // capped to max winners + assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 5)); + assert_eq!(ValidatorCount::::get(), 50); + + // further increment does not do anything + assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 4)); + assert_eq!(ValidatorCount::::get(), 50); + }) +} + +#[test] +fn scale_validator_count_is_capped() { + ExtBuilder::default().build_and_execute(|| { + MaxWinners::set(50); + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 20)); + + // scaled by full value + assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_eq!(ValidatorCount::::get(), 40); + + // capped to max winners + assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_eq!(ValidatorCount::::get(), 50); + + // further scale does not do anything + assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_eq!(ValidatorCount::::get(), 50); + }) +} From 63bd882eff26e850451479cd3a86454aec73bfe0 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Wed, 12 Oct 2022 08:29:43 +0200 Subject: [PATCH 43/66] dont panic --- frame/election-provider-multi-phase/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index d7eb7faa346b1..c769c33858526 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -560,6 +560,7 @@ pub mod pallet { use super::*; use frame_election_provider_support::{InstantElectionProvider, NposSolver}; use frame_support::{pallet_prelude::*, traits::EstimateCallFee}; + use frame_support::traits::DefensiveResult; use frame_system::pallet_prelude::*; #[pallet::config] @@ -1079,7 +1080,7 @@ pub mod pallet { let supports: BoundedVec<_, T::MaxWinners> = supports .into_inner() .try_into() - .expect("integrity test guarantees both bounds are equal; qed"); + .defensive_map_err(|_| Error::::BoundNotMet)?; let solution = ReadySolution { supports, From d348b920dc5382d3a2f97f2a381be9c0de918373 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 13:46:30 +0200 Subject: [PATCH 44/66] add test for bad data provider --- .../src/benchmarking.rs | 6 +-- .../election-provider-multi-phase/src/lib.rs | 1 - .../election-provider-multi-phase/src/mock.rs | 14 +++++-- .../src/signed.rs | 37 +++++++++++++++---- 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 110f2d91f8ed3..addf31348ee44 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -220,11 +220,7 @@ frame_benchmarking::benchmarks! { let receiver = account("receiver", 0, SEED); let initial_balance = T::Currency::minimum_balance() * 10u32.into(); T::Currency::make_free_balance_be(&receiver, initial_balance); - let ready = ReadySolution { - supports: vec![].try_into().unwrap(), - score: Default::default(), - compute: Default::default() - }; + let ready = Default::default(); let deposit: BalanceOf = 10u32.into(); let reward: BalanceOf = T::SignedRewardBase::get(); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index c769c33858526..54c91774ad6c0 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1391,7 +1391,6 @@ impl Pallet { let voters = T::DataProvider::electing_voters(Some(voter_limit)) .map_err(ElectionError::DataProvider)?; - // TODO: make sure a test for this exist/is written. // Defensive-only. if targets.len() > target_limit || voters.len() > voter_limit { debug_assert!(false, "Snapshot limit has not been respected."); diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 7306fb19ca6b7..49853d3d7696e 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -419,11 +419,15 @@ pub type Extrinsic = sp_runtime::testing::TestXt; parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; + // only used in testing to manipulate mock behaviour + pub static DataProviderAllowBadData: bool = false; } #[derive(Default)] pub struct ExtBuilder {} + + pub struct StakingMock; impl ElectionDataProvider for StakingMock { type AccountId = AccountId; @@ -433,7 +437,9 @@ impl ElectionDataProvider for StakingMock { fn electable_targets(maybe_max_len: Option) -> data_provider::Result> { let targets = Targets::get(); - if maybe_max_len.map_or(false, |max_len| targets.len() > max_len) { + if !DataProviderAllowBadData::get() && + maybe_max_len.map_or(false, |max_len| targets.len() > max_len) + { return Err("Targets too big") } @@ -444,8 +450,10 @@ impl ElectionDataProvider for StakingMock { maybe_max_len: Option, ) -> data_provider::Result>> { let mut voters = Voters::get(); - if let Some(max_len) = maybe_max_len { - voters.truncate(max_len) + if !DataProviderAllowBadData::get() { + if let Some(max_len) = maybe_max_len { + voters.truncate(max_len) + } } Ok(voters) diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 6ab4d5a74b8f2..435743c3139f8 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -526,14 +526,7 @@ impl Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{ - mock::{ - balances, multi_phase_events, raw_solution, roll_to, roll_to_signed, Balances, - ExtBuilder, MockedWeightInfo, MultiPhase, Runtime, RuntimeOrigin, SignedMaxRefunds, - SignedMaxSubmissions, SignedMaxWeight, - }, - Error, Event, Perbill, Phase, - }; + use crate::{mock::*, Error, Event, Perbill, Phase}; use frame_support::{assert_noop, assert_ok, assert_storage_noop}; #[test] @@ -553,6 +546,34 @@ mod tests { }) } + #[test] + #[should_panic(expected = "Snapshot limit has not been respected.")] + fn data_provider_should_respect_target_limits() { + ExtBuilder::default().build_and_execute(|| { + // given a reduced expectation of maximum electable targets + MaxElectableTargets::set(2); + // and a data provider that does not respect limits + DataProviderAllowBadData::set(true); + + // should panic here + let _ = MultiPhase::create_snapshot(); + }) + } + + #[test] + #[should_panic(expected = "Snapshot limit has not been respected.")] + fn data_provider_should_respect_voter_limits() { + ExtBuilder::default().build_and_execute(|| { + // given a reduced expectation of maximum electing voters + MaxElectingVoters::set(2); + // and a data provider that does not respect limits + DataProviderAllowBadData::set(true); + + // should panic here + let _ = MultiPhase::create_snapshot(); + }) + } + #[test] fn should_pay_deposit() { ExtBuilder::default().build_and_execute(|| { From 34967a22fca8f42e95f68c5e3e38356631583daa Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Thu, 13 Oct 2022 16:17:04 +0200 Subject: [PATCH 45/66] Update frame/staking/src/pallet/impls.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/staking/src/pallet/impls.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 03fd0c361837c..acae746d68bad 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1101,8 +1101,7 @@ impl pallet_session::SessionManager for Pallet { fn new_session(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {}", new_index); CurrentPlannedSession::::put(new_index); - let validators: Option> = Self::new_session(new_index, false); - validators.map(|v| v.into_inner()) + Self::new_session(new_index, false).map(|v| v.into_inner()); } fn new_session_genesis(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {} at genesis", new_index); From b68c381fcb0ef3826595400b8dfd8cd4cc6a83e3 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 15:48:39 +0200 Subject: [PATCH 46/66] fix namespace conflict and add test for onchain max winners less than desired targets --- .../election-provider-support/src/onchain.rs | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 7b50951495806..4df769f512984 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -24,7 +24,7 @@ use crate::{ InstantElectionProvider, NposSolver, WeightInfo, }; use frame_support::{dispatch::DispatchClass, traits::Get}; -use sp_npos_elections::*; +use sp_npos_elections::{BoundedSupports, VoteWeight, ElectionResult, assignment_ratio_to_staked_normalized, to_supports}; use sp_std::{collections::btree_map::BTreeMap, marker::PhantomData, prelude::*}; /// Errors of the on-chain election. @@ -183,6 +183,7 @@ impl ElectionProvider for OnChainExecution { #[cfg(test)] mod tests { + use frame_support::{assert_noop, parameter_types}; use super::*; use crate::{ElectionProvider, PhragMMS, SequentialPhragmen}; use frame_support::traits::ConstU32; @@ -235,8 +236,9 @@ mod tests { struct PhragmenParams; struct PhragMMSParams; - frame_support::parameter_types! { + parameter_types! { pub static MaxWinners: u32 = 10; + pub static DesiredTargets: u32 = 2; } impl Config for PhragmenParams { @@ -283,7 +285,7 @@ mod tests { } fn desired_targets() -> data_provider::Result { - Ok(2) + Ok(DesiredTargets::get()) } fn next_election_prediction(_: BlockNumber) -> BlockNumber { @@ -305,6 +307,20 @@ mod tests { }) } + #[test] + fn too_many_winners_when_desired_targets_exceed_max_winners() { + sp_io::TestExternalities::new_empty().execute_with(|| { + // given desired targets larger than max winners + DesiredTargets::set(10); + MaxWinners::set(9); + + assert_noop!( + as ElectionProvider>::elect(), + Error::TooManyWinners, + ); + }) + } + #[test] fn onchain_phragmms_works() { sp_io::TestExternalities::new_empty().execute_with(|| { From fddcf5451f87785467d9d998f9dd87d0e02972bf Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:01:55 +0200 Subject: [PATCH 47/66] defensive unwrap --- frame/election-provider-support/src/onchain.rs | 1 - frame/staking/src/pallet/impls.rs | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 4df769f512984..c0631130c768c 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -108,7 +108,6 @@ fn elect_with_input_bounds( let desired_targets = T::DataProvider::desired_targets().map_err(Error::DataProvider)?; if desired_targets > T::MaxWinners::get() { - // TODO: make sure a test for this exists in this crate. // early exit return Err(Error::TooManyWinners) } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index acae746d68bad..6567191844f17 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -470,7 +470,8 @@ impl Pallet { .ok()? .into_inner() .try_into() - .expect("both bounds checked in integrity test to be equal; qed") + // both bounds checked in integrity test to be equal + .defensive_unwrap_or_default() } else { let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); From b5a913ebfcb0a403cad1c1760afa7c867897dd17 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:14:44 +0200 Subject: [PATCH 48/66] early convert to bounded vec --- frame/staking/src/pallet/impls.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 6567191844f17..4521fff4e967b 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -522,7 +522,9 @@ impl Pallet { new_planned_era: EraIndex, ) -> BoundedVec> { let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); - + let elected_stashes: BoundedVec<_, MaxWinnersOf> = elected_stashes + .try_into() + .expect("since we only map through exposures, size of elected_stashes is always same as exposures; qed"); // Populate stakers, exposures, and the snapshot of validator prefs. let mut total_stake: BalanceOf = Zero::zero(); exposures.into_iter().for_each(|(stash, exposure)| { @@ -556,7 +558,7 @@ impl Pallet { ); } - elected_stashes.try_into().expect("since we only map through exposures, size of elected_stashes is always same as exposures; qed") + elected_stashes } /// Consume a set of [`BoundedSupports`] from [`sp_npos_elections`] and collect them into a From 920442e8e4349a1a195e16e8d9ca132a8ec627d2 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:28:01 +0200 Subject: [PATCH 49/66] fix syntax --- frame/staking/src/pallet/impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 4521fff4e967b..9a6f710879282 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1104,7 +1104,7 @@ impl pallet_session::SessionManager for Pallet { fn new_session(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {}", new_index); CurrentPlannedSession::::put(new_index); - Self::new_session(new_index, false).map(|v| v.into_inner()); + Self::new_session(new_index, false).map(|v| v.into_inner()) } fn new_session_genesis(new_index: SessionIndex) -> Option> { log!(trace, "planning new session {} at genesis", new_index); From e68a85159e24bb9d5a4d31849e00f63d2cab25a5 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:28:13 +0200 Subject: [PATCH 50/66] fmt --- frame/election-provider-multi-phase/src/lib.rs | 6 ++++-- frame/election-provider-multi-phase/src/mock.rs | 2 -- frame/election-provider-support/src/onchain.rs | 7 ++++--- frame/staking/src/tests.rs | 15 ++++++++++++--- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 54c91774ad6c0..46c8ae62c1b58 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -559,8 +559,10 @@ pub use pallet::*; pub mod pallet { use super::*; use frame_election_provider_support::{InstantElectionProvider, NposSolver}; - use frame_support::{pallet_prelude::*, traits::EstimateCallFee}; - use frame_support::traits::DefensiveResult; + use frame_support::{ + pallet_prelude::*, + traits::{DefensiveResult, EstimateCallFee}, + }; use frame_system::pallet_prelude::*; #[pallet::config] diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index a01fa14cce560..3fd3595e597fe 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -426,8 +426,6 @@ parameter_types! { #[derive(Default)] pub struct ExtBuilder {} - - pub struct StakingMock; impl ElectionDataProvider for StakingMock { type AccountId = AccountId; diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index c0631130c768c..713323fd17e63 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -24,7 +24,9 @@ use crate::{ InstantElectionProvider, NposSolver, WeightInfo, }; use frame_support::{dispatch::DispatchClass, traits::Get}; -use sp_npos_elections::{BoundedSupports, VoteWeight, ElectionResult, assignment_ratio_to_staked_normalized, to_supports}; +use sp_npos_elections::{ + assignment_ratio_to_staked_normalized, to_supports, BoundedSupports, ElectionResult, VoteWeight, +}; use sp_std::{collections::btree_map::BTreeMap, marker::PhantomData, prelude::*}; /// Errors of the on-chain election. @@ -182,10 +184,9 @@ impl ElectionProvider for OnChainExecution { #[cfg(test)] mod tests { - use frame_support::{assert_noop, parameter_types}; use super::*; use crate::{ElectionProvider, PhragMMS, SequentialPhragmen}; - use frame_support::traits::ConstU32; + use frame_support::{assert_noop, parameter_types, traits::ConstU32}; use sp_npos_elections::Support; use sp_runtime::Perbill; type AccountId = u64; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 988b46a39583d..6490fcf1001fb 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5667,15 +5667,24 @@ fn scale_validator_count_is_capped() { assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 20)); // scaled by full value - assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_ok!(Staking::scale_validator_count( + RuntimeOrigin::root(), + Percent::from_percent(200) + )); assert_eq!(ValidatorCount::::get(), 40); // capped to max winners - assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_ok!(Staking::scale_validator_count( + RuntimeOrigin::root(), + Percent::from_percent(200) + )); assert_eq!(ValidatorCount::::get(), 50); // further scale does not do anything - assert_ok!(Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(200))); + assert_ok!(Staking::scale_validator_count( + RuntimeOrigin::root(), + Percent::from_percent(200) + )); assert_eq!(ValidatorCount::::get(), 50); }) } From 69da4413b30114f687f7c1cd94d9a1d3ac60b728 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:34:17 +0200 Subject: [PATCH 51/66] fix doc --- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 46c8ae62c1b58..f3dad701e3eeb 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -114,8 +114,8 @@ //! If we reach the end of both phases (i.e. call to [`ElectionProvider::elect`] happens) and no //! good solution is queued, then the fallback strategy [`pallet::Config::Fallback`] is used to //! determine what needs to be done. The on-chain election is slow, and contains no balancing or -//! reduction post-processing. [`NoFallback`] does nothing and enables [`Phase::Emergency`], which -//! is a more *fail-safe* approach. +//! reduction post-processing. If [`Fallback`] fails, the next phase [`Phase::Emergency`] is +//! enabled, which is a more *fail-safe* approach. //! //! ### Emergency Phase //! From f0be8d89c8fd4fe80d260b03f125b200b43e22f9 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:48:39 +0200 Subject: [PATCH 52/66] fix rustdoc --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index f3dad701e3eeb..3d50b892e7ade 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -114,7 +114,7 @@ //! If we reach the end of both phases (i.e. call to [`ElectionProvider::elect`] happens) and no //! good solution is queued, then the fallback strategy [`pallet::Config::Fallback`] is used to //! determine what needs to be done. The on-chain election is slow, and contains no balancing or -//! reduction post-processing. If [`Fallback`] fails, the next phase [`Phase::Emergency`] is +//! reduction post-processing. If [`pallet::Config::Fallback`] fails, the next phase [`Phase::Emergency`] is //! enabled, which is a more *fail-safe* approach. //! //! ### Emergency Phase From c5e494a60b1cd13b96f54d4aa7050592ebedfe98 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 16:55:47 +0200 Subject: [PATCH 53/66] fmt --- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 3d50b892e7ade..b6c3c823d0e91 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -114,8 +114,8 @@ //! If we reach the end of both phases (i.e. call to [`ElectionProvider::elect`] happens) and no //! good solution is queued, then the fallback strategy [`pallet::Config::Fallback`] is used to //! determine what needs to be done. The on-chain election is slow, and contains no balancing or -//! reduction post-processing. If [`pallet::Config::Fallback`] fails, the next phase [`Phase::Emergency`] is -//! enabled, which is a more *fail-safe* approach. +//! reduction post-processing. If [`pallet::Config::Fallback`] fails, the next phase +//! [`Phase::Emergency`] is enabled, which is a more *fail-safe* approach. //! //! ### Emergency Phase //! From 2d60cb079d5271cf8c01ddf1d68cf75467338c3b Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 13 Oct 2022 20:00:08 +0200 Subject: [PATCH 54/66] fix maxwinner count for benchmarking --- frame/election-provider-multi-phase/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 3fd3595e597fe..d95e46dfa1a18 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -297,7 +297,7 @@ parameter_types! { pub static MockWeightInfo: MockedWeightInfo = MockedWeightInfo::Real; pub static MaxElectingVoters: VoterIndex = u32::max_value(); pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value(); - pub static MaxWinners: u32 = 100; + pub static MaxWinners: u32 = 200; pub static EpochLength: u64 = 30; pub static OnChainFallback: bool = true; From a3d7ec0d22b2a63e1d3516d9acc0301fd53e8277 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 14 Oct 2022 21:36:02 +0200 Subject: [PATCH 55/66] add instant election for noelection --- frame/election-provider-support/src/lib.rs | 25 ++++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 9c104b5a5b5e5..e357cc0811d36 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -417,22 +417,22 @@ pub trait InstantElectionProvider: ElectionProviderBase { /// An election provider that does nothing whatsoever. pub struct NoElection(sp_std::marker::PhantomData); -impl ElectionProviderBase - for NoElection<(AccountId, BlockNumber, DataProvider)> +impl ElectionProviderBase + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where - DataProvider: ElectionDataProvider, + DataProvider: ElectionDataProvider, MaxWinners: Get, { type AccountId = AccountId; type BlockNumber = BlockNumber; type Error = &'static str; - type MaxWinners = frame_support::traits::ConstU32<0>; + type MaxWinners = MaxWinners; type DataProvider = DataProvider; } -impl ElectionProvider - for NoElection<(AccountId, BlockNumber, DataProvider)> +impl ElectionProvider + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where - DataProvider: ElectionDataProvider, + DataProvider: ElectionDataProvider, MaxWinners: Get, { fn ongoing() -> bool { false @@ -443,6 +443,17 @@ where } } + +impl InstantElectionProvider +for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> + where + DataProvider: ElectionDataProvider, MaxWinners: Get, +{ + fn instant_elect(_: Option, _: Option) -> Result, Self::Error> { + Err("`NoElection` cannot do anything.") + } +} + /// A utility trait for something to implement `ElectionDataProvider` in a sensible way. /// /// This is generic over `AccountId` and it can represent a validator, a nominator, or any other From 1e3e2398dcb7bce80d1e20436ad22b58f7d0de98 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 14 Oct 2022 21:47:13 +0200 Subject: [PATCH 56/66] fmt --- frame/election-provider-support/src/lib.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index e357cc0811d36..2804ff4599acb 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -420,7 +420,8 @@ pub struct NoElection(sp_std::marker::PhantomData); impl ElectionProviderBase for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where - DataProvider: ElectionDataProvider, MaxWinners: Get, + DataProvider: ElectionDataProvider, + MaxWinners: Get, { type AccountId = AccountId; type BlockNumber = BlockNumber; @@ -432,7 +433,8 @@ where impl ElectionProvider for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where - DataProvider: ElectionDataProvider, MaxWinners: Get, + DataProvider: ElectionDataProvider, + MaxWinners: Get, { fn ongoing() -> bool { false @@ -443,13 +445,16 @@ where } } - impl InstantElectionProvider -for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> - where - DataProvider: ElectionDataProvider, MaxWinners: Get, + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> +where + DataProvider: ElectionDataProvider, + MaxWinners: Get, { - fn instant_elect(_: Option, _: Option) -> Result, Self::Error> { + fn instant_elect( + _: Option, + _: Option, + ) -> Result, Self::Error> { Err("`NoElection` cannot do anything.") } } From 2c43b04a75f7ad5a77001679f4cc7f79636afc9f Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Fri, 14 Oct 2022 21:56:37 +0200 Subject: [PATCH 57/66] fix compile --- frame/nomination-pools/benchmarking/src/mock.rs | 2 +- frame/nomination-pools/test-staking/src/mock.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index 0f617f0bf6f4b..2806234e21f7e 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -110,7 +110,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); type ElectionProvider = - frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking)>; + frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ConstU32<0>)>; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = VoterList; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/nomination-pools/test-staking/src/mock.rs b/frame/nomination-pools/test-staking/src/mock.rs index 8c04e40d71df4..ced0abf0a4c18 100644 --- a/frame/nomination-pools/test-staking/src/mock.rs +++ b/frame/nomination-pools/test-staking/src/mock.rs @@ -124,7 +124,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); type ElectionProvider = - frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking)>; + frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ConstU32<0>)>; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = VoterList; type TargetList = pallet_staking::UseValidatorsMap; From 99e38b3e9b7a843b21aeeec3234937ae8d39e6d6 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 11:28:10 +0200 Subject: [PATCH 58/66] pr feedbacks --- bin/node/runtime/src/lib.rs | 14 ++++++--- .../election-provider-multi-phase/src/lib.rs | 13 ++++---- .../src/signed.rs | 30 ++++++++++++++----- .../src/unsigned.rs | 3 +- .../nomination-pools/benchmarking/src/mock.rs | 2 +- .../nomination-pools/test-staking/src/mock.rs | 2 +- frame/staking/src/pallet/impls.rs | 5 ++++ frame/staking/src/pallet/mod.rs | 11 ++++--- 8 files changed, 53 insertions(+), 27 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index e5a8359ec70cf..a81d0c09cd194 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -626,7 +626,13 @@ frame_election_provider_support::generate_solution_type!( parameter_types! { pub MaxNominations: u32 = ::LIMIT as u32; - pub MaxElectingVoters: u32 = 10_000; + pub MaxElectingVoters: u32 = 40_000; + pub MaxElectableTargets: u16 = 10_000; + // OnChain values are lower. + pub MaxOnChainElectingVoters: u32 = 5000; + pub MaxOnChainElectableTargets: u16 = 1250; + // The maximum winners that can be elected by the Election pallet which is equivalent to the + // maximum active validators the staking pallet can have. pub MaxActiveValidators: u32 = 1000; } @@ -679,8 +685,8 @@ impl onchain::Config for OnChainSeqPhragmen { type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; type MaxWinners = ::MaxWinners; - type VotersBound = MaxElectingVoters; - type TargetsBound = ConstU32<2_000>; + type VotersBound = MaxOnChainElectingVoters; + type TargetsBound = MaxOnChainElectableTargets; } impl pallet_election_provider_multi_phase::MinerConfig for Runtime { @@ -727,7 +733,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type GovernanceFallback = onchain::OnChainExecution; type Solver = SequentialPhragmen, OffchainRandomBalancing>; type ForceOrigin = EnsureRootOrHalfCouncil; - type MaxElectableTargets = ConstU16<{ u16::MAX }>; + type MaxElectableTargets = MaxElectableTargets; type MaxWinners = MaxActiveValidators; type MaxElectingVoters = MaxElectingVoters; type BenchmarkingConfig = ElectionProviderBenchmarkConfig; diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index b6c3c823d0e91..e1dce9a01cfb7 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -662,7 +662,10 @@ pub mod pallet { #[pallet::constant] type MaxElectableTargets: Get>; - /// The maximum number of winners that can be elected from the electable targets. + /// The maximum number of winners that can be elected by this `ElectionProvider` + /// implementation. + /// + /// Note: This must always be greater or equal to `T::DataProvider::desired_targets()`. #[pallet::constant] type MaxWinners: Get; @@ -1393,9 +1396,7 @@ impl Pallet { let voters = T::DataProvider::electing_voters(Some(voter_limit)) .map_err(ElectionError::DataProvider)?; - // Defensive-only. if targets.len() > target_limit || voters.len() > voter_limit { - debug_assert!(false, "Snapshot limit has not been respected."); return Err(ElectionError::DataProvider("Snapshot too big for submission.")) } @@ -1534,6 +1535,8 @@ impl Pallet { let known_score = supports.evaluate(); ensure!(known_score == score, FeasibilityError::InvalidScore); + // Miner mines a solution with target size equal to `DataProvider::desired_targets()`, + // which can never be larger than `MaxWinners`. let supports = supports.try_into().map_err(|_| FeasibilityError::BoundNotMet)?; Ok(ReadySolution { supports, compute, score }) } @@ -1570,9 +1573,7 @@ impl Pallet { .map_err(|fe| ElectionError::Fallback(fe)) .and_then(|supports| { Ok(ReadySolution { - supports: supports - .try_into() - .map_err(|_| FeasibilityError::BoundNotMet)?, + supports, score: Default::default(), compute: ElectionCompute::Fallback, }) diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 435743c3139f8..4e66fd211caae 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -526,7 +526,7 @@ impl Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{mock::*, Error, Event, Perbill, Phase}; + use crate::{mock::*, ElectionError, Error, Event, Perbill, Phase}; use frame_support::{assert_noop, assert_ok, assert_storage_noop}; #[test] @@ -547,7 +547,6 @@ mod tests { } #[test] - #[should_panic(expected = "Snapshot limit has not been respected.")] fn data_provider_should_respect_target_limits() { ExtBuilder::default().build_and_execute(|| { // given a reduced expectation of maximum electable targets @@ -555,13 +554,14 @@ mod tests { // and a data provider that does not respect limits DataProviderAllowBadData::set(true); - // should panic here - let _ = MultiPhase::create_snapshot(); + assert_noop!( + MultiPhase::create_snapshot(), + ElectionError::DataProvider("Snapshot too big for submission."), + ); }) } #[test] - #[should_panic(expected = "Snapshot limit has not been respected.")] fn data_provider_should_respect_voter_limits() { ExtBuilder::default().build_and_execute(|| { // given a reduced expectation of maximum electing voters @@ -569,8 +569,24 @@ mod tests { // and a data provider that does not respect limits DataProviderAllowBadData::set(true); - // should panic here - let _ = MultiPhase::create_snapshot(); + assert_noop!( + MultiPhase::create_snapshot(), + ElectionError::DataProvider("Snapshot too big for submission."), + ); + }) + } + + #[test] + fn desired_targets_greater_than_max_winners() { + ExtBuilder::default().build_and_execute(|| { + // given desired_targets bigger than MaxWinners + DesiredTargets::set(4); + MaxWinners::set(3); + + let (_, _, actual_desired_targets) = MultiPhase::create_snapshot_external().unwrap(); + + // snapshot is created with min of desired_targets and MaxWinners + assert_eq!(actual_desired_targets, 3); }) } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 71f118355518a..7340605dfe621 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1171,8 +1171,7 @@ mod tests { // set a better score let ready = ReadySolution { score: ElectionScore { minimal_stake: 10, ..Default::default() }, - supports: bounded_vec![], - compute: Default::default(), + ..Default::default() }; >::put(ready); diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index 2806234e21f7e..dca5d660af4ce 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -110,7 +110,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); type ElectionProvider = - frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ConstU32<0>)>; + frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ())>; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = VoterList; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/nomination-pools/test-staking/src/mock.rs b/frame/nomination-pools/test-staking/src/mock.rs index ced0abf0a4c18..c35144553842b 100644 --- a/frame/nomination-pools/test-staking/src/mock.rs +++ b/frame/nomination-pools/test-staking/src/mock.rs @@ -124,7 +124,7 @@ impl pallet_staking::Config for Runtime { type MaxNominatorRewardedPerValidator = ConstU32<64>; type OffendingValidatorsThreshold = (); type ElectionProvider = - frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ConstU32<0>)>; + frame_election_provider_support::NoElection<(AccountId, BlockNumber, Staking, ())>; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = VoterList; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 9a6f710879282..15ece0429ed87 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1591,6 +1591,11 @@ impl Pallet { Nominators::::count() + Validators::::count(), "wrong external count" ); + + ensure!( + ValidatorCount::::get() <= + ::MaxWinners::get() + ); Ok(()) } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 4b79b384ff03c..41f191c6ea950 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -648,6 +648,10 @@ pub mod pallet { ), _ => Ok(()), }); + assert!( + ValidatorCount::::get() <= + ::MaxWinners::get() + ); } // all voters are reported to the `VoterList`. @@ -791,12 +795,7 @@ pub mod pallet { ::MaxWinners::get() == ::MaxWinners::get() ); - // ensure desired targets requested is always lower than max winners - // supported by the election provider. - // assert!( - // ValidatorCount::::get() <= - // ::MaxWinners::get() - // ); + sp_std::if_std! { sp_io::TestExternalities::new_empty().execute_with(|| assert!( From fcd317e3c555075c8e9f97284d9df82ded1dcf8d Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 14:32:51 +0200 Subject: [PATCH 59/66] always error at validator count exceeding max winners --- frame/staking/src/pallet/mod.rs | 31 +++++++++++++++---------- frame/staking/src/tests.rs | 40 +++++++++++++-------------------- 2 files changed, 34 insertions(+), 37 deletions(-) diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 41f191c6ea950..064dd20899506 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -34,7 +34,7 @@ use frame_support::{ use frame_system::{ensure_root, ensure_signed, pallet_prelude::*}; use sp_runtime::{ traits::{CheckedSub, SaturatedConversion, StaticLookup, Zero}, - Perbill, Percent, + ArithmeticError, Perbill, Percent, }; use sp_staking::{EraIndex, SessionIndex}; use sp_std::prelude::*; @@ -756,8 +756,6 @@ pub mod pallet { CommissionTooLow, /// Some bound is not met. BoundNotMet, - /// Validator count exceeded the maximum supported by the `ElectionProvider`. - TooManyElectionWinners, } #[pallet::hooks] @@ -1282,7 +1280,7 @@ pub mod pallet { // support by election provider. ensure!( new <= ::MaxWinners::get(), - Error::::TooManyElectionWinners + Error::::TooManyValidators ); ValidatorCount::::put(new); Ok(()) @@ -1302,10 +1300,14 @@ pub mod pallet { #[pallet::compact] additional: u32, ) -> DispatchResult { ensure_root(origin)?; - ValidatorCount::::mutate(|n| { - *n = ::MaxWinners::get() - .min(*n + additional); - }); + let old = ValidatorCount::::get(); + let new = old.checked_add(additional).ok_or(ArithmeticError::Overflow)?; + ensure!( + new <= ::MaxWinners::get(), + Error::::TooManyValidators + ); + + ValidatorCount::::put(new); Ok(()) } @@ -1320,10 +1322,15 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_validator_count())] pub fn scale_validator_count(origin: OriginFor, factor: Percent) -> DispatchResult { ensure_root(origin)?; - ValidatorCount::::mutate(|n| { - *n = ::MaxWinners::get() - .min(*n + factor * *n) - }); + let old = ValidatorCount::::get(); + let new = old.checked_add(factor.mul_floor(old)).ok_or(ArithmeticError::Overflow)?; + + ensure!( + new <= ::MaxWinners::get(), + Error::::TooManyValidators + ); + + ValidatorCount::::put(new); Ok(()) } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 6490fcf1001fb..6609b9087637d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5635,56 +5635,46 @@ fn cannot_set_unsupported_validator_count() { // setting validator count above 100 does not work assert_noop!( Staking::set_validator_count(RuntimeOrigin::root(), 51), - Error::::TooManyElectionWinners, + Error::::TooManyValidators, ); }) } #[test] -fn increase_validator_count_is_capped() { +fn increase_validator_count_errors() { ExtBuilder::default().build_and_execute(|| { MaxWinners::set(50); assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 40)); - // increased by full value + // increase works assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 6)); assert_eq!(ValidatorCount::::get(), 46); - // capped to max winners - assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 5)); - assert_eq!(ValidatorCount::::get(), 50); - - // further increment does not do anything - assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 4)); - assert_eq!(ValidatorCount::::get(), 50); + // errors + assert_noop!( + Staking::increase_validator_count(RuntimeOrigin::root(), 5), + Error::::TooManyValidators, + ); }) } #[test] -fn scale_validator_count_is_capped() { +fn scale_validator_count_errors() { ExtBuilder::default().build_and_execute(|| { MaxWinners::set(50); assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 20)); - // scaled by full value + // scale value works assert_ok!(Staking::scale_validator_count( RuntimeOrigin::root(), Percent::from_percent(200) )); assert_eq!(ValidatorCount::::get(), 40); - // capped to max winners - assert_ok!(Staking::scale_validator_count( - RuntimeOrigin::root(), - Percent::from_percent(200) - )); - assert_eq!(ValidatorCount::::get(), 50); - - // further scale does not do anything - assert_ok!(Staking::scale_validator_count( - RuntimeOrigin::root(), - Percent::from_percent(200) - )); - assert_eq!(ValidatorCount::::get(), 50); + // errors + assert_noop!( + Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(126)), + Error::::TooManyValidators, + ); }) } From 5516ffa54784c9ddbf4dcd4fb218a7d9f541d453 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 14:35:00 +0200 Subject: [PATCH 60/66] add useful error message --- frame/staking/src/pallet/impls.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 15ece0429ed87..a8edd3ce02dbb 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1594,7 +1594,8 @@ impl Pallet { ensure!( ValidatorCount::::get() <= - ::MaxWinners::get() + ::MaxWinners::get(), + "validator count exceeded election max winners" ); Ok(()) } From a576237f694a301b1c055b361d1f4388dc763d9f Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 15:16:46 +0200 Subject: [PATCH 61/66] pr comments --- frame/election-provider-multi-phase/src/lib.rs | 18 ++++++++++++------ frame/election-provider-support/src/onchain.rs | 2 +- frame/staking/src/pallet/impls.rs | 8 ++++++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index e1dce9a01cfb7..cf387024fccbd 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -544,8 +544,8 @@ pub enum FeasibilityError { InvalidRound, /// Comparison against `MinimumUntrustedScore` failed. UntrustedScoreTooLow, - /// A bound was not satisfied. - BoundNotMet, + /// Data Provider returned too many desired targets + TooManyDesiredTargets, } impl From for FeasibilityError { @@ -966,7 +966,7 @@ pub mod pallet { ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); // bound supports with T::MaxWinners - let supports = supports.try_into().map_err(|_| Error::::BoundNotMet)?; + let supports = supports.try_into().map_err(|_| Error::::TooManyWinners)?; // Note: we don't `rotate_round` at this point; the next call to // `ElectionProvider::elect` will succeed and take care of that. @@ -1158,6 +1158,8 @@ pub mod pallet { FallbackFailed, /// Some bound not met BoundNotMet, + /// Submitted solution has too many winners + TooManyWinners, } #[pallet::validate_unsigned] @@ -1475,6 +1477,11 @@ impl Pallet { Self::desired_targets().ok_or(FeasibilityError::SnapshotUnavailable)?; ensure!(winners.len() as u32 == desired_targets, FeasibilityError::WrongWinnerCount); + // Fail early if targets requested by data provider exceed maximum winners supported. + ensure!( + desired_targets <= ::MaxWinners::get(), + FeasibilityError::TooManyDesiredTargets + ); // Ensure that the solution's score can pass absolute min-score. let submitted_score = raw_solution.score; @@ -1535,9 +1542,8 @@ impl Pallet { let known_score = supports.evaluate(); ensure!(known_score == score, FeasibilityError::InvalidScore); - // Miner mines a solution with target size equal to `DataProvider::desired_targets()`, - // which can never be larger than `MaxWinners`. - let supports = supports.try_into().map_err(|_| FeasibilityError::BoundNotMet)?; + // Size of winners in miner solution is equal to `desired_targets` <= `MaxWinners`. + let supports = supports.try_into().expect("checked desired_targets <= MaxWinners; qed"); Ok(ReadySolution { supports, compute, score }) } diff --git a/frame/election-provider-support/src/onchain.rs b/frame/election-provider-support/src/onchain.rs index 713323fd17e63..483c402fe249c 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -81,7 +81,7 @@ pub trait Config { /// Upper bound on maximum winners from electable targets. /// - /// As as noted in the documentation of [`ElectionProviderBase::MaxWinners`], this value should + /// As noted in the documentation of [`ElectionProviderBase::MaxWinners`], this value should /// always be more than `DataProvider::desired_target`. type MaxWinners: Get; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index a8edd3ce02dbb..a80c58fcc9127 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -521,10 +521,14 @@ impl Pallet { >, new_planned_era: EraIndex, ) -> BoundedVec> { - let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); - let elected_stashes: BoundedVec<_, MaxWinnersOf> = elected_stashes + let elected_stashes: BoundedVec<_, MaxWinnersOf> = exposures + .iter() + .cloned() + .map(|(x, _)| x) + .collect::>() .try_into() .expect("since we only map through exposures, size of elected_stashes is always same as exposures; qed"); + // Populate stakers, exposures, and the snapshot of validator prefs. let mut total_stake: BalanceOf = Zero::zero(); exposures.into_iter().for_each(|(stash, exposure)| { From 70ae6ddcf603104935f9c9e6d3c028f4d67dc42c Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 15:39:15 +0200 Subject: [PATCH 62/66] import fix --- frame/election-provider-support/src/lib.rs | 7 +++++++ frame/staking/src/pallet/impls.rs | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 2804ff4599acb..4202c14a89469 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -382,6 +382,13 @@ pub trait ElectionProviderBase { AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, >; + + fn desired_targets_checked() -> u32 { + match Self::DataProvider::desired_targets() { + Ok(desired_targets) => desired_targets.min(Self::MaxWinners::get()), + Err(e) => Self::MaxWinners::get(), + } + } } /// Elect a new set of winners, bounded by `MaxWinners`. diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index a80c58fcc9127..7eab0a6ccd0ee 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ScoreProvider, - SortedListProvider, VoteWeight, VoterOf, + data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, From f6b6ae6d04979f86b2d392137395d3effcb82f7c Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 16:00:54 +0200 Subject: [PATCH 63/66] add checked_desired_targets --- frame/election-provider-support/src/lib.rs | 13 ++++++++++--- frame/staking/src/pallet/impls.rs | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 4202c14a89469..eeb4de0eedbc4 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -383,10 +383,17 @@ pub trait ElectionProviderBase { BlockNumber = Self::BlockNumber, >; - fn desired_targets_checked() -> u32 { + /// checked call to [`Self::DataProvider::desired_targets()`] ensuring the value never exceeds + /// [`Self::MaxWinners`]. + fn desired_targets_checked() -> data_provider::Result { match Self::DataProvider::desired_targets() { - Ok(desired_targets) => desired_targets.min(Self::MaxWinners::get()), - Err(e) => Self::MaxWinners::get(), + Ok(desired_targets) => + if desired_targets <= Self::MaxWinners::get() { + Ok(desired_targets) + } else { + Err("desired_targets should not be greater than MaxWinners") + }, + Err(e) => Err(e), } } } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 7eab0a6ccd0ee..8e22493feb75d 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,7 +18,7 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ScoreProvider, SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ @@ -1598,7 +1598,7 @@ impl Pallet { ensure!( ValidatorCount::::get() <= - ::MaxWinners::get(), + ::MaxWinners::get(), "validator count exceeded election max winners" ); Ok(()) From 7d3a5665fcaf737bbfa6ef0712dc5fdfd31983e0 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 18 Oct 2022 16:16:18 +0200 Subject: [PATCH 64/66] fmt --- frame/staking/src/pallet/impls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 8e22493feb75d..9018ee1903a2e 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, - ScoreProvider, SortedListProvider, VoteWeight, VoterOf, + data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ScoreProvider, + SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, From 3e6498571b94305ac1d9aa777f6e3b081288ea7b Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 7 Nov 2022 17:14:14 +0530 Subject: [PATCH 65/66] fmt --- frame/election-provider-multi-phase/src/signed.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 837db024136e5..9d629ad77fd79 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -537,7 +537,7 @@ impl Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{mock::*, ElectionError, ElectionCompute, Error, Event, Perbill, Phase}; + use crate::{mock::*, ElectionCompute, ElectionError, Error, Event, Perbill, Phase}; use frame_support::{assert_noop, assert_ok, assert_storage_noop}; #[test] From a8234caa352fb3a59fe099203c6b271ad2d371bb Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Mon, 7 Nov 2022 18:54:15 +0530 Subject: [PATCH 66/66] fix rust doc --- 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 eeb4de0eedbc4..38924a18e2f54 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -383,7 +383,7 @@ pub trait ElectionProviderBase { BlockNumber = Self::BlockNumber, >; - /// checked call to [`Self::DataProvider::desired_targets()`] ensuring the value never exceeds + /// checked call to `Self::DataProvider::desired_targets()` ensuring the value never exceeds /// [`Self::MaxWinners`]. fn desired_targets_checked() -> data_provider::Result { match Self::DataProvider::desired_targets() {