Skip to content

Commit

Permalink
prep council election pallet for being dissolved (paritytech#11790)
Browse files Browse the repository at this point in the history 
* prep council election pallet for being dissolved + make it temporarily bounded

* fix tests

* fix

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

* fix bench?
  • Loading branch information
@kianenigma @ark0f
kianenigma authored and ark0f committed Feb 27, 2023
1 parent faa7498 commit 341d29b
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 151 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions frame/elections-phragmen/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ sp-std = { version = "4.0.0", default-features = false, path = "../../primitives
[dev-dependencies]
pallet-balances = { version = "4.0.0-dev", path = "../balances" }
sp-core = { version = "6.0.0", path = "../../primitives/core" }
sp-tracing = { path = "../../primitives/tracing" }
substrate-test-utils = { version = "4.0.0-dev", path = "../../test-utils" }

[features]
Expand Down
86 changes: 10 additions & 76 deletions frame/elections-phragmen/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,12 @@
use super::*;

use frame_benchmarking::{account, benchmarks, whitelist, BenchmarkError, BenchmarkResult};
use frame_support::{
dispatch::{DispatchResultWithPostInfo, UnfilteredDispatchable},
traits::OnInitialize,
};
use frame_support::{dispatch::DispatchResultWithPostInfo, traits::OnInitialize};
use frame_system::RawOrigin;

use crate::Pallet as Elections;

const BALANCE_FACTOR: u32 = 250;
const MAX_VOTERS: u32 = 500;
const MAX_CANDIDATES: u32 = 200;

type Lookup<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

Expand Down Expand Up @@ -345,38 +340,6 @@ benchmarks! {
))?;
}

// -- Root ones
#[extra] // this calls into phragmen and consumes a full block for now.
remove_member_without_replacement_extra {
// worse case is when we remove a member and we have no runner as a replacement. This
// triggers phragmen again. The only parameter is how many candidates will compete for the
// new slot.
let c in 1 .. MAX_CANDIDATES;
clean::<T>();

// fill only desired members. no runners-up.
let all_members = fill_seats_up_to::<T>(T::DesiredMembers::get())?;
assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get());

// submit a new one to compensate, with self-vote.
let replacements = submit_candidates_with_self_vote::<T>(c, "new_candidate")?;

// create some voters for these replacements.
distribute_voters::<T>(replacements, MAX_VOTERS, MAXIMUM_VOTE)?;

let to_remove = as_lookup::<T>(all_members[0].clone());
}: remove_member(RawOrigin::Root, to_remove, false)
verify {
// must still have the desired number of members members.
assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get());
#[cfg(test)]
{
// reset members in between benchmark tests.
use crate::tests::MEMBERS;
MEMBERS.with(|m| *m.borrow_mut() = vec![]);
}
}

remove_member_with_replacement {
// easy case. We have a runner up. Nothing will have that much of an impact. m will be
// number of members and runners. There is always at least one runner.
Expand All @@ -385,7 +348,7 @@ benchmarks! {

let _ = fill_seats_up_to::<T>(m)?;
let removing = as_lookup::<T>(<Elections<T>>::members_ids()[0].clone());
}: remove_member(RawOrigin::Root, removing, true)
}: remove_member(RawOrigin::Root, removing, true, false)
verify {
// must still have enough members.
assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get());
Expand All @@ -397,39 +360,6 @@ benchmarks! {
}
}

remove_member_wrong_refund {
// The root call by mistake indicated that this will have no replacement, while it has!
// this has now consumed a lot of weight and need to refund.
let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get();
clean::<T>();

let _ = fill_seats_up_to::<T>(m)?;
let removing = as_lookup::<T>(<Elections<T>>::members_ids()[0].clone());
let who = T::Lookup::lookup(removing.clone()).expect("member was added above");
let call = Call::<T>::remove_member { who: removing, has_replacement: false }.encode();
}: {
assert_eq!(
<Call<T> as Decode>::decode(&mut &*call)
.expect("call is encoded above, encoding must be correct")
.dispatch_bypass_filter(RawOrigin::Root.into())
.unwrap_err()
.error,
Error::<T>::InvalidReplacement.into(),
);
}
verify {
// must still have enough members.
assert_eq!(<Elections<T>>::members().len() as u32, T::DesiredMembers::get());
// on fail, `who` must still be a member
assert!(<Elections<T>>::members_ids().contains(&who));
#[cfg(test)]
{
// reset members in between benchmark tests.
use crate::tests::MEMBERS;
MEMBERS.with(|m| *m.borrow_mut() = vec![]);
}
}

clean_defunct_voters {
// total number of voters.
let v in (MAX_VOTERS / 2) .. MAX_VOTERS;
Expand All @@ -439,7 +369,7 @@ benchmarks! {
// remove any previous stuff.
clean::<T>();

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

// all candidates leave.
Expand Down Expand Up @@ -474,7 +404,7 @@ benchmarks! {
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, votes_per_voter as usize)?;
let _ = distribute_voters::<T>(all_candidates, v.saturating_sub(c), votes_per_voter as usize)?;
}: {
<Elections<T>>::on_initialize(T::TermDuration::get());
}
Expand Down Expand Up @@ -503,7 +433,11 @@ benchmarks! {
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, votes_per_voter as usize)?;
let _ = distribute_voters::<T>(
all_candidates,
fixed_v - c,
votes_per_voter as usize,
)?;
}: {
<Elections<T>>::on_initialize(T::TermDuration::get());
}
Expand All @@ -525,7 +459,7 @@ benchmarks! {
#[extra]
election_phragmen_v {
let v in 4 .. 16;
let fixed_c = MAX_CANDIDATES;
let fixed_c = MAX_CANDIDATES / 10;
let fixed_e = 64;
clean::<T>();

Expand Down
129 changes: 54 additions & 75 deletions frame/elections-phragmen/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@

use codec::{Decode, Encode};
use frame_support::{
dispatch::WithPostDispatchInfo,
traits::{
defensive_prelude::*, ChangeMembers, Contains, ContainsLengthBound, Currency,
CurrencyToVote, Get, InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced,
Expand All @@ -126,6 +125,17 @@ 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 @@ -385,8 +395,9 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;

let actual_count = <Candidates<T>>::decode_len().unwrap_or(0);
ensure!(actual_count as u32 <= candidate_count, Error::<T>::InvalidWitnessData);
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);

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

Expand Down Expand Up @@ -469,7 +480,11 @@ pub mod pallet {
/// the outgoing member is slashed.
///
/// If a runner-up is available, then the best runner-up will be removed and replaces the
/// outgoing member. Otherwise, a new phragmen election is started.
/// outgoing member. Otherwise, if `rerun_election` is `true`, a new phragmen election is
/// started, else, nothing happens.
///
/// If `slash_bond` is set to true, the bond of the member being removed is slashed. Else,
/// it is returned.
///
/// The dispatch origin of this call must be root.
///
Expand All @@ -479,33 +494,24 @@ pub mod pallet {
/// If we have a replacement, we use a small weight. Else, since this is a root call and
/// will go into phragmen, we assume full block for now.
/// # </weight>
#[pallet::weight(if *has_replacement {
T::WeightInfo::remove_member_with_replacement()
} else {
#[pallet::weight(if *rerun_election {
T::WeightInfo::remove_member_without_replacement()
} else {
T::WeightInfo::remove_member_with_replacement()
})]
pub fn remove_member(
origin: OriginFor<T>,
who: <T::Lookup as StaticLookup>::Source,
has_replacement: bool,
slash_bond: bool,
rerun_election: bool,
) -> DispatchResultWithPostInfo {
ensure_root(origin)?;
let who = T::Lookup::lookup(who)?;

let will_have_replacement = <RunnersUp<T>>::decode_len().map_or(false, |l| l > 0);
if will_have_replacement != has_replacement {
// In both cases, we will change more weight than need. Refund and abort.
return Err(Error::<T>::InvalidReplacement.with_weight(
// refund. The weight value comes from a benchmark which is special to this.
T::WeightInfo::remove_member_wrong_refund(),
))
}

let had_replacement = Self::remove_and_replace_member(&who, true)?;
debug_assert_eq!(has_replacement, had_replacement);
let _ = Self::remove_and_replace_member(&who, slash_bond)?;
Self::deposit_event(Event::MemberKicked { member: who });

if !had_replacement {
if rerun_election {
Self::do_phragmen();
}

Expand Down Expand Up @@ -585,10 +591,10 @@ pub mod pallet {
UnableToPayBond,
/// Must be a voter.
MustBeVoter,
/// Cannot report self.
ReportSelf,
/// Duplicated candidate submission.
DuplicatedCandidate,
/// Too many candidates have been created.
TooManyCandidates,
/// Member cannot re-submit candidacy.
MemberSubmit,
/// Runner cannot re-submit candidacy.
Expand Down Expand Up @@ -893,7 +899,7 @@ impl<T: Config> Pallet<T> {

if candidates_and_deposit.len().is_zero() {
Self::deposit_event(Event::EmptyTerm);
return T::DbWeight::get().reads(5)
return T::DbWeight::get().reads(3)
}

// All of the new winners that come out of phragmen will thus have a deposit recorded.
Expand All @@ -906,10 +912,28 @@ impl<T: Config> Pallet<T> {
let to_balance = |e: ExtendedBalance| T::CurrencyToVote::to_currency(e, total_issuance);

let mut num_edges: u32 = 0;

// used for prime election.
let voters_and_stakes = Voting::<T>::iter()
.map(|(voter, Voter { stake, votes, .. })| (voter, stake, votes))
.collect::<Vec<_>>();
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 {
voters_and_stakes.push((voter, stake, votes));
Ok(())
} else {
Err(())
}
}) {
Ok(_) => (),
Err(_) => {
log::error!(
target: "runtime::elections-phragmen",
"Failed to run election. Number of voters exceeded",
);
Self::deposit_event(Event::ElectionError);
return T::DbWeight::get().reads(3 + MAX_VOTERS as u64)
},
}

// used for phragmen.
let voters_and_votes = voters_and_stakes
.iter()
Expand Down Expand Up @@ -1137,7 +1161,7 @@ mod tests {
use sp_runtime::{
testing::Header,
traits::{BlakeTwo256, IdentityLookup},
BuildStorage, ModuleError,
BuildStorage,
};
use substrate_test_utils::assert_eq_uvec;

Expand Down Expand Up @@ -1321,6 +1345,7 @@ mod tests {
self
}
pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
sp_tracing::try_init_simple();
MEMBERS.with(|m| {
*m.borrow_mut() =
self.genesis_members.iter().map(|(m, _)| m.clone()).collect::<Vec<_>>()
Expand Down Expand Up @@ -2494,60 +2519,14 @@ mod tests {
assert_ok!(submit_candidacy(Origin::signed(3)));
assert_ok!(vote(Origin::signed(3), vec![3], 30));

assert_ok!(Elections::remove_member(Origin::root(), 4, false));
assert_ok!(Elections::remove_member(Origin::root(), 4, true, true));

assert_eq!(balances(&4), (35, 2)); // slashed
assert_eq!(Elections::election_rounds(), 2); // new election round
assert_eq!(members_ids(), vec![3, 5]); // new members
});
}

#[test]
fn remove_member_should_indicate_replacement() {
ExtBuilder::default().build_and_execute(|| {
assert_ok!(submit_candidacy(Origin::signed(5)));
assert_ok!(submit_candidacy(Origin::signed(4)));

assert_ok!(vote(Origin::signed(4), vec![4], 40));
assert_ok!(vote(Origin::signed(5), vec![5], 50));

System::set_block_number(5);
Elections::on_initialize(System::block_number());
assert_eq!(members_ids(), vec![4, 5]);

// no replacement yet.
let unwrapped_error = Elections::remove_member(Origin::root(), 4, true).unwrap_err();
assert!(matches!(
unwrapped_error.error,
DispatchError::Module(ModuleError { message: Some("InvalidReplacement"), .. })
));
assert!(unwrapped_error.post_info.actual_weight.is_some());
});

ExtBuilder::default().desired_runners_up(1).build_and_execute(|| {
assert_ok!(submit_candidacy(Origin::signed(5)));
assert_ok!(submit_candidacy(Origin::signed(4)));
assert_ok!(submit_candidacy(Origin::signed(3)));

assert_ok!(vote(Origin::signed(3), vec![3], 30));
assert_ok!(vote(Origin::signed(4), vec![4], 40));
assert_ok!(vote(Origin::signed(5), vec![5], 50));

System::set_block_number(5);
Elections::on_initialize(System::block_number());
assert_eq!(members_ids(), vec![4, 5]);
assert_eq!(runners_up_ids(), vec![3]);

// there is a replacement! and this one needs a weight refund.
let unwrapped_error = Elections::remove_member(Origin::root(), 4, false).unwrap_err();
assert!(matches!(
unwrapped_error.error,
DispatchError::Module(ModuleError { message: Some("InvalidReplacement"), .. })
));
assert!(unwrapped_error.post_info.actual_weight.is_some());
});
}

#[test]
fn seats_should_be_released_when_no_vote() {
ExtBuilder::default().build_and_execute(|| {
Expand Down Expand Up @@ -2684,7 +2663,7 @@ mod tests {
Elections::on_initialize(System::block_number());

assert_eq!(members_ids(), vec![2, 4]);
assert_ok!(Elections::remove_member(Origin::root(), 2, true));
assert_ok!(Elections::remove_member(Origin::root(), 2, true, false));
assert_eq!(members_ids(), vec![4, 5]);
});
}
Expand Down

0 comments on commit 341d29b

Please sign in to comment.