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

* configurable council elections pallet

* configurable council elections pallet

* add warning

* reduce sizes

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_elections_phragmen

* fix stuff

* make assert

* fix docs

* fix docs again

* fix docs again

* Update frame/elections-phragmen/src/lib.rs

Co-authored-by: Gonçalo Pestana <[email protected]>

* Update frame/elections-phragmen/src/lib.rs

Co-authored-by: Gonçalo Pestana <[email protected]>

* Update frame/elections-phragmen/src/lib.rs

Co-authored-by: Gonçalo Pestana <[email protected]>

* fix docs

---------

Co-authored-by: command-bot <>
Co-authored-by: Gonçalo Pestana <[email protected]>

Author: 2 people

bin/node/runtime/src/lib.rs

@@ -1003,8 +1003,9 @@ parameter_types! {
 	pub const TermDuration: BlockNumber = 7 * DAYS;
 	pub const DesiredMembers: u32 = 13;
 	pub const DesiredRunnersUp: u32 = 7;
-	pub const MaxVoters: u32 = 10 * 1000;
-	pub const MaxCandidates: u32 = 1000;
+	pub const MaxVotesPerVoter: u32 = 16;
+	pub const MaxVoters: u32 = 512;
+	pub const MaxCandidates: u32 = 64;
 	pub const ElectionsPhragmenPalletId: LockIdentifier = *b"phrelect";
 }
 
@@ -1029,6 +1030,7 @@ 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 .. (MAXIMUM_VOTE as u32);
+		let v in 1 .. T::MaxVotesPerVoter::get();
 		clean::<T>();
 
 		// create a bunch of candidates.
@@ -168,7 +168,7 @@ benchmarks! {
 	}: vote(RawOrigin::Signed(caller), votes, stake)
 
 	vote_more {
-		let v in 2 .. (MAXIMUM_VOTE as u32);
+		let v in 2 .. T::MaxVotesPerVoter::get();
 		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 .. (MAXIMUM_VOTE as u32);
+		let v in 2 .. T::MaxVotesPerVoter::get();
 		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 = MAXIMUM_VOTE as u32;
+		let v = T::MaxVotesPerVoter::get();
 		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, MAXIMUM_VOTE)?;
+		distribute_voters::<T>(all_candidates, v, T::MaxVotesPerVoter::get() as usize)?;
 
 		// 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() as u32 * MAXIMUM_VOTE as u32;
+		let e in (T::MaxVoters::get()) .. T::MaxVoters::get() * T::MaxVotesPerVoter::get();
 		clean::<T>();
 
 		// so we have a situation with v and e. we want e to basically always be in the range of `e
-		// -> 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);
+		// -> 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());
 
 		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,68 +421,6 @@ 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 [`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.
+//! 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.
 //!
 //! See [`Call::vote`], [`Call::remove_voter`].
 //!
@@ -124,9 +124,6 @@ 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<
@@ -254,19 +251,29 @@ pub mod pallet {
 
 		/// The maximum number of candidates in a phragmen 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.
+		/// 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.
 		#[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.
+		/// 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 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;
 	}
@@ -284,6 +291,41 @@ 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]
@@ -307,10 +349,6 @@ 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)
@@ -324,8 +362,10 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let who = ensure_signed(origin)?;
 
-			// votes should not be empty and more than `MAXIMUM_VOTE` in any case.
-			ensure!(votes.len() <= MAXIMUM_VOTE, Error::<T>::MaximumVotesExceeded);
+			ensure!(
+				votes.len() <= T::MaxVotesPerVoter::get() as usize,
+				Error::<T>::MaximumVotesExceeded
+			);
 			ensure!(!votes.is_empty(), Error::<T>::NoVotes);
 
 			let candidates_count = <Candidates<T>>::decode_len().unwrap_or(0);
@@ -1006,15 +1046,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 `MAXIMUM_VOTE` = 16)
+					// 15x multiplier, an so on... (assuming `T::MaxVotesPerVoter` = 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)| {
-								((MAXIMUM_VOTE - vote_position) as u32, who)
+								((T::MaxVotesPerVoter::get() as usize - 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]
@@ -1173,16 +1213,9 @@ 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 = BlockWeights;
+		type BlockWeights = ();
 		type BlockLength = ();
 		type DbWeight = ();
 		type RuntimeOrigin = RuntimeOrigin;
@@ -1297,6 +1330,7 @@ mod tests {
 		type KickedMember = ();
 		type WeightInfo = ();
 		type MaxVoters = PhragmenMaxVoters;
+		type MaxVotesPerVoter = ConstU32<16>;
 		type MaxCandidates = PhragmenMaxCandidates;
 	}