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

prep council election pallet for being dissolved #11790

Merged
merged 8 commits into from
Jul 12, 2022
Merged

prep council election pallet for being dissolved #11790

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
78d7e04
prep council election pallet for being dissolved + make it temporaril…
kianenigma Jul 6, 2022
a97fbba
Merge branch 'master' of github.com:paritytech/substrate into kiz-cou…
kianenigma Jul 7, 2022
855420a
fix tests
kianenigma Jul 7, 2022
924f7ef
fix
kianenigma Jul 8, 2022
8689d51
Update frame/elections-phragmen/src/lib.rs
kianenigma Jul 11, 2022
16d0277
fix bench?
kianenigma Jul 12, 2022
c5a4572
Merge branch 'kiz-council-dissolve' of github.com:paritytech/substrat…
kianenigma Jul 12, 2022
161ab86
Merge branch 'master' of github.com:paritytech/substrate into kiz-cou…
kianenigma Jul 12, 2022
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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
Open in desktop
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
84 changes: 9 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 @@ -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,43 +348,10 @@ 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)
verify {
// must still have enough 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_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(),
);
}
}: remove_member(RawOrigin::Root, removing, true, false)
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.
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;
Copy link
Member

Choose a reason for hiding this comment

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

que

let fixed_e = 64;
clean::<T>();

Expand Down
129 changes: 54 additions & 75 deletions frame/elections-phragmen/src/lib.rs
View file
Open in desktop
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