Use proper bounded vector type for nominations (#10601)

* Use proper bounded vector type for nominations

* add docs and tweak chill_other for cleanup purposes

* Fix the build

* remove TODO

* add a bit more doc

* even more docs
gushc

* Update frame/staking/src/pallet/mod.rs

Co-authored-by: Zeke Mostov <[email protected]>

* Update frame/staking/src/pallet/mod.rs

Co-authored-by: Zeke Mostov <[email protected]>

* Fix the nasty bug

* also bound the Snapshot type

* fix doc test

* document bounded_vec

* self-review

* remove unused

* Fix build

* frame-support: repetition overload for bounded_vec

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix

* remove the need to allocate into unbounded voters etc etc

* Don't expect

* unbreal the build again

* handle macro a bit better

Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>

Author: 3 people

bin/node/cli/src/chain_spec.rs

@@ -23,8 +23,8 @@ use hex_literal::hex;
 use node_runtime::{
 	constants::currency::*, wasm_binary_unwrap, AuthorityDiscoveryConfig, BabeConfig,
 	BalancesConfig, Block, CouncilConfig, DemocracyConfig, ElectionsConfig, GrandpaConfig,
-	ImOnlineConfig, IndicesConfig, SessionConfig, SessionKeys, SocietyConfig, StakerStatus,
-	StakingConfig, SudoConfig, SystemConfig, TechnicalCommitteeConfig, MAX_NOMINATIONS,
+	ImOnlineConfig, IndicesConfig, MaxNominations, SessionConfig, SessionKeys, SocietyConfig,
+	StakerStatus, StakingConfig, SudoConfig, SystemConfig, TechnicalCommitteeConfig,
 };
 use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
 use sc_chain_spec::ChainSpecExtension;
@@ -278,7 +278,7 @@ pub fn testnet_genesis(
 		.map(|x| (x.0.clone(), x.1.clone(), STASH, StakerStatus::Validator))
 		.chain(initial_nominators.iter().map(|x| {
 			use rand::{seq::SliceRandom, Rng};
-			let limit = (MAX_NOMINATIONS as usize).min(initial_authorities.len());
+			let limit = (MaxNominations::get() as usize).min(initial_authorities.len());
 			let count = rng.gen::<usize>() % limit;
 			let nominations = initial_authorities
 				.as_slice()

bin/node/runtime/src/lib.rs

@@ -541,7 +541,7 @@ impl pallet_staking::BenchmarkingConfig for StakingBenchmarkingConfig {
 }
 
 impl pallet_staking::Config for Runtime {
-	const MAX_NOMINATIONS: u32 = MAX_NOMINATIONS;
+	type MaxNominations = MaxNominations;
 	type Currency = Balances;
 	type UnixTime = Timestamp;
 	type CurrencyToVote = U128CurrencyToVote;
@@ -605,7 +605,9 @@ sp_npos_elections::generate_solution_type!(
 	>(16)
 );
 
-pub const MAX_NOMINATIONS: u32 = <NposSolution16 as sp_npos_elections::NposSolution>::LIMIT as u32;
+parameter_types! {
+	pub MaxNominations: u32 = <NposSolution16 as sp_npos_elections::NposSolution>::LIMIT as u32;
+}
 
 /// The numbers configured here could always be more than the the maximum limits of staking pallet
 /// to ensure election snapshot will not run out of memory. For now, we set them to smaller values
@@ -1792,7 +1794,7 @@ mod tests {
 	#[test]
 	fn perbill_as_onchain_accuracy() {
 		type OnChainAccuracy = <Runtime as onchain::Config>::Accuracy;
-		let maximum_chain_accuracy: Vec<UpperOf<OnChainAccuracy>> = (0..MAX_NOMINATIONS)
+		let maximum_chain_accuracy: Vec<UpperOf<OnChainAccuracy>> = (0..MaxNominations::get())
 			.map(|_| <UpperOf<OnChainAccuracy>>::from(OnChainAccuracy::one().deconstruct()))
 			.collect();
 		let _: UpperOf<OnChainAccuracy> =

frame/babe/src/mock.rs

@@ -178,7 +178,7 @@ impl onchain::Config for Test {
 }
 
 impl pallet_staking::Config for Test {
-	const MAX_NOMINATIONS: u32 = 16;
+	type MaxNominations = ConstU32<16>;
 	type RewardRemainder = ();
 	type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote;
 	type Event = Event;

frame/election-provider-multi-phase/src/benchmarking.rs

@@ -20,7 +20,11 @@
 use super::*;
 use crate::{unsigned::IndexAssignmentOf, Pallet as MultiPhase};
 use frame_benchmarking::account;
-use frame_support::{assert_ok, traits::Hooks};
+use frame_support::{
+	assert_ok,
+	traits::{Hooks, TryCollect},
+	BoundedVec,
+};
 use frame_system::RawOrigin;
 use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng};
 use sp_arithmetic::{per_things::Percent, traits::One};
@@ -69,11 +73,12 @@ fn solution_with_size<T: Config>(
 	let active_voters = (0..active_voters_count)
 		.map(|i| {
 			// chose a random subset of winners.
-			let winner_votes = winners
+			let winner_votes: BoundedVec<_, _> = winners
 				.as_slice()
 				.choose_multiple(&mut rng, <SolutionOf<T>>::LIMIT)
 				.cloned()
-				.collect::<Vec<_>>();
+				.try_collect()
+				.expect("<SolutionOf<T>>::LIMIT is the correct bound; qed.");
 			let voter = frame_benchmarking::account::<T::AccountId>("Voter", i, SEED);
 			(voter, stake, winner_votes)
 		})
@@ -87,10 +92,11 @@ fn solution_with_size<T: Config>(
 		.collect::<Vec<T::AccountId>>();
 	let rest_voters = (active_voters_count..size.voters)
 		.map(|i| {
-			let votes = (&non_winners)
+			let votes: BoundedVec<_, _> = (&non_winners)
 				.choose_multiple(&mut rng, <SolutionOf<T>>::LIMIT)
 				.cloned()
-				.collect::<Vec<T::AccountId>>();
+				.try_collect()
+				.expect("<SolutionOf<T>>::LIMIT is the correct bound; qed.");
 			let voter = frame_benchmarking::account::<T::AccountId>("Voter", i, SEED);
 			(voter, stake, votes)
 		})
@@ -152,7 +158,7 @@ fn set_up_data_provider<T: Config>(v: u32, t: u32) {
 		info,
 		"setting up with voters = {} [degree = {}], targets = {}",
 		v,
-		T::DataProvider::MAXIMUM_VOTES_PER_VOTER,
+		<T::DataProvider as ElectionDataProvider>::MaxVotesPerVoter::get(),
 		t
 	);
 
@@ -165,14 +171,16 @@ fn set_up_data_provider<T: Config>(v: u32, t: u32) {
 		})
 		.collect::<Vec<_>>();
 	// we should always have enough voters to fill.
-	assert!(targets.len() > T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize);
-	targets.truncate(T::DataProvider::MAXIMUM_VOTES_PER_VOTER as usize);
+	assert!(
+		targets.len() > <T::DataProvider as ElectionDataProvider>::MaxVotesPerVoter::get() as usize
+	);
+	targets.truncate(<T::DataProvider as ElectionDataProvider>::MaxVotesPerVoter::get() as usize);
 
 	// fill voters.
 	(0..v).for_each(|i| {
 		let voter = frame_benchmarking::account::<T::AccountId>("Voter", i, SEED);
 		let weight = T::Currency::minimum_balance().saturated_into::<u64>() * 1000;
-		T::DataProvider::add_voter(voter, weight, targets.clone());
+		T::DataProvider::add_voter(voter, weight, targets.clone().try_into().unwrap());
 	});
 }
 

frame/election-provider-multi-phase/src/helpers.rs

@@ -17,7 +17,7 @@
 
 //! Some helper functions/macros for this crate.
 
-use super::{Config, SolutionTargetIndexOf, SolutionVoterIndexOf, VoteWeight};
+use crate::{unsigned::VoterOf, Config, SolutionTargetIndexOf, SolutionVoterIndexOf, VoteWeight};
 use sp_std::{collections::btree_map::BTreeMap, prelude::*};
 
 #[macro_export]
@@ -34,7 +34,7 @@ macro_rules! log {
 ///
 /// This can be used to efficiently build index getter closures.
 pub fn generate_voter_cache<T: Config>(
-	snapshot: &Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>,
+	snapshot: &Vec<VoterOf<T>>,
 ) -> BTreeMap<T::AccountId, usize> {
 	let mut cache: BTreeMap<T::AccountId, usize> = BTreeMap::new();
 	snapshot.iter().enumerate().for_each(|(i, (x, _, _))| {
@@ -97,7 +97,7 @@ pub fn voter_index_fn_usize<T: Config>(
 /// Not meant to be used in production.
 #[cfg(test)]
 pub fn voter_index_fn_linear<T: Config>(
-	snapshot: &Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>,
+	snapshot: &Vec<VoterOf<T>>,
 ) -> impl Fn(&T::AccountId) -> Option<SolutionVoterIndexOf<T>> + '_ {
 	move |who| {
 		snapshot
@@ -148,7 +148,7 @@ pub fn target_index_fn_linear<T: Config>(
 /// Create a function that can map a voter index ([`SolutionVoterIndexOf`]) to the actual voter
 /// account using a linearly indexible snapshot.
 pub fn voter_at_fn<T: Config>(
-	snapshot: &Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>,
+	snapshot: &Vec<VoterOf<T>>,
 ) -> impl Fn(SolutionVoterIndexOf<T>) -> Option<T::AccountId> + '_ {
 	move |i| {
 		<SolutionVoterIndexOf<T> as TryInto<usize>>::try_into(i)
@@ -174,7 +174,7 @@ pub fn target_at_fn<T: Config>(
 /// This is not optimized and uses a linear search.
 #[cfg(test)]
 pub fn stake_of_fn_linear<T: Config>(
-	snapshot: &Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>,
+	snapshot: &Vec<VoterOf<T>>,
 ) -> impl Fn(&T::AccountId) -> VoteWeight + '_ {
 	move |who| {
 		snapshot
@@ -192,7 +192,7 @@ pub fn stake_of_fn_linear<T: Config>(
 /// The cache need must be derived from the same snapshot. Zero is returned if a voter is
 /// non-existent.
 pub fn stake_of_fn<'a, T: Config>(
-	snapshot: &'a Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>,
+	snapshot: &'a Vec<VoterOf<T>>,
 	cache: &'a BTreeMap<T::AccountId, usize>,
 ) -> impl Fn(&T::AccountId) -> VoteWeight + 'a {
 	move |who| {

frame/election-provider-multi-phase/src/lib.rs

@@ -269,6 +269,7 @@ const LOG_TARGET: &'static str = "runtime::election-provider";
 pub mod signed;
 pub mod unsigned;
 pub mod weights;
+use unsigned::VoterOf;
 pub use weights::WeightInfo;
 
 pub use signed::{
@@ -448,11 +449,13 @@ pub struct ReadySolution<A> {
 ///
 /// These are stored together because they are often accessed together.
 #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)]
-pub struct RoundSnapshot<A> {
+#[codec(mel_bound(T: Config))]
+#[scale_info(skip_type_params(T))]
+pub struct RoundSnapshot<T: Config> {
 	/// All of the voters.
-	pub voters: Vec<(A, VoteWeight, Vec<A>)>,
+	pub voters: Vec<VoterOf<T>>,
 	/// All of the targets.
-	pub targets: Vec<A>,
+	pub targets: Vec<T::AccountId>,
 }
 
 /// Encodes the length of a solution or a snapshot.
@@ -820,7 +823,7 @@ pub mod pallet {
 			// NOTE that this pallet does not really need to enforce this in runtime. The
 			// solution cannot represent any voters more than `LIMIT` anyhow.
 			assert_eq!(
-				<T::DataProvider as ElectionDataProvider>::MAXIMUM_VOTES_PER_VOTER,
+				<T::DataProvider as ElectionDataProvider>::MaxVotesPerVoter::get(),
 				<SolutionOf<T> as NposSolution>::LIMIT as u32,
 			);
 		}
@@ -1140,7 +1143,7 @@ pub mod pallet {
 	/// This is created at the beginning of the signed phase and cleared upon calling `elect`.
 	#[pallet::storage]
 	#[pallet::getter(fn snapshot)]
-	pub type Snapshot<T: Config> = StorageValue<_, RoundSnapshot<T::AccountId>>;
+	pub type Snapshot<T: Config> = StorageValue<_, RoundSnapshot<T>>;
 
 	/// Desired number of targets to elect for this round.
 	///
@@ -1257,7 +1260,7 @@ impl<T: Config> Pallet<T> {
 	/// Extracted for easier weight calculation.
 	fn create_snapshot_internal(
 		targets: Vec<T::AccountId>,
-		voters: Vec<crate::unsigned::Voter<T>>,
+		voters: Vec<VoterOf<T>>,
 		desired_targets: u32,
 	) {
 		let metadata =
@@ -1270,7 +1273,7 @@ impl<T: Config> Pallet<T> {
 		// instead of using storage APIs, we do a manual encoding into a fixed-size buffer.
 		// `encoded_size` encodes it without storing it anywhere, this should not cause any
 		// allocation.
-		let snapshot = RoundSnapshot { voters, targets };
+		let snapshot = RoundSnapshot::<T> { voters, targets };
 		let size = snapshot.encoded_size();
 		log!(debug, "snapshot pre-calculated size {:?}", size);
 		let mut buffer = Vec::with_capacity(size);
@@ -1288,7 +1291,7 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Extracted for easier weight calculation.
 	fn create_snapshot_external(
-	) -> Result<(Vec<T::AccountId>, Vec<crate::unsigned::Voter<T>>, u32), ElectionError<T>> {
+	) -> Result<(Vec<T::AccountId>, Vec<VoterOf<T>>, u32), ElectionError<T>> {
 		let target_limit = <SolutionTargetIndexOf<T>>::max_value().saturated_into::<usize>();
 		// for now we have just a single block snapshot.
 		let voter_limit = T::VoterSnapshotPerBlock::get().saturated_into::<usize>();