Revert "Configurable voting-degree in council elections pallet (#13305)"

This reverts commit 8e9f249.

Author: Ross Bulat

bin/node/runtime/src/lib.rs

@@ -1003,9 +1003,8 @@ parameter_types! {
 	pub const TermDuration: BlockNumber = 7 * DAYS;
 	pub const DesiredMembers: u32 = 13;
 	pub const DesiredRunnersUp: u32 = 7;
-	pub const MaxVotesPerVoter: u32 = 16;
-	pub const MaxVoters: u32 = 512;
-	pub const MaxCandidates: u32 = 64;
+	pub const MaxVoters: u32 = 10 * 1000;
+	pub const MaxCandidates: u32 = 1000;
 	pub const ElectionsPhragmenPalletId: LockIdentifier = *b"phrelect";
 }
 
@@ -1030,7 +1029,6 @@ impl pallet_elections_phragmen::Config for Runtime {
 	type DesiredRunnersUp = DesiredRunnersUp;
 	type TermDuration = TermDuration;
 	type MaxVoters = MaxVoters;
-	type MaxVotesPerVoter = MaxVotesPerVoter;
 	type MaxCandidates = MaxCandidates;
 	type WeightInfo = pallet_elections_phragmen::weights::SubstrateWeight<Runtime>;
 }

frame/elections-phragmen/src/benchmarking.rs

@@ -148,7 +148,7 @@ fn clean<T: Config>() {
 benchmarks! {
 	// -- Signed ones
 	vote_equal {
-		let v in 1 .. T::MaxVotesPerVoter::get();
+		let v in 1 .. (MAXIMUM_VOTE as u32);
 		clean::<T>();
 
 		// create a bunch of candidates.
@@ -168,7 +168,7 @@ benchmarks! {
 	}: vote(RawOrigin::Signed(caller), votes, stake)
 
 	vote_more {
-		let v in 2 .. T::MaxVotesPerVoter::get();
+		let v in 2 .. (MAXIMUM_VOTE as u32);
 		clean::<T>();
 
 		// create a bunch of candidates.
@@ -190,7 +190,7 @@ benchmarks! {
 	}: vote(RawOrigin::Signed(caller), votes, stake / <BalanceOf<T>>::from(10u32))
 
 	vote_less {
-		let v in 2 .. T::MaxVotesPerVoter::get();
+		let v in 2 .. (MAXIMUM_VOTE as u32);
 		clean::<T>();
 
 		// create a bunch of candidates.
@@ -212,7 +212,7 @@ benchmarks! {
 
 	remove_voter {
 		// we fix the number of voted candidates to max
-		let v = T::MaxVotesPerVoter::get();
+		let v = MAXIMUM_VOTE as u32;
 		clean::<T>();
 
 		// create a bunch of candidates.
@@ -368,7 +368,7 @@ benchmarks! {
 		clean::<T>();
 
 		let all_candidates = submit_candidates::<T>(T::MaxCandidates::get(), "candidates")?;
-		distribute_voters::<T>(all_candidates, v, T::MaxVotesPerVoter::get() as usize)?;
+		distribute_voters::<T>(all_candidates, v, MAXIMUM_VOTE)?;
 
 		// all candidates leave.
 		<Candidates<T>>::kill();
@@ -389,17 +389,17 @@ benchmarks! {
 		// that we give all candidates a self vote to make sure they are all considered.
 		let c in 1 .. T::MaxCandidates::get();
 		let v in 1 .. T::MaxVoters::get();
-		let e in (T::MaxVoters::get()) .. T::MaxVoters::get() * T::MaxVotesPerVoter::get();
+		let e in (T::MaxVoters::get()) .. T::MaxVoters::get() as u32 * MAXIMUM_VOTE as u32;
 		clean::<T>();
 
 		// so we have a situation with v and e. we want e to basically always be in the range of `e
-		// -> e * T::MaxVotesPerVoter::get()`, but we cannot express that now with the benchmarks.
-		// So what we do is: when c is being iterated, v, and e are max and fine. when v is being
-		// iterated, e is being set to max and this is a problem. In these cases, we cap e to a
-		// lower value, namely v * T::MaxVotesPerVoter::get(). when e is being iterated, v is at
-		// max, and again fine. all in all, votes_per_voter can never be more than
-		// T::MaxVotesPerVoter::get(). Note that this might cause `v` to be an overestimate.
-		let votes_per_voter = (e / v).min(T::MaxVotesPerVoter::get());
+		// -> e * MAXIMUM_VOTE`, but we cannot express that now with the benchmarks. So what we do
+		// is: when c is being iterated, v, and e are max and fine. when v is being iterated, e is
+		// being set to max and this is a problem. In these cases, we cap e to a lower value, namely
+		// v * MAXIMUM_VOTE. when e is being iterated, v is at max, and again fine. all in all,
+		// votes_per_voter can never be more than MAXIMUM_VOTE. Note that this might cause `v` to be
+		// an overestimate.
+		let votes_per_voter = (e / v).min(MAXIMUM_VOTE as u32);
 
 		let all_candidates = submit_candidates_with_self_vote::<T>(c, "candidates")?;
 		let _ = distribute_voters::<T>(all_candidates, v.saturating_sub(c), votes_per_voter as usize)?;
@@ -421,6 +421,68 @@ benchmarks! {
 		}
 	}
 
+	#[extra]
+	election_phragmen_c_e {
+		let c in 1 .. T::MaxCandidates::get();
+		let e in (T::MaxVoters::get()) .. T::MaxVoters::get() * MAXIMUM_VOTE as u32;
+		let fixed_v = T::MaxVoters::get();
+		clean::<T>();
+
+		let votes_per_voter = e / fixed_v;
+
+		let all_candidates = submit_candidates_with_self_vote::<T>(c, "candidates")?;
+		let _ = distribute_voters::<T>(
+			all_candidates,
+			fixed_v - c,
+			votes_per_voter as usize,
+		)?;
+	}: {
+		<Elections<T>>::on_initialize(T::TermDuration::get());
+	}
+	verify {
+		assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get().min(c));
+		assert_eq!(
+			<Elections<T>>::runners_up().len() as u32,
+			T::DesiredRunnersUp::get().min(c.saturating_sub(T::DesiredMembers::get())),
+		);
+
+		#[cfg(test)]
+		{
+			// reset members in between benchmark tests.
+			use crate::tests::MEMBERS;
+			MEMBERS.with(|m| *m.borrow_mut() = vec![]);
+		}
+	}
+
+	#[extra]
+	election_phragmen_v {
+		let v in 4 .. 16;
+		let fixed_c = T::MaxCandidates::get() / 10;
+		let fixed_e = 64;
+		clean::<T>();
+
+		let votes_per_voter = fixed_e / v;
+
+		let all_candidates = submit_candidates_with_self_vote::<T>(fixed_c, "candidates")?;
+		let _ = distribute_voters::<T>(all_candidates, v, votes_per_voter as usize)?;
+	}: {
+		<Elections<T>>::on_initialize(T::TermDuration::get());
+	}
+	verify {
+		assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get().min(fixed_c));
+		assert_eq!(
+			<Elections<T>>::runners_up().len() as u32,
+			T::DesiredRunnersUp::get().min(fixed_c.saturating_sub(T::DesiredMembers::get())),
+		);
+
+		#[cfg(test)]
+		{
+			// reset members in between benchmark tests.
+			use crate::tests::MEMBERS;
+			MEMBERS.with(|m| *m.borrow_mut() = vec![]);
+		}
+	}
+
 	impl_benchmark_test_suite!(
 		Elections,
 		crate::tests::ExtBuilder::default().desired_members(13).desired_runners_up(7),

frame/elections-phragmen/src/lib.rs

@@ -43,14 +43,14 @@
 //! ### Voting
 //!
 //! Voters can vote for a limited number of the candidates by providing a list of account ids,
-//! bounded by [`Config::MaxVotesPerVoter`]. Invalid votes (voting for non-candidates) and duplicate
-//! votes are ignored during election. Yet, a voter _might_ vote for a future candidate. Voters
-//! reserve a bond as they vote. Each vote defines a `value`. This amount is locked from the account
-//! of the voter and indicates the weight of the vote. Voters can update their votes at any time by
-//! calling `vote()` again. This can update the vote targets (which might update the deposit) or
-//! update the vote's stake ([`Voter::stake`]). After a round, votes are kept and might still be
-//! valid for further rounds. A voter is responsible for calling `remove_voter` once they are done
-//! to have their bond back and remove the lock.
+//! bounded by [`MAXIMUM_VOTE`]. Invalid votes (voting for non-candidates) and duplicate votes are
+//! ignored during election. Yet, a voter _might_ vote for a future candidate. Voters reserve a bond
+//! as they vote. Each vote defines a `value`. This amount is locked from the account of the voter
+//! and indicates the weight of the vote. Voters can update their votes at any time by calling
+//! `vote()` again. This can update the vote targets (which might update the deposit) or update the
+//! vote's stake ([`Voter::stake`]). After a round, votes are kept and might still be valid for
+//! further rounds. A voter is responsible for calling `remove_voter` once they are done to have
+//! their bond back and remove the lock.
 //!
 //! See [`Call::vote`], [`Call::remove_voter`].
 //!
@@ -124,6 +124,9 @@ pub mod migrations;
 
 const LOG_TARGET: &str = "runtime::elections-phragmen";
 
+/// The maximum votes allowed per voter.
+pub const MAXIMUM_VOTE: usize = 16;
+
 type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
 type NegativeImbalanceOf<T> = <<T as Config>::Currency as Currency<
@@ -251,29 +254,19 @@ pub mod pallet {
 
 		/// The maximum number of candidates in a phragmen election.
 		///
-		/// Warning: This impacts the size of the election which is run onchain. Chose wisely, and
-		/// consider how it will impact `T::WeightInfo::election_phragmen`.
-		///
-		/// When this limit is reached no more candidates are accepted in the election.
+		/// Warning: The election happens onchain, and this value will determine
+		/// the size of the election. When this limit is reached no more
+		/// candidates are accepted in the election.
 		#[pallet::constant]
 		type MaxCandidates: Get<u32>;
 
 		/// The maximum number of voters to allow in a phragmen election.
 		///
-		/// Warning: This impacts the size of the election which is run onchain. Chose wisely, and
-		/// consider how it will impact `T::WeightInfo::election_phragmen`.
-		///
+		/// Warning: This impacts the size of the election which is run onchain.
 		/// When the limit is reached the new voters are ignored.
 		#[pallet::constant]
 		type MaxVoters: Get<u32>;
 
-		/// Maximum numbers of votes per voter.
-		///
-		/// Warning: This impacts the size of the election which is run onchain. Chose wisely, and
-		/// consider how it will impact `T::WeightInfo::election_phragmen`.
-		#[pallet::constant]
-		type MaxVotesPerVoter: Get<u32>;
-
 		/// Weight information for extrinsics in this pallet.
 		type WeightInfo: WeightInfo;
 	}
@@ -291,41 +284,6 @@ pub mod pallet {
 				Weight::zero()
 			}
 		}
-
-		fn integrity_test() {
-			let block_weight = T::BlockWeights::get().max_block;
-			// mind the order.
-			let election_weight = T::WeightInfo::election_phragmen(
-				T::MaxCandidates::get(),
-				T::MaxVoters::get(),
-				T::MaxVotesPerVoter::get() * T::MaxVoters::get(),
-			);
-
-			let to_seconds = |w: &Weight| {
-				w.ref_time() as f32 /
-					frame_support::weights::constants::WEIGHT_REF_TIME_PER_SECOND as f32
-			};
-
-			frame_support::log::debug!(
-				target: LOG_TARGET,
-				"election weight {}s ({:?}) // chain's block weight {}s ({:?})",
-				to_seconds(&election_weight),
-				election_weight,
-				to_seconds(&block_weight),
-				block_weight,
-			);
-			assert!(
-				election_weight.all_lt(block_weight),
-				"election weight {}s ({:?}) will exceed a {}s chain's block weight ({:?}) (MaxCandidates {}, MaxVoters {}, MaxVotesPerVoter {} -- tweak these parameters)",
-				election_weight,
-				to_seconds(&election_weight),
-				to_seconds(&block_weight),
-				block_weight,
-				T::MaxCandidates::get(),
-				T::MaxVoters::get(),
-				T::MaxVotesPerVoter::get(),
-			);
-		}
 	}
 
 	#[pallet::call]
@@ -349,6 +307,10 @@ pub mod pallet {
 		///
 		/// It is the responsibility of the caller to **NOT** place all of their balance into the
 		/// lock and keep some for further operations.
+		///
+		/// # <weight>
+		/// We assume the maximum weight among all 3 cases: vote_equal, vote_more and vote_less.
+		/// # </weight>
 		#[pallet::call_index(0)]
 		#[pallet::weight(
 			T::WeightInfo::vote_more(votes.len() as u32)
@@ -362,10 +324,8 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 
-			ensure!(
-				votes.len() <= T::MaxVotesPerVoter::get() as usize,
-				Error::<T>::MaximumVotesExceeded
-			);
+			// votes should not be empty and more than `MAXIMUM_VOTE` in any case.
+			ensure!(votes.len() <= MAXIMUM_VOTE, Error::<T>::MaximumVotesExceeded);
 			ensure!(!votes.is_empty(), Error::<T>::NoVotes);
 
 			let candidates_count = <Candidates<T>>::decode_len().unwrap_or(0);
@@ -1046,15 +1006,15 @@ impl<T: Config> Pallet<T> {
 					// count](https://en.wikipedia.org/wiki/Borda_count). We weigh everyone's vote for
 					// that new member by a multiplier based on the order of the votes. i.e. the
 					// first person a voter votes for gets a 16x multiplier, the next person gets a
-					// 15x multiplier, an so on... (assuming `T::MaxVotesPerVoter` = 16)
+					// 15x multiplier, an so on... (assuming `MAXIMUM_VOTE` = 16)
 					let mut prime_votes = new_members_sorted_by_id
 						.iter()
 						.map(|c| (&c.0, BalanceOf::<T>::zero()))
 						.collect::<Vec<_>>();
 					for (_, stake, votes) in voters_and_stakes.into_iter() {
 						for (vote_multiplier, who) in
 							votes.iter().enumerate().map(|(vote_position, who)| {
-								((T::MaxVotesPerVoter::get() as usize - vote_position) as u32, who)
+								((MAXIMUM_VOTE - vote_position) as u32, who)
 							}) {
 							if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) {
 								prime_votes[i].1 = prime_votes[i]
@@ -1213,9 +1173,16 @@ mod tests {
 	};
 	use substrate_test_utils::assert_eq_uvec;
 
+	parameter_types! {
+		pub BlockWeights: frame_system::limits::BlockWeights =
+			frame_system::limits::BlockWeights::simple_max(
+				frame_support::weights::Weight::from_ref_time(1024).set_proof_size(u64::MAX),
+			);
+	}
+
 	impl frame_system::Config for Test {
 		type BaseCallFilter = frame_support::traits::Everything;
-		type BlockWeights = ();
+		type BlockWeights = BlockWeights;
 		type BlockLength = ();
 		type DbWeight = ();
 		type RuntimeOrigin = RuntimeOrigin;
@@ -1330,7 +1297,6 @@ mod tests {
 		type KickedMember = ();
 		type WeightInfo = ();
 		type MaxVoters = PhragmenMaxVoters;
-		type MaxVotesPerVoter = ConstU32<16>;
 		type MaxCandidates = PhragmenMaxCandidates;
 	}