diff --git a/Cargo.lock b/Cargo.lock index 26f43fa4ae0ba..f98d98a3c571f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5673,6 +5673,7 @@ dependencies = [ "sp-npos-elections", "sp-runtime", "sp-std", + "sp-tracing", "substrate-test-utils", ] diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index 0f195f12cce3c..d71a74f76a114 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -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] diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 44dd6aff09f0c..9a9d448449eca 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -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 = <::Lookup as StaticLookup>::Source; @@ -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::(); - - // fill only desired members. no runners-up. - let all_members = fill_seats_up_to::(T::DesiredMembers::get())?; - assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); - - // submit a new one to compensate, with self-vote. - let replacements = submit_candidates_with_self_vote::(c, "new_candidate")?; - - // create some voters for these replacements. - distribute_voters::(replacements, MAX_VOTERS, MAXIMUM_VOTE)?; - - let to_remove = as_lookup::(all_members[0].clone()); - }: remove_member(RawOrigin::Root, to_remove, false) - verify { - // must still have the desired number of members members. - assert_eq!(>::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. @@ -385,7 +348,7 @@ benchmarks! { let _ = fill_seats_up_to::(m)?; let removing = as_lookup::(>::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!(>::members().len() as u32, T::DesiredMembers::get()); @@ -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::(); - - let _ = fill_seats_up_to::(m)?; - let removing = as_lookup::(>::members_ids()[0].clone()); - let who = T::Lookup::lookup(removing.clone()).expect("member was added above"); - let call = Call::::remove_member { who: removing, has_replacement: false }.encode(); - }: { - assert_eq!( - as Decode>::decode(&mut &*call) - .expect("call is encoded above, encoding must be correct") - .dispatch_bypass_filter(RawOrigin::Root.into()) - .unwrap_err() - .error, - Error::::InvalidReplacement.into(), - ); - } - verify { - // must still have enough members. - assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); - // on fail, `who` must still be a member - assert!(>::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; @@ -439,7 +369,7 @@ benchmarks! { // remove any previous stuff. clean::(); - let all_candidates = submit_candidates::(v, "candidates")?; + let all_candidates = submit_candidates::(MAX_CANDIDATES, "candidates")?; distribute_voters::(all_candidates, v, MAXIMUM_VOTE)?; // all candidates leave. @@ -474,7 +404,7 @@ benchmarks! { let votes_per_voter = (e / v).min(MAXIMUM_VOTE as u32); let all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; - let _ = distribute_voters::(all_candidates, v, votes_per_voter as usize)?; + let _ = distribute_voters::(all_candidates, v.saturating_sub(c), votes_per_voter as usize)?; }: { >::on_initialize(T::TermDuration::get()); } @@ -503,7 +433,11 @@ benchmarks! { let votes_per_voter = e / fixed_v; let all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; - let _ = distribute_voters::(all_candidates, fixed_v, votes_per_voter as usize)?; + let _ = distribute_voters::( + all_candidates, + fixed_v - c, + votes_per_voter as usize, + )?; }: { >::on_initialize(T::TermDuration::get()); } @@ -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::(); diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index ec2234cde5a6e..28fed28f18e5c 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -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, @@ -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 = <::Currency as Currency<::AccountId>>::Balance; type NegativeImbalanceOf = <::Currency as Currency< @@ -385,8 +395,9 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - let actual_count = >::decode_len().unwrap_or(0); - ensure!(actual_count as u32 <= candidate_count, Error::::InvalidWitnessData); + let actual_count = >::decode_len().unwrap_or(0) as u32; + ensure!(actual_count <= candidate_count, Error::::InvalidWitnessData); + ensure!(actual_count <= MAX_CANDIDATES, Error::::TooManyCandidates); let index = Self::is_candidate(&who).err().ok_or(Error::::DuplicatedCandidate)?; @@ -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. /// @@ -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. /// # - #[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, who: ::Source, - has_replacement: bool, + slash_bond: bool, + rerun_election: bool, ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let who = T::Lookup::lookup(who)?; - let will_have_replacement = >::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::::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(); } @@ -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. @@ -893,7 +899,7 @@ impl Pallet { 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. @@ -906,10 +912,28 @@ impl Pallet { 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::::iter() - .map(|(voter, Voter { stake, votes, .. })| (voter, stake, votes)) - .collect::>(); + let mut voters_and_stakes = Vec::new(); + match Voting::::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() @@ -1137,7 +1161,7 @@ mod tests { use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, - BuildStorage, ModuleError, + BuildStorage, }; use substrate_test_utils::assert_eq_uvec; @@ -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::>() @@ -2494,7 +2519,7 @@ 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 @@ -2502,52 +2527,6 @@ mod tests { }); } - #[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(|| { @@ -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]); }); }