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

Configurable voting-degree in council elections pallet #13305

Merged
paritytech-processbot merged 20 commits into from
Feb 8, 2023
Merged

Configurable voting-degree in council elections pallet #13305

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9bdae1f
configurable council elections pallet
kianenigma Feb 3, 2023
17ef867
configurable council elections pallet
kianenigma Feb 3, 2023
c7136d5
add warning
kianenigma Feb 3, 2023
38926b4
reduce sizes
kianenigma Feb 3, 2023
19857b2
Merge branch 'master' of https://github.com/paritytech/substrate into…
Feb 3, 2023
c6c4f58
".git/.scripts/commands/bench/bench.sh" pallet dev pallet_elections_p…
Feb 3, 2023
401d00d
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Feb 6, 2023
3b9f751
Merge branch 'kiz-fix-election-pallet' of github.com:paritytech/subst…
kianenigma Feb 6, 2023
8fcf10b
fix stuff
kianenigma Feb 6, 2023
0160b9f
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Feb 7, 2023
897f111
make assert
kianenigma Feb 7, 2023
cc74b3b
fix docs
kianenigma Feb 7, 2023
3d4ed6c
fix docs again
kianenigma Feb 7, 2023
93f0d50
fix docs again
kianenigma Feb 7, 2023
f94174e
Update frame/elections-phragmen/src/lib.rs
kianenigma Feb 7, 2023
e1b53da
Update frame/elections-phragmen/src/lib.rs
kianenigma Feb 7, 2023
dce84c0
Update frame/elections-phragmen/src/lib.rs
kianenigma Feb 7, 2023
9d9b493
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Feb 8, 2023
4464433
fix docs
kianenigma Feb 8, 2023
b1c0eec
Merge branch 'kiz-fix-election-pallet' of github.com:paritytech/subst…
kianenigma Feb 8, 2023
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
6 changes: 4 additions & 2 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}

Expand All @@ -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>;
}
Expand Down
88 changes: 13 additions & 75 deletions frame/elections-phragmen/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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)?;
Expand All @@ -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),
Expand Down
89 changes: 66 additions & 23 deletions frame/elections-phragmen/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 [`T::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`].
//!
Expand Down Expand Up @@ -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<
Expand Down Expand Up @@ -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;
}
Expand All @@ -284,6 +291,43 @@ 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,
);
if election_weight.any_gt(block_weight) {
frame_support::log::error!(
Copy link
Member

@ggwpez ggwpez Feb 6, 2023

Choose a reason for hiding this comment

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

Should this not bee an assert!? Otherwise the test will not fail.

Copy link
Contributor Author

@kianenigma kianenigma Feb 7, 2023

Choose a reason for hiding this comment

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

Thing is, for a solo-chain, this is more or less acceptable to have an overweight election. For a parachain, it is not.

Ideally, for this we'd have @sam0x17's

if parachain_build {} else {}

For now I will move it to assert, better safe than sorry.

target: LOG_TARGET,
"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]
Expand All @@ -307,10 +351,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)
Expand All @@ -324,8 +364,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);
Expand Down Expand Up @@ -1006,15 +1048,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]
Expand Down Expand Up @@ -1297,6 +1339,7 @@ mod tests {
type KickedMember = ();
type WeightInfo = ();
type MaxVoters = PhragmenMaxVoters;
type MaxVotesPerVoter = ConstU32<16>;
type MaxCandidates = PhragmenMaxCandidates;
}

Expand Down
Loading