Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

make MAX_VOTERS and MAX_CANDIDATES in elections-phragmen configurable. Fix: #11092 #11908

Merged
merged 17 commits into from
Aug 14, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,8 @@ 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 ElectionsPhragmenPalletId: LockIdentifier = *b"phrelect";
}

Expand All @@ -989,6 +991,8 @@ impl pallet_elections_phragmen::Config for Runtime {
type DesiredMembers = DesiredMembers;
type DesiredRunnersUp = DesiredRunnersUp;
type TermDuration = TermDuration;
type MaxVoters = MaxVoters;
type MaxCandidates = MaxCandidates;
type WeightInfo = pallet_elections_phragmen::weights::SubstrateWeight<Runtime>;
}

Expand Down
26 changes: 13 additions & 13 deletions frame/elections-phragmen/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ fn endowed_account<T: Config>(name: &'static str, index: u32) -> T::AccountId {
let account: T::AccountId = account(name, index, 0);
// Fund each account with at-least his stake but still a sane amount as to not mess up
// the vote calculation.
let amount = default_stake::<T>(MAX_VOTERS) * BalanceOf::<T>::from(BALANCE_FACTOR);
let amount = default_stake::<T>(T::MaxVoters::get()) * BalanceOf::<T>::from(BALANCE_FACTOR);
let _ = T::Currency::make_free_balance_be(&account, amount);
// important to increase the total issuance since T::CurrencyToVote will need it to be sane for
// phragmen to work.
Expand Down Expand Up @@ -230,7 +230,7 @@ benchmarks! {

submit_candidacy {
// number of already existing candidates.
let c in 1 .. MAX_CANDIDATES;
let c in 1 .. T::MaxCandidates::get();
// we fix the number of members to the number of desired members and runners-up. We'll be in
// this state almost always.
let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get();
Expand Down Expand Up @@ -261,7 +261,7 @@ benchmarks! {
// this will check members, runners-up and candidate for removal. Members and runners-up are
// limited by the runtime bound, nonetheless we fill them by `m`.
// number of already existing candidates.
let c in 1 .. MAX_CANDIDATES;
let c in 1 .. T::MaxCandidates::get();
// we fix the number of members to the number of desired members and runners-up. We'll be in
// this state almost always.
let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get();
Expand Down Expand Up @@ -362,14 +362,14 @@ benchmarks! {

clean_defunct_voters {
// total number of voters.
let v in (MAX_VOTERS / 2) .. MAX_VOTERS;
let v in (T::MaxVoters::get() / 2) .. T::MaxVoters::get();
// those that are defunct and need removal.
let d in 1 .. (MAX_VOTERS / 2);
let d in 1 .. (T::MaxVoters::get() / 2);

// remove any previous stuff.
clean::<T>();

let all_candidates = submit_candidates::<T>(MAX_CANDIDATES, "candidates")?;
let all_candidates = submit_candidates::<T>(T::MaxCandidates::get(), "candidates")?;
distribute_voters::<T>(all_candidates, v, MAXIMUM_VOTE)?;

// all candidates leave.
Expand All @@ -389,9 +389,9 @@ benchmarks! {
// members, this is hard-coded in the runtime and cannot be trivially changed at this stage.
// Yet, change the number of voters, candidates and edge per voter to see the impact. Note
// that we give all candidates a self vote to make sure they are all considered.
let c in 1 .. MAX_CANDIDATES;
let v in 1 .. MAX_VOTERS;
let e in MAX_VOTERS .. MAX_VOTERS * MAXIMUM_VOTE as u32;
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;
clean::<T>();

// so we have a situation with v and e. we want e to basically always be in the range of `e
Expand Down Expand Up @@ -425,9 +425,9 @@ benchmarks! {

#[extra]
election_phragmen_c_e {
let c in 1 .. MAX_CANDIDATES;
let e in MAX_VOTERS .. MAX_VOTERS * MAXIMUM_VOTE as u32;
let fixed_v = MAX_VOTERS;
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;
Expand Down Expand Up @@ -459,7 +459,7 @@ benchmarks! {
#[extra]
election_phragmen_v {
let v in 4 .. 16;
let fixed_c = MAX_CANDIDATES / 10;
let fixed_c = T::MaxCandidates::get() / 10;
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
let fixed_e = 64;
clean::<T>();

Expand Down
40 changes: 26 additions & 14 deletions frame/elections-phragmen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,6 @@ pub mod migrations;
/// The maximum votes allowed per voter.
pub const MAXIMUM_VOTE: usize = 16;

// Some safe temp values to make the wasm execution sane while we still use this pallet.
#[cfg(test)]
pub(crate) const MAX_CANDIDATES: u32 = 100;
#[cfg(not(test))]
pub(crate) const MAX_CANDIDATES: u32 = 1000;

#[cfg(test)]
pub(crate) const MAX_VOTERS: u32 = 1000;
#[cfg(not(test))]
pub(crate) const MAX_VOTERS: u32 = 10 * 1000;

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<
Expand Down Expand Up @@ -259,6 +248,21 @@ pub mod pallet {
#[pallet::constant]
type TermDuration: Get<Self::BlockNumber>;

/// The maximum number of candidates in a phragmen election.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kianenigma Should the docs for MaxCandidates and MaxVoters be more detailed than this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a section warning that the election happens onchain, and these numbers determine how big the election will be. Also note that if the maximum candidates are reached, we don't accept anymore. If max voters are reached, we just ignore voters.

///
/// 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.
/// When the limit is reached the new voters are ignored.
#[pallet::constant]
type MaxVoters: Get<u32>;

/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
}
Expand Down Expand Up @@ -397,7 +401,10 @@ pub mod pallet {

let actual_count = <Candidates<T>>::decode_len().unwrap_or(0) as u32;
ensure!(actual_count <= candidate_count, Error::<T>::InvalidWitnessData);
ensure!(actual_count <= MAX_CANDIDATES, Error::<T>::TooManyCandidates);
ensure!(
actual_count <= <T as Config>::MaxCandidates::get(),
Error::<T>::TooManyCandidates
);

let index = Self::is_candidate(&who).err().ok_or(Error::<T>::DuplicatedCandidate)?;

Expand Down Expand Up @@ -913,10 +920,11 @@ impl<T: Config> Pallet<T> {

let mut num_edges: u32 = 0;

let max_voters = <T as Config>::MaxVoters::get() as usize;
// used for prime election.
let mut voters_and_stakes = Vec::new();
match Voting::<T>::iter().try_for_each(|(voter, Voter { stake, votes, .. })| {
if voters_and_stakes.len() < MAX_VOTERS as usize {
if voters_and_stakes.len() < max_voters {
voters_and_stakes.push((voter, stake, votes));
Ok(())
} else {
Expand All @@ -930,7 +938,7 @@ impl<T: Config> Pallet<T> {
"Failed to run election. Number of voters exceeded",
);
Self::deposit_event(Event::ElectionError);
return T::DbWeight::get().reads(3 + MAX_VOTERS as u64)
return T::DbWeight::get().reads(3 + max_voters as u64)
},
}

Expand Down Expand Up @@ -1266,6 +1274,8 @@ mod tests {

parameter_types! {
pub const ElectionsPhragmenPalletId: LockIdentifier = *b"phrelect";
pub const PhragmenMaxVoters: u32 = 1000;
pub const PhragmenMaxCandidates: u32 = 100;
}

impl Config for Test {
Expand All @@ -1284,6 +1294,8 @@ mod tests {
type LoserCandidate = ();
type KickedMember = ();
type WeightInfo = ();
type MaxVoters = PhragmenMaxVoters;
type MaxCandidates = PhragmenMaxCandidates;
}

pub type Block = sp_runtime::generic::Block<Header, UncheckedExtrinsic>;
Expand Down