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 <kian@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/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<T: Config> ElectionProvider for GenericElectionProvider<T> {
-//!         fn elect() -> Result<Supports<AccountId>, Self::Error> {
+//!         fn ongoing() -> bool { false }
+//!         fn elect() -> Result<BoundedSupportsOf<Self>, 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<u32>;
+
 	/// 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<u32> {
+		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<Support>), MaxWinners>*.   
-pub trait BoundedElectionProvider: ElectionProviderBase {
-	/// The upper bound on election winners.
-	type MaxWinners: Get<u32>;
+/// 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<BoundedSupports<Self::AccountId, Self::MaxWinners>, Self::Error>;
+	fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error>;
 }
 
-/// Same a [`BoundedElectionProvider`], but no bounds are imposed on the number
-/// of winners.
+/// A (almost) marker trait that signifies an election provider as working synchronously. i.e. being
+/// *instant*.
 ///
-/// The result is returned in a target major format, namely as
-///*Vec<(AccountId, Vec<Support>)>*.
-pub trait ElectionProvider: ElectionProviderBase {
-	/// Performs the election. This should be implemented as a self-weighing
-	/// function, similar to [`BoundedElectionProvider::elect()`].
-	fn elect() -> Result<Supports<Self::AccountId>, 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<u32>,
+		forced_input_target_bound: Option<u32>,
+	) -> Result<BoundedSupportsOf<Self>, 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.
-///
-/// 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<Supports<Self::AccountId>, Self::Error>;
-}
-
-/// An election provider to be used only for testing.
-#[cfg(feature = "std")]
+/// An election provider that does nothing whatsoever.
 pub struct NoElection<X>(sp_std::marker::PhantomData<X>);
 
-#[cfg(feature = "std")]
-impl<AccountId, BlockNumber, DataProvider> ElectionProviderBase
-	for NoElection<(AccountId, BlockNumber, DataProvider)>
+impl<AccountId, BlockNumber, DataProvider, MaxWinners> ElectionProviderBase
+	for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)>
 where
 	DataProvider: ElectionDataProvider<AccountId = AccountId, BlockNumber = BlockNumber>,
+	MaxWinners: Get<u32>,
 {
 	type AccountId = AccountId;
 	type BlockNumber = BlockNumber;
 	type Error = &'static str;
+	type MaxWinners = MaxWinners;
 	type DataProvider = DataProvider;
+}
 
+impl<AccountId, BlockNumber, DataProvider, MaxWinners> ElectionProvider
+	for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)>
+where
+	DataProvider: ElectionDataProvider<AccountId = AccountId, BlockNumber = BlockNumber>,
+	MaxWinners: Get<u32>,
+{
 	fn ongoing() -> bool {
 		false
 	}
+
+	fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error> {
+		Err("`NoElection` cannot do anything.")
+	}
 }
 
-#[cfg(feature = "std")]
-impl<AccountId, BlockNumber, DataProvider> ElectionProvider
-	for NoElection<(AccountId, BlockNumber, DataProvider)>
+impl<AccountId, BlockNumber, DataProvider, MaxWinners> InstantElectionProvider
+	for NoElection<(AccountId, BlockNumber, DataProvider, MaxWinners)>
 where
 	DataProvider: ElectionDataProvider<AccountId = AccountId, BlockNumber = BlockNumber>,
+	MaxWinners: Get<u32>,
 {
-	fn elect() -> Result<Supports<AccountId>, Self::Error> {
-		Err("<NoElection as ElectionProvider> cannot do anything.")
+	fn instant_elect(
+		_: Option<u32>,
+		_: Option<u32>,
+	) -> Result<BoundedSupportsOf<Self>, Self::Error> {
+		Err("`NoElection` cannot do anything.")
 	}
 }
 
@@ -650,3 +667,9 @@ pub type Voter<AccountId, Bound> = (AccountId, VoteWeight, BoundedVec<AccountId,
 /// Same as [`Voter`], but parameterized by an [`ElectionDataProvider`].
 pub type VoterOf<D> =
 	Voter<<D as ElectionDataProvider>::AccountId, <D as ElectionDataProvider>::MaxVotesPerVoter>;
+
+/// Same as `BoundedSupports` but parameterized by a `ElectionProviderBase`.
+pub type BoundedSupportsOf<E> = BoundedSupports<
+	<E as ElectionProviderBase>::AccountId,
+	<E as ElectionProviderBase>::MaxWinners,
+>;

substrate/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<sp_npos_elections::Error> for Error {
@@ -44,65 +49,71 @@ impl From<sp_npos_elections::Error> 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 [`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`).
-///
-/// 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<T: BoundedConfig>(PhantomData<T>);
+/// 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<T: Config>(PhantomData<T>);
 
-/// An unbounded variant of [`BoundedExecution`].
-///
-/// ### Warning
-///
-/// This can be very expensive to run frequently on-chain. Use with care.
-pub struct UnboundedExecution<T: Config>(PhantomData<T>);
+#[deprecated(note = "use OnChainExecution, which is bounded by default")]
+pub type BoundedExecution<T> = OnChainExecution<T>;
 
 /// 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 = <Self::System as frame_system::Config>::AccountId,
 		Error = sp_npos_elections::Error,
 	>;
+
 	/// Something that provides the data for election.
 	type DataProvider: ElectionDataProvider<
 		AccountId = <Self::System as frame_system::Config>::AccountId,
 		BlockNumber = <Self::System as frame_system::Config>::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<u32>;
+
+	/// Bounds the number of voters, when calling into [`Config::DataProvider`]. It might be
+	/// overwritten in the `InstantElectionProvider` impl.
 	type VotersBound: Get<u32>;
-	/// 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<u32>;
 }
 
-fn elect_with<T: Config>(
+/// Same as `BoundedSupportsOf` but for `onchain::Config`.
+pub type OnChainBoundedSupportsOf<E> = BoundedSupports<
+	<<E as Config>::System as frame_system::Config>::AccountId,
+	<E as Config>::MaxWinners,
+>;
+
+fn elect_with_input_bounds<T: Config>(
 	maybe_max_voters: Option<usize>,
 	maybe_max_targets: Option<usize>,
-) -> Result<Supports<<T::System as frame_system::Config>::AccountId>, Error> {
+) -> Result<OnChainBoundedSupportsOf<T>, 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<T: Config>(
 		DispatchClass::Mandatory,
 	);
 
-	Ok(to_supports(&staked))
+	// 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<T> =
+		to_supports(&staked).try_into().map_err(|_| Error::TooManyWinners)?;
+
+	Ok(supports)
 }
 
-impl<T: Config> ElectionProvider for UnboundedExecution<T> {
-	fn elect() -> Result<Supports<Self::AccountId>, 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::<T>(None, None)
-	}
-}
-
-impl<T: Config> ElectionProviderBase for UnboundedExecution<T> {
+impl<T: Config> ElectionProviderBase for OnChainExecution<T> {
 	type AccountId = <T::System as frame_system::Config>::AccountId;
 	type BlockNumber = <T::System as frame_system::Config>::BlockNumber;
 	type Error = Error;
+	type MaxWinners = T::MaxWinners;
 	type DataProvider = T::DataProvider;
+}
 
+impl<T: Config> InstantElectionProvider for OnChainExecution<T> {
+	fn instant_elect(
+		forced_input_voters_bound: Option<u32>,
+		forced_input_target_bound: Option<u32>,
+	) -> Result<BoundedSupportsOf<Self>, Self::Error> {
+		elect_with_input_bounds::<T>(
+			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<T: Config> ElectionProvider for OnChainExecution<T> {
 	fn ongoing() -> bool {
 		false
 	}
-}
 
-impl<T: Config> InstantElectionProvider for UnboundedExecution<T> {
-	fn elect_with_bounds(
-		max_voters: usize,
-		max_targets: usize,
-	) -> Result<Supports<Self::AccountId>, Self::Error> {
-		elect_with::<T>(Some(max_voters), Some(max_targets))
-	}
-}
-
-impl<T: BoundedConfig> ElectionProviderBase for BoundedExecution<T> {
-	type AccountId = <T::System as frame_system::Config>::AccountId;
-	type BlockNumber = <T::System as frame_system::Config>::BlockNumber;
-	type Error = Error;
-	type DataProvider = T::DataProvider;
-
-	fn ongoing() -> bool {
-		false
-	}
-}
-
-impl<T: BoundedConfig> ElectionProvider for BoundedExecution<T> {
-	fn elect() -> Result<Supports<Self::AccountId>, Self::Error> {
-		elect_with::<T>(Some(T::VotersBound::get() as usize), Some(T::TargetsBound::get() as usize))
-	}
-}
-
-impl<T: BoundedConfig> InstantElectionProvider for BoundedExecution<T> {
-	fn elect_with_bounds(
-		max_voters: usize,
-		max_targets: usize,
-	) -> Result<Supports<Self::AccountId>, Self::Error> {
-		elect_with::<T>(
-			Some(max_voters.min(T::VotersBound::get() as usize)),
-			Some(max_targets.min(T::TargetsBound::get() as usize)),
+	fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error> {
+		elect_with_input_bounds::<T>(
+			Some(T::VotersBound::get() as usize),
+			Some(T::TargetsBound::get() as usize),
 		)
 	}
 }
@@ -200,8 +185,8 @@ impl<T: BoundedConfig> InstantElectionProvider for BoundedExecution<T> {
 #[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<AccountId, Perbill>;
 		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<AccountId, Perbill>;
 		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<u32> {
-				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::<PhragmenParams>::elect().unwrap(),
+				<OnChainExecution::<PhragmenParams> 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!(
+				<OnChainExecution::<PhragmenParams> as ElectionProvider>::elect(),
+				Error::TooManyWinners,
+			);
+		})
+	}
+
 	#[test]
 	fn onchain_phragmms_works() {
 		sp_io::TestExternalities::new_empty().execute_with(|| {
 			assert_eq!(
-				BoundedExecution::<PhragMMSParams>::elect().unwrap(),
+				<OnChainExecution::<PhragMMSParams> 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)] })