From d115a3d30412f246f0b1dc178ae70d830cd66605 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 29 Apr 2020 10:48:57 +0200 Subject: [PATCH 01/30] Fist benchmark barely working --- Cargo.lock | 2 +- frame/elections-phragmen/Cargo.toml | 7 +- frame/elections-phragmen/src/benchmarking.rs | 81 ++++++++++++++++++++ frame/elections-phragmen/src/lib.rs | 2 + frame/offences/benchmarking/src/lib.rs | 2 +- frame/timestamp/src/benchmarking.rs | 2 + 6 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 frame/elections-phragmen/src/benchmarking.rs diff --git a/Cargo.lock b/Cargo.lock index 9cf78bfcfbc08..f2833b8886422 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4169,11 +4169,11 @@ dependencies = [ name = "pallet-elections-phragmen" version = "2.0.0-dev" dependencies = [ + "frame-benchmarking", "frame-support", "frame-system", "hex-literal", "pallet-balances", - "pallet-scheduler", "parity-scale-codec", "serde", "sp-core", diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index f9d681b7608e6..5599ebfea3f67 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -19,17 +19,18 @@ sp-phragmen = { version = "2.0.0-dev", default-features = false, path = "../../p frame-support = { version = "2.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "2.0.0-dev", default-features = false, path = "../system" } sp-std = { version = "2.0.0-dev", default-features = false, path = "../../primitives/std" } +frame-benchmarking = { version = "2.0.0-dev", default-features = false, path = "../benchmarking", optional = true } +sp-io = { version = "2.0.0-dev", default-features = false, path = "../../primitives/io", optional = true } [dev-dependencies] sp-io = { version = "2.0.0-dev", path = "../../primitives/io" } hex-literal = "0.2.1" pallet-balances = { version = "2.0.0-dev", path = "../balances" } -pallet-scheduler = { version = "2.0.0-dev", path = "../scheduler" } sp-core = { version = "2.0.0-dev", path = "../../primitives/core" } substrate-test-utils = { version = "2.0.0-dev", path = "../../test-utils" } [features] -default = ["std"] +default = ["std", "runtime-benchmarks"] std = [ "serde", "codec/std", @@ -39,4 +40,4 @@ std = [ "frame-system/std", "sp-std/std", ] -runtime-benchmarks = ["frame-support/runtime-benchmarks"] +runtime-benchmarks = ["frame-benchmarking", "sp-io"] diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs new file mode 100644 index 0000000000000..07ff7ee3ae1c1 --- /dev/null +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -0,0 +1,81 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Elections-Phragmen pallet benchmarking. + +#![cfg(feature = "runtime-benchmarks")] + +use super::*; + +use codec::{Encode, Decode}; +use frame_system::RawOrigin; +use sp_io::hashing::blake2_256; +use frame_benchmarking::benchmarks; +use sp_runtime::traits::Bounded; + +use crate::Module as Elections; + +const SEED: u32 = 0; +const DEFAULT_STAKE: u32 = 1000; + +fn account(name: &'static str, index: u32) -> T::AccountId { + let entropy = (name, index).using_encoded(blake2_256); + T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() +} + +/// Add `c` new candidates. +fn submit_candidates(c: u32) -> Result, &'static str> { + (0..c).map(|i| { + let account = account::("candidate", i); + let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); + >::submit_candidacy(RawOrigin::Signed(account).into()) + .map_err(|_| "failed to submit candidacy")?; + Ok(account) + }).collect::>() +} + +benchmarks! { + _ { } + + vote { + // range of candidates to vote for. + let x in 0 .. (MAXIMUM_VOTE as u32); + + // create a bunch of candidates. + let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32)?; + + let caller = account::("caller", SEED); + let stake = BalanceOf::::from(DEFAULT_STAKE); + let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); + // vote first `x` ones. + let votes = all_candidates.into_iter().take(x as usize).collect(); + }: _(RawOrigin::Signed(caller), votes, stake) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::{ExtBuilder, Test}; + use frame_support::assert_ok; + + #[test] + fn test_benchmarks() { + ExtBuilder::default().build().execute_with(|| { + assert_ok!(test_benchmark_vote::()); + }); + } +} + diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 50c26dba0fc21..acc3c30c42b3c 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -98,6 +98,8 @@ use frame_support::{ use sp_phragmen::{build_support_map, ExtendedBalance, VoteWeight, PhragmenResult}; use frame_system::{self as system, ensure_signed, ensure_root}; +mod benchmarking; + /// The maximum votes allowed per voter. pub const MAXIMUM_VOTE: usize = 16; diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index a88714a89a7fa..ccb28e4db895a 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -142,7 +142,7 @@ benchmarks! { let reporter = account("reporter", i, SEED); reporters.push(reporter); } - + let offenders = make_offenders::(o, n).expect("failed to create offenders"); let keys = ImOnline::::keys(); diff --git a/frame/timestamp/src/benchmarking.rs b/frame/timestamp/src/benchmarking.rs index c468bf82fba6f..8fd51a52edd8f 100644 --- a/frame/timestamp/src/benchmarking.rs +++ b/frame/timestamp/src/benchmarking.rs @@ -34,6 +34,7 @@ benchmarks! { set { let t in 1 .. MAX_TIME; }: _(RawOrigin::None, t.into()) + verify { ensure!(Timestamp::::now() == t.into(), "Time was not set."); } @@ -43,6 +44,7 @@ benchmarks! { Timestamp::::set(RawOrigin::None.into(), t.into())?; ensure!(DidUpdate::exists(), "Time was not set."); }: { Timestamp::::on_finalize(t.into()); } + verify { ensure!(!DidUpdate::exists(), "Time was not removed."); } From 831aa27494428ba0b42ba0148d3c599e62dd2347 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 29 Apr 2020 14:03:47 +0200 Subject: [PATCH 02/30] Debug checkpoint --- frame/elections-phragmen/src/benchmarking.rs | 138 +++++++++++++++++-- frame/elections-phragmen/src/lib.rs | 6 +- frame/vesting/src/benchmarking.rs | 2 +- 3 files changed, 133 insertions(+), 13 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 07ff7ee3ae1c1..1efef2b1eb51a 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -25,44 +25,157 @@ use frame_system::RawOrigin; use sp_io::hashing::blake2_256; use frame_benchmarking::benchmarks; use sp_runtime::traits::Bounded; +use frame_support::assert_ok; use crate::Module as Elections; const SEED: u32 = 0; const DEFAULT_STAKE: u32 = 1000; +type Lookup = <::Lookup as StaticLookup>::Source; + fn account(name: &'static str, index: u32) -> T::AccountId { let entropy = (name, index).using_encoded(blake2_256); T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() } +fn endowed_account(name: &'static str, index: u32) -> T::AccountId { + let account = account::(name, index); + let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); + account +} + +fn as_lookup(account: T::AccountId) -> Lookup { + T::Lookup::unlookup(account) +} + /// Add `c` new candidates. fn submit_candidates(c: u32) -> Result, &'static str> { (0..c).map(|i| { - let account = account::("candidate", i); - let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); - >::submit_candidacy(RawOrigin::Signed(account).into()) + let account = endowed_account::("candidate", i); + >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) .map_err(|_| "failed to submit candidacy")?; Ok(account) }).collect::>() } +/// Submit one voter. +fn submit_voter(caller: T::AccountId, votes: Vec, stake: BalanceOf) + -> Result<(), &'static str> +{ + >::vote(RawOrigin::Signed(caller).into(), votes, stake) + .map_err(|_| "failed to submit vote") +} + benchmarks! { - _ { } + // TODO: is this needed? or is it just nice to be explicit that we don't have common stuff? + _ {} + // -- Signed ones vote { - // range of candidates to vote for. - let x in 0 .. (MAXIMUM_VOTE as u32); + // range of candidates to vote for. Direct argument of the dispatchable. + let x in 1 .. (MAXIMUM_VOTE as u32); // create a bunch of candidates. let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32)?; - let caller = account::("caller", SEED); + let caller = endowed_account::("caller", SEED); let stake = BalanceOf::::from(DEFAULT_STAKE); - let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); // vote first `x` ones. let votes = all_candidates.into_iter().take(x as usize).collect(); }: _(RawOrigin::Signed(caller), votes, stake) + + remove_voter { + // No complexity parameter. while we can vote for numerous candidates and then remove them + // (and I think each can have slightly different performance), we cannot express this via + // the parameters. Hence, we only check for the median, 8 votes to be removed. Only use + // account seed as complexity parameter. + // TODO: what should be the upper bound here? the first benchmark runs 16 iterations.. this one 1000? + let s in 0 .. 1000; + + let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; + + // create a bunch of candidates. + let all_candidates = submit_candidates::(votes_to_remove)?; + + let caller = endowed_account::("caller", s); + let stake = BalanceOf::::from(DEFAULT_STAKE); + submit_voter::(caller.clone(), all_candidates, stake)?; + }: _(RawOrigin::Signed(caller)) + + report_defunct_voter { + // has no complexity parameter. two voters exist. One only votes fro outdated candidates. + // The other one can report the defunct one. + let s in 0 .. 1000; + + // create a bunch of candidates. + let candidate_count = (MAXIMUM_VOTE) as u32; + let all_candidates = submit_candidates::(candidate_count)?; + + let stake = BalanceOf::::from(DEFAULT_STAKE); + + // account 1 votes for the first half + let account_1 = endowed_account::("caller_1", s); + let bailing = all_candidates.iter().take(candidate_count as usize / 2).cloned().collect::>(); + submit_voter::( + account_1.clone(), + bailing.clone(), + stake, + )?; + let account_1_lookup = as_lookup::(account_1); + + // account 2 votes for the second half + let account_2 = endowed_account::("caller_2", s); + submit_voter::( + account_2.clone(), + all_candidates.iter().rev().take(candidate_count as usize / 2).cloned().collect(), + stake, + )?; + + // all the first chunk of candidates bail out + bailing.into_iter().for_each(|b| { + assert_ok!(>::renounce_candidacy(RawOrigin::Signed(b).into())); + }); + }: _(RawOrigin::Signed(account_2), account_1_lookup) + + submit_candidacy { + // No complexity parameter. Use account seed to iterate. + let s in 0 .. 1000; + + let candidate_account = endowed_account::("candidate", s); + }: _(RawOrigin::Signed(candidate_account.clone())) + + + renounce_candidacy { + // No complexity parameter. Use account seed to iterate. + let s in 0 .. 1000; + + let candidate_account = endowed_account::("candidate", s); + >::submit_candidacy(RawOrigin::Signed(candidate_account.clone()).into()) + .map_err(|_| "failed to submit candidacy")?; + }: _(RawOrigin::Signed(candidate_account.clone())) + + // -- Root ones + remove_member { + // worse case is when we remove a member and we have no runner as a replacement. This + // triggers phragmen again. + + // create only enough candidates for members. + let candidate_count = T::DesiredMembers::get(); + let all_candidates = submit_candidates::(candidate_count)?; + >::do_phragmen(); + + assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); + assert_eq!(>::runners_up().len(), 0); + + let to_remove = all_candidates[0].clone(); + }: _(RawOrigin::Root, as_lookup::(to_remove)) + verify { + println!("{:?}", >::events()); + assert!(false); + } + + on_initialize {}: {} } #[cfg(test)] @@ -72,9 +185,14 @@ mod tests { use frame_support::assert_ok; #[test] - fn test_benchmarks() { - ExtBuilder::default().build().execute_with(|| { + fn test_benchmarks_elections_phragmen() { + ExtBuilder::default().build_and_execute(|| { assert_ok!(test_benchmark_vote::()); + assert_ok!(test_benchmark_remove_voter::()); + assert_ok!(test_benchmark_report_defunct_voter::()); + assert_ok!(test_benchmark_submit_candidacy::()); + assert_ok!(test_benchmark_renounce_candidacy::()); + assert_ok!(test_benchmark_remove_member::()); }); } } diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index d138144bfbda9..2f82afd41efbd 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -454,6 +454,7 @@ decl_module! { } let mut runners_up_with_stake = Self::runners_up(); + // runners-up are NOT sorted based on account id. Linear search. if let Some(index) = runners_up_with_stake.iter() .position(|(ref r, ref _s)| r == &who) { @@ -462,7 +463,8 @@ decl_module! { T::Currency::unreserve(&who, T::CandidacyBond::get()); // update storage. >::put(runners_up_with_stake); - // safety guard to make sure we do only one arm. Better to read runners later. + + // safety guard to make sure we do only one arm. Better to read candidates later. return Ok(()); } @@ -473,7 +475,7 @@ decl_module! { T::Currency::unreserve(&who, T::CandidacyBond::get()); // update storage. >::put(candidates); - // safety guard to make sure we do only one arm. Better to read runners later. + return Ok(()); } diff --git a/frame/vesting/src/benchmarking.rs b/frame/vesting/src/benchmarking.rs index 10f19af65ee18..f94d4d6a20486 100644 --- a/frame/vesting/src/benchmarking.rs +++ b/frame/vesting/src/benchmarking.rs @@ -201,7 +201,7 @@ mod tests { use frame_support::assert_ok; #[test] - fn test_benchmarks() { + fn test_benchmarks_elections_phragmen() { ExtBuilder::default().existential_deposit(256).build().execute_with(|| { assert_ok!(test_benchmark_vest_locked::()); assert_ok!(test_benchmark_vest_unlocked::()); From 630b6a7bc84741d83af208d4143e36b592369706 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 30 Apr 2020 13:15:02 +0200 Subject: [PATCH 03/30] add rest of benchmarks --- frame/benchmarking/src/lib.rs | 4 + frame/elections-phragmen/Cargo.toml | 7 +- frame/elections-phragmen/src/benchmarking.rs | 116 ++++++++++++++++--- frame/elections-phragmen/src/lib.rs | 14 ++- frame/vesting/src/benchmarking.rs | 2 +- 5 files changed, 123 insertions(+), 20 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 8a9df7e4cf341..3c9c210b55048 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -921,6 +921,10 @@ macro_rules! impl_benchmark_tests { let selected_benchmark = SelectedBenchmark::$name; let components = >::components(&selected_benchmark); + assert!( + components.len() != 0, + "You need to add components to your benchmark!", + ); for (_, (name, low, high)) in components.iter().enumerate() { // Test only the low and high value, assuming values in the middle won't break for component_value in vec![low, high] { diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index 5599ebfea3f67..a7ad6459ab2d1 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -40,4 +40,9 @@ std = [ "frame-system/std", "sp-std/std", ] -runtime-benchmarks = ["frame-benchmarking", "sp-io"] +runtime-benchmarks = [ + "frame-benchmarking", + "sp-io", + "frame-support/runtime-benchmarks", + "frame-system/runtime-benchmarks", + ] diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 1efef2b1eb51a..4e96bf06c23b3 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -25,30 +25,39 @@ use frame_system::RawOrigin; use sp_io::hashing::blake2_256; use frame_benchmarking::benchmarks; use sp_runtime::traits::Bounded; -use frame_support::assert_ok; +use frame_support::{assert_ok, traits::OnInitialize}; use crate::Module as Elections; const SEED: u32 = 0; -const DEFAULT_STAKE: u32 = 1000; +const BALANCE_FACTOR: u32 = 250; type Lookup = <::Lookup as StaticLookup>::Source; +/// grab a new account fn account(name: &'static str, index: u32) -> T::AccountId { let entropy = (name, index).using_encoded(blake2_256); T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() } +/// grab new account with infinite balance. fn endowed_account(name: &'static str, index: u32) -> T::AccountId { let account = account::(name, index); let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); account } +/// Account ot lookup type of system trait. fn as_lookup(account: T::AccountId) -> Lookup { T::Lookup::unlookup(account) } +/// Get a reasonable amount of stake based on the execution trait's configuration +fn default_stake(factor: u32) -> BalanceOf { + let factor = BalanceOf::::from(factor); + T::Currency::minimum_balance() * factor +} + /// Add `c` new candidates. fn submit_candidates(c: u32) -> Result, &'static str> { (0..c).map(|i| { @@ -63,12 +72,11 @@ fn submit_candidates(c: u32) -> Result, &'static str fn submit_voter(caller: T::AccountId, votes: Vec, stake: BalanceOf) -> Result<(), &'static str> { - >::vote(RawOrigin::Signed(caller).into(), votes, stake) - .map_err(|_| "failed to submit vote") + >::vote(RawOrigin::Signed(caller).into(), votes, stake) + .map_err(|e| "failed to submit vote") } benchmarks! { - // TODO: is this needed? or is it just nice to be explicit that we don't have common stuff? _ {} // -- Signed ones @@ -80,7 +88,8 @@ benchmarks! { let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32)?; let caller = endowed_account::("caller", SEED); - let stake = BalanceOf::::from(DEFAULT_STAKE); + let stake = default_stake::(BALANCE_FACTOR); + // vote first `x` ones. let votes = all_candidates.into_iter().take(x as usize).collect(); }: _(RawOrigin::Signed(caller), votes, stake) @@ -90,7 +99,9 @@ benchmarks! { // (and I think each can have slightly different performance), we cannot express this via // the parameters. Hence, we only check for the median, 8 votes to be removed. Only use // account seed as complexity parameter. - // TODO: what should be the upper bound here? the first benchmark runs 16 iterations.. this one 1000? + // TODO: what should be the upper bound here? the first benchmark runs 16 iterations.. this + // one 1000? These are used only to iterate and run the benchmark s times. They are not a + // parameter in any way. let s in 0 .. 1000; let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; @@ -99,7 +110,7 @@ benchmarks! { let all_candidates = submit_candidates::(votes_to_remove)?; let caller = endowed_account::("caller", s); - let stake = BalanceOf::::from(DEFAULT_STAKE); + let stake = default_stake::(BALANCE_FACTOR); submit_voter::(caller.clone(), all_candidates, stake)?; }: _(RawOrigin::Signed(caller)) @@ -112,11 +123,16 @@ benchmarks! { let candidate_count = (MAXIMUM_VOTE) as u32; let all_candidates = submit_candidates::(candidate_count)?; - let stake = BalanceOf::::from(DEFAULT_STAKE); + let stake = default_stake::(BALANCE_FACTOR); // account 1 votes for the first half let account_1 = endowed_account::("caller_1", s); - let bailing = all_candidates.iter().take(candidate_count as usize / 2).cloned().collect::>(); + let bailing = all_candidates + .iter() + .take(candidate_count as usize / 2) + .cloned() + .collect::>(); + submit_voter::( account_1.clone(), bailing.clone(), @@ -145,7 +161,6 @@ benchmarks! { let candidate_account = endowed_account::("candidate", s); }: _(RawOrigin::Signed(candidate_account.clone())) - renounce_candidacy { // No complexity parameter. Use account seed to iterate. let s in 0 .. 1000; @@ -159,23 +174,87 @@ benchmarks! { remove_member { // worse case is when we remove a member and we have no runner as a replacement. This // triggers phragmen again. + let x in 0 .. 1000; + let stake = default_stake::(BALANCE_FACTOR); // create only enough candidates for members. let candidate_count = T::DesiredMembers::get(); let all_candidates = submit_candidates::(candidate_count)?; - >::do_phragmen(); + let _ = all_candidates.iter().map(|c| + submit_voter::(c.clone(), vec![c.clone()], stake) + ).collect::>()?; + assert_eq!(>::candidates().len() as u32, candidate_count); + >::do_phragmen(); assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); assert_eq!(>::runners_up().len(), 0); + assert_eq!(>::candidates().len(), 0); + + // submit a new one to compensate + let replacement = endowed_account::("candidate", 999); + >::submit_candidacy(RawOrigin::Signed(replacement.clone()).into()) + .map_err(|_| "failed to submit candidacy")?; + submit_voter::(replacement.clone(), vec![replacement.clone()], stake)?; let to_remove = all_candidates[0].clone(); }: _(RawOrigin::Root, as_lookup::(to_remove)) verify { - println!("{:?}", >::events()); - assert!(false); + // must still have 2 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![]); + } } - on_initialize {}: {} + on_initialize { + // if n % TermDuration is zero, then we run phragmen. The weight function must and should + // check this as it is cheap to do so. TermDuration is not a storage item, it is a constant + // encoded in the runtime. Assumed number of voters is 50,000. + let x in 0 .. 100; + let stake = default_stake::(BALANCE_FACTOR); + + let candidate_count = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + let all_candidates = submit_candidates::(candidate_count)?; + + // add self vote to all of them for now. + let _ = all_candidates.iter().map(|c| + submit_voter::(c.clone(), vec![c.clone()], stake) + ).collect::>()?; + + // create 50,000 voters voting for a set of 8 of candidates. Note that this only works for + // when desired members is more than 8. + for i in 0..50_000 { + let starting_index = i % candidate_count; + let votes = all_candidates + .iter() + .cloned() + .chain(all_candidates.iter().cloned()) + .skip(starting_index as usize) + .take(8) + .collect::>(); + let voter = endowed_account::("voter", i); + submit_voter::(voter, votes, stake)?; + } + }: { + // elect + >::on_initialize(T::TermDuration::get()); + } + verify { + assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); + assert_eq!(>::runners_up().len() as u32, T::DesiredRunnersUp::get()); + + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + println!("verified and done with iteration {}", x); + } } #[cfg(test)] @@ -186,13 +265,18 @@ mod tests { #[test] fn test_benchmarks_elections_phragmen() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default() + .desired_members(13) + .desired_runners_up(7) + .build_and_execute( + || { assert_ok!(test_benchmark_vote::()); assert_ok!(test_benchmark_remove_voter::()); assert_ok!(test_benchmark_report_defunct_voter::()); assert_ok!(test_benchmark_submit_candidacy::()); assert_ok!(test_benchmark_renounce_candidacy::()); assert_ok!(test_benchmark_remove_member::()); + assert_ok!(test_benchmark_on_initialize::()); }); } } diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 2f82afd41efbd..a11be6b31a6aa 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -1081,6 +1081,7 @@ mod tests { voter_bond: u64, term_duration: u64, desired_runners_up: u32, + desired_members: u32, } impl Default for ExtBuilder { @@ -1089,8 +1090,9 @@ mod tests { genesis_members: vec![], balance_factor: 1, voter_bond: 2, - desired_runners_up: 0, term_duration: 5, + desired_runners_up: 0, + desired_members: 2, } } } @@ -1112,11 +1114,19 @@ mod tests { self.genesis_members = members; self } - pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + pub fn desired_members(mut self, count: u32) -> Self { + self.desired_members = count; + self + } + fn set_constants(&self) { VOTING_BOND.with(|v| *v.borrow_mut() = self.voter_bond); TERM_DURATION.with(|v| *v.borrow_mut() = self.term_duration); DESIRED_RUNNERS_UP.with(|v| *v.borrow_mut() = self.desired_runners_up); + DESIRED_MEMBERS.with(|m| *m.borrow_mut() = self.desired_members); MEMBERS.with(|m| *m.borrow_mut() = self.genesis_members.iter().map(|(m, _)| m.clone()).collect::>()); + } + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + self.set_constants(); let mut ext: sp_io::TestExternalities = GenesisConfig { pallet_balances: Some(pallet_balances::GenesisConfig::{ balances: vec![ diff --git a/frame/vesting/src/benchmarking.rs b/frame/vesting/src/benchmarking.rs index f94d4d6a20486..10f19af65ee18 100644 --- a/frame/vesting/src/benchmarking.rs +++ b/frame/vesting/src/benchmarking.rs @@ -201,7 +201,7 @@ mod tests { use frame_support::assert_ok; #[test] - fn test_benchmarks_elections_phragmen() { + fn test_benchmarks() { ExtBuilder::default().existential_deposit(256).build().execute_with(|| { assert_ok!(test_benchmark_vest_locked::()); assert_ok!(test_benchmark_vest_unlocked::()); From 55879f634126afc8522c3d46d24eb2e63fe3c756 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 30 Apr 2020 13:15:49 +0200 Subject: [PATCH 04/30] Add to runtime --- bin/node/runtime/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 98507abf5d9ad..b148724fe2e94 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -921,6 +921,7 @@ impl_runtime_apis! { add_benchmark!(params, batches, b"treasury", Treasury); add_benchmark!(params, batches, b"utility", Utility); add_benchmark!(params, batches, b"vesting", Vesting); + add_benchmark!(params, batches, b"elections", Elections); if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) } Ok(batches) From c26ae0b1b8610d6ea8bf90bf406d60c2b1718f27 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 30 Apr 2020 15:57:27 +0200 Subject: [PATCH 05/30] Fix build --- frame/elections-phragmen/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index a7ad6459ab2d1..bb4682fc5df1a 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -30,7 +30,7 @@ sp-core = { version = "2.0.0-dev", path = "../../primitives/core" } substrate-test-utils = { version = "2.0.0-dev", path = "../../test-utils" } [features] -default = ["std", "runtime-benchmarks"] +default = ["std"] std = [ "serde", "codec/std", From dd5679790349423cb25408d4dfa338a6f0341bf4 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 4 May 2020 09:37:56 +0200 Subject: [PATCH 06/30] Update frame/elections-phragmen/src/benchmarking.rs Co-authored-by: Shawn Tabrizi --- frame/elections-phragmen/src/benchmarking.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 4e96bf06c23b3..8fc7abc4cb97d 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -115,7 +115,7 @@ benchmarks! { }: _(RawOrigin::Signed(caller)) report_defunct_voter { - // has no complexity parameter. two voters exist. One only votes fro outdated candidates. + // has no complexity parameter. two voters exist. One only votes for outdated candidates. // The other one can report the defunct one. let s in 0 .. 1000; @@ -280,4 +280,3 @@ mod tests { }); } } - From c3629f621ee77d3afd7763b10103b93f8e225144 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 4 May 2020 09:38:03 +0200 Subject: [PATCH 07/30] Update frame/elections-phragmen/src/benchmarking.rs Co-authored-by: Shawn Tabrizi --- frame/elections-phragmen/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 8fc7abc4cb97d..ca999b3fe33a0 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -73,7 +73,7 @@ fn submit_voter(caller: T::AccountId, votes: Vec, stake: -> Result<(), &'static str> { >::vote(RawOrigin::Signed(caller).into(), votes, stake) - .map_err(|e| "failed to submit vote") + .map_err(|_| "failed to submit vote") } benchmarks! { From 23b733f82a38d0e05137da8ff44ec1be27516fc9 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 4 May 2020 16:00:29 +0200 Subject: [PATCH 08/30] major imp --- frame/elections-phragmen/src/benchmarking.rs | 361 ++++++++++++++----- frame/elections-phragmen/src/lib.rs | 23 +- 2 files changed, 279 insertions(+), 105 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index ca999b3fe33a0..9c528fd7d4b29 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -31,6 +31,9 @@ use crate::Module as Elections; const SEED: u32 = 0; const BALANCE_FACTOR: u32 = 250; +const MAX_LOCKS: u32 = 20; +const MAX_VOTERS: u32 = 50_000; +const MAX_CANDIDATES: u32 = 1000; type Lookup = <::Lookup as StaticLookup>::Source; @@ -59,21 +62,57 @@ fn default_stake(factor: u32) -> BalanceOf { } /// Add `c` new candidates. -fn submit_candidates(c: u32) -> Result, &'static str> { +fn submit_candidates(c: u32, prefix: &'static str) -> Result, &'static str> { (0..c).map(|i| { - let account = endowed_account::("candidate", i); + let account = endowed_account::(prefix, i); >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) - .map_err(|_| "failed to submit candidacy")?; + .map_err(|e| { println!("Error = {:?}", e); "failed to submit candidacy"})?; Ok(account) }).collect::>() } +/// Add `c` new candidates with self vote. +fn submit_candidates_with_self_vote(c: u32, prefix: &'static str) -> Result, &'static str> { + let candidates = submit_candidates::(c, prefix)?; + let stake = default_stake::(BALANCE_FACTOR); + let _ = candidates.iter().map(|c| + submit_voter::(c.clone(), vec![c.clone()], stake) + ).collect::>()?; + Ok(candidates) +} + + /// Submit one voter. fn submit_voter(caller: T::AccountId, votes: Vec, stake: BalanceOf) -> Result<(), &'static str> { >::vote(RawOrigin::Signed(caller).into(), votes, stake) - .map_err(|_| "failed to submit vote") + .map_err(|e| { println!("Error = {:?}", e); "failed to submit vote"}) +} + +/// add `n` locks to account `who`. +fn add_locks(who: &T::AccountId, n: u8) { + for id in 0..n { + let lock_id = [id; 8]; + let locked = 100; + let reasons = WithdrawReason::Transfer | WithdrawReason::Reserve; + T::Currency::set_lock(lock_id, who, locked.into(), reasons); + } +} + +/// Fill the seats of members and runners-up up until `m`. Note that this might include either only +/// members, or members and runners-up. +fn fill_seats_up_to(m: u32) -> Result, &'static str> { + let candidates = submit_candidates_with_self_vote::(m, "fill_seats_up_to")?; + + assert_eq!(>::candidates().len() as u32, m); + >::do_phragmen(); + assert_eq!(>::candidates().len(), 0); + assert_eq!( + >::members().len() + >::runners_up().len(), + m as usize, + ); + Ok(candidates) } benchmarks! { @@ -81,127 +120,235 @@ benchmarks! { // -- Signed ones vote { - // range of candidates to vote for. Direct argument of the dispatchable. + // range of candidates to vote for. let x in 1 .. (MAXIMUM_VOTE as u32); + // range of locks that the caller already had. This extrinsic adds a lock. + let l in 1 .. MAX_LOCKS; // create a bunch of candidates. - let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32)?; + let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32, "candidates")?; let caller = endowed_account::("caller", SEED); + add_locks::(&caller, l as u8); + let stake = default_stake::(BALANCE_FACTOR); // vote first `x` ones. let votes = all_candidates.into_iter().take(x as usize).collect(); + }: _(RawOrigin::Signed(caller), votes, stake) remove_voter { - // No complexity parameter. while we can vote for numerous candidates and then remove them - // (and I think each can have slightly different performance), we cannot express this via - // the parameters. Hence, we only check for the median, 8 votes to be removed. Only use - // account seed as complexity parameter. - // TODO: what should be the upper bound here? the first benchmark runs 16 iterations.. this - // one 1000? These are used only to iterate and run the benchmark s times. They are not a - // parameter in any way. - let s in 0 .. 1000; + // range of candidates that we have voted for. This may or may have a significant impact on + // the outcome. + let c in 1 .. (MAXIMUM_VOTE as u32); + // range of locks that the caller already had. This extrinsic removes a lock. + let l in 1 .. MAX_LOCKS; let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; // create a bunch of candidates. - let all_candidates = submit_candidates::(votes_to_remove)?; + let all_candidates = submit_candidates::(c, "candidates")?; + + let caller = endowed_account::("caller", SEED); + add_locks::(&caller, l as u8); - let caller = endowed_account::("caller", s); let stake = default_stake::(BALANCE_FACTOR); submit_voter::(caller.clone(), all_candidates, stake)?; + }: _(RawOrigin::Signed(caller)) - report_defunct_voter { - // has no complexity parameter. two voters exist. One only votes for outdated candidates. - // The other one can report the defunct one. - let s in 0 .. 1000; - - // create a bunch of candidates. - let candidate_count = (MAXIMUM_VOTE) as u32; - let all_candidates = submit_candidates::(candidate_count)?; + report_defunct_voter_correct { + // number fo already existing members or runners + let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number of candidates that the reported voter voted for. This may or may not have any + // impact on the outcome. + let c in 1 .. (MAXIMUM_VOTE as u32); let stake = default_stake::(BALANCE_FACTOR); - // account 1 votes for the first half - let account_1 = endowed_account::("caller_1", s); - let bailing = all_candidates - .iter() - .take(candidate_count as usize / 2) - .cloned() - .collect::>(); + // create m members and runners combined. + let _ = fill_seats_up_to::(m)?; + + // create a bunch of candidates as well. + let all_candidates = submit_candidates::(c, "candidates")?; + // account 1 is the reporter and it doesn't matter how many it votes. + let account_1 = endowed_account::("caller_1", 0); submit_voter::( account_1.clone(), - bailing.clone(), + all_candidates.iter().take(1).cloned().collect(), stake, )?; - let account_1_lookup = as_lookup::(account_1); - // account 2 votes for the second half - let account_2 = endowed_account::("caller_2", s); + // account 2 votes for all of the mentioned candidates. + let account_2 = endowed_account::("caller_2", 1); submit_voter::( account_2.clone(), - all_candidates.iter().rev().take(candidate_count as usize / 2).cloned().collect(), + all_candidates.clone(), stake, )?; - // all the first chunk of candidates bail out - bailing.into_iter().for_each(|b| { + let account_2_lookup = as_lookup::(account_2.clone()); + + // all the bailers go away. + all_candidates.into_iter().for_each(|b| { assert_ok!(>::renounce_candidacy(RawOrigin::Signed(b).into())); }); - }: _(RawOrigin::Signed(account_2), account_1_lookup) + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) + verify { + assert!(>::is_voter(&account_1)); + assert!(!>::is_voter(&account_2)); + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } + + report_defunct_voter_incorrect { + // number fo already existing members or runners + let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number of candidates that the reported voter voted for. This may or may not have any + // impact on the outcome. + let c in 1 .. (MAXIMUM_VOTE as u32); + + let stake = default_stake::(BALANCE_FACTOR); + + // create m members and runners combined. + let _ = fill_seats_up_to::(m)?; + + // create a bunch of candidates as well. + let all_candidates = submit_candidates::(c, "candidates")?; + + // account 1 is the reporter and it doesn't matter how many it votes. + let account_1 = endowed_account::("caller_1", 0); + submit_voter::( + account_1.clone(), + all_candidates.iter().take(1).cloned().collect(), + stake, + )?; + + // account 2 votes for all of the mentioned candidates. + let account_2 = endowed_account::("caller_2", 1); + submit_voter::( + account_2.clone(), + all_candidates.clone(), + stake, + )?; + + let account_2_lookup = as_lookup::(account_2.clone()); + // no one bails out. account_1 is slashed and removed as voter now. + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) + verify { + assert!(!>::is_voter(&account_1)); + assert!(>::is_voter(&account_2)); + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } submit_candidacy { - // No complexity parameter. Use account seed to iterate. - let s in 0 .. 1000; + // number fo already existing members or runners + let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + let stake = default_stake::(BALANCE_FACTOR); + + // create m members and runners combined. + let _ = fill_seats_up_to::(m)?; - let candidate_account = endowed_account::("candidate", s); + // we assume worse case that: extrinsic is successful and candidate is not duplicate. + let candidate_account = endowed_account::("candidate", 0); }: _(RawOrigin::Signed(candidate_account.clone())) - renounce_candidacy { - // No complexity parameter. Use account seed to iterate. - let s in 0 .. 1000; + renounce_candidacy_candidate { + // this will check members, runners-up and candidate for removal. Members and runners-up are + // limited by the runtime bound, so we just fill them to maximum, while the number of + // candidates can be big. Note that we should be gentle with maximum candidates, as all + // candidates except for those who will make it in as a winner will lose their bond and get + // removed, so most likely we will not have too many candidates all at once. + // number of already existing candidates. + let c in 1 .. MAX_CANDIDATES; + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); - let candidate_account = endowed_account::("candidate", s); - >::submit_candidacy(RawOrigin::Signed(candidate_account.clone()).into()) - .map_err(|_| "failed to submit candidacy")?; - }: _(RawOrigin::Signed(candidate_account.clone())) + // create m members and runners combined. + let _ = fill_seats_up_to::(m)?; + + let all_candidates = submit_candidates::(c, "candidates")?; + + let bailing = all_candidates[0].clone(); + }: renounce_candidacy(RawOrigin::Signed(bailing.clone())) + verify { + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } + + renounce_candidacy_member_runner_up { + // removing members and runners will be cheaper most likely. The number of candidates will \ + // have no impact. + // number of already existing candidates. + let c in 1 .. MAX_CANDIDATES; + let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + + // create m members and runners combined. + let members_and_runners_up = fill_seats_up_to::(m)?; + let _ = submit_candidates::(c, "candidates")?; + + let bailing = members_and_runners_up[0].clone(); + }: renounce_candidacy(RawOrigin::Signed(bailing.clone())) + verify { + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } // -- Root ones - remove_member { + remove_member_without_replacement { // worse case is when we remove a member and we have no runner as a replacement. This // triggers phragmen again. let x in 0 .. 1000; let stake = default_stake::(BALANCE_FACTOR); - // create only enough candidates for members. - let candidate_count = T::DesiredMembers::get(); - let all_candidates = submit_candidates::(candidate_count)?; - let _ = all_candidates.iter().map(|c| - submit_voter::(c.clone(), vec![c.clone()], stake) - ).collect::>()?; + let all_members = fill_seats_up_to::(T::DesiredMembers::get())?; + + // submit a new one to compensate, with self-vote + let _replacement = submit_candidates_with_self_vote::(1, "new_candidate"); + let to_remove = all_members[0].clone(); - assert_eq!(>::candidates().len() as u32, candidate_count); - >::do_phragmen(); + }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) + verify { + // must still have 2 members. assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); - assert_eq!(>::runners_up().len(), 0); - assert_eq!(>::candidates().len(), 0); + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } - // submit a new one to compensate - let replacement = endowed_account::("candidate", 999); - >::submit_candidacy(RawOrigin::Signed(replacement.clone()).into()) - .map_err(|_| "failed to submit candidacy")?; - submit_voter::(replacement.clone(), vec![replacement.clone()], stake)?; + // -- Root ones + remove_member_with_replacement { + // easy case. We have a runner up. + let x in 0 .. 1000; + let stake = default_stake::(BALANCE_FACTOR); - let to_remove = all_candidates[0].clone(); - }: _(RawOrigin::Root, as_lookup::(to_remove)) + let all_members = fill_seats_up_to::(T::DesiredMembers::get() + 1)?; + let to_remove = all_members[0].clone(); + }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) verify { // must still have 2 members. assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); - #[cfg(test)] { // reset members in between benchmark tests. @@ -213,28 +360,21 @@ benchmarks! { on_initialize { // if n % TermDuration is zero, then we run phragmen. The weight function must and should // check this as it is cheap to do so. TermDuration is not a storage item, it is a constant - // encoded in the runtime. Assumed number of voters is 50,000. - let x in 0 .. 100; + // encoded in the runtime. + let c in 1 .. MAX_CANDIDATES; let stake = default_stake::(BALANCE_FACTOR); - let candidate_count = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); - let all_candidates = submit_candidates::(candidate_count)?; - - // add self vote to all of them for now. - let _ = all_candidates.iter().map(|c| - submit_voter::(c.clone(), vec![c.clone()], stake) - ).collect::>()?; + let mut all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; - // create 50,000 voters voting for a set of 8 of candidates. Note that this only works for - // when desired members is more than 8. - for i in 0..50_000 { - let starting_index = i % candidate_count; + // create 50,000 voters voting for a set of at most 8 of candidates. + for i in 0..MAX_VOTERS { + let starting_index = i % c; + // to ensure that votes are different, + all_candidates.rotate_left((i % c) as usize); let votes = all_candidates .iter() .cloned() - .chain(all_candidates.iter().cloned()) - .skip(starting_index as usize) - .take(8) + .take(MAXIMUM_VOTE) .collect::>(); let voter = endowed_account::("voter", i); submit_voter::(voter, votes, stake)?; @@ -244,8 +384,14 @@ benchmarks! { >::on_initialize(T::TermDuration::get()); } verify { - assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); - assert_eq!(>::runners_up().len() as u32, T::DesiredRunnersUp::get()); + assert_eq!( + >::members().len() as u32, + T::DesiredMembers::get().min(c), + ); + assert_eq!( + >::runners_up().len() as u32, + T::DesiredRunnersUp::get().min(c.saturating_sub(T::DesiredMembers::get())), + ); #[cfg(test)] { @@ -253,7 +399,6 @@ benchmarks! { use crate::tests::MEMBERS; MEMBERS.with(|m| *m.borrow_mut() = vec![]); } - println!("verified and done with iteration {}", x); } } @@ -265,18 +410,44 @@ mod tests { #[test] fn test_benchmarks_elections_phragmen() { - ExtBuilder::default() - .desired_members(13) - .desired_runners_up(7) - .build_and_execute( - || { + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_vote::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_remove_voter::()); - assert_ok!(test_benchmark_report_defunct_voter::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_report_defunct_voter_correct::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_report_defunct_voter_incorrect::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_submit_candidacy::()); - assert_ok!(test_benchmark_renounce_candidacy::()); - assert_ok!(test_benchmark_remove_member::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_renounce_candidacy_candidate::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_renounce_candidacy_member_runner_up::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_remove_member_without_replacement::()); + }); + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_remove_member_with_replacement::()); + }); + + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_on_initialize::()); }); + } } diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index a11be6b31a6aa..3dbd3fcb41ef2 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -286,9 +286,11 @@ decl_module! { /// /// The `votes` should: /// - not be empty. - /// - be less than the number of candidates. + /// - be less than the number of possible candidates. Note that all current members and + /// runners-up are also automatically candidates for the next round. /// /// Upon voting, `value` units of `who`'s balance is locked and a bond amount is reserved. + /// /// It is the responsibility of the caller to not place all of their balance into the lock /// and keep some for further transactions. /// @@ -301,6 +303,9 @@ decl_module! { fn vote(origin, votes: Vec, #[compact] value: BalanceOf) { let who = ensure_signed(origin)?; + ensure!(votes.len() <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); + ensure!(!votes.is_empty(), Error::::NoVotes); + let candidates_count = >::decode_len().unwrap_or(0) as usize; let members_count = >::decode_len().unwrap_or(0) as usize; // addition is valid: candidates and members never overlap. @@ -308,19 +313,15 @@ decl_module! { ensure!(!allowed_votes.is_zero(), Error::::UnableToVote); ensure!(votes.len() <= allowed_votes, Error::::TooManyVotes); - ensure!(votes.len() <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); - ensure!(!votes.is_empty(), Error::::NoVotes); - ensure!( - value > T::Currency::minimum_balance(), - Error::::LowBalance, - ); + ensure!(value > T::Currency::minimum_balance(), Error::::LowBalance); if !Self::is_voter(&who) { // first time voter. Reserve bond. T::Currency::reserve(&who, T::VotingBond::get()) .map_err(|_| Error::::UnableToPayBond)?; } + // Amount to be locked up. let locked_balance = value.min(T::Currency::total_balance(&who)); @@ -357,7 +358,7 @@ decl_module! { /// /// A defunct voter is defined to be: /// - a voter whose current submitted votes are all invalid. i.e. all of them are no - /// longer a candidate nor an active member. + /// longer a candidate nor an active member or a runner-up. /// /// # /// #### State @@ -644,7 +645,9 @@ impl Module { if Self::is_voter(who) { Self::votes_of(who) .iter() - .all(|v| !Self::is_member(v) && !Self::is_runner(v) && !Self::is_candidate(v).is_ok()) + .all(|v| + !Self::is_member(v) && !Self::is_runner(v) && !Self::is_candidate(v).is_ok() + ) } else { false } @@ -1017,7 +1020,7 @@ mod tests { new_plus_outgoing.extend_from_slice(outgoing); new_plus_outgoing.sort(); - assert_eq!(old_plus_incoming, new_plus_outgoing); + assert_eq!(old_plus_incoming, new_plus_outgoing, "change members call is incorrect!"); MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); PRIME.with(|p| *p.borrow_mut() = None); From cc6e0724eeaf94d1b20a1295d5a08c2f1303061a Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 4 May 2020 19:58:34 +0200 Subject: [PATCH 09/30] Make them run on release --- frame/elections-phragmen/src/benchmarking.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 9c528fd7d4b29..07bf9d1bfbfaa 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -25,7 +25,7 @@ use frame_system::RawOrigin; use sp_io::hashing::blake2_256; use frame_benchmarking::benchmarks; use sp_runtime::traits::Bounded; -use frame_support::{assert_ok, traits::OnInitialize}; +use frame_support::traits::OnInitialize; use crate::Module as Elections; @@ -66,7 +66,7 @@ fn submit_candidates(c: u32, prefix: &'static str) -> Result(prefix, i); >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) - .map_err(|e| { println!("Error = {:?}", e); "failed to submit candidacy"})?; + .map_err(|e| "failed to submit candidacy")?; Ok(account) }).collect::>() } @@ -87,7 +87,7 @@ fn submit_voter(caller: T::AccountId, votes: Vec, stake: -> Result<(), &'static str> { >::vote(RawOrigin::Signed(caller).into(), votes, stake) - .map_err(|e| { println!("Error = {:?}", e); "failed to submit vote"}) + .map_err(|e| "failed to submit vote") } /// add `n` locks to account `who`. @@ -193,7 +193,7 @@ benchmarks! { // all the bailers go away. all_candidates.into_iter().for_each(|b| { - assert_ok!(>::renounce_candidacy(RawOrigin::Signed(b).into())); + assert!(>::renounce_candidacy(RawOrigin::Signed(b).into()).is_ok()); }); }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) verify { From 7c5b184907676e9b717682eab4f76c726338a3f6 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 5 May 2020 08:49:57 +0200 Subject: [PATCH 10/30] Help finish phragmen benchmarks (#5886) * update caller, account, and member/runner-up creation * remove stuff * ocd --- bin/node/runtime/src/lib.rs | 2 +- frame/elections-phragmen/Cargo.toml | 4 +-- frame/elections-phragmen/src/benchmarking.rs | 32 ++++++++------------ 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index aa3e23a44a795..fee1a45151290 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -918,6 +918,7 @@ impl_runtime_apis! { add_benchmark!(params, batches, b"balances", Balances); add_benchmark!(params, batches, b"collective", Council); add_benchmark!(params, batches, b"democracy", Democracy); + add_benchmark!(params, batches, b"elections", Elections); add_benchmark!(params, batches, b"identity", Identity); add_benchmark!(params, batches, b"im-online", ImOnline); add_benchmark!(params, batches, b"offences", OffencesBench::); @@ -928,7 +929,6 @@ impl_runtime_apis! { add_benchmark!(params, batches, b"treasury", Treasury); add_benchmark!(params, batches, b"utility", Utility); add_benchmark!(params, batches, b"vesting", Vesting); - add_benchmark!(params, batches, b"elections", Elections); if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) } Ok(batches) diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index bb4682fc5df1a..0815680877bc6 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -20,7 +20,6 @@ frame-support = { version = "2.0.0-dev", default-features = false, path = "../su frame-system = { version = "2.0.0-dev", default-features = false, path = "../system" } sp-std = { version = "2.0.0-dev", default-features = false, path = "../../primitives/std" } frame-benchmarking = { version = "2.0.0-dev", default-features = false, path = "../benchmarking", optional = true } -sp-io = { version = "2.0.0-dev", default-features = false, path = "../../primitives/io", optional = true } [dev-dependencies] sp-io = { version = "2.0.0-dev", path = "../../primitives/io" } @@ -42,7 +41,6 @@ std = [ ] runtime-benchmarks = [ "frame-benchmarking", - "sp-io", "frame-support/runtime-benchmarks", "frame-system/runtime-benchmarks", - ] +] diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 07bf9d1bfbfaa..8df7758942ded 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -20,10 +20,8 @@ use super::*; -use codec::{Encode, Decode}; use frame_system::RawOrigin; -use sp_io::hashing::blake2_256; -use frame_benchmarking::benchmarks; +use frame_benchmarking::{benchmarks, account}; use sp_runtime::traits::Bounded; use frame_support::traits::OnInitialize; @@ -37,15 +35,9 @@ const MAX_CANDIDATES: u32 = 1000; type Lookup = <::Lookup as StaticLookup>::Source; -/// grab a new account -fn account(name: &'static str, index: u32) -> T::AccountId { - let entropy = (name, index).using_encoded(blake2_256); - T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() -} - /// grab new account with infinite balance. fn endowed_account(name: &'static str, index: u32) -> T::AccountId { - let account = account::(name, index); + let account: T::AccountId = account(name, index, SEED); let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); account } @@ -66,7 +58,7 @@ fn submit_candidates(c: u32, prefix: &'static str) -> Result(prefix, i); >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) - .map_err(|e| "failed to submit candidacy")?; + .map_err(|_| "failed to submit candidacy")?; Ok(account) }).collect::>() } @@ -87,7 +79,7 @@ fn submit_voter(caller: T::AccountId, votes: Vec, stake: -> Result<(), &'static str> { >::vote(RawOrigin::Signed(caller).into(), votes, stake) - .map_err(|e| "failed to submit vote") + .map_err(|_| "failed to submit vote") } /// add `n` locks to account `who`. @@ -128,7 +120,7 @@ benchmarks! { // create a bunch of candidates. let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32, "candidates")?; - let caller = endowed_account::("caller", SEED); + let caller = endowed_account::("caller", 0); add_locks::(&caller, l as u8); let stake = default_stake::(BALANCE_FACTOR); @@ -150,7 +142,7 @@ benchmarks! { // create a bunch of candidates. let all_candidates = submit_candidates::(c, "candidates")?; - let caller = endowed_account::("caller", SEED); + let caller = endowed_account::("caller", 0); add_locks::(&caller, l as u8); let stake = default_stake::(BALANCE_FACTOR); @@ -159,7 +151,7 @@ benchmarks! { }: _(RawOrigin::Signed(caller)) report_defunct_voter_correct { - // number fo already existing members or runners + // number of already existing members or runners let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of candidates that the reported voter voted for. This may or may not have any // impact on the outcome. @@ -174,7 +166,7 @@ benchmarks! { let all_candidates = submit_candidates::(c, "candidates")?; // account 1 is the reporter and it doesn't matter how many it votes. - let account_1 = endowed_account::("caller_1", 0); + let account_1 = endowed_account::("caller", 0); submit_voter::( account_1.clone(), all_candidates.iter().take(1).cloned().collect(), @@ -223,7 +215,7 @@ benchmarks! { let all_candidates = submit_candidates::(c, "candidates")?; // account 1 is the reporter and it doesn't matter how many it votes. - let account_1 = endowed_account::("caller_1", 0); + let account_1 = endowed_account::("caller", 0); submit_voter::( account_1.clone(), all_candidates.iter().take(1).cloned().collect(), @@ -261,7 +253,7 @@ benchmarks! { let _ = fill_seats_up_to::(m)?; // we assume worse case that: extrinsic is successful and candidate is not duplicate. - let candidate_account = endowed_account::("candidate", 0); + let candidate_account = endowed_account::("caller", 0); }: _(RawOrigin::Signed(candidate_account.clone())) renounce_candidacy_candidate { @@ -277,9 +269,9 @@ benchmarks! { // create m members and runners combined. let _ = fill_seats_up_to::(m)?; - let all_candidates = submit_candidates::(c, "candidates")?; + let all_candidates = submit_candidates::(c, "caller")?; - let bailing = all_candidates[0].clone(); + let bailing = all_candidates[0].clone(); // Should be ("caller", 0) }: renounce_candidacy(RawOrigin::Signed(bailing.clone())) verify { #[cfg(test)] From 6201da2a5eacc7f361f745f63fde5a7152f55ae5 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 5 May 2020 12:09:16 +0200 Subject: [PATCH 11/30] make it work with real run --- frame/elections-phragmen/src/benchmarking.rs | 46 +++++++++++++++----- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 8df7758942ded..e36cb3adfbee6 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -22,7 +22,6 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; -use sp_runtime::traits::Bounded; use frame_support::traits::OnInitialize; use crate::Module as Elections; @@ -38,7 +37,11 @@ type Lookup = <::Lookup as StaticLookup>::Source; /// grab new account with infinite balance. fn endowed_account(name: &'static str, index: u32) -> T::AccountId { let account: T::AccountId = account(name, index, SEED); - let _ = T::Currency::make_free_balance_be(&account, BalanceOf::::max_value()); + let amount = default_stake::(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. + T::Currency::issue(amount); account } @@ -96,17 +99,25 @@ fn add_locks(who: &T::AccountId, n: u8) { /// members, or members and runners-up. fn fill_seats_up_to(m: u32) -> Result, &'static str> { let candidates = submit_candidates_with_self_vote::(m, "fill_seats_up_to")?; - - assert_eq!(>::candidates().len() as u32, m); + assert_eq!(>::candidates().len() as u32, m, "wrong number of candidates."); >::do_phragmen(); - assert_eq!(>::candidates().len(), 0); + assert_eq!(>::candidates().len(), 0, "some candidates remaining."); assert_eq!( >::members().len() + >::runners_up().len(), m as usize, + "wrong number of members and runners-up", ); Ok(candidates) } +/// removes all the storage items to reverse any genesis state. +fn clean() { + >::kill(); + >::kill(); + >::kill(); + let _ = >::drain(); +} + benchmarks! { _ {} @@ -116,6 +127,7 @@ benchmarks! { let x in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic adds a lock. let l in 1 .. MAX_LOCKS; + clean::(); // create a bunch of candidates. let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32, "candidates")?; @@ -136,6 +148,7 @@ benchmarks! { let c in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic removes a lock. let l in 1 .. MAX_LOCKS; + clean::(); let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; @@ -151,11 +164,12 @@ benchmarks! { }: _(RawOrigin::Signed(caller)) report_defunct_voter_correct { - // number of already existing members or runners - let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number fo already existing members or runners + let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of candidates that the reported voter voted for. This may or may not have any // impact on the outcome. let c in 1 .. (MAXIMUM_VOTE as u32); + clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -205,6 +219,7 @@ benchmarks! { // number of candidates that the reported voter voted for. This may or may not have any // impact on the outcome. let c in 1 .. (MAXIMUM_VOTE as u32); + clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -247,6 +262,7 @@ benchmarks! { submit_candidacy { // number fo already existing members or runners let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + clean::(); let stake = default_stake::(BALANCE_FACTOR); // create m members and runners combined. @@ -264,6 +280,7 @@ benchmarks! { // removed, so most likely we will not have too many candidates all at once. // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; + clean::(); let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // create m members and runners combined. @@ -288,6 +305,7 @@ benchmarks! { // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + clean::(); // create m members and runners combined. let members_and_runners_up = fill_seats_up_to::(m)?; @@ -307,16 +325,20 @@ benchmarks! { // -- Root ones remove_member_without_replacement { // worse case is when we remove a member and we have no runner as a replacement. This - // triggers phragmen again. - let x in 0 .. 1000; + // triggers phragmen again. The only parameter is how many candidates will compete for the + // new slot + let c in 0 .. MAX_CANDIDATES; + clean::(); let stake = default_stake::(BALANCE_FACTOR); + // fill only desired members. no runners-up. let all_members = fill_seats_up_to::(T::DesiredMembers::get())?; - // submit a new one to compensate, with self-vote - let _replacement = submit_candidates_with_self_vote::(1, "new_candidate"); + // submit a new one to compensate, with self-vote. + let _replacement = submit_candidates_with_self_vote::(c, "new_candidate"); let to_remove = all_members[0].clone(); + // NOTE: we don't add any voters here. This can be observed from the on_initialize bench. }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) verify { // must still have 2 members. @@ -333,6 +355,7 @@ benchmarks! { remove_member_with_replacement { // easy case. We have a runner up. let x in 0 .. 1000; + clean::(); let stake = default_stake::(BALANCE_FACTOR); let all_members = fill_seats_up_to::(T::DesiredMembers::get() + 1)?; @@ -354,6 +377,7 @@ benchmarks! { // check this as it is cheap to do so. TermDuration is not a storage item, it is a constant // encoded in the runtime. let c in 1 .. MAX_CANDIDATES; + clean::(); let stake = default_stake::(BALANCE_FACTOR); let mut all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; From c0c443716219b40f5d24a15dfa2b8f8a11f9f082 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 5 May 2020 21:04:51 +0200 Subject: [PATCH 12/30] relax the numbers a bit --- frame/elections-phragmen/src/benchmarking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index e36cb3adfbee6..c6ac94250d7cc 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -29,8 +29,8 @@ use crate::Module as Elections; const SEED: u32 = 0; const BALANCE_FACTOR: u32 = 250; const MAX_LOCKS: u32 = 20; -const MAX_VOTERS: u32 = 50_000; -const MAX_CANDIDATES: u32 = 1000; +const MAX_VOTERS: u32 = 500; +const MAX_CANDIDATES: u32 = 100; type Lookup = <::Lookup as StaticLookup>::Source; From 85984d58506a75e870f73bec58c33451ec15a117 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 6 May 2020 12:51:36 +0200 Subject: [PATCH 13/30] New and improved version --- frame/elections-phragmen/src/benchmarking.rs | 194 +++++++++++++------ frame/elections-phragmen/src/lib.rs | 11 +- 2 files changed, 140 insertions(+), 65 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index c6ac94250d7cc..fd0a903f5a450 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -57,7 +57,9 @@ fn default_stake(factor: u32) -> BalanceOf { } /// Add `c` new candidates. -fn submit_candidates(c: u32, prefix: &'static str) -> Result, &'static str> { +fn submit_candidates(c: u32, prefix: &'static str) + -> Result, &'static str> +{ (0..c).map(|i| { let account = endowed_account::(prefix, i); >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) @@ -67,7 +69,9 @@ fn submit_candidates(c: u32, prefix: &'static str) -> Result(c: u32, prefix: &'static str) -> Result, &'static str> { +fn submit_candidates_with_self_vote(c: u32, prefix: &'static str) + -> Result, &'static str> +{ let candidates = submit_candidates::(c, prefix)?; let stake = default_stake::(BALANCE_FACTOR); let _ = candidates.iter().map(|c| @@ -95,6 +99,27 @@ fn add_locks(who: &T::AccountId, n: u8) { } } +/// create `num_voter` voters who randomly vote for at most `votes` of `all_candidates` if +/// available. +fn distribute_voters(mut all_candidates: Vec, num_voters: u32, votes: usize) + -> Result<(), &'static str> +{ + let stake = default_stake::(BALANCE_FACTOR); + let c = all_candidates.len() as u32; + for i in 0..num_voters { + // to ensure that votes are different, + all_candidates.rotate_left((i % c) as usize); + let votes = all_candidates + .iter() + .cloned() + .take(votes) + .collect::>(); + let voter = endowed_account::("voter", i); + submit_voter::(voter, votes, stake)?; + } + Ok(()) +} + /// Fill the seats of members and runners-up up until `m`. Note that this might include either only /// members, or members and runners-up. fn fill_seats_up_to(m: u32) -> Result, &'static str> { @@ -124,7 +149,7 @@ benchmarks! { // -- Signed ones vote { // range of candidates to vote for. - let x in 1 .. (MAXIMUM_VOTE as u32); + let v in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic adds a lock. let l in 1 .. MAX_LOCKS; clean::(); @@ -138,14 +163,14 @@ benchmarks! { let stake = default_stake::(BALANCE_FACTOR); // vote first `x` ones. - let votes = all_candidates.into_iter().take(x as usize).collect(); + let votes = all_candidates.into_iter().take(v as usize).collect(); }: _(RawOrigin::Signed(caller), votes, stake) remove_voter { // range of candidates that we have voted for. This may or may have a significant impact on // the outcome. - let c in 1 .. (MAXIMUM_VOTE as u32); + let v in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic removes a lock. let l in 1 .. MAX_LOCKS; clean::(); @@ -153,7 +178,7 @@ benchmarks! { let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; // create a bunch of candidates. - let all_candidates = submit_candidates::(c, "candidates")?; + let all_candidates = submit_candidates::(v, "candidates")?; let caller = endowed_account::("caller", 0); add_locks::(&caller, l as u8); @@ -164,11 +189,14 @@ benchmarks! { }: _(RawOrigin::Signed(caller)) report_defunct_voter_correct { - // number fo already existing members or runners + // number fo already existing members or runners. let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number of already existing candidates that may or may not be voted by the reported + // account. + let c in 1 .. MAX_CANDIDATES; // number of candidates that the reported voter voted for. This may or may not have any // impact on the outcome. - let c in 1 .. (MAXIMUM_VOTE as u32); + let v in 1 .. (MAXIMUM_VOTE as u32); clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -177,9 +205,11 @@ benchmarks! { let _ = fill_seats_up_to::(m)?; // create a bunch of candidates as well. - let all_candidates = submit_candidates::(c, "candidates")?; + let bailing_candidates = submit_candidates::(v, "bailing_candidates")?; + let all_candidates = submit_candidates::(c, "all_candidates")?; - // account 1 is the reporter and it doesn't matter how many it votes. + // account 1 is the reporter and it doesn't matter how many it votes. But it has to be a + // voter. let account_1 = endowed_account::("caller", 0); submit_voter::( account_1.clone(), @@ -191,14 +221,14 @@ benchmarks! { let account_2 = endowed_account::("caller_2", 1); submit_voter::( account_2.clone(), - all_candidates.clone(), + bailing_candidates.clone(), stake, )?; let account_2_lookup = as_lookup::(account_2.clone()); // all the bailers go away. - all_candidates.into_iter().for_each(|b| { + bailing_candidates.into_iter().for_each(|b| { assert!(>::renounce_candidacy(RawOrigin::Signed(b).into()).is_ok()); }); }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) @@ -214,13 +244,16 @@ benchmarks! { } report_defunct_voter_incorrect { - // number fo already existing members or runners - let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number fo already existing members or runners. + let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number of already existing candidates that may or may not be voted by the reported + // account. + let c in 1 .. MAX_CANDIDATES; // number of candidates that the reported voter voted for. This may or may not have any // impact on the outcome. - let c in 1 .. (MAXIMUM_VOTE as u32); - clean::(); + let v in 2 .. (MAXIMUM_VOTE as u32); + clean::(); let stake = default_stake::(BALANCE_FACTOR); // create m members and runners combined. @@ -237,11 +270,14 @@ benchmarks! { stake, )?; - // account 2 votes for all of the mentioned candidates. + // account 2 votes for a bunch of crap, and finally a correct candidate. let account_2 = endowed_account::("caller_2", 1); + let mut invalid: Vec = + (0..(v-1)).map(|seed| account::("invalid", 0, seed).clone()).collect(); + invalid.push(all_candidates.last().unwrap().clone()); submit_voter::( account_2.clone(), - all_candidates.clone(), + invalid, stake, )?; @@ -260,32 +296,45 @@ benchmarks! { } submit_candidacy { - // number fo already existing members or runners - let m in 0 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number fo already existing members or runners. Because candidates cannot be duplicate + // with members and previous candidates. + let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // number of already existing candidates. + let c in 1 .. MAX_CANDIDATES; + clean::(); let stake = default_stake::(BALANCE_FACTOR); // create m members and runners combined. let _ = fill_seats_up_to::(m)?; + // create previous candidates; + let _ = submit_candidates::(c, "candidates")?; + // we assume worse case that: extrinsic is successful and candidate is not duplicate. let candidate_account = endowed_account::("caller", 0); }: _(RawOrigin::Signed(candidate_account.clone())) + verify { + #[cfg(test)] + { + // reset members in between benchmark tests. + use crate::tests::MEMBERS; + MEMBERS.with(|m| *m.borrow_mut() = vec![]); + } + } renounce_candidacy_candidate { // this will check members, runners-up and candidate for removal. Members and runners-up are - // limited by the runtime bound, so we just fill them to maximum, while the number of - // candidates can be big. Note that we should be gentle with maximum candidates, as all - // candidates except for those who will make it in as a winner will lose their bond and get - // removed, so most likely we will not have too many candidates all at once. + // limited by the runtime bound, nonetheless we fill them by `m`. + // number of already existing members and runners-up. + let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; + clean::(); - let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // create m members and runners combined. let _ = fill_seats_up_to::(m)?; - let all_candidates = submit_candidates::(c, "caller")?; let bailing = all_candidates[0].clone(); // Should be ("caller", 0) @@ -300,10 +349,11 @@ benchmarks! { } renounce_candidacy_member_runner_up { - // removing members and runners will be cheaper most likely. The number of candidates will \ + // removing members and runners will be cheaper most likely. The number of candidates will // have no impact. // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; + // number of already existing members and runners-up. let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); @@ -326,22 +376,24 @@ benchmarks! { remove_member_without_replacement { // 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 0 .. MAX_CANDIDATES; + // new slot. + let c in 1 .. MAX_CANDIDATES; clean::(); - let stake = default_stake::(BALANCE_FACTOR); // 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 _replacement = submit_candidates_with_self_vote::(c, "new_candidate"); - let to_remove = all_members[0].clone(); + let replacements = submit_candidates_with_self_vote::(c, "new_candidate")?; - // NOTE: we don't add any voters here. This can be observed from the on_initialize bench. + // create some voters for these replacements. + distribute_voters::(replacements, MAX_VOTERS, MAXIMUM_VOTE)?; + + let to_remove = all_members[0].clone(); }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) verify { - // must still have 2 members. + // must still have the desired number of members members. assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); #[cfg(test)] { @@ -351,18 +403,19 @@ benchmarks! { } } - // -- Root ones remove_member_with_replacement { - // easy case. We have a runner up. - let x in 0 .. 1000; + // 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. + let m in + (T::DesiredMembers::get() + 1) + .. + T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); - let stake = default_stake::(BALANCE_FACTOR); - let all_members = fill_seats_up_to::(T::DesiredMembers::get() + 1)?; - let to_remove = all_members[0].clone(); - }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) - verify { - // must still have 2 members. + let _ = fill_seats_up_to::(m)?; + }: remove_member(RawOrigin::Root, as_lookup::(>::members_ids()[0].clone())) +verify { + // must still have enough members. assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); #[cfg(test)] { @@ -378,32 +431,47 @@ benchmarks! { // encoded in the runtime. let c in 1 .. MAX_CANDIDATES; clean::(); - let stake = default_stake::(BALANCE_FACTOR); - let mut all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; - - // create 50,000 voters voting for a set of at most 8 of candidates. - for i in 0..MAX_VOTERS { - let starting_index = i % c; - // to ensure that votes are different, - all_candidates.rotate_left((i % c) as usize); - let votes = all_candidates - .iter() - .cloned() - .take(MAXIMUM_VOTE) - .collect::>(); - let voter = endowed_account::("voter", i); - submit_voter::(voter, votes, stake)?; - } + // create c candidates. + let all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; + // create 500 voters, each voting the maximum 16 + distribute_voters::(all_candidates, MAX_VOTERS, MAXIMUM_VOTE)?; }: { // elect >::on_initialize(T::TermDuration::get()); } verify { + assert_eq!(>::members().len() as u32, T::DesiredMembers::get().min(c)); assert_eq!( - >::members().len() as u32, - T::DesiredMembers::get().min(c), + >::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![]); + } + } + + phragmen { + // This is just to focus on phragmen in the context of this module. We always select 20 + // 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 1 .. (MAXIMUM_VOTE as u32); + clean::(); + + let all_candidates = submit_candidates_with_self_vote::(c, "candidates")?; + let _ = distribute_voters::(all_candidates, v, e as usize)?; + }: { + >::on_initialize(T::TermDuration::get()); + } + verify { + assert_eq!(>::members().len() as u32, T::DesiredMembers::get().min(c)); assert_eq!( >::runners_up().len() as u32, T::DesiredRunnersUp::get().min(c.saturating_sub(T::DesiredMembers::get())), @@ -457,6 +525,7 @@ mod tests { ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_remove_member_without_replacement::()); }); + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { assert_ok!(test_benchmark_remove_member_with_replacement::()); }); @@ -465,5 +534,8 @@ mod tests { assert_ok!(test_benchmark_on_initialize::()); }); + ExtBuilder::default().desired_members(13).desired_runners_up(7).build_and_execute(|| { + assert_ok!(test_benchmark_phragmen::()); + }); } } diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 9acc35340b52f..69b6ce1d3558d 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -393,6 +393,7 @@ decl_module! { let is_candidate = Self::is_candidate(&who); ensure!(is_candidate.is_err(), Error::::DuplicatedCandidate); + // assured to be an error, error always contains the index. let index = is_candidate.unwrap_err(); @@ -528,7 +529,9 @@ decl_event!( ); impl Module { - /// Attempts to remove a member `who`. If a runner up exists, it is used as the replacement. + /// Attempts to remove a member `who`. If a runner-up exists, it is used as the replacement and + /// Ok(true). is returned. + /// /// Otherwise, `Ok(false)` is returned to signal the caller. /// /// In both cases, [`Members`], [`ElectionRounds`] and [`RunnersUp`] storage are updated @@ -567,7 +570,7 @@ impl Module { /// Check if `who` is a candidate. It returns the insert index if the element does not exists as /// an error. /// - /// State: O(LogN) given N candidates. + /// O(LogN) given N candidates. fn is_candidate(who: &T::AccountId) -> Result<(), usize> { Self::candidates().binary_search(who).map(|_| ()) } @@ -581,14 +584,14 @@ impl Module { /// Check if `who` is currently an active member. /// - /// Limited number of members. Binary search. Constant time factor. O(1) + /// O(LogN) given N members. Since members are limited, O(1). fn is_member(who: &T::AccountId) -> bool { Self::members().binary_search_by(|(a, _b)| a.cmp(who)).is_ok() } /// Check if `who` is currently an active runner. /// - /// Limited number of runners-up. Binary search. Constant time factor. O(1) + /// O(LogN) given N runners-up. Since runners-up are limited, O(1). fn is_runner(who: &T::AccountId) -> bool { Self::runners_up().iter().position(|(a, _b)| a == who).is_some() } From 023d1b34584f02561349832c9a1c6aca85f54bf0 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Tue, 12 May 2020 14:05:52 +0200 Subject: [PATCH 14/30] Make elections-phragmen weighable and secure. (#5949) * Make elections-phragmen weighable. * Update frame/elections-phragmen/src/lib.rs Co-authored-by: Alexander Popiak * Update frame/elections-phragmen/src/lib.rs Co-authored-by: Alexander Popiak * Fix all tests * Fix everything * Add note Co-authored-by: Alexander Popiak --- frame/elections-phragmen/src/benchmarking.rs | 50 +- frame/elections-phragmen/src/lib.rs | 653 ++++++++++++------- frame/support/src/lib.rs | 15 +- primitives/runtime/src/lib.rs | 22 +- 4 files changed, 496 insertions(+), 244 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index fd0a903f5a450..9762f6891d4a7 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -56,14 +56,27 @@ fn default_stake(factor: u32) -> BalanceOf { T::Currency::minimum_balance() * factor } +/// Get the current number of candidates +fn candidate_count() -> u32 { + >::decode_len().unwrap_or(0usize) as u32 +} + +/// Index of a candidate. Panics if index is not found. +fn index_of_candidate(c: &T::AccountId) -> u32 { + >::candidates().iter().position(|x| x == c) + .expect("Candidate does not exist") as u32 +} + /// Add `c` new candidates. fn submit_candidates(c: u32, prefix: &'static str) -> Result, &'static str> { (0..c).map(|i| { let account = endowed_account::(prefix, i); - >::submit_candidacy(RawOrigin::Signed(account.clone()).into()) - .map_err(|_| "failed to submit candidacy")?; + >::submit_candidacy( + RawOrigin::Signed(account.clone()).into(), + candidate_count::(), + ).map_err(|_| "failed to submit candidacy")?; Ok(account) }).collect::>() } @@ -229,9 +242,13 @@ benchmarks! { // all the bailers go away. bailing_candidates.into_iter().for_each(|b| { - assert!(>::renounce_candidacy(RawOrigin::Signed(b).into()).is_ok()); + let index = index_of_candidate::(&b); + assert!(>::renounce_candidacy( + RawOrigin::Signed(b).into(), + Renouncing::Candidate(index), + ).is_ok()); }); - }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup, candidate_count::()) verify { assert!(>::is_voter(&account_1)); assert!(!>::is_voter(&account_2)); @@ -283,7 +300,7 @@ benchmarks! { let account_2_lookup = as_lookup::(account_2.clone()); // no one bails out. account_1 is slashed and removed as voter now. - }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup) + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup, candidate_count::()) verify { assert!(!>::is_voter(&account_1)); assert!(>::is_voter(&account_2)); @@ -313,7 +330,7 @@ benchmarks! { // we assume worse case that: extrinsic is successful and candidate is not duplicate. let candidate_account = endowed_account::("caller", 0); - }: _(RawOrigin::Signed(candidate_account.clone())) + }: _(RawOrigin::Signed(candidate_account.clone()), candidate_count::()) verify { #[cfg(test)] { @@ -338,7 +355,8 @@ benchmarks! { let all_candidates = submit_candidates::(c, "caller")?; let bailing = all_candidates[0].clone(); // Should be ("caller", 0) - }: renounce_candidacy(RawOrigin::Signed(bailing.clone())) + let index = index_of_candidate::(&bailing); + }: renounce_candidacy(RawOrigin::Signed(bailing), Renouncing::Candidate(index)) verify { #[cfg(test)] { @@ -362,7 +380,14 @@ benchmarks! { let _ = submit_candidates::(c, "candidates")?; let bailing = members_and_runners_up[0].clone(); - }: renounce_candidacy(RawOrigin::Signed(bailing.clone())) + let renouncing = if >::is_member(&bailing) { + Renouncing::Member + } else if >::is_runner_up(&bailing) { + Renouncing::RunnerUp + } else { + panic!("Bailing must be a member or runner-up for this bench to be sane."); + }; + }: renounce_candidacy(RawOrigin::Signed(bailing.clone()), renouncing) verify { #[cfg(test)] { @@ -390,8 +415,8 @@ benchmarks! { // create some voters for these replacements. distribute_voters::(replacements, MAX_VOTERS, MAXIMUM_VOTE)?; - let to_remove = all_members[0].clone(); - }: remove_member(RawOrigin::Root, as_lookup::(to_remove)) + 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()); @@ -413,8 +438,9 @@ benchmarks! { clean::(); let _ = fill_seats_up_to::(m)?; - }: remove_member(RawOrigin::Root, as_lookup::(>::members_ids()[0].clone())) -verify { + let removing = as_lookup::(>::members_ids()[0].clone()); + }: remove_member(RawOrigin::Root, removing, true) + verify { // must still have enough members. assert_eq!(>::members().len() as u32, T::DesiredMembers::get()); #[cfg(test)] diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index fd530e0d9b81d..a3c673fd0a224 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -82,14 +82,17 @@ #![cfg_attr(not(feature = "std"), no_std)] +use codec::{Encode, Decode}; use sp_std::prelude::*; use sp_runtime::{ - print, DispatchResult, DispatchError, Perbill, traits::{Zero, StaticLookup, Convert}, + print, DispatchResult, DispatchError, RuntimeDebug, Perbill, + traits::{Zero, StaticLookup, Convert}, }; use frame_support::{ decl_storage, decl_event, ensure, decl_module, decl_error, weights::{Weight, DispatchClass}, storage::{StorageMap, IterableStorageMap}, + dispatch::{DispatchResultWithPostInfo, WithPostDispatchInfo}, traits::{ Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons, ChangeMembers, OnUnbalanced, WithdrawReason, Contains, BalanceStatus, InitializeMembers, @@ -104,10 +107,22 @@ mod benchmarking; /// The maximum votes allowed per voter. pub const MAXIMUM_VOTE: usize = 16; -type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; +type BalanceOf = + <::Currency as Currency<::AccountId>>::Balance; type NegativeImbalanceOf = <::Currency as Currency<::AccountId>>::NegativeImbalance; +/// An indication that the renouncing account currently has which of the below roles. +#[derive(Encode, Decode, Clone, PartialEq, RuntimeDebug)] +pub enum Renouncing { + /// A member is renouncing. + Member, + /// A runner-up is renouncing. + RunnerUp, + /// A candidate is renouncing. + Candidate(#[codec(compact)] u32), +} + pub trait Trait: frame_system::Trait { /// The overarching event type.c type Event: From> + Into<::Event>; @@ -240,10 +255,14 @@ decl_error! { RunnerSubmit, /// Candidate does not have enough funds. InsufficientCandidateFunds, - /// Origin is not a candidate, member or a runner up. - InvalidOrigin, /// Not a member. NotMember, + /// The provided count of number of candidates is incorrect. + InvalidCandidateCount, + /// The renouncing origin presented a wrong `Renouncing` parameter. + InvalidRenouncing, + /// Prediction regarding replacement after member removal is wrong. + InvalidReplacement, } } @@ -339,17 +358,32 @@ decl_module! { /// - a voter whose current submitted votes are all invalid. i.e. all of them are no /// longer a candidate nor an active member or a runner-up. /// + /// + /// The origin must provide the number of current candidates for the purpose of accurate + /// weight calculation. + /// /// # /// #### State /// Reads: O(NLogM) given M current candidates and N votes for `target`. /// Writes: O(1) /// # #[weight = 1_000_000_000] - fn report_defunct_voter(origin, target: ::Source) { + fn report_defunct_voter( + origin, + target: ::Source, + #[compact] candidate_count: u32, + ) { let reporter = ensure_signed(origin)?; let target = T::Lookup::lookup(target)?; ensure!(reporter != target, Error::::ReportSelf); + + let actual_count = >::decode_len().unwrap_or(0); + // TODO: refund based on actual is less than predicted. + ensure!( + actual_count as u32 <= candidate_count, + Error::::InvalidCandidateCount, + ); ensure!(Self::is_voter(&reporter), Error::::MustBeVoter); // Checking if someone is a candidate and a member here is O(LogN), making the whole @@ -373,7 +407,6 @@ decl_module! { Self::deposit_event(RawEvent::VoterReported(target, reporter, valid)); } - /// Submit oneself for candidacy. /// /// A candidate will either: @@ -388,9 +421,16 @@ decl_module! { /// Writes: O(1) /// # #[weight = 500_000_000] - fn submit_candidacy(origin) { + fn submit_candidacy(origin, #[compact] candidate_count: u32) { let who = ensure_signed(origin)?; + let actual_count = >::decode_len().unwrap_or(0); + // TODO: refund based on actual is less than predicted. + ensure!( + actual_count as u32 <= candidate_count, + Error::::InvalidCandidateCount, + ); + let is_candidate = Self::is_candidate(&who); ensure!(is_candidate.is_err(), Error::::DuplicatedCandidate); @@ -398,7 +438,7 @@ decl_module! { let index = is_candidate.unwrap_err(); ensure!(!Self::is_member(&who), Error::::MemberSubmit); - ensure!(!Self::is_runner(&who), Error::::RunnerSubmit); + ensure!(!Self::is_runner_up(&who), Error::::RunnerSubmit); T::Currency::reserve(&who, T::CandidacyBond::get()) .map_err(|_| Error::::InsufficientCandidateFunds)?; @@ -410,57 +450,52 @@ decl_module! { /// outcomes exist: /// - `origin` is a candidate and not elected in any set. In this case, the bond is /// unreserved, returned and origin is removed as a candidate. - /// - `origin` is a current runner up. In this case, the bond is unreserved, returned and - /// origin is removed as a runner. + /// - `origin` is a current runner-up. In this case, the bond is unreserved, returned and + /// origin is removed as a runner-up. /// - `origin` is a current member. In this case, the bond is unreserved and origin is /// removed as a member, consequently not being a candidate for the next round anymore. /// Similar to [`remove_voter`], if replacement runners exists, they are immediately used. #[weight = (2_000_000_000, DispatchClass::Operational)] - fn renounce_candidacy(origin) { + fn renounce_candidacy(origin, renouncing: Renouncing) { let who = ensure_signed(origin)?; - - // NOTE: this function attempts the 3 conditions (being a candidate, member, runner) and - // fails if none are matched. Unlike other Palette functions and modules where checks - // happen first and then execution happens, this function is written the other way - // around. The main intention is that reading all of the candidates, members and runners - // from storage is expensive. Furthermore, we know (soft proof) that they are always - // mutually exclusive. Hence, we try one, and only then decode more storage. - - if let Ok(_replacement) = Self::remove_and_replace_member(&who) { - T::Currency::unreserve(&who, T::CandidacyBond::get()); - Self::deposit_event(RawEvent::MemberRenounced(who.clone())); - - // safety guard to make sure we do only one arm. Better to read runners later. - return Ok(()); - } - - let mut runners_up_with_stake = Self::runners_up(); - // runners-up are NOT sorted based on account id. Linear search. - if let Some(index) = runners_up_with_stake.iter() - .position(|(ref r, ref _s)| r == &who) - { - runners_up_with_stake.remove(index); - // unreserve the bond - T::Currency::unreserve(&who, T::CandidacyBond::get()); - // update storage. - >::put(runners_up_with_stake); - - // safety guard to make sure we do only one arm. Better to read candidates later. - return Ok(()); - } - - let mut candidates = Self::candidates(); - if let Ok(index) = candidates.binary_search(&who) { - candidates.remove(index); - // unreserve the bond - T::Currency::unreserve(&who, T::CandidacyBond::get()); - // update storage. - >::put(candidates); - - return Ok(()); - } - - Err(Error::::InvalidOrigin)? + match renouncing { + Renouncing::Member => { + // returns NoMember error in case of error. + let _ = Self::remove_and_replace_member(&who)?; + T::Currency::unreserve(&who, T::CandidacyBond::get()); + Self::deposit_event(RawEvent::MemberRenounced(who.clone())); + }, + Renouncing::RunnerUp => { + let mut runners_up_with_stake = Self::runners_up(); + if let Some(index) = runners_up_with_stake + .iter() + .position(|(ref r, ref _s)| r == &who) + { + runners_up_with_stake.remove(index); + // unreserve the bond + T::Currency::unreserve(&who, T::CandidacyBond::get()); + // update storage. + >::put(runners_up_with_stake); + } else { + Err(Error::::InvalidRenouncing)?; + } + } + Renouncing::Candidate(index) => { + // TODO: probably need the length of candidates here as well. + let mut candidates = Self::candidates(); + let index = index as usize; + if let Some(c) = candidates.get(index) { + ensure!(*c == who, Error::::InvalidRenouncing); + candidates.remove(index); + // unreserve the bond + T::Currency::unreserve(&who, T::CandidacyBond::get()); + // update storage. + >::put(candidates); + } else { + Err(Error::::InvalidRenouncing)?; + } + } + }; } /// Remove a particular member from the set. This is effective immediately and the bond of @@ -477,19 +512,46 @@ decl_module! { /// Writes: O(do_phragmen) /// # #[weight = (2_000_000_000, DispatchClass::Operational)] - fn remove_member(origin, who: ::Source) -> DispatchResult { + fn remove_member( + origin, + who: ::Source, + has_replacement: bool, + ) -> DispatchResultWithPostInfo { ensure_root(origin)?; let who = T::Lookup::lookup(who)?; + let will_have_replacement = >::decode_len().unwrap_or(0) > 0; + match (will_have_replacement, has_replacement) { + (true, true) | (false, false) => { + // correct prediction. nothing + } + (false, true) => { + // prediction was that we will have a replacement, so we don't charge a whole + // lot of weight. just abort. + return Err(Error::::InvalidReplacement.into()); + } + (true, false) => { + // prediction was that we will NOT have a replacement, and now this call is + // aborting whilst charging a metric ton of weight. Refund and abort. + // TODO: fix refund. + return Err(Error::::InvalidReplacement.with_weight(2_000_000_000).into()); + } + } + + Self::remove_and_replace_member(&who).map(|had_replacement| { let (imbalance, _) = T::Currency::slash_reserved(&who, T::CandidacyBond::get()); T::KickedMember::on_unbalanced(imbalance); Self::deposit_event(RawEvent::MemberKicked(who.clone())); if !had_replacement { + // if we end up here, we will charge a full block weight. Self::do_phragmen(); } - }) + + // no refund needed. + None.into() + }).map_err(|e| e.into()) } /// What to do at the end of each block. Checks if an election needs to happen or not. @@ -534,10 +596,12 @@ impl Module { /// /// Otherwise, `Ok(false)` is returned to signal the caller. /// - /// In both cases, [`Members`], [`ElectionRounds`] and [`RunnersUp`] storage are updated - /// accordingly. Furthermore, the membership change is reported. + /// If a replacement exists, `Members` and `RunnersUp` storage is updated, where the first + /// element of `RunnersUp` is used as the replacement and `Ok(true)` is returned. Else, + /// `Ok(false)` is returned with no storage updated. /// - /// O(phragmen) in the worse case. + /// Note that this function _will_ call into `T::ChangeMembers` in case any change happens + /// (`Ok(true)`). fn remove_and_replace_member(who: &T::AccountId) -> Result { let mut members_with_stake = Self::members(); if let Ok(index) = members_with_stake.binary_search_by(|(ref m, ref _s)| m.cmp(who)) { @@ -589,10 +653,10 @@ impl Module { Self::members().binary_search_by(|(a, _b)| a.cmp(who)).is_ok() } - /// Check if `who` is currently an active runner. + /// Check if `who` is currently an active runner-up. /// /// O(LogN) given N runners-up. Since runners-up are limited, O(1). - fn is_runner(who: &T::AccountId) -> bool { + fn is_runner_up(who: &T::AccountId) -> bool { Self::runners_up().iter().position(|(a, _b)| a == who).is_some() } @@ -631,7 +695,7 @@ impl Module { Self::votes_of(who) .iter() .all(|v| - !Self::is_member(v) && !Self::is_runner(v) && !Self::is_candidate(v).is_ok() + !Self::is_member(v) && !Self::is_runner_up(v) && !Self::is_candidate(v).is_ok() ) } else { false @@ -891,7 +955,9 @@ impl ContainsLengthBound for Module { mod tests { use super::*; use std::cell::RefCell; - use frame_support::{assert_ok, assert_noop, parameter_types, weights::Weight}; + use frame_support::{assert_ok, assert_noop, assert_err_ignore_postinfo, parameter_types, + weights::Weight, + }; use substrate_test_utils::assert_eq_uvec; use sp_core::H256; use sp_runtime::{ @@ -1206,6 +1272,10 @@ mod tests { ensure_members_has_approval_stake(); } + fn submit_candidacy(origin: Origin) -> DispatchResult { + Elections::submit_candidacy(origin, Elections::candidates().len() as u32) + } + #[test] fn params_should_work() { ExtBuilder::default().build_and_execute(|| { @@ -1317,7 +1387,7 @@ mod tests { assert!(Elections::is_candidate(&2).is_err()); assert_eq!(balances(&1), (10, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(1))); + assert_ok!(submit_candidacy(Origin::signed(1))); assert_eq!(balances(&1), (7, 3)); assert_eq!(Elections::candidates(), vec![1]); @@ -1326,7 +1396,7 @@ mod tests { assert!(Elections::is_candidate(&2).is_err()); assert_eq!(balances(&2), (20, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_eq!(balances(&2), (17, 3)); assert_eq!(Elections::candidates(), vec![1, 2]); @@ -1341,8 +1411,8 @@ mod tests { ExtBuilder::default().build_and_execute(|| { assert_eq!(Elections::candidates(), Vec::::new()); - assert_ok!(Elections::submit_candidacy(Origin::signed(1))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(1))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert!(Elections::is_candidate(&1).is_ok()); assert!(Elections::is_candidate(&2).is_ok()); @@ -1367,10 +1437,10 @@ mod tests { fn dupe_candidate_submission_should_not_work() { ExtBuilder::default().build_and_execute(|| { assert_eq!(Elections::candidates(), Vec::::new()); - assert_ok!(Elections::submit_candidacy(Origin::signed(1))); + assert_ok!(submit_candidacy(Origin::signed(1))); assert_eq!(Elections::candidates(), vec![1]); assert_noop!( - Elections::submit_candidacy(Origin::signed(1)), + submit_candidacy(Origin::signed(1)), Error::::DuplicatedCandidate, ); }); @@ -1380,7 +1450,7 @@ mod tests { fn member_candidacy_submission_should_not_work() { // critically important to make sure that outgoing candidates and losers are not mixed up. ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); System::set_block_number(5); @@ -1391,7 +1461,7 @@ mod tests { assert_eq!(Elections::candidates(), vec![]); assert_noop!( - Elections::submit_candidacy(Origin::signed(5)), + submit_candidacy(Origin::signed(5)), Error::::MemberSubmit, ); }); @@ -1400,9 +1470,9 @@ mod tests { #[test] fn runner_candidate_submission_should_not_work() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(2), vec![5, 4], 20)); assert_ok!(Elections::vote(Origin::signed(1), vec![3], 10)); @@ -1414,7 +1484,7 @@ mod tests { assert_eq!(Elections::runners_up_ids(), vec![3]); assert_noop!( - Elections::submit_candidacy(Origin::signed(3)), + submit_candidacy(Origin::signed(3)), Error::::RunnerSubmit, ); }); @@ -1425,7 +1495,7 @@ mod tests { ExtBuilder::default().build_and_execute(|| { assert_eq!(Elections::candidates(), Vec::::new()); assert_noop!( - Elections::submit_candidacy(Origin::signed(7)), + submit_candidacy(Origin::signed(7)), Error::::InsufficientCandidateFunds, ); }); @@ -1437,7 +1507,7 @@ mod tests { assert_eq!(Elections::candidates(), Vec::::new()); assert_eq!(balances(&2), (20, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_eq!(balances(&2), (18, 2)); @@ -1451,7 +1521,7 @@ mod tests { assert_eq!(Elections::candidates(), Vec::::new()); assert_eq!(balances(&2), (20, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 12)); assert_eq!(balances(&2), (18, 2)); @@ -1464,8 +1534,8 @@ mod tests { ExtBuilder::default().build_and_execute(|| { assert_eq!(balances(&2), (20, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_eq!(balances(&2), (18, 2)); @@ -1493,8 +1563,8 @@ mod tests { #[test] fn can_vote_for_old_members_even_when_no_new_candidates() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 20)); @@ -1511,9 +1581,9 @@ mod tests { #[test] fn prime_works() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(1), vec![4, 3], 10)); assert_ok!(Elections::vote(Origin::signed(2), vec![4], 20)); @@ -1535,9 +1605,9 @@ mod tests { #[test] fn prime_votes_for_exiting_members_are_removed() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(1), vec![4, 3], 10)); assert_ok!(Elections::vote(Origin::signed(2), vec![4], 20)); @@ -1545,7 +1615,7 @@ mod tests { assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::renounce_candidacy(Origin::signed(4))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(1))); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1565,9 +1635,9 @@ mod tests { .build_and_execute( || { // when we have only candidates - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_noop!( // content of the vote is irrelevant. @@ -1583,7 +1653,7 @@ mod tests { assert_ok!(Elections::end_block(System::block_number())); // now we have 2 members, 1 runner-up, and 1 new candidate - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(1), vec![9, 99, 999, 9999], 5)); assert_noop!( @@ -1596,8 +1666,8 @@ mod tests { #[test] fn cannot_vote_for_less_than_ed() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_noop!( Elections::vote(Origin::signed(2), vec![4], 1), @@ -1609,8 +1679,8 @@ mod tests { #[test] fn can_vote_for_more_than_total_balance_but_moot() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 30)); // you can lie but won't get away with it. @@ -1622,7 +1692,7 @@ mod tests { #[test] fn remove_voter_should_work() { ExtBuilder::default().voter_bond(8).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![5], 30)); @@ -1654,7 +1724,7 @@ mod tests { #[test] fn dupe_remove_should_fail() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_ok!(Elections::remove_voter(Origin::signed(2))); @@ -1667,9 +1737,9 @@ mod tests { #[test] fn removed_voter_should_not_be_counted() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -1688,18 +1758,54 @@ mod tests { fn reporter_must_be_voter() { ExtBuilder::default().build_and_execute(|| { assert_noop!( - Elections::report_defunct_voter(Origin::signed(1), 2), + Elections::report_defunct_voter(Origin::signed(1), 2, 0), Error::::MustBeVoter, ); }); } + #[test] + fn reporter_must_provide_length() { + ExtBuilder::default().build_and_execute(|| { + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + + // both are defunct. + assert_ok!(Elections::vote(Origin::signed(5), vec![99], 50)); + assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); + + // 2 candidates! incorrect candidate length. + assert_noop!( + Elections::report_defunct_voter(Origin::signed(4), 5, 1), + Error::::InvalidCandidateCount, + ); + + // correct candidate length. + assert_ok!(Elections::report_defunct_voter(Origin::signed(4), 5, 2)); + }); + } + + #[test] + fn reporter_can_overestimate_length() { + ExtBuilder::default().build_and_execute(|| { + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + + // both are defunct. + assert_ok!(Elections::vote(Origin::signed(5), vec![99], 50)); + assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); + + // 2 candidates! overestimation is okay. + assert_ok!(Elections::report_defunct_voter(Origin::signed(4), 5, 3)); + }); + } + #[test] fn can_detect_defunct_voter() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(6))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(6))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -1724,7 +1830,7 @@ mod tests { // defunct assert_eq!(Elections::is_defunct_voter(&3), true); - assert_ok!(Elections::submit_candidacy(Origin::signed(1))); + assert_ok!(submit_candidacy(Origin::signed(1))); assert_ok!(Elections::vote(Origin::signed(1), vec![1], 10)); // has a candidate voted for. @@ -1736,8 +1842,8 @@ mod tests { #[test] fn report_voter_should_work_and_earn_reward() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -1754,7 +1860,7 @@ mod tests { assert_eq!(balances(&3), (28, 2)); assert_eq!(balances(&5), (45, 5)); - assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 3)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 3, 0)); assert!(System::events().iter().any(|event| { event.event == Event::elections_phragmen(RawEvent::VoterReported(3, 5, true)) })); @@ -1767,8 +1873,8 @@ mod tests { #[test] fn report_voter_should_slash_when_bad_report() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -1782,7 +1888,7 @@ mod tests { assert_eq!(balances(&4), (35, 5)); assert_eq!(balances(&5), (45, 5)); - assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 4)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 4, 0)); assert!(System::events().iter().any(|event| { event.event == Event::elections_phragmen(RawEvent::VoterReported(4, 5, false)) })); @@ -1795,9 +1901,9 @@ mod tests { #[test] fn simple_voting_rounds_should_work() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 15)); @@ -1830,7 +1936,7 @@ mod tests { #[test] fn defunct_voter_will_be_counted() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); // This guy's vote is pointless for this round. assert_ok!(Elections::vote(Origin::signed(3), vec![4], 30)); @@ -1843,7 +1949,7 @@ mod tests { assert_eq!(Elections::election_rounds(), 1); // but now it has a valid target. - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(4))); System::set_block_number(10); assert_ok!(Elections::end_block(System::block_number())); @@ -1858,10 +1964,10 @@ mod tests { #[test] fn only_desired_seats_are_chosen() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); @@ -1879,8 +1985,8 @@ mod tests { #[test] fn phragmen_should_not_self_vote() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1899,10 +2005,10 @@ mod tests { #[test] fn runners_up_should_be_kept() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![2], 30)); @@ -1926,10 +2032,10 @@ mod tests { #[test] fn runners_up_should_be_next_candidates() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); @@ -1953,9 +2059,9 @@ mod tests { #[test] fn runners_up_lose_bond_once_outgoing() { ExtBuilder::default().desired_runners_up(1).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -1967,7 +2073,7 @@ mod tests { assert_eq!(Elections::runners_up_ids(), vec![2]); assert_eq!(balances(&2), (15, 5)); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); System::set_block_number(10); @@ -1983,7 +2089,7 @@ mod tests { ExtBuilder::default().build_and_execute(|| { assert_eq!(balances(&5), (50, 0)); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_eq!(balances(&5), (47, 3)); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); @@ -2007,8 +2113,8 @@ mod tests { #[test] fn losers_will_lose_the_bond() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); @@ -2030,8 +2136,8 @@ mod tests { #[test] fn current_members_are_always_next_candidate() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); @@ -2042,10 +2148,10 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::election_rounds(), 1); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); assert_ok!(Elections::remove_voter(Origin::signed(4))); @@ -2066,10 +2172,10 @@ mod tests { // what I mean by uninterrupted: // given no input or stimulants the same members are re-elected. ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -2099,8 +2205,8 @@ mod tests { #[test] fn remove_members_triggers_election() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); @@ -2111,10 +2217,10 @@ mod tests { assert_eq!(Elections::election_rounds(), 1); // a new candidate - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::remove_member(Origin::ROOT, 4)); + assert_ok!(Elections::remove_member(Origin::ROOT, 4, false)); assert_eq!(balances(&4), (35, 2)); // slashed assert_eq!(Elections::election_rounds(), 2); // new election round @@ -2122,12 +2228,54 @@ 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!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + + System::set_block_number(5); + assert_ok!(Elections::end_block(System::block_number())); + assert_eq!(Elections::members_ids(), vec![4, 5]); + + // no replacement yet. + assert_noop!( + Elections::remove_member(Origin::ROOT, 4, true), + Error::::InvalidReplacement, + ); + }); + + 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!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + + System::set_block_number(5); + assert_ok!(Elections::end_block(System::block_number())); + assert_eq!(Elections::members_ids(), vec![4, 5]); + assert_eq!(Elections::runners_up_ids(), vec![3]); + + // there is a replacement! and this one needs a weight refund. + assert_err_ignore_postinfo!( + Elections::remove_member(Origin::ROOT, 4, false), + Error::::InvalidReplacement, + ); + }); + } + #[test] fn seats_should_be_released_when_no_vote() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); @@ -2159,8 +2307,8 @@ mod tests { #[test] fn incoming_outgoing_are_reported() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); @@ -2169,9 +2317,9 @@ mod tests { assert_ok!(Elections::end_block(System::block_number())); assert_eq!(Elections::members_ids(), vec![4, 5]); - assert_ok!(Elections::submit_candidacy(Origin::signed(1))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(1))); + assert_ok!(submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(3))); // 5 will change their vote and becomes an `outgoing` assert_ok!(Elections::vote(Origin::signed(5), vec![4], 8)); @@ -2206,8 +2354,8 @@ mod tests { #[test] fn invalid_votes_are_moot() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -2224,10 +2372,10 @@ mod tests { #[test] fn members_are_sorted_based_on_id_runners_on_merit() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); assert_ok!(Elections::vote(Origin::signed(3), vec![2], 30)); @@ -2246,14 +2394,14 @@ mod tests { #[test] fn candidates_are_sorted() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(3))); assert_eq!(Elections::candidates(), vec![3, 5]); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::renounce_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(3), Renouncing::Candidate(1))); assert_eq!(Elections::candidates(), vec![2, 4, 5]); }) @@ -2262,9 +2410,9 @@ mod tests { #[test] fn runner_up_replacement_maintains_members_order() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -2274,39 +2422,18 @@ mod tests { assert_ok!(Elections::end_block(System::block_number())); assert_eq!(Elections::members_ids(), vec![2, 4]); - assert_ok!(Elections::remove_member(Origin::ROOT, 2)); + assert_ok!(Elections::remove_member(Origin::ROOT, 2, true)); assert_eq!(Elections::members_ids(), vec![4, 5]); }); } - #[test] - fn runner_up_replacement_works_when_out_of_order() { - ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); - - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![2], 50)); - - System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); - - assert_eq!(Elections::members_ids(), vec![2, 4]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(3))); - }); - } - #[test] fn can_renounce_candidacy_member_with_runners_bond_is_refunded() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -2319,7 +2446,7 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![2, 3]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(4))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Member)); assert_eq!(balances(&4), (38, 2)); // 2 is voting bond. assert_eq!(Elections::members_ids(), vec![3, 5]); @@ -2330,8 +2457,8 @@ mod tests { #[test] fn can_renounce_candidacy_member_without_runners_bond_is_refunded() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); @@ -2339,33 +2466,25 @@ mod tests { System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![]); - assert_eq!(Elections::candidates(), vec![2, 3]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(4))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Member)); assert_eq!(balances(&4), (38, 2)); // 2 is voting bond. // no replacement assert_eq!(Elections::members_ids(), vec![5]); assert_eq!(Elections::runners_up_ids(), vec![]); - // still candidate - assert_eq!(Elections::candidates(), vec![2, 3]); }) } #[test] fn can_renounce_candidacy_runner() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::submit_candidacy(Origin::signed(2))); + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(Elections::vote(Origin::signed(5), vec![4], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); @@ -2378,7 +2497,7 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![2, 3]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(3))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(3), Renouncing::RunnerUp)); assert_eq!(balances(&3), (28, 2)); // 2 is voting bond. assert_eq!(Elections::members_ids(), vec![4, 5]); @@ -2386,14 +2505,38 @@ mod tests { }) } + // #[test] + // fn runner_up_replacement_works_when_out_of_order() { + // ExtBuilder::default().desired_runners_up(2).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!(submit_candidacy(Origin::signed(2))); + + // assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); + // assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + // assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + // assert_ok!(Elections::vote(Origin::signed(5), vec![2], 50)); + + // System::set_block_number(5); + // assert_ok!(Elections::end_block(System::block_number())); + + // assert_eq!(Elections::members_ids(), vec![2, 4]); + // assert_eq!(ELections::runners_up_ids(), vec![3, 5]); + // assert_ok!(Elections::renounce_candidacy(Origin::signed(3), Renouncing::RunnerUp)); + // assert_eq!(Elections::members_ids(), vec![2, 4]); + // assert_eq!(ELections::runners_up_ids(), vec![5]); + // }); + // } + #[test] fn can_renounce_candidacy_candidate() { ExtBuilder::default().build_and_execute(|| { - assert_ok!(Elections::submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(5))); assert_eq!(balances(&5), (47, 3)); assert_eq!(Elections::candidates(), vec![5]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(5))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(5), Renouncing::Candidate(0))); assert_eq!(balances(&5), (50, 0)); assert_eq!(Elections::candidates(), vec![]); }) @@ -2403,8 +2546,78 @@ mod tests { fn wrong_renounce_candidacy_should_fail() { ExtBuilder::default().build_and_execute(|| { assert_noop!( - Elections::renounce_candidacy(Origin::signed(5)), - Error::::InvalidOrigin, + Elections::renounce_candidacy(Origin::signed(5), Renouncing::Candidate(0)), + Error::::InvalidRenouncing, + ); + assert_noop!( + Elections::renounce_candidacy(Origin::signed(5), Renouncing::Member), + Error::::NotMember, + ); + assert_noop!( + Elections::renounce_candidacy(Origin::signed(5), Renouncing::RunnerUp), + Error::::InvalidRenouncing, + ); + }) + } + + #[test] + fn non_member_renounce_member_should_fail() { + 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!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + + System::set_block_number(5); + assert_ok!(Elections::end_block(System::block_number())); + + assert_eq!(Elections::members_ids(), vec![4, 5]); + assert_eq!(Elections::runners_up_ids(), vec![3]); + + assert_noop!( + Elections::renounce_candidacy(Origin::signed(3), Renouncing::Member), + Error::::NotMember, + ); + }) + } + + #[test] + fn non_runner_up_renounce_runner_up_should_fail() { + 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!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + + System::set_block_number(5); + assert_ok!(Elections::end_block(System::block_number())); + + assert_eq!(Elections::members_ids(), vec![4, 5]); + assert_eq!(Elections::runners_up_ids(), vec![3]); + + assert_noop!( + Elections::renounce_candidacy(Origin::signed(4), Renouncing::RunnerUp), + Error::::InvalidRenouncing, + ); + }) + } + + #[test] + fn wrong_candidate_index_renounce_should_fail() { + ExtBuilder::default().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_noop!( + Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(0)), + Error::::InvalidRenouncing, ); }) } diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 6bad5985abcb6..08c70950409e9 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -213,7 +213,20 @@ macro_rules! assert_err { #[cfg(feature = "std")] macro_rules! assert_err_ignore_postinfo { ( $x:expr , $y:expr $(,)? ) => { - assert_err!($x.map(|_| ()).map_err(|e| e.error), $y); + $crate::assert_err!($x.map(|_| ()).map_err(|e| e.error), $y); + } +} + +#[macro_export] +#[cfg(feature = "std")] +macro_rules! assert_err_with_weight { + ($call:expr, $err:expr, $weight:expr $(,)? ) => { + if let Err(dispatch_err_with_post) = $call { + $crate::assert_err!($call.map(|_| ()).map_err(|e| e.error), $err); + assert_eq!(dispatch_err_with_post.post_info, $weight.into()); + } else { + panic!("expected Err(_), got Ok(_).") + } } } diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index b55edbd88d7f1..a71d52c826fef 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -373,16 +373,16 @@ impl From for DispatchOutcome { } } -/// This is the legacy return type of `Dispatchable`. It is still exposed for compatibilty -/// reasons. The new return type is `DispatchResultWithInfo`. -/// FRAME runtimes should use frame_support::dispatch::DispatchResult +/// This is the legacy return type of `Dispatchable`. It is still exposed for compatibility reasons. +/// The new return type is `DispatchResultWithInfo`. FRAME runtimes should use +/// `frame_support::dispatch::DispatchResult`. pub type DispatchResult = sp_std::result::Result<(), DispatchError>; /// Return type of a `Dispatchable` which contains the `DispatchResult` and additional information /// about the `Dispatchable` that is only known post dispatch. pub type DispatchResultWithInfo = sp_std::result::Result>; -/// Reason why a dispatch call failed +/// Reason why a dispatch call failed. #[derive(Eq, PartialEq, Clone, Copy, Encode, Decode, RuntimeDebug)] #[cfg_attr(feature = "std", derive(Serialize))] pub enum DispatchError { @@ -392,11 +392,11 @@ pub enum DispatchError { CannotLookup, /// A bad origin. BadOrigin, - /// A custom error in a module + /// A custom error in a module. Module { - /// Module index, matching the metadata module index + /// Module index, matching the metadata module index. index: u8, - /// Module specific error value + /// Module specific error value. error: u8, /// Optional error message. #[codec(skip)] @@ -404,15 +404,15 @@ pub enum DispatchError { }, } -/// Result of a `Dispatchable` which contains the `DispatchResult` and additional information -/// about the `Dispatchable` that is only known post dispatch. +/// Result of a `Dispatchable` which contains the `DispatchResult` and additional information about +/// the `Dispatchable` that is only known post dispatch. #[derive(Eq, PartialEq, Clone, Copy, Encode, Decode, RuntimeDebug)] pub struct DispatchErrorWithPostInfo where Info: Eq + PartialEq + Clone + Copy + Encode + Decode + traits::Printable { - /// Addditional information about the `Dispatchable` which is only known post dispatch. + /// Additional information about the `Dispatchable` which is only known post dispatch. pub post_info: Info, - /// The actual `DispatchResult` indicating whether the dispatch was succesfull. + /// The actual `DispatchResult` indicating whether the dispatch was successful. pub error: DispatchError, } From f4bd4945bc706b78808d6071b0337891ef722ecf Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 12 May 2020 15:04:41 +0200 Subject: [PATCH 15/30] Doc update --- frame/elections-phragmen/src/benchmarking.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index fd0a903f5a450..3cc9c6e33523f 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -194,8 +194,8 @@ benchmarks! { // number of already existing candidates that may or may not be voted by the reported // account. let c in 1 .. MAX_CANDIDATES; - // number of candidates that the reported voter voted for. This may or may not have any - // impact on the outcome. + // number of candidates that the reported voter voted for. The worse case of search here is + // basically `c * v`. let v in 1 .. (MAXIMUM_VOTE as u32); clean::(); @@ -249,8 +249,8 @@ benchmarks! { // number of already existing candidates that may or may not be voted by the reported // account. let c in 1 .. MAX_CANDIDATES; - // number of candidates that the reported voter voted for. This may or may not have any - // impact on the outcome. + // number of candidates that the reported voter voted for. The worse case of search here is + // basically `c * v`. let v in 2 .. (MAXIMUM_VOTE as u32); clean::(); From 2db6966cb5b7cf03e45a22a90d5ec58af0cd4407 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 12 May 2020 16:31:41 +0200 Subject: [PATCH 16/30] Fix some complexity params --- frame/elections-phragmen/src/benchmarking.rs | 64 ++++++++++------ frame/elections-phragmen/src/lib.rs | 81 +++++++++++++++----- 2 files changed, 100 insertions(+), 45 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index c349ff57d55b7..9880192074739 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -56,11 +56,25 @@ fn default_stake(factor: u32) -> BalanceOf { T::Currency::minimum_balance() * factor } -/// Get the current number of candidates +/// Get the current number of candidates. fn candidate_count() -> u32 { >::decode_len().unwrap_or(0usize) as u32 } +/// Get the number of voter of a voter. +fn vote_count_of(who: &T::AccountId) -> u32 { + >::get(who).1.len() as u32 +} + +/// A `DefunctVoter` struct with correct value +fn defunct_for(who: T::AccountId) -> DefunctVoter> { + DefunctVoter { + who: as_lookup::(who.clone()), + candidate_count: candidate_count::(), + vote_count: vote_count_of::(&who), + } +} + /// Index of a candidate. Panics if index is not found. fn index_of_candidate(c: &T::AccountId) -> u32 { >::candidates().iter().position(|x| x == c) @@ -161,10 +175,10 @@ benchmarks! { // -- Signed ones vote { - // range of candidates to vote for. - let v in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic adds a lock. let l in 1 .. MAX_LOCKS; + // we fix the number of voted candidates to max + let v = MAXIMUM_VOTE as u32; clean::(); // create a bunch of candidates. @@ -181,15 +195,12 @@ benchmarks! { }: _(RawOrigin::Signed(caller), votes, stake) remove_voter { - // range of candidates that we have voted for. This may or may have a significant impact on - // the outcome. - let v in 1 .. (MAXIMUM_VOTE as u32); // range of locks that the caller already had. This extrinsic removes a lock. let l in 1 .. MAX_LOCKS; + // we fix the number of voted candidates to max + let v = MAXIMUM_VOTE as u32; clean::(); - let votes_to_remove = (MAXIMUM_VOTE / 2) as u32; - // create a bunch of candidates. let all_candidates = submit_candidates::(v, "candidates")?; @@ -202,14 +213,15 @@ benchmarks! { }: _(RawOrigin::Signed(caller)) report_defunct_voter_correct { - // number fo already existing members or runners. - let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of already existing candidates that may or may not be voted by the reported // account. let c in 1 .. MAX_CANDIDATES; // number of candidates that the reported voter voted for. The worse case of search here is // basically `c * v`. let v in 1 .. (MAXIMUM_VOTE as u32); + // we fix the number of members to when members and runners-up to the desired. We'll be in + // this state almost always. + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -238,8 +250,6 @@ benchmarks! { stake, )?; - let account_2_lookup = as_lookup::(account_2.clone()); - // all the bailers go away. bailing_candidates.into_iter().for_each(|b| { let index = index_of_candidate::(&b); @@ -248,7 +258,8 @@ benchmarks! { Renouncing::Candidate(index), ).is_ok()); }); - }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup, candidate_count::()) + let defunct = defunct_for::(account_2.clone()); + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), defunct) verify { assert!(>::is_voter(&account_1)); assert!(!>::is_voter(&account_2)); @@ -261,14 +272,15 @@ benchmarks! { } report_defunct_voter_incorrect { - // number fo already existing members or runners. - let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of already existing candidates that may or may not be voted by the reported // account. let c in 1 .. MAX_CANDIDATES; // number of candidates that the reported voter voted for. The worse case of search here is // basically `c * v`. - let v in 2 .. (MAXIMUM_VOTE as u32); + let v in 1 .. (MAXIMUM_VOTE as u32); + // we fix the number of members to when members and runners-up to the desired. We'll be in + // this state almost always. + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -298,9 +310,9 @@ benchmarks! { stake, )?; - let account_2_lookup = as_lookup::(account_2.clone()); + let defunct = defunct_for::(account_2.clone()); // no one bails out. account_1 is slashed and removed as voter now. - }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), account_2_lookup, candidate_count::()) + }: report_defunct_voter(RawOrigin::Signed(account_1.clone()), defunct) verify { assert!(!>::is_voter(&account_1)); assert!(>::is_voter(&account_2)); @@ -313,11 +325,11 @@ benchmarks! { } submit_candidacy { - // number fo already existing members or runners. Because candidates cannot be duplicate - // with members and previous candidates. - let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; + // we fix the number of members to when members and runners-up to the desired. We'll be in + // this state almost always. + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); let stake = default_stake::(BALANCE_FACTOR); @@ -343,10 +355,11 @@ benchmarks! { renounce_candidacy_candidate { // 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 members and runners-up. - let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; + // we fix the number of members to when members and runners-up to the desired. We'll be in + // this state almost always. + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); @@ -371,8 +384,9 @@ benchmarks! { // have no impact. // number of already existing candidates. let c in 1 .. MAX_CANDIDATES; - // number of already existing members and runners-up. - let m in 1 .. T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + // we fix the number of members to when members and runners-up to the desired. We'll be in + // this state almost always. + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); // create m members and runners combined. diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index a3c673fd0a224..65924cec86d06 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -123,6 +123,19 @@ pub enum Renouncing { Candidate(#[codec(compact)] u32), } +/// Information needed to prove the defunct-ness of a voter. +#[derive(Encode, Decode, Clone, PartialEq, RuntimeDebug)] +pub struct DefunctVoter { + /// the voter's who's being challenged for being defunct + pub who: AccountId, + /// The number of votes that `who` has placed. + #[codec(compact)] + pub vote_count: u32, + /// The number of current active candidates. + #[codec(compact)] + pub candidate_count: u32 +} + pub trait Trait: frame_system::Trait { /// The overarching event type.c type Event: From> + Into<::Event>; @@ -259,6 +272,8 @@ decl_error! { NotMember, /// The provided count of number of candidates is incorrect. InvalidCandidateCount, + /// The provided count of number of votes is incorrect. + InvalidVoteCount, /// The renouncing origin presented a wrong `Renouncing` parameter. InvalidRenouncing, /// Prediction regarding replacement after member removal is wrong. @@ -370,27 +385,27 @@ decl_module! { #[weight = 1_000_000_000] fn report_defunct_voter( origin, - target: ::Source, - #[compact] candidate_count: u32, + defunct: DefunctVoter<::Source>, ) { let reporter = ensure_signed(origin)?; - let target = T::Lookup::lookup(target)?; + let target = T::Lookup::lookup(defunct.who)?; ensure!(reporter != target, Error::::ReportSelf); + ensure!(Self::is_voter(&reporter), Error::::MustBeVoter); - let actual_count = >::decode_len().unwrap_or(0); - // TODO: refund based on actual is less than predicted. + let DefunctVoter { candidate_count, vote_count, .. } = defunct; + + // TODO: refund if actual is less than predicted. Or not?.. with <= they are paying for it. ensure!( - actual_count as u32 <= candidate_count, + >::decode_len().unwrap_or(0) as u32 <= candidate_count, Error::::InvalidCandidateCount, ); - ensure!(Self::is_voter(&reporter), Error::::MustBeVoter); + // TODO: We read this again in is_defunct_voter, still duplicate overlay read. + ensure!( + >::get(&target).1.len() as u32 <= vote_count, + Error::::InvalidVoteCount, + ); - // Checking if someone is a candidate and a member here is O(LogN), making the whole - // function O(MLonN) with N candidates in total and M of them being voted by `target`. - // We could easily add another mapping to be able to check if someone is a candidate in - // `O(1)` but that would make the process of removing candidates at the end of each - // round slightly harder. Note that for now we have a bound of number of votes (`N`). let valid = Self::is_defunct_voter(&target); if valid { // reporter will get the voting bond of the target @@ -1276,6 +1291,14 @@ mod tests { Elections::submit_candidacy(origin, Elections::candidates().len() as u32) } + fn defunct_for(who: u64) -> DefunctVoter { + DefunctVoter { + who, + candidate_count: Elections::candidates().len() as u32, + vote_count: Elections::votes_of(&who).len() as u32 + } + } + #[test] fn params_should_work() { ExtBuilder::default().build_and_execute(|| { @@ -1758,30 +1781,48 @@ mod tests { fn reporter_must_be_voter() { ExtBuilder::default().build_and_execute(|| { assert_noop!( - Elections::report_defunct_voter(Origin::signed(1), 2, 0), + Elections::report_defunct_voter(Origin::signed(1), defunct_for(2)), Error::::MustBeVoter, ); }); } #[test] - fn reporter_must_provide_length() { + fn reporter_must_provide_lengths() { ExtBuilder::default().build_and_execute(|| { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); // both are defunct. - assert_ok!(Elections::vote(Origin::signed(5), vec![99], 50)); + assert_ok!(Elections::vote(Origin::signed(5), vec![99, 999, 9999], 50)); assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); // 2 candidates! incorrect candidate length. assert_noop!( - Elections::report_defunct_voter(Origin::signed(4), 5, 1), + Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { + who: 5, + candidate_count: 1, + vote_count: 3, + }), + Error::::InvalidCandidateCount, + ); + + // 3 votes! incorrect vote length + assert_noop!( + Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { + who: 5, + candidate_count: 2, + vote_count: 1, + }), Error::::InvalidCandidateCount, ); // correct candidate length. - assert_ok!(Elections::report_defunct_voter(Origin::signed(4), 5, 2)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { + who: 5, + candidate_count: 2, + vote_count: 3, + })); }); } @@ -1796,7 +1837,7 @@ mod tests { assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); // 2 candidates! overestimation is okay. - assert_ok!(Elections::report_defunct_voter(Origin::signed(4), 5, 3)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(4), defunct_for(5))); }); } @@ -1860,7 +1901,7 @@ mod tests { assert_eq!(balances(&3), (28, 2)); assert_eq!(balances(&5), (45, 5)); - assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 3, 0)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(5), defunct_for(3))); assert!(System::events().iter().any(|event| { event.event == Event::elections_phragmen(RawEvent::VoterReported(3, 5, true)) })); @@ -1888,7 +1929,7 @@ mod tests { assert_eq!(balances(&4), (35, 5)); assert_eq!(balances(&5), (45, 5)); - assert_ok!(Elections::report_defunct_voter(Origin::signed(5), 4, 0)); + assert_ok!(Elections::report_defunct_voter(Origin::signed(5), defunct_for(4))); assert!(System::events().iter().any(|event| { event.event == Event::elections_phragmen(RawEvent::VoterReported(4, 5, false)) })); From 2cf975857d2ad59184e2487cbeec7398f38b68e2 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 May 2020 13:08:10 +0200 Subject: [PATCH 17/30] Once more ready to benchmark --- frame/elections-phragmen/src/benchmarking.rs | 87 ++-- frame/elections-phragmen/src/lib.rs | 506 ++++++++++++------- frame/staking/src/lib.rs | 2 +- 3 files changed, 364 insertions(+), 231 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 9880192074739..bad8ecaa9a276 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -26,9 +26,7 @@ use frame_support::traits::OnInitialize; use crate::Module as Elections; -const SEED: u32 = 0; const BALANCE_FACTOR: u32 = 250; -const MAX_LOCKS: u32 = 20; const MAX_VOTERS: u32 = 500; const MAX_CANDIDATES: u32 = 100; @@ -36,7 +34,7 @@ type Lookup = <::Lookup as StaticLookup>::Source; /// grab new account with infinite balance. fn endowed_account(name: &'static str, index: u32) -> T::AccountId { - let account: T::AccountId = account(name, index, SEED); + let account: T::AccountId = account(name, index, 0); let amount = default_stake::(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 @@ -75,12 +73,6 @@ fn defunct_for(who: T::AccountId) -> DefunctVoter> { } } -/// Index of a candidate. Panics if index is not found. -fn index_of_candidate(c: &T::AccountId) -> u32 { - >::candidates().iter().position(|x| x == c) - .expect("Candidate does not exist") as u32 -} - /// Add `c` new candidates. fn submit_candidates(c: u32, prefix: &'static str) -> Result, &'static str> @@ -112,30 +104,19 @@ fn submit_candidates_with_self_vote(c: u32, prefix: &'static str) fn submit_voter(caller: T::AccountId, votes: Vec, stake: BalanceOf) -> Result<(), &'static str> { - >::vote(RawOrigin::Signed(caller).into(), votes, stake) + >::vote(RawOrigin::Signed(caller).into(), votes, stake, true) .map_err(|_| "failed to submit vote") } -/// add `n` locks to account `who`. -fn add_locks(who: &T::AccountId, n: u8) { - for id in 0..n { - let lock_id = [id; 8]; - let locked = 100; - let reasons = WithdrawReason::Transfer | WithdrawReason::Reserve; - T::Currency::set_lock(lock_id, who, locked.into(), reasons); - } -} - /// create `num_voter` voters who randomly vote for at most `votes` of `all_candidates` if /// available. fn distribute_voters(mut all_candidates: Vec, num_voters: u32, votes: usize) -> Result<(), &'static str> { let stake = default_stake::(BALANCE_FACTOR); - let c = all_candidates.len() as u32; for i in 0..num_voters { - // to ensure that votes are different, - all_candidates.rotate_left((i % c) as usize); + // to ensure that votes are different + all_candidates.rotate_left(1); let votes = all_candidates .iter() .cloned() @@ -171,32 +152,50 @@ fn clean() { } benchmarks! { - _ {} + _ { + // User account seed + let u in 0 .. 1000 => (); + } // -- Signed ones vote { - // range of locks that the caller already had. This extrinsic adds a lock. - let l in 1 .. MAX_LOCKS; + let u in ...; // we fix the number of voted candidates to max - let v = MAXIMUM_VOTE as u32; + let v = MAXIMUM_VOTE; clean::(); // create a bunch of candidates. let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32, "candidates")?; - let caller = endowed_account::("caller", 0); - add_locks::(&caller, l as u8); - + let caller = endowed_account::("caller", u); let stake = default_stake::(BALANCE_FACTOR); - // vote first `x` ones. - let votes = all_candidates.into_iter().take(v as usize).collect(); + // vote for all of them. + let votes = all_candidates.into_iter().take(v).collect(); + + }: _(RawOrigin::Signed(caller), votes, stake, true) + + vote_update { + let u in ...; + // we fix the number of voted candidates to max + let v = MAXIMUM_VOTE; + clean::(); + + // create a bunch of candidates. + let all_candidates = submit_candidates::(MAXIMUM_VOTE as u32, "candidates")?; - }: _(RawOrigin::Signed(caller), votes, stake) + let caller = endowed_account::("caller", u); + let stake = default_stake::(BALANCE_FACTOR); + + // original votes. + let mut votes = all_candidates.into_iter().take(v).collect::>(); + submit_voter::(caller.clone(), votes.clone(), stake)?; + // new votes. + votes.rotate_left(1); + }: vote(RawOrigin::Signed(caller), votes, stake, false) remove_voter { - // range of locks that the caller already had. This extrinsic removes a lock. - let l in 1 .. MAX_LOCKS; + let u in ...; // we fix the number of voted candidates to max let v = MAXIMUM_VOTE as u32; clean::(); @@ -204,8 +203,7 @@ benchmarks! { // create a bunch of candidates. let all_candidates = submit_candidates::(v, "candidates")?; - let caller = endowed_account::("caller", 0); - add_locks::(&caller, l as u8); + let caller = endowed_account::("caller", u); let stake = default_stake::(BALANCE_FACTOR); submit_voter::(caller.clone(), all_candidates, stake)?; @@ -252,10 +250,10 @@ benchmarks! { // all the bailers go away. bailing_candidates.into_iter().for_each(|b| { - let index = index_of_candidate::(&b); + let count = candidate_count::(); assert!(>::renounce_candidacy( RawOrigin::Signed(b).into(), - Renouncing::Candidate(index), + Renouncing::Candidate(count), ).is_ok()); }); let defunct = defunct_for::(account_2.clone()); @@ -368,8 +366,8 @@ benchmarks! { let all_candidates = submit_candidates::(c, "caller")?; let bailing = all_candidates[0].clone(); // Should be ("caller", 0) - let index = index_of_candidate::(&bailing); - }: renounce_candidacy(RawOrigin::Signed(bailing), Renouncing::Candidate(index)) + let count = candidate_count::(); + }: renounce_candidacy(RawOrigin::Signed(bailing), Renouncing::Candidate(count)) verify { #[cfg(test)] { @@ -380,18 +378,15 @@ benchmarks! { } renounce_candidacy_member_runner_up { - // removing members and runners will be cheaper most likely. The number of candidates will - // have no impact. - // number of already existing candidates. - let c in 1 .. MAX_CANDIDATES; + // removing members and runners will be cheaper than a candidate. // we fix the number of members to when members and runners-up to the desired. We'll be in // this state almost always. + let u in ...; let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); // create m members and runners combined. let members_and_runners_up = fill_seats_up_to::(m)?; - let _ = submit_candidates::(c, "candidates")?; let bailing = members_and_runners_up[0].clone(); let renouncing = if >::is_member(&bailing) { diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 65924cec86d06..f5498565bfa06 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -90,7 +90,10 @@ use sp_runtime::{ }; use frame_support::{ decl_storage, decl_event, ensure, decl_module, decl_error, - weights::{Weight, DispatchClass}, + weights::{ + Weight, DispatchClass, Pays, FunctionOf, + constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}, + }, storage::{StorageMap, IterableStorageMap}, dispatch::{DispatchResultWithPostInfo, WithPostDispatchInfo}, traits::{ @@ -119,7 +122,7 @@ pub enum Renouncing { Member, /// A runner-up is renouncing. RunnerUp, - /// A candidate is renouncing. + /// A candidate is renouncing, while the given total number of candidates exists. Candidate(#[codec(compact)] u32), } @@ -230,6 +233,7 @@ decl_storage! { T::Origin::from(Some(member.clone()).into()), vec![member.clone()], *stake, + true, ).expect("Genesis member could not vote."); member.clone() @@ -278,6 +282,8 @@ decl_error! { InvalidRenouncing, /// Prediction regarding replacement after member removal is wrong. InvalidReplacement, + /// The vote's would_create flag is invalid. + InvalidWouldCreate, } } @@ -294,25 +300,49 @@ decl_module! { const TermDuration: T::BlockNumber = T::TermDuration::get(); const ModuleId: LockIdentifier = T::ModuleId::get(); - /// Vote for a set of candidates for the upcoming round of election. + /// Vote for a set of candidates for the upcoming round of election. This can be called to + /// set the initial votes, with `would_create = true`, or update already existing votes + /// with `would_create = false`. + /// + /// Upon initial voting, `value` units of `who`'s balance is locked and a bond amount is + /// reserved. /// /// The `votes` should: /// - not be empty. /// - be less than the number of possible candidates. Note that all current members and /// runners-up are also automatically candidates for the next round. /// - /// Upon voting, `value` units of `who`'s balance is locked and a bond amount is reserved. - /// /// It is the responsibility of the caller to not place all of their balance into the lock /// and keep some for further transactions. /// /// # - /// #### State - /// Reads: O(1) - /// Writes: O(V) given `V` votes. V is bounded by 16. + /// Base weight: 50us + /// State reads: + /// - Candidates.len() + Members.len() + RunnersUp.len() + /// - Voting (is_voter) + /// TODO: check this: This is all jst under one key: AccountData + /// - AccountBalance(who) (unreserve + total_balance) + /// State writes: + /// - Voting + /// - AccountBalance(who) (unreserve -- only when would_create is true) + /// - Lock /// # - #[weight = 100_000_000] - fn vote(origin, votes: Vec, #[compact] value: BalanceOf) { + #[weight = FunctionOf( + |(_, _, would_create): (_, _, &bool)| + 50 * WEIGHT_PER_MICROS + if *would_create { + T::DbWeight::get().reads_writes(5, 3) + } else { + T::DbWeight::get().reads_writes(5, 2) + }, + DispatchClass::Normal, + Pays::Yes, + )] + fn vote( + origin, + votes: Vec, + #[compact] value: BalanceOf, + would_create: bool, + ) { let who = ensure_signed(origin)?; ensure!(votes.len() <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); @@ -321,6 +351,7 @@ decl_module! { let candidates_count = >::decode_len().unwrap_or(0); let members_count = >::decode_len().unwrap_or(0); let runners_up_count = >::decode_len().unwrap_or(0); + // addition is valid: candidates, members and runners-up will never overlap. let allowed_votes = candidates_count + members_count + runners_up_count; @@ -328,9 +359,10 @@ decl_module! { ensure!(votes.len() <= allowed_votes, Error::::TooManyVotes); ensure!(value > T::Currency::minimum_balance(), Error::::LowBalance); + ensure!(Self::is_voter(&who) == !would_create, Error::::InvalidWouldCreate); - if !Self::is_voter(&who) { - // first time voter. Reserve bond. + // first time voter. Reserve bond. + if would_create { T::Currency::reserve(&who, T::VotingBond::get()) .map_err(|_| Error::::UnableToPayBond)?; } @@ -352,14 +384,18 @@ decl_module! { /// Remove `origin` as a voter. This removes the lock and returns the bond. /// /// # - /// #### State - /// Reads: O(1) - /// Writes: O(1) + /// Base weight: 35us + /// State reads: + /// - Voting (is_voter) + /// - AccountData(who) + /// State writes: + /// - Voting + /// - Reserved + /// - Locks /// # - #[weight = 0] + #[weight = 35 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(2, 3)] fn remove_voter(origin) { let who = ensure_signed(origin)?; - ensure!(Self::is_voter(&who), Error::::MustBeVoter); Self::do_remove_voter(&who, true); @@ -378,11 +414,27 @@ decl_module! { /// weight calculation. /// /// # - /// #### State - /// Reads: O(NLogM) given M current candidates and N votes for `target`. - /// Writes: O(1) + /// No Base weight based on min square analysis. + /// State reads: + /// - Voting(reporter) + /// - Candidate.len() + /// - Voting(Target) + /// - Candidates, Members, RunnersUp (is_defunct_voter) [and Voting(Target) -- already counted] + /// State writes: + /// - Lock(reporter || target) + /// - AccountBalance(reporter) + AccountBalance(target) + /// - Voting(reporter || target) + /// Note: the db access is worse with respect to db, which is when the report is correct. + /// TODO: T::BadReport??? /// # - #[weight = 1_000_000_000] + #[weight = FunctionOf( + |(defunct,): (&DefunctVoter<::Source>,)| + defunct.candidate_count as Weight * (2 * WEIGHT_PER_MICROS) + + defunct.vote_count as Weight * (18 * WEIGHT_PER_MICROS) + + T::DbWeight::get().reads_writes(6, 4), + DispatchClass::Normal, + Pays::Yes, + )] fn report_defunct_voter( origin, defunct: DefunctVoter<::Source>, @@ -395,7 +447,6 @@ decl_module! { let DefunctVoter { candidate_count, vote_count, .. } = defunct; - // TODO: refund if actual is less than predicted. Or not?.. with <= they are paying for it. ensure!( >::decode_len().unwrap_or(0) as u32 <= candidate_count, Error::::InvalidCandidateCount, @@ -431,16 +482,30 @@ decl_module! { /// removed. /// /// # - /// #### State - /// Reads: O(LogN) Given N candidates. - /// Writes: O(1) + /// Base weight = 35us + /// Complexity of candidate_count: 0.373us + /// State reads: + /// - Candidates.len() + /// - Candidates + /// - Members + /// - RunnersUp + /// - AccountBalance(who) + /// State writes: + /// - AccountBalance(who) + /// - Candidates /// # - #[weight = 500_000_000] + #[weight = FunctionOf( + |(candidate_count,): (&u32,)| + 35 * WEIGHT_PER_MICROS + + *candidate_count as Weight * (375 * WEIGHT_PER_NANOS) + + T::DbWeight::get().reads_writes(5, 2), + DispatchClass::Normal, + Pays::Yes, + )] fn submit_candidacy(origin, #[compact] candidate_count: u32) { let who = ensure_signed(origin)?; let actual_count = >::decode_len().unwrap_or(0); - // TODO: refund based on actual is less than predicted. ensure!( actual_count as u32 <= candidate_count, Error::::InvalidCandidateCount, @@ -470,7 +535,53 @@ decl_module! { /// - `origin` is a current member. In this case, the bond is unreserved and origin is /// removed as a member, consequently not being a candidate for the next round anymore. /// Similar to [`remove_voter`], if replacement runners exists, they are immediately used. - #[weight = (2_000_000_000, DispatchClass::Operational)] + /// + /// Base weight: 16.57us + /// If a candidate is renouncing: + /// Complexity of candidate_count: 0.235 + /// State reads: + /// - Candidates + /// - AccountBalance(who) (unreserve) + /// State writes: + /// - Candidates + /// - AccountBalance(who) (unreserve) + /// If member is renouncing: + /// Base weight: 46.25 + /// State reads: + /// - Members, RunnersUp (remove_and_replace_member), + /// - AccountData(who) (unreserve) + /// State writes: + /// - Members, RunnersUp (remove_and_replace_member), + /// - AccountData(who) (unreserve) + /// TODO: and calls into ChangeMembers?? + /// If runner is renouncing: + /// Base weight: 46.25 + /// State reads: + /// - RunnersUp (remove_and_replace_member), + /// - AccountData(who) (unreserve) + /// State writes: + /// - RunnersUp (remove_and_replace_member), + /// - AccountData(who) (unreserve) + /// + #[weight = FunctionOf( + |(renouncing,): (&Renouncing,)| match *renouncing { + Renouncing::Member => { + 47 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(3, 3) + }, + Renouncing::RunnerUp => { + 47 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(2, 2) + }, + Renouncing::Candidate(count) => { + 16 * WEIGHT_PER_MICROS + + (count as Weight) * 235 * WEIGHT_PER_NANOS + + T::DbWeight::get().reads_writes(2, 2) + } + }, + DispatchClass::Normal, + Pays::Yes, + )] fn renounce_candidacy(origin, renouncing: Renouncing) { let who = ensure_signed(origin)?; match renouncing { @@ -495,12 +606,10 @@ decl_module! { Err(Error::::InvalidRenouncing)?; } } - Renouncing::Candidate(index) => { - // TODO: probably need the length of candidates here as well. + Renouncing::Candidate(count) => { let mut candidates = Self::candidates(); - let index = index as usize; - if let Some(c) = candidates.get(index) { - ensure!(*c == who, Error::::InvalidRenouncing); + ensure!(count >= candidates.len() as u32, Error::::InvalidRenouncing); + if let Some(index) = candidates.iter().position(|x| *x == who) { candidates.remove(index); // unreserve the bond T::Currency::unreserve(&who, T::CandidacyBond::get()); @@ -522,9 +631,7 @@ decl_module! { /// Note that this does not affect the designated block number of the next election. /// /// # - /// #### State - /// Reads: O(do_phragmen) - /// Writes: O(do_phragmen) + /// ... /// # #[weight = (2_000_000_000, DispatchClass::Operational)] fn remove_member( @@ -705,7 +812,9 @@ impl Module { /// Note that false is returned if `who` is not a voter at all. /// /// O(NLogM) with M candidates and `who` having voted for `N` of them. + /// Reads Members, RunnersUp, Candidates and Voting(who) from database. fn is_defunct_voter(who: &T::AccountId) -> bool { + // TODO: optimise this if Self::is_voter(who) { Self::votes_of(who) .iter() @@ -721,6 +830,7 @@ impl Module { /// /// This will clean always clean the storage associated with the voter, and remove the balance /// lock. Optionally, it would also return the reserved voting bond if indicated by `unreserve`. + /// If unreserve is true, has 3 storage reads and 1 reads. fn do_remove_voter(who: &T::AccountId, unreserve: bool) { // remove storage and lock. Voting::::remove(who); @@ -1291,6 +1401,15 @@ mod tests { Elections::submit_candidacy(origin, Elections::candidates().len() as u32) } + fn vote(origin: Origin, votes: Vec, stake: u64) -> DispatchResult { + if let Origin::system(frame_system::RawOrigin::Signed(account)) = origin { + let would_create = !Elections::is_voter(&account); + Elections::vote(origin, votes, stake, would_create) + } else { + panic!("vote origin must be signed"); + } + } + fn defunct_for(who: u64) -> DefunctVoter { DefunctVoter { who, @@ -1474,7 +1593,7 @@ mod tests { // critically important to make sure that outgoing candidates and losers are not mixed up. ExtBuilder::default().build_and_execute(|| { assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1497,8 +1616,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5, 4], 20)); - assert_ok!(Elections::vote(Origin::signed(1), vec![3], 10)); + assert_ok!(vote(Origin::signed(2), vec![5, 4], 20)); + assert_ok!(vote(Origin::signed(1), vec![3], 10)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1531,7 +1650,7 @@ mod tests { assert_eq!(balances(&2), (20, 0)); assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); assert_eq!(balances(&2), (18, 2)); assert_eq!(has_lock(&2), 20); @@ -1545,7 +1664,7 @@ mod tests { assert_eq!(balances(&2), (20, 0)); assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 12)); + assert_ok!(vote(Origin::signed(2), vec![5], 12)); assert_eq!(balances(&2), (18, 2)); assert_eq!(has_lock(&2), 12); @@ -1559,25 +1678,30 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); assert_eq!(balances(&2), (18, 2)); assert_eq!(has_lock(&2), 20); assert_eq!(Elections::locked_stake_of(&2), 20); // can update; different stake; different lock and reserve. - assert_ok!(Elections::vote(Origin::signed(2), vec![5, 4], 15)); + assert_ok!(vote(Origin::signed(2), vec![5, 4], 15)); assert_eq!(balances(&2), (18, 2)); assert_eq!(has_lock(&2), 15); assert_eq!(Elections::locked_stake_of(&2), 15); }); } + #[test] + fn vote_update_need_to_set_flag() { + todo!(); + } + #[test] fn cannot_vote_for_no_candidate() { ExtBuilder::default().build_and_execute(|| { assert_noop!( - Elections::vote(Origin::signed(2), vec![], 20), + vote(Origin::signed(2), vec![], 20), Error::::NoVotes, ); }); @@ -1589,7 +1713,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 20)); + assert_ok!(vote(Origin::signed(2), vec![4, 5], 20)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1597,7 +1721,7 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); - assert_ok!(Elections::vote(Origin::signed(3), vec![4, 5], 10)); + assert_ok!(vote(Origin::signed(3), vec![4, 5], 10)); }); } @@ -1608,11 +1732,11 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(1), vec![4, 3], 10)); - assert_ok!(Elections::vote(Origin::signed(2), vec![4], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(1), vec![4, 3], 10)); + assert_ok!(vote(Origin::signed(2), vec![4], 20)); + 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); assert_ok!(Elections::end_block(System::block_number())); @@ -1620,7 +1744,7 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); - assert_ok!(Elections::vote(Origin::signed(3), vec![4, 5], 10)); + assert_ok!(vote(Origin::signed(3), vec![4, 5], 10)); assert_eq!(PRIME.with(|p| *p.borrow()), Some(4)); }); } @@ -1632,13 +1756,13 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(1), vec![4, 3], 10)); - assert_ok!(Elections::vote(Origin::signed(2), vec![4], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(1), vec![4, 3], 10)); + assert_ok!(vote(Origin::signed(2), vec![4], 20)); + 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)); - assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(1))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(3))); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1664,13 +1788,13 @@ mod tests { assert_noop!( // content of the vote is irrelevant. - Elections::vote(Origin::signed(1), vec![9, 99, 999, 9999], 5), + vote(Origin::signed(1), vec![9, 99, 999, 9999], 5), Error::::TooManyVotes, ); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + 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); assert_ok!(Elections::end_block(System::block_number())); @@ -1678,9 +1802,9 @@ mod tests { // now we have 2 members, 1 runner-up, and 1 new candidate assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(1), vec![9, 99, 999, 9999], 5)); + assert_ok!(vote(Origin::signed(1), vec![9, 99, 999, 9999], 5)); assert_noop!( - Elections::vote(Origin::signed(1), vec![9, 99, 999, 9_999, 99_999], 5), + vote(Origin::signed(1), vec![9, 99, 999, 9_999, 99_999], 5), Error::::TooManyVotes, ); }); @@ -1693,7 +1817,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_noop!( - Elections::vote(Origin::signed(2), vec![4], 1), + vote(Origin::signed(2), vec![4], 1), Error::::LowBalance, ); }) @@ -1705,7 +1829,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 30)); + assert_ok!(vote(Origin::signed(2), vec![4, 5], 30)); // you can lie but won't get away with it. assert_eq!(Elections::locked_stake_of(&2), 20); assert_eq!(has_lock(&2), 20); @@ -1717,8 +1841,8 @@ mod tests { ExtBuilder::default().voter_bond(8).build_and_execute(|| { assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![5], 30)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(3), vec![5], 30)); assert_eq_uvec!(all_voters(), vec![2, 3]); assert_eq!(Elections::locked_stake_of(&2), 20); @@ -1748,7 +1872,7 @@ mod tests { fn dupe_remove_should_fail() { ExtBuilder::default().build_and_execute(|| { assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); assert_ok!(Elections::remove_voter(Origin::signed(2))); assert_eq!(all_voters(), vec![]); @@ -1764,9 +1888,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); assert_ok!(Elections::remove_voter(Origin::signed(4))); @@ -1792,16 +1916,17 @@ mod tests { ExtBuilder::default().build_and_execute(|| { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); // both are defunct. - assert_ok!(Elections::vote(Origin::signed(5), vec![99, 999, 9999], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); + assert_ok!(vote(Origin::signed(5), vec![99, 999, 9999], 50)); + assert_ok!(vote(Origin::signed(4), vec![999], 40)); - // 2 candidates! incorrect candidate length. + // 3 candidates! incorrect candidate length. assert_noop!( Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { who: 5, - candidate_count: 1, + candidate_count: 2, vote_count: 3, }), Error::::InvalidCandidateCount, @@ -1811,16 +1936,16 @@ mod tests { assert_noop!( Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { who: 5, - candidate_count: 2, - vote_count: 1, + candidate_count: 3, + vote_count: 2, }), - Error::::InvalidCandidateCount, + Error::::InvalidVoteCount, ); - // correct candidate length. + // correct. assert_ok!(Elections::report_defunct_voter(Origin::signed(4), DefunctVoter { who: 5, - candidate_count: 2, + candidate_count: 3, vote_count: 3, })); }); @@ -1833,8 +1958,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); // both are defunct. - assert_ok!(Elections::vote(Origin::signed(5), vec![99], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![999], 40)); + assert_ok!(vote(Origin::signed(5), vec![99], 50)); + assert_ok!(vote(Origin::signed(4), vec![999], 40)); // 2 candidates! overestimation is okay. assert_ok!(Elections::report_defunct_voter(Origin::signed(4), defunct_for(5))); @@ -1848,12 +1973,12 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(6))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 20)); - assert_ok!(Elections::vote(Origin::signed(6), vec![6], 30)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(2), vec![4, 5], 20)); + assert_ok!(vote(Origin::signed(6), vec![6], 30)); // will be soon a defunct voter. - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1872,7 +1997,7 @@ mod tests { assert_eq!(Elections::is_defunct_voter(&3), true); assert_ok!(submit_candidacy(Origin::signed(1))); - assert_ok!(Elections::vote(Origin::signed(1), vec![1], 10)); + assert_ok!(vote(Origin::signed(1), vec![1], 10)); // has a candidate voted for. assert_eq!(Elections::is_defunct_voter(&1), false); @@ -1886,11 +2011,11 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(2), vec![4, 5], 20)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(2), vec![4, 5], 20)); // will be soon a defunct voter. - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1917,8 +2042,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -1946,9 +2071,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 15)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(4), vec![4], 15)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); assert_eq_uvec!(all_voters(), vec![2, 3, 4]); @@ -1980,8 +2105,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); // This guy's vote is pointless for this round. - assert_ok!(Elections::vote(Origin::signed(3), vec![4], 30)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(3), vec![4], 30)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2010,10 +2135,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); + 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); assert_ok!(Elections::end_block(System::block_number())); @@ -2051,10 +2176,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![2], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![4], 50)); + assert_ok!(vote(Origin::signed(2), vec![3], 20)); + assert_ok!(vote(Origin::signed(3), vec![2], 30)); + assert_ok!(vote(Origin::signed(4), vec![5], 40)); + assert_ok!(vote(Origin::signed(5), vec![4], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2078,17 +2203,17 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); + 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); assert_ok!(Elections::end_block(System::block_number())); assert_eq!(Elections::members(), vec![(4, 40), (5, 50)]); assert_eq!(Elections::runners_up(), vec![(2, 20), (3, 30)]); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 15)); + assert_ok!(vote(Origin::signed(5), vec![5], 15)); System::set_block_number(10); assert_ok!(Elections::end_block(System::block_number())); @@ -2104,9 +2229,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2115,7 +2240,7 @@ mod tests { assert_eq!(balances(&2), (15, 5)); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(10); assert_ok!(Elections::end_block(System::block_number())); @@ -2133,7 +2258,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_eq!(balances(&5), (47, 3)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); assert_eq!(balances(&5), (45, 5)); System::set_block_number(5); @@ -2157,7 +2282,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); + assert_ok!(vote(Origin::signed(4), vec![5], 40)); assert_eq!(balances(&5), (47, 3)); assert_eq!(balances(&3), (27, 3)); @@ -2180,8 +2305,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2190,10 +2315,10 @@ mod tests { assert_eq!(Elections::election_rounds(), 1); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); assert_ok!(Elections::remove_voter(Origin::signed(4))); @@ -2218,10 +2343,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); let check_at_block = |b: u32| { System::set_block_number(b.into()); @@ -2249,8 +2374,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2259,7 +2384,7 @@ mod tests { // a new candidate assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); assert_ok!(Elections::remove_member(Origin::ROOT, 4, false)); @@ -2275,8 +2400,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2294,9 +2419,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + 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); assert_ok!(Elections::end_block(System::block_number())); @@ -2318,10 +2443,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(2), vec![3], 20)); + 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)); assert_eq!(>::decode_len().unwrap(), 3); @@ -2351,8 +2476,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2363,14 +2488,14 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); // 5 will change their vote and becomes an `outgoing` - assert_ok!(Elections::vote(Origin::signed(5), vec![4], 8)); + assert_ok!(vote(Origin::signed(5), vec![4], 8)); // 4 will stay in the set - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); // 3 will become a winner - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); // these two are losers. - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); - assert_ok!(Elections::vote(Origin::signed(1), vec![1], 10)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(1), vec![1], 10)); System::set_block_number(10); assert_ok!(Elections::end_block(System::block_number())); @@ -2398,9 +2523,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![10], 50)); + 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![10], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2418,10 +2543,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![3], 20)); - assert_ok!(Elections::vote(Origin::signed(3), vec![2], 30)); - assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![4], 50)); + assert_ok!(vote(Origin::signed(2), vec![3], 20)); + assert_ok!(vote(Origin::signed(3), vec![2], 30)); + assert_ok!(vote(Origin::signed(4), vec![5], 40)); + assert_ok!(vote(Origin::signed(5), vec![4], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2442,7 +2567,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(2))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::renounce_candidacy(Origin::signed(3), Renouncing::Candidate(1))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(3), Renouncing::Candidate(4))); assert_eq!(Elections::candidates(), vec![2, 4, 5]); }) @@ -2455,9 +2580,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(5), vec![2], 50)); + assert_ok!(vote(Origin::signed(2), vec![5], 20)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![2], 50)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2476,10 +2601,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2501,8 +2626,8 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(5))); assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2527,10 +2652,10 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(3))); assert_ok!(submit_candidacy(Origin::signed(2))); - assert_ok!(Elections::vote(Origin::signed(5), vec![4], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![5], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(5), vec![4], 50)); + assert_ok!(vote(Origin::signed(4), vec![5], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2554,10 +2679,10 @@ mod tests { // assert_ok!(submit_candidacy(Origin::signed(3))); // assert_ok!(submit_candidacy(Origin::signed(2))); - // assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20)); - // assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - // assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - // assert_ok!(Elections::vote(Origin::signed(5), vec![2], 50)); + // assert_ok!(vote(Origin::signed(2), vec![5], 20)); + // 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![2], 50)); // System::set_block_number(5); // assert_ok!(Elections::end_block(System::block_number())); @@ -2577,7 +2702,7 @@ mod tests { assert_eq!(balances(&5), (47, 3)); assert_eq!(Elections::candidates(), vec![5]); - assert_ok!(Elections::renounce_candidacy(Origin::signed(5), Renouncing::Candidate(0))); + assert_ok!(Elections::renounce_candidacy(Origin::signed(5), Renouncing::Candidate(1))); assert_eq!(balances(&5), (50, 0)); assert_eq!(Elections::candidates(), vec![]); }) @@ -2608,9 +2733,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2632,9 +2757,9 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); assert_ok!(submit_candidacy(Origin::signed(3))); - assert_ok!(Elections::vote(Origin::signed(5), vec![5], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); @@ -2650,16 +2775,29 @@ mod tests { } #[test] - fn wrong_candidate_index_renounce_should_fail() { + fn wrong_candidate_count_renounce_should_fail() { ExtBuilder::default().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_noop!( - Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(0)), + Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(2)), Error::::InvalidRenouncing, ); + + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(3))); + }) + } + + #[test] + fn renounce_candidacy_count_can_overestimate() { + ExtBuilder::default().build_and_execute(|| { + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + // while we have only 3 candidates. + assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(4))); }) } @@ -2669,10 +2807,10 @@ mod tests { // TODD: this is a demonstration and should be fixed with #4593 >::put(vec![1, 1, 2, 3, 4]); - assert_ok!(Elections::vote(Origin::signed(5), vec![1], 50)); - assert_ok!(Elections::vote(Origin::signed(4), vec![4], 40)); - assert_ok!(Elections::vote(Origin::signed(3), vec![3], 30)); - assert_ok!(Elections::vote(Origin::signed(2), vec![2], 20)); + assert_ok!(vote(Origin::signed(5), vec![1], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 40)); + assert_ok!(vote(Origin::signed(3), vec![3], 30)); + assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); assert_ok!(Elections::end_block(System::block_number())); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 71042d69b3a4d..7e5f64722f230 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -264,7 +264,7 @@ //! - [Session](../pallet_session/index.html): Used to manage sessions. Also, a list of new //! validators is stored in the Session module's `Validators` at the end of each era. -#![recursion_limit = "128"] +// #![recursion_limit = "128"] #![cfg_attr(not(feature = "std"), no_std)] #[cfg(test)] From 3ec23c14d6e2edafdb54c81e937f223fc27ba9ad Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 May 2020 13:39:52 +0200 Subject: [PATCH 18/30] ready for bench --- frame/elections-phragmen/src/benchmarking.rs | 32 +++++++-- frame/elections-phragmen/src/lib.rs | 73 +++++++++++++------- 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index bad8ecaa9a276..d9bd3d14d33f1 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -440,10 +440,8 @@ benchmarks! { 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. - let m in - (T::DesiredMembers::get() + 1) - .. - T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + let u in ...; + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); clean::(); let _ = fill_seats_up_to::(m)?; @@ -460,6 +458,32 @@ 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 u in ...; + let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); + clean::(); + + let _ = fill_seats_up_to::(m)?; + let removing = as_lookup::(>::members_ids()[0].clone()); + }: { + assert_eq!( + >::remove_member(RawOrigin::Root.into(), removing, false).unwrap_err().error, + Error::::InvalidReplacement.into(), + ); + } + verify { + // must still have enough 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![]); + } + } + on_initialize { // if n % TermDuration is zero, then we run phragmen. The weight function must and should // check this as it is cheap to do so. TermDuration is not a storage item, it is a constant diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index f5498565bfa06..04bed9680997b 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -85,7 +85,7 @@ use codec::{Encode, Decode}; use sp_std::prelude::*; use sp_runtime::{ - print, DispatchResult, DispatchError, RuntimeDebug, Perbill, + DispatchError, RuntimeDebug, Perbill, traits::{Zero, StaticLookup, Convert}, }; use frame_support::{ @@ -320,12 +320,11 @@ decl_module! { /// State reads: /// - Candidates.len() + Members.len() + RunnersUp.len() /// - Voting (is_voter) - /// TODO: check this: This is all jst under one key: AccountData /// - AccountBalance(who) (unreserve + total_balance) /// State writes: /// - Voting - /// - AccountBalance(who) (unreserve -- only when would_create is true) /// - Lock + /// - AccountBalance(who) (unreserve -- only when would_create is true) /// # #[weight = FunctionOf( |(_, _, would_create): (_, _, &bool)| @@ -385,13 +384,14 @@ decl_module! { /// /// # /// Base weight: 35us + /// All state access is from do_remove_voter. /// State reads: - /// - Voting (is_voter) + /// - Voting /// - AccountData(who) /// State writes: /// - Voting - /// - Reserved /// - Locks + /// - AccountData(who) /// # #[weight = 35 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(2, 3)] fn remove_voter(origin) { @@ -425,7 +425,6 @@ decl_module! { /// - AccountBalance(reporter) + AccountBalance(target) /// - Voting(reporter || target) /// Note: the db access is worse with respect to db, which is when the report is correct. - /// TODO: T::BadReport??? /// # #[weight = FunctionOf( |(defunct,): (&DefunctVoter<::Source>,)| @@ -451,7 +450,8 @@ decl_module! { >::decode_len().unwrap_or(0) as u32 <= candidate_count, Error::::InvalidCandidateCount, ); - // TODO: We read this again in is_defunct_voter, still duplicate overlay read. + // Optimisation Note: We read this again in is_defunct_voter, still duplicate overlay + // read. ensure!( >::get(&target).1.len() as u32 <= vote_count, Error::::InvalidVoteCount, @@ -536,8 +536,8 @@ decl_module! { /// removed as a member, consequently not being a candidate for the next round anymore. /// Similar to [`remove_voter`], if replacement runners exists, they are immediately used. /// - /// Base weight: 16.57us /// If a candidate is renouncing: + /// Base weight: 16.57us /// Complexity of candidate_count: 0.235 /// State reads: /// - Candidates @@ -553,7 +553,6 @@ decl_module! { /// State writes: /// - Members, RunnersUp (remove_and_replace_member), /// - AccountData(who) (unreserve) - /// TODO: and calls into ChangeMembers?? /// If runner is renouncing: /// Base weight: 46.25 /// State reads: @@ -562,9 +561,16 @@ decl_module! { /// State writes: /// - RunnersUp (remove_and_replace_member), /// - AccountData(who) (unreserve) + /// + /// TODO: and calls into ChangeMembers?? /// #[weight = FunctionOf( |(renouncing,): (&Renouncing,)| match *renouncing { + Renouncing::Candidate(count) => { + 16 * WEIGHT_PER_MICROS + + (count as Weight) * 235 * WEIGHT_PER_NANOS + + T::DbWeight::get().reads_writes(2, 2) + }, Renouncing::Member => { 47 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 3) @@ -572,11 +578,6 @@ decl_module! { Renouncing::RunnerUp => { 47 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(2, 2) - }, - Renouncing::Candidate(count) => { - 16 * WEIGHT_PER_MICROS + - (count as Weight) * 235 * WEIGHT_PER_NANOS + - T::DbWeight::get().reads_writes(2, 2) } }, DispatchClass::Normal, @@ -631,9 +632,24 @@ decl_module! { /// Note that this does not affect the designated block number of the next election. /// /// # - /// ... + /// If we have a replacement: + /// - Base weight: 47.15 us + /// - State reads: + /// - RunnersUp.len() + /// - Members, RunnersUp (remove_and_replace_member) + /// - State writes: + /// - Members, RunnersUp (remove_and_replace_member) + /// Else, since this is a root call and will go into phragmen, we assume full block for now. /// # - #[weight = (2_000_000_000, DispatchClass::Operational)] + #[weight = FunctionOf( + |(_, has_replacement): (_, &bool)| if *has_replacement { + 48 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 2) + } else { + T::MaximumBlockWeight::get() + }, + DispatchClass::Normal, + Pays::Yes, + )] fn remove_member( origin, who: ::Source, @@ -655,8 +671,11 @@ decl_module! { (true, false) => { // prediction was that we will NOT have a replacement, and now this call is // aborting whilst charging a metric ton of weight. Refund and abort. - // TODO: fix refund. - return Err(Error::::InvalidReplacement.with_weight(2_000_000_000).into()); + return Err(Error::::InvalidReplacement.with_weight( + // refund to the variant where we have a replacement. This still an + // overestimate but fine for now. + 48 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 0) + ).into()); } } @@ -678,12 +697,8 @@ decl_module! { /// What to do at the end of each block. Checks if an election needs to happen or not. fn on_initialize(n: T::BlockNumber) -> Weight { - if let Err(e) = Self::end_block(n) { - print("Guru meditation"); - print(e); - } - - 0 + // returns the correct weight. + Self::end_block(n) } } } @@ -724,6 +739,8 @@ impl Module { /// /// Note that this function _will_ call into `T::ChangeMembers` in case any change happens /// (`Ok(true)`). + /// + /// If replacement exists, this will read and write from/into both `Members` and `RunnersUp`. fn remove_and_replace_member(who: &T::AccountId) -> Result { let mut members_with_stake = Self::members(); if let Ok(index) = members_with_stake.binary_search_by(|(ref m, ref _s)| m.cmp(who)) { @@ -831,6 +848,8 @@ impl Module { /// This will clean always clean the storage associated with the voter, and remove the balance /// lock. Optionally, it would also return the reserved voting bond if indicated by `unreserve`. /// If unreserve is true, has 3 storage reads and 1 reads. + /// + /// DB access: Voting and Lock are always written to, if unreserve, then 1 read and write added. fn do_remove_voter(who: &T::AccountId, unreserve: bool) { // remove storage and lock. Voting::::remove(who); @@ -855,13 +874,14 @@ impl Module { /// /// Runs phragmen election and cleans all the previous candidate state. The voter state is NOT /// cleaned and voters must themselves submit a transaction to retract. - fn end_block(block_number: T::BlockNumber) -> DispatchResult { + fn end_block(block_number: T::BlockNumber) -> Weight { if !Self::term_duration().is_zero() { if (block_number % Self::term_duration()).is_zero() { Self::do_phragmen(); + return T::MaximumBlockWeight::get() } } - Ok(()) + 0 } /// Run the phragmen election with all required side processes and state updates. @@ -2429,6 +2449,7 @@ mod tests { assert_eq!(Elections::runners_up_ids(), vec![3]); // there is a replacement! and this one needs a weight refund. + // TODO: check refund assert_err_ignore_postinfo!( Elections::remove_member(Origin::ROOT, 4, false), Error::::InvalidReplacement, From 71678cf214fffd7b85de45cc7f7ad44121d216f7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 May 2020 14:45:41 +0200 Subject: [PATCH 19/30] final tunes --- frame/elections-phragmen/src/lib.rs | 115 ++++++++++++++++------------ 1 file changed, 66 insertions(+), 49 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 04bed9680997b..50b90376abb25 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -1106,7 +1106,7 @@ mod tests { use substrate_test_utils::assert_eq_uvec; use sp_core::H256; use sp_runtime::{ - Perbill, testing::Header, BuildStorage, + Perbill, testing::Header, BuildStorage, DispatchResult, traits::{BlakeTwo256, IdentityLookup, Block as BlockT}, }; use crate as elections_phragmen; @@ -1468,7 +1468,7 @@ mod tests { // they will persist since they have self vote. System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![1, 2]); }) @@ -1485,7 +1485,7 @@ mod tests { // they will persist since they have self vote. System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![1, 2]); }) @@ -1533,7 +1533,7 @@ mod tests { assert_eq!(Elections::candidates(), vec![]); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![]); assert_eq!(Elections::runners_up(), vec![]); @@ -1584,7 +1584,7 @@ mod tests { assert_eq!(Elections::runners_up(), vec![]); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert!(Elections::is_candidate(&1).is_err()); assert!(Elections::is_candidate(&2).is_err()); @@ -1616,7 +1616,7 @@ mod tests { assert_ok!(vote(Origin::signed(2), vec![5], 20)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![5]); assert_eq!(Elections::runners_up(), vec![]); @@ -1640,7 +1640,7 @@ mod tests { assert_ok!(vote(Origin::signed(1), vec![3], 10)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![3]); @@ -1714,7 +1714,24 @@ mod tests { #[test] fn vote_update_need_to_set_flag() { - todo!(); + ExtBuilder::default().build_and_execute(|| { + assert_eq!(balances(&2), (20, 0)); + + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20, true)); + + assert_eq!(balances(&2), (18, 2)); + assert_eq!(has_lock(&2), 20); + assert_eq!(Elections::locked_stake_of(&2), 20); + + // can update only with correct flag + assert_noop!( + Elections::vote(Origin::signed(2), vec![5], 20, true), + Error::::InvalidWouldCreate, + ); + assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20, false)); + }); } #[test] @@ -1736,7 +1753,7 @@ mod tests { assert_ok!(vote(Origin::signed(2), vec![4, 5], 20)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); @@ -1759,7 +1776,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); @@ -1785,7 +1802,7 @@ mod tests { assert_ok!(Elections::renounce_candidacy(Origin::signed(4), Renouncing::Candidate(3))); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![3, 5]); assert_eq!(Elections::candidates(), vec![]); @@ -1817,7 +1834,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // now we have 2 members, 1 runner-up, and 1 new candidate assert_ok!(submit_candidacy(Origin::signed(2))); @@ -1915,7 +1932,7 @@ mod tests { assert_ok!(Elections::remove_voter(Origin::signed(4))); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![3, 5]); }); @@ -2001,7 +2018,7 @@ mod tests { assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![6]); @@ -2038,7 +2055,7 @@ mod tests { assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); @@ -2066,7 +2083,7 @@ mod tests { assert_ok!(vote(Origin::signed(4), vec![4], 40)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::candidates(), vec![]); @@ -2107,7 +2124,7 @@ mod tests { assert_eq!(Elections::election_rounds(), 0); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members(), vec![(3, 30), (5, 20)]); assert_eq!(Elections::runners_up(), vec![]); @@ -2129,7 +2146,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members(), vec![(5, 50)]); assert_eq!(Elections::election_rounds(), 1); @@ -2138,7 +2155,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // candidate 4 is affected by an old vote. assert_eq!(Elections::members(), vec![(4, 30), (5, 50)]); @@ -2161,7 +2178,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::election_rounds(), 1); assert_eq!(Elections::members_ids(), vec![4, 5]); @@ -2175,7 +2192,7 @@ mod tests { assert_ok!(submit_candidacy(Origin::signed(4))); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::candidates(), vec![]); assert_eq!(Elections::election_rounds(), 1); @@ -2202,7 +2219,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![4], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // sorted based on account id. assert_eq!(Elections::members_ids(), vec![4, 5]); // sorted based on merit (least -> most) @@ -2229,14 +2246,14 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members(), vec![(4, 40), (5, 50)]); assert_eq!(Elections::runners_up(), vec![(2, 20), (3, 30)]); assert_ok!(vote(Origin::signed(5), vec![5], 15)); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members(), vec![(3, 30), (4, 40)]); assert_eq!(Elections::runners_up(), vec![(5, 15), (2, 20)]); }); @@ -2254,7 +2271,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![2]); assert_eq!(balances(&2), (15, 5)); @@ -2263,7 +2280,7 @@ mod tests { assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::runners_up_ids(), vec![3]); assert_eq!(balances(&2), (15, 2)); @@ -2282,14 +2299,14 @@ mod tests { assert_eq!(balances(&5), (45, 5)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![5]); assert_ok!(Elections::remove_voter(Origin::signed(5))); assert_eq!(balances(&5), (47, 3)); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![]); assert_eq!(balances(&5), (47, 0)); @@ -2308,7 +2325,7 @@ mod tests { assert_eq!(balances(&3), (27, 3)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![5]); @@ -2329,7 +2346,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::election_rounds(), 1); @@ -2346,7 +2363,7 @@ mod tests { assert_eq!(Elections::candidates(), vec![2, 3]); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // 4 removed; 5 and 3 are the new best. assert_eq!(Elections::members_ids(), vec![3, 5]); @@ -2370,7 +2387,7 @@ mod tests { let check_at_block = |b: u32| { System::set_block_number(b.into()); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // we keep re-electing the same folks. assert_eq!(Elections::members(), vec![(4, 40), (5, 50)]); assert_eq!(Elections::runners_up(), vec![(2, 20), (3, 30)]); @@ -2398,7 +2415,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::election_rounds(), 1); @@ -2424,7 +2441,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); // no replacement yet. @@ -2444,7 +2461,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![3]); @@ -2474,7 +2491,7 @@ mod tests { assert_eq!(Elections::election_rounds(), 0); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![3, 5]); assert_eq!(Elections::election_rounds(), 1); @@ -2485,7 +2502,7 @@ mod tests { // meanwhile, no one cares to become a candidate again. System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![]); assert_eq!(Elections::election_rounds(), 2); }); @@ -2501,7 +2518,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![5], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_ok!(submit_candidacy(Origin::signed(1))); @@ -2519,7 +2536,7 @@ mod tests { assert_ok!(vote(Origin::signed(1), vec![1], 10)); System::set_block_number(10); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // 3, 4 are new members, must still be bonded, nothing slashed. assert_eq!(Elections::members(), vec![(3, 30), (4, 48)]); @@ -2549,7 +2566,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![10], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq_uvec!(Elections::members_ids(), vec![3, 4]); assert_eq!(Elections::election_rounds(), 1); @@ -2570,7 +2587,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![4], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); // id: low -> high. assert_eq!(Elections::members(), vec![(4, 50), (5, 40)]); // merit: low -> high. @@ -2606,7 +2623,7 @@ mod tests { assert_ok!(vote(Origin::signed(5), vec![2], 50)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![2, 4]); assert_ok!(Elections::remove_member(Origin::ROOT, 2, true)); @@ -2628,7 +2645,7 @@ mod tests { assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![2, 3]); @@ -2651,7 +2668,7 @@ mod tests { assert_ok!(vote(Origin::signed(4), vec![4], 40)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![]); @@ -2679,7 +2696,7 @@ mod tests { assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![2, 3]); @@ -2706,7 +2723,7 @@ mod tests { // assert_ok!(vote(Origin::signed(5), vec![2], 50)); // System::set_block_number(5); - // assert_ok!(Elections::end_block(System::block_number())); + // Elections::end_block(System::block_number()); // assert_eq!(Elections::members_ids(), vec![2, 4]); // assert_eq!(ELections::runners_up_ids(), vec![3, 5]); @@ -2759,7 +2776,7 @@ mod tests { assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![3]); @@ -2783,7 +2800,7 @@ mod tests { assert_ok!(vote(Origin::signed(3), vec![3], 30)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![4, 5]); assert_eq!(Elections::runners_up_ids(), vec![3]); @@ -2834,7 +2851,7 @@ mod tests { assert_ok!(vote(Origin::signed(2), vec![2], 20)); System::set_block_number(5); - assert_ok!(Elections::end_block(System::block_number())); + Elections::end_block(System::block_number()); assert_eq!(Elections::members_ids(), vec![1, 4]); assert_eq!(Elections::runners_up_ids(), vec![2, 3]); From 8990de6d69b31ddfb6ea54d6e8c56e468c0ed856 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Wed, 13 May 2020 14:54:26 +0200 Subject: [PATCH 20/30] Update frame/elections-phragmen/src/lib.rs --- frame/elections-phragmen/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 50b90376abb25..ce4541bcbb5e0 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -415,6 +415,8 @@ decl_module! { /// /// # /// No Base weight based on min square analysis. + /// Complexity of candidate_count: 1.755 us + /// Complexity of vote_count: 17.97us /// State reads: /// - Voting(reporter) /// - Candidate.len() From 2d963b25fa57e9f6935ee40b4a55749cf74ddb89 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 May 2020 15:14:42 +0200 Subject: [PATCH 21/30] Fix fix --- frame/elections-phragmen/src/lib.rs | 7 +++---- frame/staking/src/lib.rs | 2 +- frame/support/src/lib.rs | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 50b90376abb25..a99ed6e554adb 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -831,7 +831,6 @@ impl Module { /// O(NLogM) with M candidates and `who` having voted for `N` of them. /// Reads Members, RunnersUp, Candidates and Voting(who) from database. fn is_defunct_voter(who: &T::AccountId) -> bool { - // TODO: optimise this if Self::is_voter(who) { Self::votes_of(who) .iter() @@ -1100,7 +1099,7 @@ impl ContainsLengthBound for Module { mod tests { use super::*; use std::cell::RefCell; - use frame_support::{assert_ok, assert_noop, assert_err_ignore_postinfo, parameter_types, + use frame_support::{assert_ok, assert_noop, assert_err_with_weight, parameter_types, weights::Weight, }; use substrate_test_utils::assert_eq_uvec; @@ -2466,10 +2465,10 @@ mod tests { assert_eq!(Elections::runners_up_ids(), vec![3]); // there is a replacement! and this one needs a weight refund. - // TODO: check refund - assert_err_ignore_postinfo!( + assert_err_with_weight!( Elections::remove_member(Origin::ROOT, 4, false), Error::::InvalidReplacement, + Some(48000000) // only thing that matters for now is that it is NOT the full block. ); }); } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 7e5f64722f230..71042d69b3a4d 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -264,7 +264,7 @@ //! - [Session](../pallet_session/index.html): Used to manage sessions. Also, a list of new //! validators is stored in the Session module's `Validators` at the end of each era. -// #![recursion_limit = "128"] +#![recursion_limit = "128"] #![cfg_attr(not(feature = "std"), no_std)] #[cfg(test)] diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 08c70950409e9..1a7ba18a3dbd8 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -223,7 +223,7 @@ macro_rules! assert_err_with_weight { ($call:expr, $err:expr, $weight:expr $(,)? ) => { if let Err(dispatch_err_with_post) = $call { $crate::assert_err!($call.map(|_| ()).map_err(|e| e.error), $err); - assert_eq!(dispatch_err_with_post.post_info, $weight.into()); + assert_eq!(dispatch_err_with_post.post_info.actual_weight, $weight.into()); } else { panic!("expected Err(_), got Ok(_).") } From 7c83d285550d915330bd860dfcfea758698d113e Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Wed, 13 May 2020 15:16:02 +0200 Subject: [PATCH 22/30] Update frame/elections-phragmen/src/lib.rs --- frame/elections-phragmen/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index ce4541bcbb5e0..66471b9dfda8e 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -410,8 +410,8 @@ decl_module! { /// longer a candidate nor an active member or a runner-up. /// /// - /// The origin must provide the number of current candidates for the purpose of accurate - /// weight calculation. + /// The origin must provide the number of current candidates and votes of the reported target + /// for the purpose of accurate weight calculation. /// /// # /// No Base weight based on min square analysis. From 189c3315b31e8bc6f72a47168b89a2dd86e3cfd2 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 May 2020 19:07:32 +0200 Subject: [PATCH 23/30] Update frame/elections-phragmen/src/benchmarking.rs Co-authored-by: Alexander Popiak --- frame/elections-phragmen/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index d9bd3d14d33f1..559e4f5dd7336 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -43,7 +43,7 @@ fn endowed_account(name: &'static str, index: u32) -> T::AccountId { account } -/// Account ot lookup type of system trait. +/// Account to lookup type of system trait. fn as_lookup(account: T::AccountId) -> Lookup { T::Lookup::unlookup(account) } From 2be28b69ca571a6239e09dce5733f0fcc21b8eb4 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 May 2020 19:17:40 +0200 Subject: [PATCH 24/30] Update frame/elections-phragmen/src/benchmarking.rs Co-authored-by: Alexander Popiak --- frame/elections-phragmen/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 559e4f5dd7336..cacc3cc328b94 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -59,7 +59,7 @@ fn candidate_count() -> u32 { >::decode_len().unwrap_or(0usize) as u32 } -/// Get the number of voter of a voter. +/// Get the number of votes of a voter. fn vote_count_of(who: &T::AccountId) -> u32 { >::get(who).1.len() as u32 } From c3bdc88e8929f813dc2a6f9291ff188888787478 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 May 2020 19:21:27 +0200 Subject: [PATCH 25/30] Update to latest weights --- frame/elections-phragmen/src/lib.rs | 84 ++++++++++++++--------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 1eafb165054e1..2f2bcf94353a2 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -316,22 +316,22 @@ decl_module! { /// and keep some for further transactions. /// /// # - /// Base weight: 50us + /// Base weight: if would_create { 47.93 µs } else { 38.46 µs } /// State reads: /// - Candidates.len() + Members.len() + RunnersUp.len() /// - Voting (is_voter) - /// - AccountBalance(who) (unreserve + total_balance) + /// - [AccountBalance(who) (unreserve + total_balance)] /// State writes: /// - Voting /// - Lock - /// - AccountBalance(who) (unreserve -- only when would_create is true) + /// - [AccountBalance(who) (unreserve -- only when would_create is true)] /// # #[weight = FunctionOf( |(_, _, would_create): (_, _, &bool)| - 50 * WEIGHT_PER_MICROS + if *would_create { - T::DbWeight::get().reads_writes(5, 3) + if *would_create { + 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) } else { - T::DbWeight::get().reads_writes(5, 2) + 40 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) }, DispatchClass::Normal, Pays::Yes, @@ -383,17 +383,17 @@ decl_module! { /// Remove `origin` as a voter. This removes the lock and returns the bond. /// /// # - /// Base weight: 35us + /// Base weight: 36.8 µs /// All state access is from do_remove_voter. /// State reads: /// - Voting - /// - AccountData(who) + /// - [AccountData(who)] /// State writes: /// - Voting /// - Locks - /// - AccountData(who) + /// - [AccountData(who)] /// # - #[weight = 35 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(2, 3)] + #[weight = 35 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 2)] fn remove_voter(origin) { let who = ensure_signed(origin)?; ensure!(Self::is_voter(&who), Error::::MustBeVoter); @@ -410,13 +410,13 @@ decl_module! { /// longer a candidate nor an active member or a runner-up. /// /// - /// The origin must provide the number of current candidates and votes of the reported target + /// The origin must provide the number of current candidates and votes of the reported target /// for the purpose of accurate weight calculation. /// /// # /// No Base weight based on min square analysis. - /// Complexity of candidate_count: 1.755 us - /// Complexity of vote_count: 17.97us + /// Complexity of candidate_count: 1.755 µs + /// Complexity of vote_count: 18.51 µs /// State reads: /// - Voting(reporter) /// - Candidate.len() @@ -424,15 +424,15 @@ decl_module! { /// - Candidates, Members, RunnersUp (is_defunct_voter) [and Voting(Target) -- already counted] /// State writes: /// - Lock(reporter || target) - /// - AccountBalance(reporter) + AccountBalance(target) + /// - [AccountBalance(reporter)] + AccountBalance(target) /// - Voting(reporter || target) /// Note: the db access is worse with respect to db, which is when the report is correct. /// # #[weight = FunctionOf( |(defunct,): (&DefunctVoter<::Source>,)| defunct.candidate_count as Weight * (2 * WEIGHT_PER_MICROS) + - defunct.vote_count as Weight * (18 * WEIGHT_PER_MICROS) + - T::DbWeight::get().reads_writes(6, 4), + defunct.vote_count as Weight * (19 * WEIGHT_PER_MICROS) + + T::DbWeight::get().reads_writes(6, 3), DispatchClass::Normal, Pays::Yes, )] @@ -484,23 +484,23 @@ decl_module! { /// removed. /// /// # - /// Base weight = 35us - /// Complexity of candidate_count: 0.373us + /// Base weight = 33.33 µs + /// Complexity of candidate_count: 0.375 µs /// State reads: /// - Candidates.len() /// - Candidates /// - Members /// - RunnersUp - /// - AccountBalance(who) + /// - [AccountBalance(who)] /// State writes: - /// - AccountBalance(who) + /// - [AccountBalance(who)] /// - Candidates /// # #[weight = FunctionOf( |(candidate_count,): (&u32,)| 35 * WEIGHT_PER_MICROS + *candidate_count as Weight * (375 * WEIGHT_PER_NANOS) + - T::DbWeight::get().reads_writes(5, 2), + T::DbWeight::get().reads_writes(4, 1), DispatchClass::Normal, Pays::Yes, )] @@ -539,47 +539,47 @@ decl_module! { /// Similar to [`remove_voter`], if replacement runners exists, they are immediately used. /// /// If a candidate is renouncing: - /// Base weight: 16.57us - /// Complexity of candidate_count: 0.235 + /// Base weight: 17.28 µs + /// Complexity of candidate_count: 0.235 µs /// State reads: /// - Candidates - /// - AccountBalance(who) (unreserve) + /// - [AccountBalance(who) (unreserve)] /// State writes: /// - Candidates - /// - AccountBalance(who) (unreserve) + /// - [AccountBalance(who) (unreserve)] /// If member is renouncing: - /// Base weight: 46.25 + /// Base weight: 46.25 µs /// State reads: /// - Members, RunnersUp (remove_and_replace_member), - /// - AccountData(who) (unreserve) + /// - [AccountData(who) (unreserve)] /// State writes: /// - Members, RunnersUp (remove_and_replace_member), - /// - AccountData(who) (unreserve) + /// - [AccountData(who) (unreserve)] /// If runner is renouncing: - /// Base weight: 46.25 + /// Base weight: 46.25 µs /// State reads: /// - RunnersUp (remove_and_replace_member), - /// - AccountData(who) (unreserve) + /// - [AccountData(who) (unreserve)] /// State writes: /// - RunnersUp (remove_and_replace_member), - /// - AccountData(who) (unreserve) + /// - [AccountData(who) (unreserve)] /// /// TODO: and calls into ChangeMembers?? /// #[weight = FunctionOf( |(renouncing,): (&Renouncing,)| match *renouncing { Renouncing::Candidate(count) => { - 16 * WEIGHT_PER_MICROS + + 18 * WEIGHT_PER_MICROS + (count as Weight) * 235 * WEIGHT_PER_NANOS + - T::DbWeight::get().reads_writes(2, 2) + T::DbWeight::get().reads_writes(1, 1) }, Renouncing::Member => { - 47 * WEIGHT_PER_MICROS + - T::DbWeight::get().reads_writes(3, 3) + 46 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(2, 2) }, Renouncing::RunnerUp => { - 47 * WEIGHT_PER_MICROS + - T::DbWeight::get().reads_writes(2, 2) + 46 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(1, 1) } }, DispatchClass::Normal, @@ -635,7 +635,7 @@ decl_module! { /// /// # /// If we have a replacement: - /// - Base weight: 47.15 us + /// - Base weight: 50.93 µs /// - State reads: /// - RunnersUp.len() /// - Members, RunnersUp (remove_and_replace_member) @@ -645,7 +645,7 @@ decl_module! { /// # #[weight = FunctionOf( |(_, has_replacement): (_, &bool)| if *has_replacement { - 48 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 2) + 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 2) } else { T::MaximumBlockWeight::get() }, @@ -674,9 +674,9 @@ decl_module! { // prediction was that we will NOT have a replacement, and now this call is // aborting whilst charging a metric ton of weight. Refund and abort. return Err(Error::::InvalidReplacement.with_weight( - // refund to the variant where we have a replacement. This still an - // overestimate but fine for now. - 48 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 0) + // refund. The weight value comes from a benchmark which is special to this. + // 5.751 µs + 6 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 0) ).into()); } } From 43035842b29eda7305698f03d9d130780a407169 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 May 2020 09:14:58 +0200 Subject: [PATCH 26/30] Some fixes --- frame/elections-phragmen/src/lib.rs | 125 ++++++++++------------------ 1 file changed, 46 insertions(+), 79 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 2f2bcf94353a2..befaaec4d0492 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -90,10 +90,7 @@ use sp_runtime::{ }; use frame_support::{ decl_storage, decl_event, ensure, decl_module, decl_error, - weights::{ - Weight, DispatchClass, Pays, FunctionOf, - constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}, - }, + weights::{Weight, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}}, storage::{StorageMap, IterableStorageMap}, dispatch::{DispatchResultWithPostInfo, WithPostDispatchInfo}, traits::{ @@ -326,16 +323,11 @@ decl_module! { /// - Lock /// - [AccountBalance(who) (unreserve -- only when would_create is true)] /// # - #[weight = FunctionOf( - |(_, _, would_create): (_, _, &bool)| - if *would_create { - 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) - } else { - 40 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) - }, - DispatchClass::Normal, - Pays::Yes, - )] + #[weight = if *would_create { + 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) + } else { + 40 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) + }] fn vote( origin, votes: Vec, @@ -428,14 +420,11 @@ decl_module! { /// - Voting(reporter || target) /// Note: the db access is worse with respect to db, which is when the report is correct. /// # - #[weight = FunctionOf( - |(defunct,): (&DefunctVoter<::Source>,)| - defunct.candidate_count as Weight * (2 * WEIGHT_PER_MICROS) + - defunct.vote_count as Weight * (19 * WEIGHT_PER_MICROS) + - T::DbWeight::get().reads_writes(6, 3), - DispatchClass::Normal, - Pays::Yes, - )] + #[weight = + defunct.candidate_count as Weight * (2 * WEIGHT_PER_MICROS) + + defunct.vote_count as Weight * (19 * WEIGHT_PER_MICROS) + + T::DbWeight::get().reads_writes(6, 3) + ] fn report_defunct_voter( origin, defunct: DefunctVoter<::Source>, @@ -496,14 +485,11 @@ decl_module! { /// - [AccountBalance(who)] /// - Candidates /// # - #[weight = FunctionOf( - |(candidate_count,): (&u32,)| - 35 * WEIGHT_PER_MICROS + - *candidate_count as Weight * (375 * WEIGHT_PER_NANOS) + - T::DbWeight::get().reads_writes(4, 1), - DispatchClass::Normal, - Pays::Yes, - )] + #[weight = + 35 * WEIGHT_PER_MICROS + + *candidate_count as Weight * (375 * WEIGHT_PER_NANOS) + + T::DbWeight::get().reads_writes(4, 1) + ] fn submit_candidacy(origin, #[compact] candidate_count: u32) { let who = ensure_signed(origin)?; @@ -566,25 +552,21 @@ decl_module! { /// /// TODO: and calls into ChangeMembers?? /// - #[weight = FunctionOf( - |(renouncing,): (&Renouncing,)| match *renouncing { - Renouncing::Candidate(count) => { - 18 * WEIGHT_PER_MICROS + - (count as Weight) * 235 * WEIGHT_PER_NANOS + - T::DbWeight::get().reads_writes(1, 1) - }, - Renouncing::Member => { - 46 * WEIGHT_PER_MICROS + - T::DbWeight::get().reads_writes(2, 2) - }, - Renouncing::RunnerUp => { - 46 * WEIGHT_PER_MICROS + - T::DbWeight::get().reads_writes(1, 1) - } + #[weight = match *renouncing { + Renouncing::Candidate(count) => { + 18 * WEIGHT_PER_MICROS + + (count as Weight) * 235 * WEIGHT_PER_NANOS + + T::DbWeight::get().reads_writes(1, 1) }, - DispatchClass::Normal, - Pays::Yes, - )] + Renouncing::Member => { + 46 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(2, 2) + }, + Renouncing::RunnerUp => { + 46 * WEIGHT_PER_MICROS + + T::DbWeight::get().reads_writes(1, 1) + } + }] fn renounce_candidacy(origin, renouncing: Renouncing) { let who = ensure_signed(origin)?; match renouncing { @@ -643,15 +625,11 @@ decl_module! { /// - Members, RunnersUp (remove_and_replace_member) /// Else, since this is a root call and will go into phragmen, we assume full block for now. /// # - #[weight = FunctionOf( - |(_, has_replacement): (_, &bool)| if *has_replacement { - 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 2) - } else { - T::MaximumBlockWeight::get() - }, - DispatchClass::Normal, - Pays::Yes, - )] + #[weight = if *has_replacement { + 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(3, 2) + } else { + T::MaximumBlockWeight::get() + }] fn remove_member( origin, who: ::Source, @@ -661,26 +639,14 @@ decl_module! { let who = T::Lookup::lookup(who)?; let will_have_replacement = >::decode_len().unwrap_or(0) > 0; - match (will_have_replacement, has_replacement) { - (true, true) | (false, false) => { - // correct prediction. nothing - } - (false, true) => { - // prediction was that we will have a replacement, so we don't charge a whole - // lot of weight. just abort. - return Err(Error::::InvalidReplacement.into()); - } - (true, false) => { - // prediction was that we will NOT have a replacement, and now this call is - // aborting whilst charging a metric ton of weight. Refund and abort. - return Err(Error::::InvalidReplacement.with_weight( - // refund. The weight value comes from a benchmark which is special to this. - // 5.751 µs - 6 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 0) - ).into()); - } - } - + if will_have_replacement != has_replacement { + // In both cases, we will change more weight than neede. Refund and abort. + return Err(Error::::InvalidReplacement.with_weight( + // refund. The weight value comes from a benchmark which is special to this. + // 5.751 µs + 6 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(1, 0) + )); + } // else, prediction was correct. Self::remove_and_replace_member(&who).map(|had_replacement| { let (imbalance, _) = T::Currency::slash_reserved(&who, T::CandidacyBond::get()); @@ -2446,9 +2412,10 @@ mod tests { assert_eq!(Elections::members_ids(), vec![4, 5]); // no replacement yet. - assert_noop!( + assert_err_with_weight!( Elections::remove_member(Origin::ROOT, 4, true), Error::::InvalidReplacement, + Some(6000000), ); }); @@ -2470,7 +2437,7 @@ mod tests { assert_err_with_weight!( Elections::remove_member(Origin::ROOT, 4, false), Error::::InvalidReplacement, - Some(48000000) // only thing that matters for now is that it is NOT the full block. + Some(6000000) // only thing that matters for now is that it is NOT the full block. ); }); } From 5f74c7ac5db7e3fffe6db6ed079e39c5fe477ca5 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 May 2020 11:09:18 +0200 Subject: [PATCH 27/30] Fix dual voter read from @thiolliere --- frame/elections-phragmen/src/lib.rs | 72 ++++++++++++++--------------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index befaaec4d0492..755e2adf00e9c 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -413,7 +413,7 @@ decl_module! { /// - Voting(reporter) /// - Candidate.len() /// - Voting(Target) - /// - Candidates, Members, RunnersUp (is_defunct_voter) [and Voting(Target) -- already counted] + /// - Candidates, Members, RunnersUp (is_defunct_voter) /// State writes: /// - Lock(reporter || target) /// - [AccountBalance(reporter)] + AccountBalance(target) @@ -441,14 +441,20 @@ decl_module! { >::decode_len().unwrap_or(0) as u32 <= candidate_count, Error::::InvalidCandidateCount, ); - // Optimisation Note: We read this again in is_defunct_voter, still duplicate overlay - // read. + + let (_, votes) = >::get(&target); + // indirect way to ensure target is a voter. We could call into `::contains()`, but it + // would have the same effect with one extra db access. Note that votes cannot be + // submitted with length 0. Hence, a non-zero length means that the target is a voter. + ensure!(votes.len() > 0, Error::::MustBeVoter); + + // ensure that the size of votes that need to be searched is correct. ensure!( - >::get(&target).1.len() as u32 <= vote_count, + votes.len() as u32 <= vote_count, Error::::InvalidVoteCount, ); - let valid = Self::is_defunct_voter(&target); + let valid = Self::is_defunct_voter(&votes); if valid { // reporter will get the voting bond of the target T::Currency::repatriate_reserved(&target, &reporter, T::VotingBond::get(), BalanceStatus::Free)?; @@ -792,22 +798,15 @@ impl Module { Self::runners_up().into_iter().map(|(r, _)| r).collect::>() } - /// Check if `who` is a defunct voter. - /// - /// Note that false is returned if `who` is not a voter at all. + /// Check if `votes` will correspond to a defunct voter. As no origin is part of the inputs, + /// this function does not check the origin at all. /// /// O(NLogM) with M candidates and `who` having voted for `N` of them. /// Reads Members, RunnersUp, Candidates and Voting(who) from database. - fn is_defunct_voter(who: &T::AccountId) -> bool { - if Self::is_voter(who) { - Self::votes_of(who) - .iter() - .all(|v| - !Self::is_member(v) && !Self::is_runner_up(v) && !Self::is_candidate(v).is_ok() - ) - } else { - false - } + fn is_defunct_voter(votes: &[T::AccountId]) -> bool { + votes.iter().all(|v| + !Self::is_member(v) && !Self::is_runner_up(v) && !Self::is_candidate(v).is_ok() + ) } /// Remove a certain someone as a voter. @@ -832,11 +831,6 @@ impl Module { Voting::::get(who).0 } - /// The locked stake of a voter. - fn votes_of(who: &T::AccountId) -> Vec { - Voting::::get(who).1 - } - /// Check there's nothing to do this block. /// /// Runs phragmen election and cleans all the previous candidate state. The voter state is NOT @@ -1397,11 +1391,15 @@ mod tests { } } + fn votes_of(who: &u64) -> Vec { + Voting::::get(who).1 + } + fn defunct_for(who: u64) -> DefunctVoter { DefunctVoter { who, candidate_count: Elections::candidates().len() as u32, - vote_count: Elections::votes_of(&who).len() as u32 + vote_count: votes_of(&who).len() as u32 } } @@ -1420,7 +1418,7 @@ mod tests { assert!(Elections::is_candidate(&1).is_err()); assert_eq!(all_voters(), vec![]); - assert_eq!(Elections::votes_of(&1), vec![]); + assert_eq!(votes_of(&1), vec![]); }); } @@ -1851,13 +1849,13 @@ mod tests { assert_eq_uvec!(all_voters(), vec![2, 3]); assert_eq!(Elections::locked_stake_of(&2), 20); assert_eq!(Elections::locked_stake_of(&3), 30); - assert_eq!(Elections::votes_of(&2), vec![5]); - assert_eq!(Elections::votes_of(&3), vec![5]); + assert_eq!(votes_of(&2), vec![5]); + assert_eq!(votes_of(&3), vec![5]); assert_ok!(Elections::remove_voter(Origin::signed(2))); assert_eq_uvec!(all_voters(), vec![3]); - assert_eq!(Elections::votes_of(&2), vec![]); + assert_eq!(votes_of(&2), vec![]); assert_eq!(Elections::locked_stake_of(&2), 0); assert_eq!(balances(&2), (20, 0)); @@ -1992,19 +1990,19 @@ mod tests { assert_eq!(Elections::candidates(), vec![]); // all of them have a member or runner-up that they voted for. - assert_eq!(Elections::is_defunct_voter(&5), false); - assert_eq!(Elections::is_defunct_voter(&4), false); - assert_eq!(Elections::is_defunct_voter(&2), false); - assert_eq!(Elections::is_defunct_voter(&6), false); + assert_eq!(Elections::is_defunct_voter(&votes_of(&5)), false); + assert_eq!(Elections::is_defunct_voter(&votes_of(&4)), false); + assert_eq!(Elections::is_defunct_voter(&votes_of(&2)), false); + assert_eq!(Elections::is_defunct_voter(&votes_of(&6)), false); // defunct - assert_eq!(Elections::is_defunct_voter(&3), true); + assert_eq!(Elections::is_defunct_voter(&votes_of(&3)), true); assert_ok!(submit_candidacy(Origin::signed(1))); assert_ok!(vote(Origin::signed(1), vec![1], 10)); // has a candidate voted for. - assert_eq!(Elections::is_defunct_voter(&1), false); + assert_eq!(Elections::is_defunct_voter(&votes_of(&1)), false); }); } @@ -2081,9 +2079,9 @@ mod tests { assert_eq_uvec!(all_voters(), vec![2, 3, 4]); - assert_eq!(Elections::votes_of(&2), vec![5]); - assert_eq!(Elections::votes_of(&3), vec![3]); - assert_eq!(Elections::votes_of(&4), vec![4]); + assert_eq!(votes_of(&2), vec![5]); + assert_eq!(votes_of(&3), vec![3]); + assert_eq!(votes_of(&4), vec![4]); assert_eq!(Elections::candidates(), vec![3, 4, 5]); assert_eq!(>::decode_len().unwrap(), 3); From 2f096f5887ead87f546459469b109fe2dd101116 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 May 2020 11:27:12 +0200 Subject: [PATCH 28/30] Remove todos --- frame/elections-phragmen/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 755e2adf00e9c..56faa7d411d0d 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -556,7 +556,7 @@ decl_module! { /// - RunnersUp (remove_and_replace_member), /// - [AccountData(who) (unreserve)] /// - /// TODO: and calls into ChangeMembers?? + /// Weight note: The call into changeMembers need to be accounted for. /// #[weight = match *renouncing { Renouncing::Candidate(count) => { @@ -2808,7 +2808,6 @@ mod tests { #[test] fn behavior_with_dupe_candidate() { ExtBuilder::default().desired_runners_up(2).build_and_execute(|| { - // TODD: this is a demonstration and should be fixed with #4593 >::put(vec![1, 1, 2, 3, 4]); assert_ok!(vote(Origin::signed(5), vec![1], 50)); From 4ba9e43b2b4b54c5ff28862a6bf414e083814986 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 May 2020 13:09:51 +0200 Subject: [PATCH 29/30] review from @shawntabrizi --- frame/elections-phragmen/src/lib.rs | 66 ++++++++--------------------- 1 file changed, 18 insertions(+), 48 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 56faa7d411d0d..39ae136b1d139 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -230,7 +230,6 @@ decl_storage! { T::Origin::from(Some(member.clone()).into()), vec![member.clone()], *stake, - true, ).expect("Genesis member could not vote."); member.clone() @@ -279,8 +278,6 @@ decl_error! { InvalidRenouncing, /// Prediction regarding replacement after member removal is wrong. InvalidReplacement, - /// The vote's would_create flag is invalid. - InvalidWouldCreate, } } @@ -298,8 +295,7 @@ decl_module! { const ModuleId: LockIdentifier = T::ModuleId::get(); /// Vote for a set of candidates for the upcoming round of election. This can be called to - /// set the initial votes, with `would_create = true`, or update already existing votes - /// with `would_create = false`. + /// set the initial votes, or update already existing votes. /// /// Upon initial voting, `value` units of `who`'s balance is locked and a bond amount is /// reserved. @@ -313,7 +309,7 @@ decl_module! { /// and keep some for further transactions. /// /// # - /// Base weight: if would_create { 47.93 µs } else { 38.46 µs } + /// Base weight: 47.93 µs /// State reads: /// - Candidates.len() + Members.len() + RunnersUp.len() /// - Voting (is_voter) @@ -321,18 +317,13 @@ decl_module! { /// State writes: /// - Voting /// - Lock - /// - [AccountBalance(who) (unreserve -- only when would_create is true)] + /// - [AccountBalance(who) (unreserve -- only when creating a new voter)] /// # - #[weight = if *would_create { - 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) - } else { - 40 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2) - }] + #[weight = 50 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 2)] fn vote( origin, votes: Vec, #[compact] value: BalanceOf, - would_create: bool, ) { let who = ensure_signed(origin)?; @@ -350,10 +341,9 @@ decl_module! { ensure!(votes.len() <= allowed_votes, Error::::TooManyVotes); ensure!(value > T::Currency::minimum_balance(), Error::::LowBalance); - ensure!(Self::is_voter(&who) == !would_create, Error::::InvalidWouldCreate); // first time voter. Reserve bond. - if would_create { + if !Self::is_voter(&who) { T::Currency::reserve(&who, T::VotingBond::get()) .map_err(|_| Error::::UnableToPayBond)?; } @@ -421,9 +411,9 @@ decl_module! { /// Note: the db access is worse with respect to db, which is when the report is correct. /// # #[weight = - defunct.candidate_count as Weight * (2 * WEIGHT_PER_MICROS) + - defunct.vote_count as Weight * (19 * WEIGHT_PER_MICROS) + - T::DbWeight::get().reads_writes(6, 3) + Weight::from(defunct.candidate_count).saturating_mul(2 * WEIGHT_PER_MICROS) + .saturating_add(Weight::from(defunct.vote_count).saturating_mul(19 * WEIGHT_PER_MICROS)) + .saturating_add(T::DbWeight::get().reads_writes(6, 3)) ] fn report_defunct_voter( origin, @@ -492,9 +482,9 @@ decl_module! { /// - Candidates /// # #[weight = - 35 * WEIGHT_PER_MICROS + - *candidate_count as Weight * (375 * WEIGHT_PER_NANOS) + - T::DbWeight::get().reads_writes(4, 1) + (35 * WEIGHT_PER_MICROS) + .saturating_add(Weight::from(*candidate_count).saturating_mul(375 * WEIGHT_PER_NANOS)) + .saturating_add(T::DbWeight::get().reads_writes(4, 1)) ] fn submit_candidacy(origin, #[compact] candidate_count: u32) { let who = ensure_signed(origin)?; @@ -560,9 +550,9 @@ decl_module! { /// #[weight = match *renouncing { Renouncing::Candidate(count) => { - 18 * WEIGHT_PER_MICROS + - (count as Weight) * 235 * WEIGHT_PER_NANOS + - T::DbWeight::get().reads_writes(1, 1) + (18 * WEIGHT_PER_MICROS) + .saturating_add(Weight::from(count).saturating_mul(235 * WEIGHT_PER_NANOS)) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) }, Renouncing::Member => { 46 * WEIGHT_PER_MICROS + @@ -1383,9 +1373,11 @@ mod tests { } fn vote(origin: Origin, votes: Vec, stake: u64) -> DispatchResult { + // historical note: helper function was created in a period of time in which the API of vote + // call was changing. Currently it is a wrapper for the original call and does not do much. + // Nonetheless, totally harmless if let Origin::system(frame_system::RawOrigin::Signed(account)) = origin { - let would_create = !Elections::is_voter(&account); - Elections::vote(origin, votes, stake, would_create) + Elections::vote(origin, votes, stake) } else { panic!("vote origin must be signed"); } @@ -1677,28 +1669,6 @@ mod tests { }); } - #[test] - fn vote_update_need_to_set_flag() { - ExtBuilder::default().build_and_execute(|| { - assert_eq!(balances(&2), (20, 0)); - - assert_ok!(submit_candidacy(Origin::signed(5))); - assert_ok!(submit_candidacy(Origin::signed(4))); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20, true)); - - assert_eq!(balances(&2), (18, 2)); - assert_eq!(has_lock(&2), 20); - assert_eq!(Elections::locked_stake_of(&2), 20); - - // can update only with correct flag - assert_noop!( - Elections::vote(Origin::signed(2), vec![5], 20, true), - Error::::InvalidWouldCreate, - ); - assert_ok!(Elections::vote(Origin::signed(2), vec![5], 20, false)); - }); - } - #[test] fn cannot_vote_for_no_candidate() { ExtBuilder::default().build_and_execute(|| { From d85090c0c99c64d28d43442ee4c82e36f516fff0 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 May 2020 13:15:05 +0200 Subject: [PATCH 30/30] Fix bench tests. --- frame/elections-phragmen/src/benchmarking.rs | 6 +++--- frame/elections-phragmen/src/lib.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index cacc3cc328b94..6de9ad57e244f 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -104,7 +104,7 @@ fn submit_candidates_with_self_vote(c: u32, prefix: &'static str) fn submit_voter(caller: T::AccountId, votes: Vec, stake: BalanceOf) -> Result<(), &'static str> { - >::vote(RawOrigin::Signed(caller).into(), votes, stake, true) + >::vote(RawOrigin::Signed(caller).into(), votes, stake) .map_err(|_| "failed to submit vote") } @@ -173,7 +173,7 @@ benchmarks! { // vote for all of them. let votes = all_candidates.into_iter().take(v).collect(); - }: _(RawOrigin::Signed(caller), votes, stake, true) + }: _(RawOrigin::Signed(caller), votes, stake) vote_update { let u in ...; @@ -192,7 +192,7 @@ benchmarks! { submit_voter::(caller.clone(), votes.clone(), stake)?; // new votes. votes.rotate_left(1); - }: vote(RawOrigin::Signed(caller), votes, stake, false) + }: vote(RawOrigin::Signed(caller), votes, stake) remove_voter { let u in ...; diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 39ae136b1d139..3b88cb6451ad5 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -1375,8 +1375,8 @@ mod tests { fn vote(origin: Origin, votes: Vec, stake: u64) -> DispatchResult { // historical note: helper function was created in a period of time in which the API of vote // call was changing. Currently it is a wrapper for the original call and does not do much. - // Nonetheless, totally harmless - if let Origin::system(frame_system::RawOrigin::Signed(account)) = origin { + // Nonetheless, totally harmless. + if let Origin::system(frame_system::RawOrigin::Signed(_account)) = origin { Elections::vote(origin, votes, stake) } else { panic!("vote origin must be signed");