From fc0facef083545e8b0a258de5c856062c636ecf4 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Wed, 9 Nov 2022 10:11:51 +0100 Subject: [PATCH] Bound Election and Staking by MaxActiveValidators (#12436) * bounding election provider with kian * multi phase implement bounded election provider * election provider blanket implementation * staking compiles * fix test for election provider support * fmt * fixing epmp tests, does not compile yet * fix epmp tests * fix staking tests * fmt * fix runtime tests * fmt * remove outdated wip tags * add enum error * sort and truncate supports * comment * error when unsupported number of election winners * compiling wip after kian's suggestions * fix TODOs * remove,fix tags * ensure validator count does not exceed maxwinners * clean up * some more clean up and todos * handle too many winners * rename parameter for mock * todo * add sort and truncate rule if there are too many winners * fmt * fail, not swallow emergency result bound not met * remove too many winners resolution as it can be guaranteed to be bounded * fix benchmark * give MaxWinners more contextual name * make ready solution generic over T * kian feedback * fix stuff * Kian's way of solvign this * comment fix * fix compile * remove use of BoundedExecution * fmt * comment out failing integrity test * cap validator count increment to max winners * dont panic * add test for bad data provider * Update frame/staking/src/pallet/impls.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * fix namespace conflict and add test for onchain max winners less than desired targets * defensive unwrap * early convert to bounded vec * fix syntax * fmt * fix doc * fix rustdoc * fmt * fix maxwinner count for benchmarking * add instant election for noelection * fmt * fix compile * pr feedbacks * always error at validator count exceeding max winners * add useful error message * pr comments * import fix * add checked_desired_targets * fmt * fmt * fix rust doc Co-authored-by: parity-processbot <> Co-authored-by: kianenigma Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- bin/node/runtime/src/lib.rs | 26 ++- frame/babe/src/mock.rs | 5 +- .../src/benchmarking.rs | 8 +- .../election-provider-multi-phase/src/lib.rs | 164 ++++++++-------- .../election-provider-multi-phase/src/mock.rs | 49 ++--- .../src/signed.rs | 48 ++++- frame/election-provider-support/src/lib.rs | 137 ++++++++------ .../election-provider-support/src/onchain.rs | 178 +++++++++--------- frame/fast-unstake/src/mock.rs | 9 +- frame/grandpa/src/mock.rs | 5 +- .../nomination-pools/benchmarking/src/mock.rs | 2 +- .../nomination-pools/test-staking/src/mock.rs | 2 +- frame/offences/benchmarking/src/mock.rs | 5 +- frame/root-offences/src/mock.rs | 5 +- frame/session/benchmarking/src/mock.rs | 5 +- frame/staking/src/lib.rs | 4 + frame/staking/src/mock.rs | 6 +- frame/staking/src/pallet/impls.rs | 79 +++++--- frame/staking/src/pallet/mod.rs | 55 +++++- frame/staking/src/tests.rs | 54 ++++++ primitives/npos-elections/src/lib.rs | 4 +- 21 files changed, 538 insertions(+), 312 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 999b178d10c55..d94a31422be8b 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; @@ -628,7 +628,14 @@ 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; } /// The numbers configured here could always be more than the the maximum limits of staking pallet @@ -679,11 +686,9 @@ impl onchain::Config for OnChainSeqPhragmen { >; type DataProvider = ::DataProvider; type WeightInfo = frame_election_provider_support::weights::SubstrateWeight; -} - -impl onchain::BoundedConfig for OnChainSeqPhragmen { - type VotersBound = MaxElectingVoters; - type TargetsBound = ConstU32<2_000>; + type MaxWinners = ::MaxWinners; + type VotersBound = MaxOnChainElectingVoters; + type TargetsBound = MaxOnChainElectableTargets; } impl pallet_election_provider_multi_phase::MinerConfig for Runtime { @@ -726,11 +731,12 @@ 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 }>; + type MaxElectableTargets = MaxElectableTargets; + type MaxWinners = MaxActiveValidators; 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..204de8aae172e 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -178,6 +178,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 { @@ -199,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/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index a0fd0a9a0512f..10041f6aec07c 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![], - score: Default::default(), - compute: Default::default() - }; + let ready = Default::default(); let deposit: BalanceOf = 10u32.into(); let reward: BalanceOf = T::SignedRewardBase::get(); @@ -403,7 +399,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 3e5a6d12f575c..bc19e5143424c 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 [`pallet::Config::Fallback`] fails, the next phase +//! [`Phase::Emergency`] is enabled, which is a more *fail-safe* approach. //! //! ### Emergency Phase //! @@ -231,23 +231,25 @@ use codec::{Decode, Encode}; use frame_election_provider_support::{ - ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, - NposSolution, + BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + InstantElectionProvider, NposSolution, }; use frame_support::{ dispatch::DispatchClass, ensure, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, 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::{ - assignment_ratio_to_staked_normalized, ElectionScore, EvaluateSupport, Supports, VoteWeight, + assignment_ratio_to_staked_normalized, BoundedSupports, ElectionScore, EvaluateSupport, + Supports, VoteWeight, }; use sp_runtime::{ transaction_validity::{ @@ -311,33 +313,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 ElectionProvider for NoFallback { - 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> { - Err("NoFallback.") - } -} - /// Current phase of the pallet. #[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)] pub enum Phase { @@ -445,13 +420,23 @@ impl Default for RawSolution { } /// A checked solution, ready to be enacted. -#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] -pub struct ReadySolution { +#[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. /// /// 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. @@ -465,7 +450,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. @@ -561,6 +545,8 @@ pub enum FeasibilityError { InvalidRound, /// Comparison against `MinimumUntrustedScore` failed. UntrustedScoreTooLow, + /// Data Provider returned too many desired targets + TooManyDesiredTargets, } impl From for FeasibilityError { @@ -574,7 +560,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::{ + pallet_prelude::*, + traits::{DefensiveResult, EstimateCallFee}, + }; use frame_system::pallet_prelude::*; #[pallet::config] @@ -674,6 +663,13 @@ pub mod pallet { #[pallet::constant] type MaxElectableTargets: Get>; + /// 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; + /// Handler for the slashed deposits. type SlashHandler: OnUnbalanced>; @@ -691,6 +687,7 @@ pub mod pallet { AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Self::DataProvider, + MaxWinners = Self::MaxWinners, >; /// Configuration of the governance-only fallback. @@ -701,6 +698,7 @@ pub mod pallet { AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Self::DataProvider, + MaxWinners = Self::MaxWinners, >; /// OCW election solution miner algorithm implementation. @@ -968,9 +966,11 @@ pub mod pallet { T::ForceOrigin::ensure_origin(origin)?; ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); + // bound supports with T::MaxWinners + 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. - let solution = ReadySolution { supports, score: Default::default(), @@ -1073,17 +1073,20 @@ 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::instant_elect(maybe_max_voters, maybe_max_targets).map_err( + |e| { + log!(error, "GovernanceFallback failed: {:?}", e); + Error::::FallbackFailed + }, + )?; - 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 - })?; + // transform BoundedVec<_, T::GovernanceFallback::MaxWinners> into + // `BoundedVec<_, T::MaxWinners>` + let supports: BoundedVec<_, T::MaxWinners> = supports + .into_inner() + .try_into() + .defensive_map_err(|_| Error::::BoundNotMet)?; let solution = ReadySolution { supports, @@ -1154,6 +1157,10 @@ pub mod pallet { CallNotAllowed, /// The fallback failed FallbackFailed, + /// Some bound not met + BoundNotMet, + /// Submitted solution has too many winners + TooManyWinners, } #[pallet::validate_unsigned] @@ -1227,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. /// @@ -1393,31 +1400,28 @@ 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)?; - // 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.")) } - // 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 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_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)) @@ -1466,7 +1470,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. @@ -1479,6 +1483,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; @@ -1539,6 +1548,8 @@ impl Pallet { let known_score = supports.evaluate(); ensure!(known_score == score, FeasibilityError::InvalidScore); + // 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 }) } @@ -1558,7 +1569,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: // @@ -1570,13 +1581,15 @@ impl Pallet { >::take() .ok_or(ElectionError::::NothingQueued) .or_else(|_| { - ::elect() - .map(|supports| ReadySolution { - supports, - score: Default::default(), - compute: ElectionCompute::Fallback, - }) + T::Fallback::instant_elect(None, None) .map_err(|fe| ElectionError::Fallback(fe)) + .and_then(|supports| { + Ok(ReadySolution { + supports, + score: Default::default(), + compute: ElectionCompute::Fallback, + }) + }) }) .map(|ReadySolution { compute, score, supports }| { Self::deposit_event(Event::ElectionFinalized { compute, score }); @@ -1609,18 +1622,19 @@ impl ElectionProviderBase for Pallet { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; type Error = ElectionError; + type MaxWinners = T::MaxWinners; type DataProvider = T::DataProvider; +} +impl ElectionProvider for Pallet { fn ongoing() -> bool { match Self::current_phase() { Phase::Off => false, _ => true, } } -} -impl ElectionProvider for Pallet { - fn elect() -> Result, Self::Error> { + 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. @@ -1636,6 +1650,7 @@ impl ElectionProvider 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 { @@ -1854,7 +1869,6 @@ mod tests { }, Phase, }; - use frame_election_provider_support::ElectionProvider; use frame_support::{assert_noop, assert_ok}; use sp_npos_elections::{BalancingConfig, Support}; diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index f645886d78899..8ab7e5bbf733d 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}; @@ -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 = 200; pub static EpochLength: u64 = 30; pub static OnChainFallback: bool = true; @@ -308,6 +309,9 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen, Balancing>; type DataProvider = StakingMock; type WeightInfo = (); + type MaxWinners = MaxWinners; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } pub struct MockFallback; @@ -316,30 +320,19 @@ impl ElectionProviderBase for MockFallback { type BlockNumber = u64; type Error = &'static str; type DataProvider = StakingMock; - - fn ongoing() -> bool { - false - } -} -impl ElectionProvider for MockFallback { - fn elect() -> Result, Self::Error> { - Self::elect_with_bounds(Bounded::max_value(), Bounded::max_value()) - } + type MaxWinners = MaxWinners; } impl InstantElectionProvider for MockFallback { - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, - ) -> Result, 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.") } } } @@ -404,10 +397,12 @@ 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; + type MaxWinners = MaxWinners; type MinerConfig = Self; type Solver = SequentialPhragmen, Balancing>; } @@ -424,6 +419,8 @@ 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)] @@ -438,7 +435,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") } @@ -449,8 +448,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 cda87dd6c839a..9d629ad77fd79 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -462,7 +462,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, @@ -537,7 +537,7 @@ impl Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{mock::*, 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] @@ -557,6 +557,50 @@ mod tests { }) } + #[test] + 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); + + assert_noop!( + MultiPhase::create_snapshot(), + ElectionError::DataProvider("Snapshot too big for submission."), + ); + }) + } + + #[test] + 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); + + 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); + }) + } + #[test] fn should_pay_deposit() { ExtBuilder::default().build_and_execute(|| { diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 5ee65e102bd06..38924a18e2f54 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,90 +367,109 @@ pub trait ElectionProviderBase { /// The error type that is returned by the provider. type Error: Debug; + /// 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. + 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; + /// 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) => + if desired_targets <= Self::MaxWinners::get() { + Ok(desired_targets) + } else { + Err("desired_targets should not be greater than MaxWinners") + }, + Err(e) => Err(e), + } + } } /// 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 [`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 +/// 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>; + fn elect() -> Result, Self::Error>; } -/// 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: ElectionProvider { - /// 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, - ) -> Result, Self::Error>; +/// 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, + 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)> +impl ElectionProviderBase + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where DataProvider: ElectionDataProvider, + MaxWinners: Get, { type AccountId = AccountId; type BlockNumber = BlockNumber; type Error = &'static str; + type MaxWinners = MaxWinners; type DataProvider = DataProvider; +} +impl ElectionProvider + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> +where + DataProvider: ElectionDataProvider, + MaxWinners: Get, +{ fn ongoing() -> bool { false } + + fn elect() -> Result, Self::Error> { + Err("`NoElection` cannot do anything.") + } } -#[cfg(feature = "std")] -impl ElectionProvider - for NoElection<(AccountId, BlockNumber, DataProvider)> +impl InstantElectionProvider + for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)> where DataProvider: ElectionDataProvider, + MaxWinners: Get, { - fn elect() -> Result, Self::Error> { - Err(" cannot do anything.") + fn instant_elect( + _: Option, + _: Option, + ) -> Result, Self::Error> { + Err("`NoElection` cannot do anything.") } } @@ -650,3 +667,9 @@ pub type Voter = (AccountId, VoteWeight, BoundedVec = Voter<::AccountId, ::MaxVotesPerVoter>; + +/// Same as `BoundedSupports` but parameterized by a `ElectionProviderBase`. +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 88aa6ca7267a0..483c402fe249c 100644 --- a/frame/election-provider-support/src/onchain.rs +++ b/frame/election-provider-support/src/onchain.rs @@ -20,11 +20,13 @@ //! careful when using it onchain. use crate::{ - Debug, ElectionDataProvider, ElectionProvider, ElectionProviderBase, InstantElectionProvider, - NposSolver, WeightInfo, + BoundedSupportsOf, Debug, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + InstantElectionProvider, NposSolver, WeightInfo, }; use frame_support::{dispatch::DispatchClass, traits::Get}; -use sp_npos_elections::*; +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. @@ -34,6 +36,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 { @@ -44,65 +49,71 @@ 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. -/// -/// The [`ElectionProvider`] 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`). +/// This implements both `ElectionProvider` and `InstantElectionProvider`. /// -/// 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; -} -pub trait BoundedConfig: Config { - /// Bounds the number of voters. + /// Upper bound on maximum winners from electable targets. + /// + /// As noted in the documentation of [`ElectionProviderBase::MaxWinners`], this value should + /// always be more than `DataProvider::desired_target`. + type MaxWinners: Get; + + /// 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; } -fn elect_with( +/// Same as `BoundedSupportsOf` but for `onchain::Config`. +pub type OnChainBoundedSupportsOf = BoundedSupports< + <::System as frame_system::Config>::AccountId, + ::MaxWinners, +>; + +fn elect_with_input_bounds( 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)?; let desired_targets = T::DataProvider::desired_targets().map_err(Error::DataProvider)?; + if desired_targets > T::MaxWinners::get() { + // early exit + return Err(Error::TooManyWinners) + } + let voters_len = voters.len() as u32; let targets_len = targets.len() as u32; @@ -130,69 +141,43 @@ fn elect_with( DispatchClass::Mandatory, ); - Ok(to_supports(&staked)) -} - -impl ElectionProvider for UnboundedExecution { - 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" - ); - } + // 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)?; - elect_with::(None, None) - } + Ok(supports) } -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, - ) -> Result, Self::Error> { - elect_with::(Some(max_voters), Some(max_targets)) +impl InstantElectionProvider for OnChainExecution { + fn instant_elect( + forced_input_voters_bound: Option, + forced_input_target_bound: Option, + ) -> Result, Self::Error> { + 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 ElectionProvider for BoundedExecution { - 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)), + fn elect() -> Result, Self::Error> { + elect_with_input_bounds::( + Some(T::VotersBound::get() as usize), + Some(T::TargetsBound::get() as usize), ) } } @@ -200,8 +185,8 @@ impl InstantElectionProvider for BoundedExecution { #[cfg(test)] mod tests { use super::*; - use crate::{PhragMMS, SequentialPhragmen}; - use frame_support::traits::ConstU32; + use crate::{ElectionProvider, PhragMMS, SequentialPhragmen}; + use frame_support::{assert_noop, parameter_types, traits::ConstU32}; use sp_npos_elections::Support; use sp_runtime::Perbill; type AccountId = u64; @@ -251,14 +236,17 @@ mod tests { struct PhragmenParams; struct PhragMMSParams; + parameter_types! { + pub static MaxWinners: u32 = 10; + pub static DesiredTargets: u32 = 2; + } + impl Config for PhragmenParams { type System = Runtime; type Solver = SequentialPhragmen; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - } - - impl BoundedConfig for PhragmenParams { + type MaxWinners = MaxWinners; type VotersBound = ConstU32<600>; type TargetsBound = ConstU32<400>; } @@ -268,9 +256,7 @@ mod tests { type Solver = PhragMMS; type DataProvider = mock_data_provider::DataProvider; type WeightInfo = (); - } - - impl BoundedConfig for PhragMMSParams { + type MaxWinners = MaxWinners; type VotersBound = ConstU32<600>; type TargetsBound = ConstU32<400>; } @@ -299,7 +285,7 @@ mod tests { } fn desired_targets() -> data_provider::Result { - Ok(2) + Ok(DesiredTargets::get()) } fn next_election_prediction(_: BlockNumber) -> BlockNumber { @@ -312,7 +298,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)] }) @@ -321,11 +307,25 @@ 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(|| { 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/fast-unstake/src/mock.rs b/frame/fast-unstake/src/mock.rs index bac2d9aa9c8e4..d66f4ba5663d9 100644 --- a/frame/fast-unstake/src/mock.rs +++ b/frame/fast-unstake/src/mock.rs @@ -107,22 +107,23 @@ 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; 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::ElectionProvider for MockElection { - fn elect() -> Result, Self::Error> { + fn elect() -> Result, Self::Error> { Err(()) } } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 573a74d2bfae8..1a97b1345fe5d 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -182,6 +182,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 { @@ -203,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/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index db01989f2b563..6959aa9783ee5 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, ())>; 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 5758b884e348d..568dec7b3a340 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, ())>; type GenesisElectionProvider = Self::ElectionProvider; type VoterList = VoterList; type TargetList = pallet_staking::UseValidatorsMap; diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index c4c1d2aca8d07..e022d81c5b5bd 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 3f0a26afc1358..65bfcad4b26fc 100644 --- a/frame/root-offences/src/mock.rs +++ b/frame/root-offences/src/mock.rs @@ -145,6 +145,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 }>; } pub struct OnStakerSlashMock(core::marker::PhantomData); @@ -188,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..2db7eb385111c 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 a0144463540be..0f5b8e0123ab6 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -333,6 +333,10 @@ 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::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 24e5aa97160ee..16e4e5ddd7aa2 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -238,6 +238,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; @@ -256,6 +257,9 @@ impl onchain::Config for OnChainSeqPhragmen { type Solver = SequentialPhragmen; type DataProvider = Staking; type WeightInfo = (); + type MaxWinners = MaxWinners; + type VotersBound = ConstU32<{ u32::MAX }>; + type TargetsBound = ConstU32<{ u32::MAX }>; } pub struct MockReward {} @@ -295,7 +299,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 a9e9899b9761a..9be01dd823104 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,15 +18,15 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, ElectionDataProvider, ElectionProvider, ElectionProviderBase, ScoreProvider, - SortedListProvider, Supports, VoteWeight, VoterOf, + data_provider, BoundedSupportsOf, ElectionDataProvider, ElectionProvider, ScoreProvider, + SortedListProvider, VoteWeight, VoterOf, }; use frame_support::{ dispatch::WithPostDispatchInfo, pallet_prelude::*, traits::{ Currency, CurrencyToVote, Defensive, DefensiveResult, EstimateNextNewSession, Get, - Imbalance, LockableCurrency, OnUnbalanced, UnixTime, WithdrawReasons, + Imbalance, LockableCurrency, OnUnbalanced, TryCollect, UnixTime, WithdrawReasons, }, weights::Weight, }; @@ -44,7 +44,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, + Forcing, IndividualExposure, MaxWinnersOf, Nominations, PositiveImbalanceOf, RewardDestination, SessionInterface, StakingLedger, ValidatorPrefs, }; @@ -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,8 +429,11 @@ 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 +459,26 @@ 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() + // both bounds checked in integrity test to be equal + .defensive_unwrap_or_default() } else { - T::ElectionProvider::elect().map_err(|e| { + let result = ::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); Self::deposit_event(Event::StakingElectionFailed); - }) - } - .ok()?; + }); + result.ok()? + }; let exposures = Self::collect_exposures(election_result); if (exposures.len() as u32) < Self::minimum_validator_count().max(1) { @@ -502,10 +515,19 @@ 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 { - let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); + ) -> BoundedVec> { + 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(); @@ -543,11 +565,11 @@ impl Pallet { elected_stashes } - /// 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 +598,8 @@ impl Pallet { let exposure = Exposure { own, others, total }; (validator, exposure) }) - .collect::)>>() + .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. @@ -1085,12 +1108,12 @@ 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) + 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); 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); @@ -1500,7 +1523,7 @@ impl StakingInterface for Pallet { } fn election_ongoing() -> bool { - ::ongoing() + T::ElectionProvider::ongoing() } fn force_unstake(who: Self::AccountId) -> sp_runtime::DispatchResult { @@ -1626,6 +1649,12 @@ impl Pallet { Nominators::::count() + Validators::::count(), "wrong external count" ); + + ensure!( + ValidatorCount::::get() <= + ::MaxWinners::get(), + "validator count exceeded election max winners" + ); Ok(()) } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index df90873ab2e59..8fddba2150370 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::{SortedListProvider, VoteWeight}; +use frame_election_provider_support::{ + ElectionProvider, ElectionProviderBase, SortedListProvider, VoteWeight, +}; use frame_support::{ dispatch::Codec, pallet_prelude::*, @@ -32,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::*; @@ -107,7 +109,7 @@ pub mod pallet { type CurrencyToVote: CurrencyToVote>; /// Something that provides the election functionality. - type ElectionProvider: frame_election_provider_support::ElectionProvider< + type ElectionProvider: ElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, // we only accept an election provider that has staking as data provider. @@ -115,7 +117,7 @@ pub mod pallet { >; /// Something that provides the election functionality at genesis. - type GenesisElectionProvider: frame_election_provider_support::ElectionProvider< + type GenesisElectionProvider: ElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber, DataProvider = Pallet, @@ -646,6 +648,10 @@ pub mod pallet { ), _ => Ok(()), }); + assert!( + ValidatorCount::::get() <= + ::MaxWinners::get() + ); } // all voters are reported to the `VoterList`. @@ -743,8 +749,8 @@ 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 - /// 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, @@ -782,6 +788,12 @@ 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!( @@ -1264,11 +1276,18 @@ pub mod pallet { #[pallet::compact] new: u32, ) -> DispatchResult { ensure_root(origin)?; + // ensure new validator count does not exceed maximum winners + // support by election provider. + ensure!( + new <= ::MaxWinners::get(), + Error::::TooManyValidators + ); ValidatorCount::::put(new); Ok(()) } - /// Increments the ideal number of validators. + /// Increments the ideal number of validators upto maximum of + /// `ElectionProviderBase::MaxWinners`. /// /// The dispatch origin must be Root. /// @@ -1281,11 +1300,19 @@ pub mod pallet { #[pallet::compact] additional: u32, ) -> DispatchResult { ensure_root(origin)?; - ValidatorCount::::mutate(|n| *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(()) } - /// 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. /// @@ -1295,7 +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 += 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 4812c105c0d80..6609b9087637d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -5624,3 +5624,57 @@ 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::::TooManyValidators, + ); + }) +} + +#[test] +fn increase_validator_count_errors() { + ExtBuilder::default().build_and_execute(|| { + MaxWinners::set(50); + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 40)); + + // increase works + assert_ok!(Staking::increase_validator_count(RuntimeOrigin::root(), 6)); + assert_eq!(ValidatorCount::::get(), 46); + + // errors + assert_noop!( + Staking::increase_validator_count(RuntimeOrigin::root(), 5), + Error::::TooManyValidators, + ); + }) +} + +#[test] +fn scale_validator_count_errors() { + ExtBuilder::default().build_and_execute(|| { + MaxWinners::set(50); + assert_ok!(Staking::set_validator_count(RuntimeOrigin::root(), 20)); + + // scale value works + assert_ok!(Staking::scale_validator_count( + RuntimeOrigin::root(), + Percent::from_percent(200) + )); + assert_eq!(ValidatorCount::::get(), 40); + + // errors + assert_noop!( + Staking::scale_validator_count(RuntimeOrigin::root(), Percent::from_percent(126)), + Error::::TooManyValidators, + ); + }) +} 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`]. ///