From 55cf64847b303df7b500cddf6454b5ccd802f19c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:14:45 -0700 Subject: [PATCH 01/37] force rebag for unbond, rebond, and bond_extra --- frame/bags-list/src/list/tests.rs | 55 ++++++---- frame/bags-list/src/mock.rs | 2 +- frame/staking/src/benchmarking.rs | 168 +++++++++++++++++++++++++++-- frame/staking/src/testing_utils.rs | 37 +++++++ 4 files changed, 228 insertions(+), 34 deletions(-) diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index a6850bdd99cdc..92562f9a1fcd7 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -105,30 +105,41 @@ fn remove_last_node_in_bags_cleans_bag() { #[test] fn migrate_works() { - ExtBuilder::default().add_ids(vec![(710, 15), (711, 16), (712, 2_000)]).build_and_execute(|| { - // given - assert_eq!(get_bags(), vec![(10, vec![1]), (20, vec![710, 711]), (1000, vec![2, 3, 4]), (2_000, vec![712])]); - let old_thresholds = ::BagThresholds::get(); - assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]); + ExtBuilder::default() + .add_ids(vec![(710, 15), (711, 16), (712, 2_000)]) + .build_and_execute(|| { + // given + assert_eq!( + get_bags(), + vec![ + (10, vec![1]), + (20, vec![710, 711]), + (1000, vec![2, 3, 4]), + (2_000, vec![712]) + ] + ); + let old_thresholds = ::BagThresholds::get(); + assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]); - // when the new thresholds adds `15` and removes `2_000` - const NEW_THRESHOLDS: &'static [VoteWeight] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; - BagThresholds::set(NEW_THRESHOLDS); - // and we call - List::::migrate(old_thresholds); + // when the new thresholds adds `15` and removes `2_000` + const NEW_THRESHOLDS: &'static [VoteWeight] = + &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; + BagThresholds::set(NEW_THRESHOLDS); + // and we call + List::::migrate(old_thresholds); - // then - assert_eq!( - get_bags(), - vec![ - (10, vec![1]), - (15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15 - (20, vec![711]), - (1000, vec![2, 3, 4]), - (10_000, vec![712]), // nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000 - ] - ); - }); + // then + assert_eq!( + get_bags(), + vec![ + (10, vec![1]), + (15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15 + (20, vec![711]), + (1000, vec![2, 3, 4]), + (10_000, vec![712]), // nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000 + ] + ); + }); } mod list { diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index 440288511d333..f845d2be2c38c 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -33,7 +33,7 @@ impl frame_election_provider_support::VoteWeightProvider for StakingM 710 => 15, 711 => 16, 712 => 2_000, // special cases used for migrate test - _ => NextVoteWeight::get() + _ => NextVoteWeight::get(), } } #[cfg(any(feature = "runtime-benchmarks", test))] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index d8eef256fd782..070ba5873704d 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -21,9 +21,10 @@ use super::*; use crate::Pallet as Staking; use testing_utils::*; +use frame_election_provider_support::{SortedListProvider, VoteWeight}; use frame_support::{ pallet_prelude::*, - traits::{Currency, Get, Imbalance}, + traits::{Currency, CurrencyToVote, Get, Imbalance}, }; use sp_runtime::{ traits::{StaticLookup, Zero}, @@ -131,6 +132,78 @@ pub fn create_validator_with_nominators( Ok((v_stash, nominators)) } +struct RebagScenario { + dest_stash1: T::AccountId, + /// Stash that is expected to be rebagged. + origin_stash1: T::AccountId, + /// Controller of the Stash that is expected to be rebagged. + origin_controller1: T::AccountId, + origin_stash2: T::AccountId, + dest_bag_thresh: BalanceOf, + origin_bag_thresh: BalanceOf, +} + +impl RebagScenario { + /// An expensive rebag scenario: + /// + /// - the node to be rebagged (r) is the head of a bag that has at least one other node. The bag + /// itself will need to be read and written to update the head. The node pointed to by + /// r.next will need to be read and written as it will need to have its prev pointer updated. + /// + /// - the destination bag has at least one node, which will need its next pointer updated. + fn new( + origin_bag_thresh: BalanceOf, + dest_bag_thresh: BalanceOf, + ) -> Result { + let total_issuance = T::Currency::total_issuance(); + ensure!( + !origin_bag_thresh.is_zero() && !dest_bag_thresh.is_zero(), + "both thresholds must be greater than 0" + ); + + // create_stash_controller takes a factor, so we compute it. + let origin_factor: BalanceOf = + (origin_bag_thresh * 10u32.into() / T::Currency::minimum_balance()); + let dest_factor: BalanceOf = + (dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance()); + + // create a validator to nominate + let validator = create_validators::(1, 100).unwrap().first().unwrap().clone(); + + // create an account in the destination bag + let (dest_stash1, dest_controller1) = + create_stash_controller_b::(USER_SEED + 1, dest_factor, Default::default())?; + Staking::::nominate( + RawOrigin::Signed(dest_controller1).into(), + vec![validator.clone()], + )?; + + // create accounts in origin bag + let (origin_stash1, origin_controller1) = + create_stash_controller_b::(USER_SEED + 2, origin_factor, Default::default())?; + Staking::::nominate( + RawOrigin::Signed(origin_controller1.clone()).into(), + vec![validator.clone()], + )?; + + let (origin_stash2, origin_controller2) = + create_stash_controller_b::(USER_SEED + 3, origin_factor, Default::default())?; + Staking::::nominate( + RawOrigin::Signed(origin_controller2.clone()).into(), + vec![validator.clone()], + )?; + + Ok(RebagScenario { + dest_stash1, + origin_stash1, + origin_controller1, + origin_stash2, + dest_bag_thresh, + origin_bag_thresh, + }) + } +} + const USER_SEED: u32 = 999666; benchmarks! { @@ -148,12 +221,31 @@ benchmarks! { } bond_extra { - let (stash, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; - let max_additional = T::Currency::minimum_balance() * 10u32.into(); - let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // The worst case scenario includes the voter changing bags. + + let total_issuance = T::Currency::total_issuance(); + // the bag the voter will start at + let origin_bag_thresh = + T::CurrencyToVote::to_currency(1u128, total_issuance); + // the bag we will move the voter to + let dest_bag_thresh = + T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + + let scenario + = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + + let max_additional = dest_bag_thresh - origin_bag_thresh; + + let stash = scenario.origin_stash1.clone(); + let controller = scenario.origin_controller1.clone(); + let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let original_bonded: BalanceOf = ledger.active; whitelist_account!(stash); - }: _(RawOrigin::Signed(stash), max_additional) + + }: _(RawOrigin::Signed(stash.clone()), max_additional) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; @@ -161,8 +253,24 @@ benchmarks! { } unbond { - let (_, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; - let amount = T::Currency::minimum_balance() * 10u32.into(); + // Clean up any existing state. + clear_validators_and_nominators::(); + + // The worst case scenario includes the voter changing bags. + + let total_issuance = T::Currency::total_issuance(); + // the bag the voter will start at + let origin_bag_thresh = + T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + // the bag we will move the voter to + let dest_bag_thresh = + T::CurrencyToVote::to_currency(1u128, total_issuance); + let scenario + = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + + let stash = scenario.origin_stash1.clone(); + let controller = scenario.origin_controller1.clone(); + let amount = origin_bag_thresh - dest_bag_thresh; let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; whitelist_account!(controller); @@ -440,23 +548,61 @@ benchmarks! { rebond { let l in 1 .. MAX_UNLOCKING_CHUNKS as u32; - let (_, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; - let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); + + // Clean up any existing state. + clear_validators_and_nominators::(); + + // The worst case scenario includes the voter changing bags. + + let total_issuance = T::Currency::total_issuance(); + // the bag the voter will start at + let origin_bag_thresh = + T::CurrencyToVote::to_currency(1u128, total_issuance); + // the bag we will move the voter to + let dest_bag_thresh = + T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + let scenario + = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + + // rebond an amount that will put the user into the destination bag + let rebond_amount = dest_bag_thresh - origin_bag_thresh; + + // spread that amount to rebond across `l` unlocking chunks, + let value = rebond_amount / l.into(); + // so the sum of unlocking chunks puts voter into the dest bag + assert!(value * l.into() + origin_bag_thresh > origin_bag_thresh); + assert!(value * l.into() + origin_bag_thresh <= dest_bag_thresh); let unlock_chunk = UnlockChunk::> { - value: 1u32.into(), + value, era: EraIndex::zero(), }; + + let stash = scenario.origin_stash1.clone(); + let controller = scenario.origin_controller1.clone(); + let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); + for _ in 0 .. l { staking_ledger.unlocking.push(unlock_chunk.clone()) } Ledger::::insert(controller.clone(), staking_ledger.clone()); let original_bonded: BalanceOf = staking_ledger.active; whitelist_account!(controller); - }: _(RawOrigin::Signed(controller.clone()), (l + 100).into()) + + assert_eq!( + T::SortedListProvider::iter().collect::>(), + vec![scenario.dest_stash1.clone(), stash.clone(), scenario.origin_stash2.clone()] + ); + }: _(RawOrigin::Signed(controller.clone()), rebond_amount) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); + + // the ordering in the list doesn't change + assert_eq!( + T::SortedListProvider::iter().collect::>(), + vec![scenario.dest_stash1.clone(), stash.clone(), scenario.origin_stash2.clone()] + ); } set_history_depth { diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 10348761268ae..b8d3b773e5a55 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -51,6 +51,7 @@ pub fn create_funded_user( n: u32, balance_factor: u32, ) -> T::AccountId { + // REFACTOR TODO let user = account(string, n, SEED); let balance = T::Currency::minimum_balance() * balance_factor.into(); T::Currency::make_free_balance_be(&user, balance); @@ -59,6 +60,22 @@ pub fn create_funded_user( user } +/// Grab a funded user. +pub fn create_funded_user_b( + string: &'static str, + n: u32, + balance_factor: crate::BalanceOf, +) -> T::AccountId { + use sp_runtime::traits::Bounded; + let user = account(string, n, SEED); + let balance = + (T::Currency::minimum_balance() * balance_factor).max(BalanceOf::::max_value()); + T::Currency::make_free_balance_be(&user, balance); + // ensure T::CurrencyToVote will work correctly. + T::Currency::issue(balance); // TODO I don't get this .. will drop NegativeImbalance which cancels itself out + user +} + /// Create a stash and controller pair. pub fn create_stash_controller( n: u32, @@ -79,6 +96,26 @@ pub fn create_stash_controller( return Ok((stash, controller)) } +/// Create a stash and controller pair. +pub fn create_stash_controller_b( + n: u32, + balance_factor: crate::BalanceOf, + destination: RewardDestination, +) -> Result<(T::AccountId, T::AccountId), &'static str> { + let stash = create_funded_user_b::("stash", n, balance_factor); + let controller = create_funded_user_b::("controller", n, balance_factor); + let controller_lookup: ::Source = + T::Lookup::unlookup(controller.clone()); + let amount = T::Currency::minimum_balance() * (balance_factor / 10u32.into()).max(1u32.into()); + Staking::::bond( + RawOrigin::Signed(stash.clone()).into(), + controller_lookup, + amount, + destination, + )?; + Ok((stash, controller)) +} + /// Create a stash and controller pair, where the controller is dead, and payouts go to controller. /// This is used to test worst case payout scenarios. pub fn create_stash_and_dead_controller( From ff55aba4377003643da0cd4b71c7379034bfbe21 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:25:17 -0700 Subject: [PATCH 02/37] nit --- frame/staking/src/testing_utils.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index b8d3b773e5a55..e6a8a37b3b218 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -51,7 +51,6 @@ pub fn create_funded_user( n: u32, balance_factor: u32, ) -> T::AccountId { - // REFACTOR TODO let user = account(string, n, SEED); let balance = T::Currency::minimum_balance() * balance_factor.into(); T::Currency::make_free_balance_be(&user, balance); From 37c84367726d7a405ddb5cf64de17365f6149a3a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:35:58 -0700 Subject: [PATCH 03/37] Improve utils --- frame/staking/src/benchmarking.rs | 6 +++--- frame/staking/src/testing_utils.rs | 19 +++++++------------ 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 070ba5873704d..09056f48b4485 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -172,7 +172,7 @@ impl RebagScenario { // create an account in the destination bag let (dest_stash1, dest_controller1) = - create_stash_controller_b::(USER_SEED + 1, dest_factor, Default::default())?; + create_stash_controller_with_max_free::(USER_SEED + 1, dest_factor, Default::default())?; Staking::::nominate( RawOrigin::Signed(dest_controller1).into(), vec![validator.clone()], @@ -180,14 +180,14 @@ impl RebagScenario { // create accounts in origin bag let (origin_stash1, origin_controller1) = - create_stash_controller_b::(USER_SEED + 2, origin_factor, Default::default())?; + create_stash_controller_with_max_free::(USER_SEED + 2, origin_factor, Default::default())?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), vec![validator.clone()], )?; let (origin_stash2, origin_controller2) = - create_stash_controller_b::(USER_SEED + 3, origin_factor, Default::default())?; + create_stash_controller_with_max_free::(USER_SEED + 3, origin_factor, Default::default())?; Staking::::nominate( RawOrigin::Signed(origin_controller2.clone()).into(), vec![validator.clone()], diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index e6a8a37b3b218..b3b53c66d11ce 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -59,16 +59,11 @@ pub fn create_funded_user( user } -/// Grab a funded user. -pub fn create_funded_user_b( - string: &'static str, - n: u32, - balance_factor: crate::BalanceOf, -) -> T::AccountId { +/// Grab a funded user with max Balance. +pub fn create_funded_user_with_max(string: &'static str, n: u32) -> T::AccountId { use sp_runtime::traits::Bounded; let user = account(string, n, SEED); - let balance = - (T::Currency::minimum_balance() * balance_factor).max(BalanceOf::::max_value()); + let balance = BalanceOf::::max_value(); T::Currency::make_free_balance_be(&user, balance); // ensure T::CurrencyToVote will work correctly. T::Currency::issue(balance); // TODO I don't get this .. will drop NegativeImbalance which cancels itself out @@ -95,14 +90,14 @@ pub fn create_stash_controller( return Ok((stash, controller)) } -/// Create a stash and controller pair. -pub fn create_stash_controller_b( +/// Create a stash and controller pair with max free balance. +pub fn create_stash_controller_with_max_free( n: u32, balance_factor: crate::BalanceOf, destination: RewardDestination, ) -> Result<(T::AccountId, T::AccountId), &'static str> { - let stash = create_funded_user_b::("stash", n, balance_factor); - let controller = create_funded_user_b::("controller", n, balance_factor); + let stash = create_funded_user_with_max::("stash", n); + let controller = create_funded_user_with_max::("controller", n); let controller_lookup: ::Source = T::Lookup::unlookup(controller.clone()); let amount = T::Currency::minimum_balance() * (balance_factor / 10u32.into()).max(1u32.into()); From f3b8c8ad866640e4d45c66b920163ff012a42a2d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:36:32 -0700 Subject: [PATCH 04/37] fmt --- frame/staking/src/benchmarking.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 09056f48b4485..43dc23658731a 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -171,23 +171,32 @@ impl RebagScenario { let validator = create_validators::(1, 100).unwrap().first().unwrap().clone(); // create an account in the destination bag - let (dest_stash1, dest_controller1) = - create_stash_controller_with_max_free::(USER_SEED + 1, dest_factor, Default::default())?; + let (dest_stash1, dest_controller1cargo) = create_stash_controller_with_max_free::( + USER_SEED + 1, + dest_factor, + Default::default(), + )?; Staking::::nominate( RawOrigin::Signed(dest_controller1).into(), vec![validator.clone()], )?; // create accounts in origin bag - let (origin_stash1, origin_controller1) = - create_stash_controller_with_max_free::(USER_SEED + 2, origin_factor, Default::default())?; + let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( + USER_SEED + 2, + origin_factor, + Default::default(), + )?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), vec![validator.clone()], )?; - let (origin_stash2, origin_controller2) = - create_stash_controller_with_max_free::(USER_SEED + 3, origin_factor, Default::default())?; + let (origin_stash2, origin_controller2) = create_stash_controller_with_max_free::( + USER_SEED + 3, + origin_factor, + Default::default(), + )?; Staking::::nominate( RawOrigin::Signed(origin_controller2.clone()).into(), vec![validator.clone()], From 7b6686afb174c64d762cbe84c9433d1edef03599 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:55:20 -0700 Subject: [PATCH 05/37] nits --- frame/staking/src/benchmarking.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 43dc23658731a..2728217cd9d78 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -139,8 +139,6 @@ struct RebagScenario { /// Controller of the Stash that is expected to be rebagged. origin_controller1: T::AccountId, origin_stash2: T::AccountId, - dest_bag_thresh: BalanceOf, - origin_bag_thresh: BalanceOf, } impl RebagScenario { @@ -155,7 +153,6 @@ impl RebagScenario { origin_bag_thresh: BalanceOf, dest_bag_thresh: BalanceOf, ) -> Result { - let total_issuance = T::Currency::total_issuance(); ensure!( !origin_bag_thresh.is_zero() && !dest_bag_thresh.is_zero(), "both thresholds must be greater than 0" @@ -163,15 +160,15 @@ impl RebagScenario { // create_stash_controller takes a factor, so we compute it. let origin_factor: BalanceOf = - (origin_bag_thresh * 10u32.into() / T::Currency::minimum_balance()); + origin_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); let dest_factor: BalanceOf = - (dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance()); + dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); // create a validator to nominate let validator = create_validators::(1, 100).unwrap().first().unwrap().clone(); // create an account in the destination bag - let (dest_stash1, dest_controller1cargo) = create_stash_controller_with_max_free::( + let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( USER_SEED + 1, dest_factor, Default::default(), @@ -202,14 +199,7 @@ impl RebagScenario { vec![validator.clone()], )?; - Ok(RebagScenario { - dest_stash1, - origin_stash1, - origin_controller1, - origin_stash2, - dest_bag_thresh, - origin_bag_thresh, - }) + Ok(RebagScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2 }) } } From 804151c191275295cf95c2d4f4272dd01e37d28d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 10 Aug 2021 15:46:10 -0700 Subject: [PATCH 06/37] Move generate_bags to its own pallet --- Cargo.lock | 23 +++++++++---- Cargo.toml | 3 +- frame/bags-list/Cargo.toml | 14 +------- frame/bags-list/src/lib.rs | 12 +++++-- frame/election-provider-support/src/lib.rs | 8 +++++ frame/generate-bags/Cargo.toml | 32 +++++++++++++++++++ .../node-runtime}/Cargo.toml | 5 ++- .../node-runtime}/src/main.rs | 3 +- .../src/lib.rs} | 5 ++- frame/staking/Cargo.toml | 3 +- frame/staking/src/pallet/impls.rs | 10 ++++++ 11 files changed, 88 insertions(+), 30 deletions(-) create mode 100644 frame/generate-bags/Cargo.toml rename frame/{bags-list/generate-bags => generate-bags/node-runtime}/Cargo.toml (74%) rename frame/{bags-list/generate-bags => generate-bags/node-runtime}/src/main.rs (95%) rename frame/{bags-list/src/generate_bags.rs => generate-bags/src/lib.rs} (97%) diff --git a/Cargo.lock b/Cargo.lock index 1f914b2706d91..694e9ba45cf12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2228,6 +2228,22 @@ version = "0.3.55" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f5f3913fa0bfe7ee1fd8248b6b9f42a5af4b9d65ec2dd2c3c26132b950ecfc2" +[[package]] +name = "generate-bags" +version = "3.0.0" +dependencies = [ + "chrono", + "frame-election-provider-support", + "frame-support", + "frame-system", + "git2", + "node-runtime", + "num-format", + "pallet-staking", + "sp-io", + "structopt", +] + [[package]] name = "generic-array" version = "0.12.4" @@ -4508,9 +4524,8 @@ name = "node-runtime-generate-bags" version = "3.0.0" dependencies = [ "frame-support", + "generate-bags", "node-runtime", - "pallet-bags-list", - "pallet-staking", "sp-io", "structopt", ] @@ -4920,16 +4935,12 @@ dependencies = [ name = "pallet-bags-list" version = "4.0.0-dev" dependencies = [ - "chrono", "frame-benchmarking", "frame-election-provider-support", "frame-support", "frame-system", - "git2", "log", - "num-format", "pallet-balances", - "pallet-staking", "parity-scale-codec", "sp-core", "sp-io", diff --git a/Cargo.toml b/Cargo.toml index 2f31b19b80c1f..ac621f34ee02b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -130,7 +130,8 @@ members = [ "frame/utility", "frame/vesting", "frame/bags-list", - "frame/bags-list/generate-bags", + "frame/generate-bags", + "frame/generate-bags/node-runtime", "primitives/api", "primitives/api/proc-macro", "primitives/api/test", diff --git a/frame/bags-list/Cargo.toml b/frame/bags-list/Cargo.toml index 7f22bd03f7b46..191a1e696e09b 100644 --- a/frame/bags-list/Cargo.toml +++ b/frame/bags-list/Cargo.toml @@ -35,12 +35,6 @@ sp-core = { version = "4.0.0-dev", path = "../../primitives/core", optional = tr sp-io = { version = "4.0.0-dev", path = "../../primitives/io", optional = true, default-features = false } sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing", optional = true, default-features = false } -# optional imports for making voter bags lists -chrono = { version = "0.4.19", optional = true } -git2 = { version = "0.13.20", default-features = false, optional = true } -num-format = { version = "0.4.0", optional = true } -pallet-staking = { version = "4.0.0-dev", path = "../staking", optional = true } - [dev-dependencies] sp-core = { version = "4.0.0-dev", path = "../../primitives/core"} sp-io = { version = "4.0.0-dev", path = "../../primitives/io"} @@ -67,10 +61,4 @@ runtime-benchmarks = [ "pallet-balances", "sp-tracing", ] -generate-bags = [ - "chrono", - "git2", - "num-format", - "std", - "pallet-staking" -] + diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 6f67e8017f2b1..f7f155e42c85e 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -56,8 +56,6 @@ use sp_std::prelude::*; #[cfg(any(feature = "runtime-benchmarks", test))] mod benchmarks; -#[cfg(feature = "generate-bags")] -pub mod generate_bags; mod list; #[cfg(test)] mod mock; @@ -265,4 +263,14 @@ impl SortedListProvider for Pallet { fn clear() { List::::clear() } + + // Benchmark helpers + // #[cfg(feature = "runtime-benchmarks")] + fn prepare_on_update_benchmark( + origin_thresh: VoteWeight, + dest_thresh: VoteWeight, + to_update: T::AccountId, + ) { + todo!() + } } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index cca3221dd2476..b33a6b6daa23f 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -331,6 +331,14 @@ pub trait SortedListProvider { fn clear(); /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; + + // Benchmark helpers + // #[cfg(feature = "runtime-benchmarks")] + fn prepare_on_update_benchmark( + origin_thresh: VoteWeight, + dest_thresh: VoteWeight, + to_update: AccountId, + ); } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and diff --git a/frame/generate-bags/Cargo.toml b/frame/generate-bags/Cargo.toml new file mode 100644 index 0000000000000..e5e8fa61c138a --- /dev/null +++ b/frame/generate-bags/Cargo.toml @@ -0,0 +1,32 @@ +[package] +name = "generate-bags" +version = "3.0.0" +authors = ["Parity Technologies "] +edition = "2018" +license = "Apache-2.0" +homepage = "https://substrate.dev" +repository = "https://github.com/paritytech/substrate/" +description = "Bag threshold generation script for pallet-bag-list" +readme = "README.md" + +[dependencies] +node-runtime = { version = "3.0.0-dev", path = "../../bin/node/runtime" } + +# FRAME +frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } +frame-election-provider-support = { version = "4.0.0-dev", path = "../election-provider-support", features = ["runtime-benchmarks"] } +frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } +pallet-staking = { version = "4.0.0-dev", default-features = false, path = "../staking" } + +# primitives +sp-io = { version = "4.0.0-dev", default-features = false, path = "../../primitives/io" } + +# third party +chrono = { version = "0.4.19" } +git2 = { version = "0.13.20", default-features = false } +num-format = { version = "0.4.0" } + +# third-party +structopt = "0.3.21" + +# TODO std feature diff --git a/frame/bags-list/generate-bags/Cargo.toml b/frame/generate-bags/node-runtime/Cargo.toml similarity index 74% rename from frame/bags-list/generate-bags/Cargo.toml rename to frame/generate-bags/node-runtime/Cargo.toml index 7d86865f15adb..a53ac1b9984a1 100644 --- a/frame/bags-list/generate-bags/Cargo.toml +++ b/frame/generate-bags/node-runtime/Cargo.toml @@ -14,11 +14,10 @@ node-runtime = { version = "3.0.0-dev", path = "../../../bin/node/runtime" } # FRAME frame-support = { version = "4.0.0-dev", default-features = false, path = "../../support" } -pallet-bags-list = { version = "4.0.0-dev", path = "../../bags-list", features = ["generate-bags"] } -pallet-staking = { version = "4.0.0-dev", default-features = false, path = "../../staking" } +generate-bags = { version = "3.0.0", path = "../" } # primitives sp-io = { version = "4.0.0-dev", path = "../../../primitives/io" } # third-party -structopt = "0.3.21" +structopt = "0.3.21" \ No newline at end of file diff --git a/frame/bags-list/generate-bags/src/main.rs b/frame/generate-bags/node-runtime/src/main.rs similarity index 95% rename from frame/bags-list/generate-bags/src/main.rs rename to frame/generate-bags/node-runtime/src/main.rs index 7f0bb5c7be74b..74c9354b54c39 100644 --- a/frame/bags-list/generate-bags/src/main.rs +++ b/frame/generate-bags/node-runtime/src/main.rs @@ -17,9 +17,10 @@ //! Make the set of bag thresholds to be used in pallet-bags-list. -use pallet_bags_list::generate_bags::generate_thresholds_module; use std::path::PathBuf; use structopt::StructOpt; +use generate_bags::generate_thresholds_module; + #[derive(Debug, StructOpt)] struct Opt { /// How many bags to generate. diff --git a/frame/bags-list/src/generate_bags.rs b/frame/generate-bags/src/lib.rs similarity index 97% rename from frame/bags-list/src/generate_bags.rs rename to frame/generate-bags/src/lib.rs index b6d65cb2dda88..a8256275fdf02 100644 --- a/frame/bags-list/src/generate_bags.rs +++ b/frame/generate-bags/src/lib.rs @@ -66,8 +66,7 @@ use std::{ /// Note that this value depends on the current issuance, a quantity known to change over time. /// This makes the project of computing a static value suitable for inclusion in a static, /// generated file _excitingly unstable_. -#[cfg(any(feature = "std", feature = "generate-bags"))] -fn existential_weight() -> VoteWeight { +fn get_existential_weight() -> VoteWeight { use frame_support::traits::{Currency, CurrencyToVote}; let existential_deposit = >::minimum_balance(); @@ -196,7 +195,7 @@ pub fn generate_thresholds_module( ::Version::get().spec_name, )?; - let existential_weight = existential_weight::(); + let existential_weight = get_existential_weight::(); num_buf.write_formatted(&existential_weight, &format); writeln!(buf)?; writeln!(buf, "/// Existential weight for this runtime.")?; diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index 0fff61b9a0288..82c1ad245af54 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -37,6 +37,7 @@ paste = "1.0" # Optional imports for benchmarking frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true } rand_chacha = { version = "0.2", default-features = false, optional = true } +pallet-bags-list = { version = "4.0.0-dev", path = "../bags-list", optional = true } # Optional imports for making voter bags lists chrono = { version = "0.4.19", optional = true } @@ -51,7 +52,6 @@ sp-npos-elections = { version = "4.0.0-dev", path = "../../primitives/npos-elect pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" } pallet-staking-reward-curve = { version = "4.0.0-dev", path = "../staking/reward-curve" } -pallet-bags-list = { version = "4.0.0-dev", path = "../bags-list" } substrate-test-utils = { version = "4.0.0-dev", path = "../../test-utils" } frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" } frame-election-provider-support = { version = "4.0.0-dev", features = ["runtime-benchmarks"], path = "../election-provider-support" } @@ -81,6 +81,7 @@ runtime-benchmarks = [ "frame-benchmarking", "frame-election-provider-support/runtime-benchmarks", "rand_chacha", + "pallet-bags-list/runtime-benchmarks", ] try-runtime = ["frame-support/try-runtime"] generate-bags = [ diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 35e9b3a43e891..ff7ab23339c28 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1253,4 +1253,14 @@ impl SortedListProvider for UseNominatorsMap { Nominators::::remove_all(None); CounterForNominators::::kill(); } + + // Benchmark helpers + // #[cfg(feature = "runtime-benchmarks")] + fn prepare_on_update_benchmark( + origin_thresh: VoteWeight, + dest_thresh: VoteWeight, + to_update: T::AccountId, + ) { + todo!() + } } From 34e219478f177bdbfb5f528e23a68d9c85b30a26 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 10 Aug 2021 16:05:33 -0700 Subject: [PATCH 07/37] Get runtime-benchmarks feature setup with prepare_on_update_benchmark --- Cargo.lock | 3 --- frame/bags-list/Cargo.toml | 1 + frame/bags-list/src/lib.rs | 2 +- frame/election-provider-support/src/lib.rs | 2 +- frame/staking/Cargo.toml | 17 +---------------- frame/staking/src/pallet/impls.rs | 2 +- 6 files changed, 5 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 694e9ba45cf12..204750eb2cd11 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5616,15 +5616,12 @@ dependencies = [ name = "pallet-staking" version = "4.0.0-dev" dependencies = [ - "chrono", "frame-benchmarking", "frame-election-provider-support", "frame-support", "frame-system", - "git2", "hex", "log", - "num-format", "pallet-authorship", "pallet-bags-list", "pallet-balances", diff --git a/frame/bags-list/Cargo.toml b/frame/bags-list/Cargo.toml index 191a1e696e09b..75423a78b6654 100644 --- a/frame/bags-list/Cargo.toml +++ b/frame/bags-list/Cargo.toml @@ -60,5 +60,6 @@ runtime-benchmarks = [ "sp-io", "pallet-balances", "sp-tracing", + "frame-election-provider-support/runtime-benchmarks", ] diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index f7f155e42c85e..ad977a52e3b24 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -265,7 +265,7 @@ impl SortedListProvider for Pallet { } // Benchmark helpers - // #[cfg(feature = "runtime-benchmarks")] + #[cfg(feature = "runtime-benchmarks")] fn prepare_on_update_benchmark( origin_thresh: VoteWeight, dest_thresh: VoteWeight, diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index b33a6b6daa23f..7581f49359cdd 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -333,7 +333,7 @@ pub trait SortedListProvider { fn sanity_check() -> Result<(), &'static str>; // Benchmark helpers - // #[cfg(feature = "runtime-benchmarks")] + #[cfg(any(feature = "runtime-benchmarks", test))] fn prepare_on_update_benchmark( origin_thresh: VoteWeight, dest_thresh: VoteWeight, diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index 82c1ad245af54..777684a212b76 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -39,11 +39,6 @@ frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = " rand_chacha = { version = "0.2", default-features = false, optional = true } pallet-bags-list = { version = "4.0.0-dev", path = "../bags-list", optional = true } -# Optional imports for making voter bags lists -chrono = { version = "0.4.19", optional = true } -git2 = { version = "0.13.20", default-features = false, optional = true } -num-format = { version = "0.4.0", optional = true } - [dev-dependencies] sp-storage = { version = "4.0.0-dev", path = "../../primitives/storage" } sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing" } @@ -52,6 +47,7 @@ sp-npos-elections = { version = "4.0.0-dev", path = "../../primitives/npos-elect pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" } pallet-staking-reward-curve = { version = "4.0.0-dev", path = "../staking/reward-curve" } +pallet-bags-list = { version = "4.0.0-dev", features = ["runtime-benchmarks"], path = "../bags-list" } substrate-test-utils = { version = "4.0.0-dev", path = "../../test-utils" } frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" } frame-election-provider-support = { version = "4.0.0-dev", features = ["runtime-benchmarks"], path = "../election-provider-support" } @@ -84,14 +80,3 @@ runtime-benchmarks = [ "pallet-bags-list/runtime-benchmarks", ] try-runtime = ["frame-support/try-runtime"] -generate-bags = [ - "chrono", - "git2", - "num-format", - "pallet-staking-reward-curve", - "pallet-balances", - "pallet-timestamp", - "sp-core", - "sp-tracing", - "std", -] diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index ff7ab23339c28..de571e34a48b5 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1255,7 +1255,7 @@ impl SortedListProvider for UseNominatorsMap { } // Benchmark helpers - // #[cfg(feature = "runtime-benchmarks")] + #[cfg(any(test, feature = "runtime-benchmarks"))] fn prepare_on_update_benchmark( origin_thresh: VoteWeight, dest_thresh: VoteWeight, From dec1e8bf43a70b912bc3f475b8cac7386cda38c4 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 12 Aug 2021 16:26:01 -0700 Subject: [PATCH 08/37] Withdraw unbonded kill working --- frame/bags-list/src/lib.rs | 8 +- frame/election-provider-support/src/lib.rs | 3 + frame/staking/src/benchmarking.rs | 120 ++++++++++++++++----- frame/staking/src/pallet/impls.rs | 13 ++- 4 files changed, 112 insertions(+), 32 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index ad977a52e3b24..a812c46868f42 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -52,6 +52,7 @@ use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider}; use frame_system::ensure_signed; use sp_std::prelude::*; + use frame_support::traits::Get; #[cfg(any(feature = "runtime-benchmarks", test))] mod benchmarks; @@ -264,6 +265,11 @@ impl SortedListProvider for Pallet { List::::clear() } + #[cfg(feature = "runtime-benchmarks")] + fn bag_thresholds() -> Vec { + T::BagThresholds::get().to_vec() + } + // Benchmark helpers #[cfg(feature = "runtime-benchmarks")] fn prepare_on_update_benchmark( @@ -271,6 +277,6 @@ impl SortedListProvider for Pallet { dest_thresh: VoteWeight, to_update: T::AccountId, ) { - todo!() + todo!() } } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 7581f49359cdd..9df71d1a13d19 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -339,6 +339,9 @@ pub trait SortedListProvider { dest_thresh: VoteWeight, to_update: AccountId, ); + + #[cfg(feature = "runtime-benchmarks")] + fn bag_thresholds() -> Vec; } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 2728217cd9d78..dc1bc99b4c65d 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -145,7 +145,7 @@ impl RebagScenario { /// An expensive rebag scenario: /// /// - the node to be rebagged (r) is the head of a bag that has at least one other node. The bag - /// itself will need to be read and written to update the head. The node pointed to by + /// itself will need to be read and written to update its head. The node pointed to by /// r.next will need to be read and written as it will need to have its prev pointer updated. /// /// - the destination bag has at least one node, which will need its next pointer updated. @@ -164,8 +164,8 @@ impl RebagScenario { let dest_factor: BalanceOf = dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); - // create a validator to nominate - let validator = create_validators::(1, 100).unwrap().first().unwrap().clone(); + // create a validators to nominate. + let validators = create_validators::(T::MAX_NOMINATIONS, 100)?; // create an account in the destination bag let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( @@ -175,7 +175,7 @@ impl RebagScenario { )?; Staking::::nominate( RawOrigin::Signed(dest_controller1).into(), - vec![validator.clone()], + validators.clone() )?; // create accounts in origin bag @@ -186,7 +186,7 @@ impl RebagScenario { )?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), - vec![validator.clone()], + validators.clone() )?; let (origin_stash2, origin_controller2) = create_stash_controller_with_max_free::( @@ -196,7 +196,7 @@ impl RebagScenario { )?; Staking::::nominate( RawOrigin::Signed(origin_controller2.clone()).into(), - vec![validator.clone()], + validators )?; Ok(RebagScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2 }) @@ -224,7 +224,6 @@ benchmarks! { clear_validators_and_nominators::(); // The worst case scenario includes the voter changing bags. - let total_issuance = T::Currency::total_issuance(); // the bag the voter will start at let origin_bag_thresh = @@ -303,26 +302,51 @@ benchmarks! { withdraw_unbonded_kill { // Slashing Spans let s in 0 .. MAX_SPANS; - let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; - add_slashing_spans::(&stash, s); - let amount = T::Currency::minimum_balance() * 10u32.into(); - Staking::::unbond(RawOrigin::Signed(controller.clone()).into(), amount)?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head, so we can reuse the rebag + // scenario setup, but we don't care about the setup of the destination bag. + let scenario + = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + assert!(T::SortedListProvider::contains(&stash)); + + let ed = T::Currency::minimum_balance(); + let mut ledger = Ledger::::get(&controller).unwrap(); + ledger.active = ed - One::one(); + Ledger::::insert(&controller, ledger); + CurrentEra::::put(EraIndex::max_value()); - let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; - let original_total: BalanceOf = ledger.total; whitelist_account!(controller); }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { assert!(!Ledger::::contains_key(controller)); + assert!(!T::SortedListProvider::contains(&stash)); } validate { - let (stash, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head (which implies they are already + // a nominator), so we can reuse the rebag scenario setup, but we don't care about the setup + // of the destination bag. + + let scenario = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + assert!(T::SortedListProvider::contains(&stash)); + + // let (stash, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; + let prefs = ValidatorPrefs::default(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), prefs) verify { - assert!(Validators::::contains_key(stash)); + assert!(Validators::::contains_key(&stash)); + assert!(!T::SortedListProvider::contains(&stash)); } kick { @@ -400,9 +424,24 @@ benchmarks! { } chill { - let (_, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head, so we can reuse the rebag + // scenario setup, but we don't care about the setup of the destination bag. + + // thresholds are inconsequential because we are just care that we are removing a head. + let scenario + = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + assert!(T::SortedListProvider::contains(&stash)); + whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) + verify { + assert!(!T::SortedListProvider::contains(&stash)); + } set_payee { let (stash, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; @@ -554,6 +593,7 @@ benchmarks! { // The worst case scenario includes the voter changing bags. let total_issuance = T::Currency::total_issuance(); + // the bag the voter will start at let origin_bag_thresh = T::CurrencyToVote::to_currency(1u128, total_issuance); @@ -569,7 +609,20 @@ benchmarks! { // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); // so the sum of unlocking chunks puts voter into the dest bag - assert!(value * l.into() + origin_bag_thresh > origin_bag_thresh); + crate::log!( + info, + "value: {:#?}, origin thresh: {:#?}, dest thresh: {:#?}", + value, + origin_bag_thresh, + dest_bag_thresh + ); + println!( + "value: {:#?}, origin thresh: {:#?}, dest thresh: {:#?}", + value, + origin_bag_thresh, + dest_bag_thresh + ); + assert!((value * l.into() + origin_bag_thresh) > origin_bag_thresh); assert!(value * l.into() + origin_bag_thresh <= dest_bag_thresh); let unlock_chunk = UnlockChunk::> { value, @@ -624,19 +677,27 @@ benchmarks! { reap_stash { let s in 1 .. MAX_SPANS; - let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; - Staking::::validate(RawOrigin::Signed(controller.clone()).into(), ValidatorPrefs::default())?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head, so we can reuse the rebag + // scenario setup, but we don't care about the setup of the destination bag. + let scenario + = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + add_slashing_spans::(&stash, s); T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance()); whitelist_account!(controller); assert!(Bonded::::contains_key(&stash)); - assert!(Validators::::contains_key(&stash)); + assert!(T::SortedListProvider::contains(&stash)); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { assert!(!Bonded::::contains_key(&stash)); - assert!(!Validators::::contains_key(&stash)); + assert!(!T::SortedListProvider::contains(&stash)); } new_era { @@ -782,8 +843,19 @@ benchmarks! { } chill_other { - let (_, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; - Staking::::validate(RawOrigin::Signed(controller.clone()).into(), ValidatorPrefs::default())?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head, so we can reuse the rebag + // scenario setup, but we don't care about the setup of the destination bag. + + // thresholds are inconsequential because we are just care that we are removing a head. + let scenario + = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + assert!(T::SortedListProvider::contains(&stash)); + Staking::::set_staking_limits( RawOrigin::Root.into(), BalanceOf::::max_value(), @@ -795,7 +867,7 @@ benchmarks! { let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { - assert!(!Validators::::contains_key(controller)); + assert!(!T::SortedListProvider::contains(&stash)); } } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index de571e34a48b5..23436f68127f4 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1255,12 +1255,11 @@ impl SortedListProvider for UseNominatorsMap { } // Benchmark helpers - #[cfg(any(test, feature = "runtime-benchmarks"))] - fn prepare_on_update_benchmark( - origin_thresh: VoteWeight, - dest_thresh: VoteWeight, - to_update: T::AccountId, - ) { - todo!() + #[cfg(any(feature = "runtime-benchmarks", test))] + fn prepare_on_update_benchmark(_: u64, _: u64, _: T::AccountId) { todo!() } + + #[cfg(any(feature = "runtime-benchmarks", test))] + fn bag_thresholds() -> Vec { + vec![100, 1_000, 10_000] } } From a354364405dc7851a3832639162a8c921794412d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 12 Aug 2021 17:55:41 -0700 Subject: [PATCH 09/37] Nominate bench working --- frame/bags-list/src/lib.rs | 4 +- frame/generate-bags/node-runtime/src/main.rs | 2 +- frame/staking/src/benchmarking.rs | 70 +++++++++++--------- frame/staking/src/pallet/impls.rs | 4 +- frame/staking/src/testing_utils.rs | 3 +- 5 files changed, 47 insertions(+), 36 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index a812c46868f42..f3aebbc2d1489 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -50,9 +50,9 @@ #![cfg_attr(not(feature = "std"), no_std)] use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider}; +use frame_support::traits::Get; use frame_system::ensure_signed; use sp_std::prelude::*; - use frame_support::traits::Get; #[cfg(any(feature = "runtime-benchmarks", test))] mod benchmarks; @@ -277,6 +277,6 @@ impl SortedListProvider for Pallet { dest_thresh: VoteWeight, to_update: T::AccountId, ) { - todo!() + todo!() } } diff --git a/frame/generate-bags/node-runtime/src/main.rs b/frame/generate-bags/node-runtime/src/main.rs index 74c9354b54c39..3879b7595ca10 100644 --- a/frame/generate-bags/node-runtime/src/main.rs +++ b/frame/generate-bags/node-runtime/src/main.rs @@ -17,9 +17,9 @@ //! Make the set of bag thresholds to be used in pallet-bags-list. +use generate_bags::generate_thresholds_module; use std::path::PathBuf; use structopt::StructOpt; -use generate_bags::generate_thresholds_module; #[derive(Debug, StructOpt)] struct Opt { diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index dc1bc99b4c65d..07017f32c12f3 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -165,7 +165,7 @@ impl RebagScenario { dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); // create a validators to nominate. - let validators = create_validators::(T::MAX_NOMINATIONS, 100)?; + let validators = create_validators::(T::MAX_NOMINATIONS, 100, 333)?; // create an account in the destination bag let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( @@ -173,10 +173,7 @@ impl RebagScenario { dest_factor, Default::default(), )?; - Staking::::nominate( - RawOrigin::Signed(dest_controller1).into(), - validators.clone() - )?; + Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators.clone())?; // create accounts in origin bag let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( @@ -186,7 +183,7 @@ impl RebagScenario { )?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), - validators.clone() + validators.clone(), )?; let (origin_stash2, origin_controller2) = create_stash_controller_with_max_free::( @@ -194,10 +191,7 @@ impl RebagScenario { origin_factor, Default::default(), )?; - Staking::::nominate( - RawOrigin::Signed(origin_controller2.clone()).into(), - validators - )?; + Staking::::nominate(RawOrigin::Signed(origin_controller2.clone()).into(), validators)?; Ok(RebagScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2 }) } @@ -339,8 +333,6 @@ benchmarks! { let stash = scenario.origin_stash1; assert!(T::SortedListProvider::contains(&stash)); - // let (stash, controller) = create_stash_controller::(USER_SEED, 100, Default::default())?; - let prefs = ValidatorPrefs::default(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), prefs) @@ -358,7 +350,7 @@ benchmarks! { // these are the other validators; there are `T::MAX_NOMINATIONS - 1` of them, so // there are a total of `T::MAX_NOMINATIONS` validators in the system. - let rest_of_validators = create_validators::(T::MAX_NOMINATIONS - 1, 100)?; + let rest_of_validators = create_validators::(T::MAX_NOMINATIONS - 1, 100, 0)?; // this is the validator that will be kicking. let (stash, controller) = create_stash_controller::( @@ -415,12 +407,28 @@ benchmarks! { // Worst case scenario, T::MAX_NOMINATIONS nominate { let n in 1 .. T::MAX_NOMINATIONS; - let (stash, controller) = create_stash_controller::(n + 1, 100, Default::default())?; - let validators = create_validators::(n, 100)?; + + // Clean up any existing state. + clear_validators_and_nominators::(); + + // The worst case scenario: a voter is inserted into a bag that already has voters so both + // the bag itself and the tail.next pointer need to be updated. + + let threshold = One::one(); // all the voters in the scenario will be in the same bag. + let scenario = RebagScenario::::new(threshold, threshold)?; + let origin_threshold_factor: BalanceOf = + threshold * 10u32.into() / T::Currency::minimum_balance(); + let (stash, controller) = create_stash_controller_with_max_free::( + SEED + 931494657, + origin_threshold_factor, // bond an amount that puts them in the bag with nodes. + Default::default(), + )?; + + let validators = create_validators::(n, 100, 0)?; whitelist_account!(controller); }: _(RawOrigin::Signed(controller), validators) verify { - assert!(Nominators::::contains_key(stash)); + // assert!(Nominators::::contains_key(stash)); } chill { @@ -493,11 +501,24 @@ benchmarks! { force_unstake { // Slashing Spans let s in 0 .. MAX_SPANS; - let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; + // Clean up any existing state. + clear_validators_and_nominators::(); + + // A worst case scenario includes the voter being a bag head, so we can reuse the rebag + // scenario setup, but we don't care about the setup of the destination bag. + + // thresholds are inconsequential because we are just care that we are removing a head. + let scenario + = RebagScenario::::new(One::one(), One::one())?; + let controller = scenario.origin_controller1; + let stash = scenario.origin_stash1; + assert!(T::SortedListProvider::contains(&stash)); + add_slashing_spans::(&stash, s); - }: _(RawOrigin::Root, stash, s) + }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); + assert!(!T::SortedListProvider::contains(&stash)); } cancel_deferred_slash { @@ -609,19 +630,6 @@ benchmarks! { // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); // so the sum of unlocking chunks puts voter into the dest bag - crate::log!( - info, - "value: {:#?}, origin thresh: {:#?}, dest thresh: {:#?}", - value, - origin_bag_thresh, - dest_bag_thresh - ); - println!( - "value: {:#?}, origin thresh: {:#?}, dest thresh: {:#?}", - value, - origin_bag_thresh, - dest_bag_thresh - ); assert!((value * l.into() + origin_bag_thresh) > origin_bag_thresh); assert!(value * l.into() + origin_bag_thresh <= dest_bag_thresh); let unlock_chunk = UnlockChunk::> { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 23436f68127f4..23c748b908826 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1256,7 +1256,9 @@ impl SortedListProvider for UseNominatorsMap { // Benchmark helpers #[cfg(any(feature = "runtime-benchmarks", test))] - fn prepare_on_update_benchmark(_: u64, _: u64, _: T::AccountId) { todo!() } + fn prepare_on_update_benchmark(_: u64, _: u64, _: T::AccountId) { + todo!() + } #[cfg(any(feature = "runtime-benchmarks", test))] fn bag_thresholds() -> Vec { diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index b3b53c66d11ce..ca47495695ef3 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -136,11 +136,12 @@ pub fn create_stash_and_dead_controller( pub fn create_validators( max: u32, balance_factor: u32, + seed: u32 ) -> Result::Source>, &'static str> { let mut validators: Vec<::Source> = Vec::with_capacity(max as usize); for i in 0..max { let (stash, controller) = - create_stash_controller::(i, balance_factor, RewardDestination::Staked)?; + create_stash_controller::(i + seed, balance_factor, RewardDestination::Staked)?; let validator_prefs = ValidatorPrefs { commission: Perbill::from_percent(50), ..Default::default() }; Staking::::validate(RawOrigin::Signed(controller).into(), validator_prefs)?; From 02a218ffd75cb4f3934af03378b24189a4d22e20 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 12 Aug 2021 18:21:07 -0700 Subject: [PATCH 10/37] some cleanup --- frame/bags-list/src/lib.rs | 16 ---------------- frame/election-provider-support/src/lib.rs | 11 ----------- frame/generate-bags/Cargo.toml | 2 -- frame/generate-bags/node-runtime/Cargo.toml | 2 +- frame/generate-bags/src/lib.rs | 4 ++-- frame/staking/src/benchmarking.rs | 6 +++++- frame/staking/src/pallet/impls.rs | 11 ----------- frame/staking/src/testing_utils.rs | 2 +- 8 files changed, 9 insertions(+), 45 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index f3aebbc2d1489..86fee4c96fb1e 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -50,7 +50,6 @@ #![cfg_attr(not(feature = "std"), no_std)] use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider}; -use frame_support::traits::Get; use frame_system::ensure_signed; use sp_std::prelude::*; @@ -264,19 +263,4 @@ impl SortedListProvider for Pallet { fn clear() { List::::clear() } - - #[cfg(feature = "runtime-benchmarks")] - fn bag_thresholds() -> Vec { - T::BagThresholds::get().to_vec() - } - - // Benchmark helpers - #[cfg(feature = "runtime-benchmarks")] - fn prepare_on_update_benchmark( - origin_thresh: VoteWeight, - dest_thresh: VoteWeight, - to_update: T::AccountId, - ) { - todo!() - } } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 9df71d1a13d19..cca3221dd2476 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -331,17 +331,6 @@ pub trait SortedListProvider { fn clear(); /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; - - // Benchmark helpers - #[cfg(any(feature = "runtime-benchmarks", test))] - fn prepare_on_update_benchmark( - origin_thresh: VoteWeight, - dest_thresh: VoteWeight, - to_update: AccountId, - ); - - #[cfg(feature = "runtime-benchmarks")] - fn bag_thresholds() -> Vec; } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and diff --git a/frame/generate-bags/Cargo.toml b/frame/generate-bags/Cargo.toml index e5e8fa61c138a..6814d0e2592e2 100644 --- a/frame/generate-bags/Cargo.toml +++ b/frame/generate-bags/Cargo.toml @@ -28,5 +28,3 @@ num-format = { version = "0.4.0" } # third-party structopt = "0.3.21" - -# TODO std feature diff --git a/frame/generate-bags/node-runtime/Cargo.toml b/frame/generate-bags/node-runtime/Cargo.toml index a53ac1b9984a1..0214b668238af 100644 --- a/frame/generate-bags/node-runtime/Cargo.toml +++ b/frame/generate-bags/node-runtime/Cargo.toml @@ -20,4 +20,4 @@ generate-bags = { version = "3.0.0", path = "../" } sp-io = { version = "4.0.0-dev", path = "../../../primitives/io" } # third-party -structopt = "0.3.21" \ No newline at end of file +structopt = "0.3.21" diff --git a/frame/generate-bags/src/lib.rs b/frame/generate-bags/src/lib.rs index a8256275fdf02..cf7bc13b53b75 100644 --- a/frame/generate-bags/src/lib.rs +++ b/frame/generate-bags/src/lib.rs @@ -66,7 +66,7 @@ use std::{ /// Note that this value depends on the current issuance, a quantity known to change over time. /// This makes the project of computing a static value suitable for inclusion in a static, /// generated file _excitingly unstable_. -fn get_existential_weight() -> VoteWeight { +fn existential_weight() -> VoteWeight { use frame_support::traits::{Currency, CurrencyToVote}; let existential_deposit = >::minimum_balance(); @@ -195,7 +195,7 @@ pub fn generate_thresholds_module( ::Version::get().spec_name, )?; - let existential_weight = get_existential_weight::(); + let existential_weight = existential_weight::(); num_buf.write_formatted(&existential_weight, &format); writeln!(buf)?; writeln!(buf, "/// Existential weight for this runtime.")?; diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 07017f32c12f3..237cb99abb11e 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -424,11 +424,15 @@ benchmarks! { Default::default(), )?; + assert!(!Nominators::::contains_key(&stash)); + assert!(!T::SortedListProvider::contains(&stash)) + let validators = create_validators::(n, 100, 0)?; whitelist_account!(controller); }: _(RawOrigin::Signed(controller), validators) verify { - // assert!(Nominators::::contains_key(stash)); + assert!(Nominators::::contains_key(&stash)); + assert!(T::SortedListProvider::contains(&stash)) } chill { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 23c748b908826..35e9b3a43e891 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1253,15 +1253,4 @@ impl SortedListProvider for UseNominatorsMap { Nominators::::remove_all(None); CounterForNominators::::kill(); } - - // Benchmark helpers - #[cfg(any(feature = "runtime-benchmarks", test))] - fn prepare_on_update_benchmark(_: u64, _: u64, _: T::AccountId) { - todo!() - } - - #[cfg(any(feature = "runtime-benchmarks", test))] - fn bag_thresholds() -> Vec { - vec![100, 1_000, 10_000] - } } diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index ca47495695ef3..75660208b8d88 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -136,7 +136,7 @@ pub fn create_stash_and_dead_controller( pub fn create_validators( max: u32, balance_factor: u32, - seed: u32 + seed: u32, ) -> Result::Source>, &'static str> { let mut validators: Vec<::Source> = Vec::with_capacity(max as usize); for i in 0..max { From 77ab8af21c0628a15f579e84f2ca01177d7140be Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 12 Aug 2021 19:31:39 -0700 Subject: [PATCH 11/37] WIP --- frame/bags-list/src/lib.rs | 5 ++ frame/bags-list/src/list/mod.rs | 7 ++- frame/election-provider-support/src/lib.rs | 7 +++ frame/generate-bags/Cargo.toml | 2 - frame/generate-bags/node-runtime/src/main.rs | 2 +- frame/staking/src/benchmarking.rs | 56 +++++++++++++++----- 6 files changed, 63 insertions(+), 16 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 86fee4c96fb1e..e9c46b01e7e7c 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -263,4 +263,9 @@ impl SortedListProvider for Pallet { fn clear() { List::::clear() } + + #[cfg(feature = "runtime-benchmarks")] + fn is_in_bag(id: &T::AccountId, weight: VoteWeight) -> bool { + list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) + } } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 6795e9464fe28..5257544cfe058 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -52,7 +52,7 @@ mod tests; /// /// Note that even if the thresholds list does not have `VoteWeight::MAX` as its final member, this /// function behaves as if it does. -fn notional_bag_for(weight: VoteWeight) -> VoteWeight { +pub(crate) fn notional_bag_for(weight: VoteWeight) -> VoteWeight { let thresholds = T::BagThresholds::get(); let idx = thresholds.partition_point(|&threshold| weight > threshold); thresholds.get(idx).copied().unwrap_or(VoteWeight::MAX) @@ -585,6 +585,11 @@ impl Bag { Ok(()) } + + #[cfg(feature = "runtime-benchmarks")] + pub(crate) fn contains(&self, id: &T::AccountId) -> bool { + self.iter().any(|n| n.id() == id) + } } /// A Node is the fundamental element comprising the doubly-linked list described by `Bag`. diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index cca3221dd2476..5bc51959ff0a9 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -331,6 +331,13 @@ pub trait SortedListProvider { fn clear(); /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; + + /// If the voter is in the notional bag for the given weight. + #[cfg(any(feature = "runtime-benchmarks", test))] + fn is_in_bag(_: &AccountId, _: VoteWeight) -> bool { + // TODO problematic if this always returns true + true // default to true for impls that don't have a bag. + } } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and diff --git a/frame/generate-bags/Cargo.toml b/frame/generate-bags/Cargo.toml index 6814d0e2592e2..a2b6c28f9350f 100644 --- a/frame/generate-bags/Cargo.toml +++ b/frame/generate-bags/Cargo.toml @@ -25,6 +25,4 @@ sp-io = { version = "4.0.0-dev", default-features = false, path = "../../primiti chrono = { version = "0.4.19" } git2 = { version = "0.13.20", default-features = false } num-format = { version = "0.4.0" } - -# third-party structopt = "0.3.21" diff --git a/frame/generate-bags/node-runtime/src/main.rs b/frame/generate-bags/node-runtime/src/main.rs index 3879b7595ca10..1758e32e911fc 100644 --- a/frame/generate-bags/node-runtime/src/main.rs +++ b/frame/generate-bags/node-runtime/src/main.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Make the set of bag thresholds to be used in pallet-bags-list. +//! Make the set of bag thresholds to be used with pallet-bags-list. use generate_bags::generate_thresholds_module; use std::path::PathBuf; diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 237cb99abb11e..c104110e8022c 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -139,6 +139,8 @@ struct RebagScenario { /// Controller of the Stash that is expected to be rebagged. origin_controller1: T::AccountId, origin_stash2: T::AccountId, + origin_bag_thresh: BalanceOf, + dest_bag_thresh: BalanceOf, } impl RebagScenario { @@ -167,14 +169,6 @@ impl RebagScenario { // create a validators to nominate. let validators = create_validators::(T::MAX_NOMINATIONS, 100, 333)?; - // create an account in the destination bag - let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( - USER_SEED + 1, - dest_factor, - Default::default(), - )?; - Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators.clone())?; - // create accounts in origin bag let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( USER_SEED + 2, @@ -191,9 +185,40 @@ impl RebagScenario { origin_factor, Default::default(), )?; - Staking::::nominate(RawOrigin::Signed(origin_controller2.clone()).into(), validators)?; + Staking::::nominate( + RawOrigin::Signed(origin_controller2.clone()).into(), + validators.clone(), + )?; + + // create an account in the destination bag + let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( + USER_SEED + 1, + dest_factor, + Default::default(), + )?; + Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators)?; + + Ok(RebagScenario { + dest_stash1, + origin_stash1, + origin_controller1, + origin_stash2, + origin_bag_thresh, + dest_bag_thresh, + }) + } - Ok(RebagScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2 }) + fn assert_preconditions(&self) { + let total_issuance = T::Currency::total_issuance(); + let dest_thresh_as_vote = T::CurrencyToVote::to_vote(self.dest_bag_thresh, total_issuance); + let origin_thresh_as_vote = T::CurrencyToVote::to_vote(self.origin_bag_thresh, total_issuance); + + // destination stash is not in the origin bag + assert!(!T::SortedListProvider::is_in_bag(&self.dest_stash1, origin_thresh_as_vote)); + // and is in the destination bag. + assert!(T::SortedListProvider::is_in_bag(&self.dest_stash1, dest_thresh_as_vote)); + // the origin stash is in the origin bag. + assert!(T::SortedListProvider::is_in_bag(&self.origin_stash1, origin_thresh_as_vote)); } } @@ -237,6 +262,8 @@ benchmarks! { let original_bonded: BalanceOf = ledger.active; whitelist_account!(stash); + scenario.assert_preconditions(); + }: _(RawOrigin::Signed(stash.clone()), max_additional) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; @@ -265,6 +292,9 @@ benchmarks! { let amount = origin_bag_thresh - dest_bag_thresh; let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; + + scenario.assert_preconditions(); + whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), amount) verify { @@ -425,7 +455,7 @@ benchmarks! { )?; assert!(!Nominators::::contains_key(&stash)); - assert!(!T::SortedListProvider::contains(&stash)) + assert!(!T::SortedListProvider::contains(&stash)); let validators = create_validators::(n, 100, 0)?; whitelist_account!(controller); @@ -650,12 +680,14 @@ benchmarks! { } Ledger::::insert(controller.clone(), staking_ledger.clone()); let original_bonded: BalanceOf = staking_ledger.active; - whitelist_account!(controller); + scenario.assert_preconditions(); assert_eq!( T::SortedListProvider::iter().collect::>(), vec![scenario.dest_stash1.clone(), stash.clone(), scenario.origin_stash2.clone()] ); + + whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), rebond_amount) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; From ecdb7e85a178c4f7fd334fa3ae2512bc3087cc8d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 15:25:44 -0700 Subject: [PATCH 12/37] update to check head pre & post conditions --- Cargo.lock | 7 +- frame/bags-list/src/lib.rs | 22 ++++- frame/bags-list/src/list/mod.rs | 13 ++- frame/bags-list/src/list/tests.rs | 3 +- frame/election-provider-support/src/lib.rs | 15 +++- frame/generate-bags/src/lib.rs | 4 +- frame/staking/src/benchmarking.rs | 96 ++++++++++++++++------ frame/staking/src/pallet/mod.rs | 3 +- 8 files changed, 117 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6dc081686f934..7e495ba5b12d5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4886,7 +4886,7 @@ dependencies = [ "frame-election-provider-support", "frame-support", "frame-system", - "log", + "log 0.4.14", "pallet-balances", "parity-scale-codec", "sp-core", @@ -5548,8 +5548,7 @@ dependencies = [ "frame-election-provider-support", "frame-support", "frame-system", - "hex", - "log", + "log 0.4.14", "pallet-authorship", "pallet-bags-list", "pallet-balances", @@ -5557,8 +5556,6 @@ dependencies = [ "pallet-staking-reward-curve", "pallet-timestamp", "parity-scale-codec", - "parking_lot 0.11.1", - "paste 1.0.4", "rand 0.8.4", "rand_chacha 0.2.2", "serde", diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index e9c46b01e7e7c..90796b2af3682 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -140,8 +140,8 @@ pub mod pallet { /// the procedure given above, then the constant ratio is equal to 2. /// - If `BagThresholds::get().len() == 200`, and the thresholds are determined according to /// the procedure given above, then the constant ratio is approximately equal to 1.248. - /// - If the threshold list begins `[1, 2, 3, ...]`, then an id with weight 0 or 1 will - /// fall into bag 0, an id with weight 2 will fall into bag 1, etc. + /// - If the threshold list begins `[1, 2, 3, ...]`, then an id with weight 0 or 1 will fall + /// into bag 0, an id with weight 2 will fall into bag 1, etc. /// /// # Migration /// @@ -265,7 +265,23 @@ impl SortedListProvider for Pallet { } #[cfg(feature = "runtime-benchmarks")] - fn is_in_bag(id: &T::AccountId, weight: VoteWeight) -> bool { + fn is_in_bag(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { + use frame_support::traits::Get; + let info = T::BagThresholds::get() + .into_iter() + .chain(std::iter::once(&VoteWeight::MAX)) // assumes this is not an explicit threshold + .filter_map(|t| { + list::Bag::::get(*t) + .map(|bag| (*t, bag.iter().map(|n| n.id().clone()).collect::>())) + }) + .collect::>(); + println!("bags info {:#?}", info); + list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } + + #[cfg(feature = "runtime-benchmarks")] + fn is_bag_head(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { + list::Bag::::get(list::notional_bag_for::(weight)).unwrap().head_id() == Some(id) + } } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 5257544cfe058..0f20e30cffaae 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -111,10 +111,10 @@ impl List { /// Postconditions: /// /// - All `bag_upper` currently in storage are members of `T::BagThresholds`. - /// - No id is changed unless required to by the difference between the old threshold list - /// and the new. - /// - ids whose bags change at all are implicitly rebagged into the appropriate bag in the - /// new threshold set. + /// - No id is changed unless required to by the difference between the old threshold list and + /// the new. + /// - ids whose bags change at all are implicitly rebagged into the appropriate bag in the new + /// threshold set. #[allow(dead_code)] pub fn migrate(old_thresholds: &[VoteWeight]) -> u32 { // we can't check all preconditions, but we can check one @@ -590,6 +590,11 @@ impl Bag { pub(crate) fn contains(&self, id: &T::AccountId) -> bool { self.iter().any(|n| n.id() == id) } + + #[cfg(feature = "runtime-benchmarks")] + pub(crate) fn head_id(&self) -> Option<&T::AccountId> { + self.head.as_ref() + } } /// A Node is the fundamental element comprising the doubly-linked list described by `Bag`. diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 92562f9a1fcd7..28d3cb031a59f 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -136,7 +136,8 @@ fn migrate_works() { (15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15 (20, vec![711]), (1000, vec![2, 3, 4]), - (10_000, vec![712]), // nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000 + (10_000, vec![712]), /* nodes in range 1_001 ..= 2_000 move from bag 2_000 + * to bag 10_000 */ ] ); }); diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 5bc51959ff0a9..a6a5fbc812ac3 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -332,11 +332,18 @@ pub trait SortedListProvider { /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; - /// If the voter is in the notional bag for the given weight. + /// Wether the voter is in the notional bag for the given weight. Only relevant for + /// implementations that use bags. #[cfg(any(feature = "runtime-benchmarks", test))] - fn is_in_bag(_: &AccountId, _: VoteWeight) -> bool { - // TODO problematic if this always returns true - true // default to true for impls that don't have a bag. + fn is_in_bag(_: &AccountId, _: VoteWeight, mock: bool) -> bool { + mock // default to returning mock value for values that are not relevant + } + + /// Wether the voter is the head of the notional bag for the given weight. Only relevant for + /// implementations that use bags. + #[cfg(any(feature = "runtime-benchmarks", test))] + fn is_bag_head(_: &AccountId, _: VoteWeight, mock: bool) -> bool { + mock // default to returning mock value for values that are not relevant } } diff --git a/frame/generate-bags/src/lib.rs b/frame/generate-bags/src/lib.rs index cf7bc13b53b75..fcbdc16a61d40 100644 --- a/frame/generate-bags/src/lib.rs +++ b/frame/generate-bags/src/lib.rs @@ -37,8 +37,8 @@ //! ``` //! //! 2. Write a little program to generate the definitions. This can be a near-identical copy of -//! `substrate/frame/bags-list/generate-bags`. This program exists only to hook together the runtime -//! definitions with the various calculations here. +//! `substrate/frame/bags-list/generate-bags`. This program exists only to hook together the +//! runtime definitions with the various calculations here. //! //! 3. Run that program: //! diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index c104110e8022c..b4c1e661cea9b 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -139,16 +139,16 @@ struct RebagScenario { /// Controller of the Stash that is expected to be rebagged. origin_controller1: T::AccountId, origin_stash2: T::AccountId, - origin_bag_thresh: BalanceOf, - dest_bag_thresh: BalanceOf, + origin_thresh_as_vote: VoteWeight, + dest_thresh_as_vote: VoteWeight, } impl RebagScenario { /// An expensive rebag scenario: /// /// - the node to be rebagged (r) is the head of a bag that has at least one other node. The bag - /// itself will need to be read and written to update its head. The node pointed to by - /// r.next will need to be read and written as it will need to have its prev pointer updated. + /// itself will need to be read and written to update its head. The node pointed to by r.next + /// will need to be read and written as it will need to have its prev pointer updated. /// /// - the destination bag has at least one node, which will need its next pointer updated. fn new( @@ -198,27 +198,75 @@ impl RebagScenario { )?; Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators)?; + let total_issuance = T::Currency::total_issuance(); + let origin_thresh_as_vote = T::CurrencyToVote::to_vote(origin_bag_thresh, total_issuance); + let dest_thresh_as_vote = T::CurrencyToVote::to_vote(dest_bag_thresh, total_issuance); + Ok(RebagScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2, - origin_bag_thresh, - dest_bag_thresh, + origin_thresh_as_vote, + dest_thresh_as_vote, }) } - fn assert_preconditions(&self) { - let total_issuance = T::Currency::total_issuance(); - let dest_thresh_as_vote = T::CurrencyToVote::to_vote(self.dest_bag_thresh, total_issuance); - let origin_thresh_as_vote = T::CurrencyToVote::to_vote(self.origin_bag_thresh, total_issuance); + fn check_preconditions(&self) { + let RebagScenario { + dest_stash1, + origin_stash2, + dest_thresh_as_vote, + origin_thresh_as_vote, + .. + } = self; + // destination stash is not in the origin bag. + assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); + // origin stash2 is in the origin bag + assert!(T::SortedListProvider::is_in_bag(&origin_stash2, *origin_thresh_as_vote, true)); + // head checks implicitly check that dest stash1 and origin stash1 are in the correct bags. + self.check_head_preconditions(); + } + fn check_postconditions(&self) { + let RebagScenario { + dest_stash1, + origin_stash1, + dest_thresh_as_vote, + origin_thresh_as_vote, + .. + } = self; // destination stash is not in the origin bag - assert!(!T::SortedListProvider::is_in_bag(&self.dest_stash1, origin_thresh_as_vote)); + assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); // and is in the destination bag. - assert!(T::SortedListProvider::is_in_bag(&self.dest_stash1, dest_thresh_as_vote)); - // the origin stash is in the origin bag. - assert!(T::SortedListProvider::is_in_bag(&self.origin_stash1, origin_thresh_as_vote)); + assert!(T::SortedListProvider::is_in_bag(&dest_stash1, *dest_thresh_as_vote, true)); + // the origin stash is in the destination bag. + assert!(T::SortedListProvider::is_in_bag(&origin_stash1, *dest_thresh_as_vote, true)); + self.check_head_postconditions(); + } + + fn check_head_preconditions(&self) { + let RebagScenario { + dest_stash1, + origin_stash1, + dest_thresh_as_vote, + origin_thresh_as_vote, + .. + } = self; + assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_bag_head(&origin_stash1, *origin_thresh_as_vote, true)); + } + + fn check_head_postconditions(&self) { + let RebagScenario { + dest_stash1, + origin_stash2, + dest_thresh_as_vote, + origin_thresh_as_vote, + .. + } = self; + assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); } } @@ -262,13 +310,15 @@ benchmarks! { let original_bonded: BalanceOf = ledger.active; whitelist_account!(stash); - scenario.assert_preconditions(); + scenario.check_preconditions(); }: _(RawOrigin::Signed(stash.clone()), max_additional) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); + + scenario.check_postconditions(); } unbond { @@ -293,7 +343,7 @@ benchmarks! { let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; - scenario.assert_preconditions(); + scenario.check_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), amount) @@ -301,6 +351,8 @@ benchmarks! { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded > new_bonded); + + scenario.check_postconditions(); } // Withdraw only updates the ledger @@ -681,11 +733,7 @@ benchmarks! { Ledger::::insert(controller.clone(), staking_ledger.clone()); let original_bonded: BalanceOf = staking_ledger.active; - scenario.assert_preconditions(); - assert_eq!( - T::SortedListProvider::iter().collect::>(), - vec![scenario.dest_stash1.clone(), stash.clone(), scenario.origin_stash2.clone()] - ); + scenario.check_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), rebond_amount) @@ -694,11 +742,7 @@ benchmarks! { let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); - // the ordering in the list doesn't change - assert_eq!( - T::SortedListProvider::iter().collect::>(), - vec![scenario.dest_stash1.clone(), stash.clone(), scenario.origin_stash2.clone()] - ); + scenario.check_postconditions(); } set_history_depth { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index fe1f63982687e..df639cd58d2f0 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -768,13 +768,14 @@ pub mod pallet { Error::::InsufficientBond ); + // ledger must be updated prior to call to `Self::weight_of`. + Self::update_ledger(&controller, &ledger); // update this staker in the sorted list, if they exist in it. if T::SortedListProvider::contains(&stash) { T::SortedListProvider::on_update(&stash, Self::weight_of(&ledger.stash)); } Self::deposit_event(Event::::Bonded(stash.clone(), extra)); - Self::update_ledger(&controller, &ledger); } Ok(()) } From d3bbd189e678ca664f4a7ac3ff54cddf79716ad6 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 15:59:54 -0700 Subject: [PATCH 13/37] Add some post condition verification stuff for on_remove --- frame/bags-list/src/lib.rs | 11 -------- frame/staking/src/benchmarking.rs | 42 ++++++++++++++++++------------- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 90796b2af3682..17d5712718db7 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -266,17 +266,6 @@ impl SortedListProvider for Pallet { #[cfg(feature = "runtime-benchmarks")] fn is_in_bag(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { - use frame_support::traits::Get; - let info = T::BagThresholds::get() - .into_iter() - .chain(std::iter::once(&VoteWeight::MAX)) // assumes this is not an explicit threshold - .filter_map(|t| { - list::Bag::::get(*t) - .map(|bag| (*t, bag.iter().map(|n| n.id().clone()).collect::>())) - }) - .collect::>(); - println!("bags info {:#?}", info); - list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index b4c1e661cea9b..cfe55a42c70e8 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -216,7 +216,6 @@ impl RebagScenario { let RebagScenario { dest_stash1, origin_stash2, - dest_thresh_as_vote, origin_thresh_as_vote, .. } = self; @@ -242,7 +241,9 @@ impl RebagScenario { assert!(T::SortedListProvider::is_in_bag(&dest_stash1, *dest_thresh_as_vote, true)); // the origin stash is in the destination bag. assert!(T::SortedListProvider::is_in_bag(&origin_stash1, *dest_thresh_as_vote, true)); - self.check_head_postconditions(); + // dest stash1 is the head of the destination bag. + assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); + self.check_origin_head_postconditions(); } fn check_head_preconditions(&self) { @@ -257,15 +258,14 @@ impl RebagScenario { assert!(T::SortedListProvider::is_bag_head(&origin_stash1, *origin_thresh_as_vote, true)); } - fn check_head_postconditions(&self) { + // Just checking the origin head is useful for scenarios where we start out with all the nodes + // in the same bag, and thus no destination node is actually ever a head. + fn check_origin_head_postconditions(&self) { let RebagScenario { - dest_stash1, origin_stash2, - dest_thresh_as_vote, origin_thresh_as_vote, .. } = self; - assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); } } @@ -385,8 +385,8 @@ benchmarks! { // scenario setup, but we don't care about the setup of the destination bag. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); let ed = T::Currency::minimum_balance(); @@ -400,6 +400,7 @@ benchmarks! { verify { assert!(!Ledger::::contains_key(controller)); assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } validate { @@ -411,8 +412,8 @@ benchmarks! { // of the destination bag. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); let prefs = ValidatorPrefs::default(); @@ -421,6 +422,7 @@ benchmarks! { verify { assert!(Validators::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } kick { @@ -527,14 +529,15 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } set_payee { @@ -596,8 +599,8 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); add_slashing_spans::(&stash, s); @@ -605,6 +608,7 @@ benchmarks! { verify { assert!(!Ledger::::contains_key(&controller)); assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } cancel_deferred_slash { @@ -772,8 +776,8 @@ benchmarks! { // scenario setup, but we don't care about the setup of the destination bag. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); add_slashing_spans::(&stash, s); T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance()); @@ -786,6 +790,7 @@ benchmarks! { verify { assert!(!Bonded::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } new_era { @@ -940,8 +945,8 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario = RebagScenario::::new(One::one(), One::one())?; - let controller = scenario.origin_controller1; - let stash = scenario.origin_stash1; + let controller = scenario.origin_controller1.clone(); + let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); Staking::::set_staking_limits( @@ -956,6 +961,7 @@ benchmarks! { }: _(RawOrigin::Signed(caller), controller.clone()) verify { assert!(!T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_postconditions(); } } From 1b825791e0f4e3d8031ddfe794bbf8a6342f100c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 16:08:00 -0700 Subject: [PATCH 14/37] Update nominate --- frame/staking/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index cfe55a42c70e8..d63ed878c219e 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -503,7 +503,7 @@ benchmarks! { let origin_threshold_factor: BalanceOf = threshold * 10u32.into() / T::Currency::minimum_balance(); let (stash, controller) = create_stash_controller_with_max_free::( - SEED + 931494657, + SEED + T::MAX_NOMINATIONS + 1, // make sure the account does not conflict with others origin_threshold_factor, // bond an amount that puts them in the bag with nodes. Default::default(), )?; From 19ebb8a11bfad568211f2e2ee59f1f6e2ff30041 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 16:10:48 -0700 Subject: [PATCH 15/37] fmt --- frame/staking/src/benchmarking.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index d63ed878c219e..7a4c53ffa55f3 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -213,12 +213,7 @@ impl RebagScenario { } fn check_preconditions(&self) { - let RebagScenario { - dest_stash1, - origin_stash2, - origin_thresh_as_vote, - .. - } = self; + let RebagScenario { dest_stash1, origin_stash2, origin_thresh_as_vote, .. } = self; // destination stash is not in the origin bag. assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); // origin stash2 is in the origin bag @@ -261,11 +256,7 @@ impl RebagScenario { // Just checking the origin head is useful for scenarios where we start out with all the nodes // in the same bag, and thus no destination node is actually ever a head. fn check_origin_head_postconditions(&self) { - let RebagScenario { - origin_stash2, - origin_thresh_as_vote, - .. - } = self; + let RebagScenario { origin_stash2, origin_thresh_as_vote, .. } = self; assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); } } From e79ad980da67c54d36598505af6286205f4fabad Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 16:44:11 -0700 Subject: [PATCH 16/37] Improvements --- frame/election-provider-support/src/lib.rs | 8 ++- frame/generate-bags/node-runtime/src/main.rs | 4 +- frame/generate-bags/src/lib.rs | 4 +- frame/staking/src/benchmarking.rs | 66 +++++++++++++------- frame/staking/src/pallet/mod.rs | 2 +- 5 files changed, 54 insertions(+), 30 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index a6a5fbc812ac3..45c4f8b735e7f 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -334,16 +334,20 @@ pub trait SortedListProvider { /// Wether the voter is in the notional bag for the given weight. Only relevant for /// implementations that use bags. + /// + /// `mock` parameter is for implementations where this method is not relevant. #[cfg(any(feature = "runtime-benchmarks", test))] fn is_in_bag(_: &AccountId, _: VoteWeight, mock: bool) -> bool { - mock // default to returning mock value for values that are not relevant + mock } /// Wether the voter is the head of the notional bag for the given weight. Only relevant for /// implementations that use bags. + /// + /// `mock` parameter is for implementations where this method is not relevant. #[cfg(any(feature = "runtime-benchmarks", test))] fn is_bag_head(_: &AccountId, _: VoteWeight, mock: bool) -> bool { - mock // default to returning mock value for values that are not relevant + mock } } diff --git a/frame/generate-bags/node-runtime/src/main.rs b/frame/generate-bags/node-runtime/src/main.rs index 1758e32e911fc..f99acdbfcea15 100644 --- a/frame/generate-bags/node-runtime/src/main.rs +++ b/frame/generate-bags/node-runtime/src/main.rs @@ -17,7 +17,7 @@ //! Make the set of bag thresholds to be used with pallet-bags-list. -use generate_bags::generate_thresholds_module; +use generate_bags::generate_thresholds; use std::path::PathBuf; use structopt::StructOpt; @@ -35,5 +35,5 @@ fn main() -> Result<(), std::io::Error> { let Opt { n_bags, output } = Opt::from_args(); let mut ext = sp_io::TestExternalities::new_empty(); - ext.execute_with(|| generate_thresholds_module::(n_bags, &output)) + ext.execute_with(|| generate_thresholds::(n_bags, &output)) } diff --git a/frame/generate-bags/src/lib.rs b/frame/generate-bags/src/lib.rs index fcbdc16a61d40..dc79812682373 100644 --- a/frame/generate-bags/src/lib.rs +++ b/frame/generate-bags/src/lib.rs @@ -38,7 +38,7 @@ //! //! 2. Write a little program to generate the definitions. This can be a near-identical copy of //! `substrate/frame/bags-list/generate-bags`. This program exists only to hook together the -//! runtime definitions with the various calculations here. +//! runtime definitions with the various calculations here. //! //! 3. Run that program: //! @@ -159,7 +159,7 @@ pub fn thresholds( /// - Module documentation noting that this is autogenerated and when. /// - Some associated constants. /// - The constant array of thresholds. -pub fn generate_thresholds_module( +pub fn generate_thresholds( n_bags: usize, output: &Path, ) -> Result<(), std::io::Error> { diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 7a4c53ffa55f3..55a764ba2100d 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -166,8 +166,12 @@ impl RebagScenario { let dest_factor: BalanceOf = dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); - // create a validators to nominate. - let validators = create_validators::(T::MAX_NOMINATIONS, 100, 333)?; + // create validators to nominate. + let validators = create_validators::( + T::MAX_NOMINATIONS, + 100, + T::MAX_NOMINATIONS * 2, // account seed prevents unintentional account collisions + )?; // create accounts in origin bag let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( @@ -214,11 +218,11 @@ impl RebagScenario { fn check_preconditions(&self) { let RebagScenario { dest_stash1, origin_stash2, origin_thresh_as_vote, .. } = self; - // destination stash is not in the origin bag. + // destination stash1 is not in the origin bag. assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); // origin stash2 is in the origin bag assert!(T::SortedListProvider::is_in_bag(&origin_stash2, *origin_thresh_as_vote, true)); - // head checks implicitly check that dest stash1 and origin stash1 are in the correct bags. + // this implicitly checks that dest stash1 and origin stash1 are in the correct bags. self.check_head_preconditions(); } @@ -230,11 +234,11 @@ impl RebagScenario { origin_thresh_as_vote, .. } = self; - // destination stash is not in the origin bag + // dest stash1 is not in the origin bag assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); // and is in the destination bag. assert!(T::SortedListProvider::is_in_bag(&dest_stash1, *dest_thresh_as_vote, true)); - // the origin stash is in the destination bag. + // origin stash1 is now in the destination bag. assert!(T::SortedListProvider::is_in_bag(&origin_stash1, *dest_thresh_as_vote, true)); // dest stash1 is the head of the destination bag. assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); @@ -242,15 +246,22 @@ impl RebagScenario { } fn check_head_preconditions(&self) { - let RebagScenario { - dest_stash1, - origin_stash1, - dest_thresh_as_vote, - origin_thresh_as_vote, - .. - } = self; - assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); - assert!(T::SortedListProvider::is_bag_head(&origin_stash1, *origin_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_bag_head( + &self.dest_stash1, + self.dest_thresh_as_vote, + true + )); + self.check_origin_head_preconditions(); + } + + // Just checking the origin head is useful for scenarios where we start out with all the nodes + // in the same bag, and thus no destination node is actually ever a head. + fn check_origin_head_preconditions(&self) { + assert!(T::SortedListProvider::is_bag_head( + &self.origin_stash1, + self.origin_thresh_as_vote, + true + )); } // Just checking the origin head is useful for scenarios where we start out with all the nodes @@ -299,16 +310,15 @@ benchmarks! { let controller = scenario.origin_controller1.clone(); let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let original_bonded: BalanceOf = ledger.active; - whitelist_account!(stash); scenario.check_preconditions(); + whitelist_account!(stash); }: _(RawOrigin::Signed(stash.clone()), max_additional) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); - scenario.check_postconditions(); } @@ -342,7 +352,6 @@ benchmarks! { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded > new_bonded); - scenario.check_postconditions(); } @@ -374,8 +383,7 @@ benchmarks! { // A worst case scenario includes the voter being a bag head, so we can reuse the rebag // scenario setup, but we don't care about the setup of the destination bag. - let scenario - = RebagScenario::::new(One::one(), One::one())?; + let scenario = RebagScenario::::new(One::one(), One::one())?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -384,8 +392,10 @@ benchmarks! { let mut ledger = Ledger::::get(&controller).unwrap(); ledger.active = ed - One::one(); Ledger::::insert(&controller, ledger); - CurrentEra::::put(EraIndex::max_value()); + + scenario.check_origin_head_preconditions(); + whitelist_account!(controller); }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { @@ -407,6 +417,8 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_preconditions(); + let prefs = ValidatorPrefs::default(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), prefs) @@ -524,6 +536,8 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_preconditions(); + whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { @@ -593,8 +607,10 @@ benchmarks! { let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - add_slashing_spans::(&stash, s); + + scenario.check_origin_head_preconditions(); + }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); @@ -772,11 +788,12 @@ benchmarks! { add_slashing_spans::(&stash, s); T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance()); - whitelist_account!(controller); assert!(Bonded::::contains_key(&stash)); assert!(T::SortedListProvider::contains(&stash)); + scenario.check_origin_head_preconditions(); + whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { assert!(!Bonded::::contains_key(&stash)); @@ -948,6 +965,9 @@ benchmarks! { Some(0), Some(Percent::from_percent(0)) )?; + + scenario.check_origin_head_preconditions(); + let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index df639cd58d2f0..695a57c06a58f 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -768,7 +768,7 @@ pub mod pallet { Error::::InsufficientBond ); - // ledger must be updated prior to call to `Self::weight_of`. + // ledger must be updated prior to calling `Self::weight_of`. Self::update_ledger(&controller, &ledger); // update this staker in the sorted list, if they exist in it. if T::SortedListProvider::contains(&stash) { From cd3a63cf90add7aadcf7ca0d27092efc026bd61a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 18:28:08 -0700 Subject: [PATCH 17/37] Fix build --- frame/session/benchmarking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/session/benchmarking/src/lib.rs b/frame/session/benchmarking/src/lib.rs index 8b84145c1acfd..349043bdbf991 100644 --- a/frame/session/benchmarking/src/lib.rs +++ b/frame/session/benchmarking/src/lib.rs @@ -126,7 +126,7 @@ fn check_membership_proof_setup( pallet_staking::ValidatorCount::::put(n); // create validators and set random session keys - for (n, who) in create_validators::(n, 1000).unwrap().into_iter().enumerate() { + for (n, who) in create_validators::(n, 1000, 0).unwrap().into_iter().enumerate() { use rand::{RngCore, SeedableRng}; let validator = T::Lookup::lookup(who).unwrap(); From 7d1828123bed7e5c01e13b8ec2233efb988dd03c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 13 Aug 2021 18:48:35 -0700 Subject: [PATCH 18/37] fix build with polkadot companion --- Cargo.lock | 140 ++++++++++++++++++++++++++--------------------------- 1 file changed, 70 insertions(+), 70 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e495ba5b12d5..a4511843910f1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -337,7 +337,7 @@ dependencies = [ "memchr", "num_cpus", "once_cell", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "pin-utils", "slab", "wasm-bindgen-futures", @@ -384,7 +384,7 @@ dependencies = [ "futures-sink", "futures-util", "memchr", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", ] [[package]] @@ -397,7 +397,7 @@ dependencies = [ "futures-sink", "futures-util", "memchr", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", ] [[package]] @@ -548,9 +548,9 @@ dependencies = [ [[package]] name = "bitflags" -version = "1.2.1" +version = "1.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf1de2fe8c75bc145a2f577add951f8134889b4795d47466a54a5c846d691693" +checksum = "2da1976d75adbe5fbc88130ecd119529cf1cc6a93ae1546d8696ee66f0d21af1" [[package]] name = "bitvec" @@ -723,9 +723,9 @@ dependencies = [ [[package]] name = "bumpalo" -version = "3.6.1" +version = "3.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "63396b8a4b9de3f4fdfb320ab6080762242f66a8ef174c49d8e19b674db4cdbe" +checksum = "9c59e7af012c713f529e7a3ee57ce9b31ddd858d4b512923602f74608b009631" [[package]] name = "byte-slice-cast" @@ -1314,9 +1314,9 @@ dependencies = [ [[package]] name = "ctor" -version = "0.1.19" +version = "0.1.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8f45d9ad417bcef4817d614a501ab55cdd96a6fdb24f49aab89a54acfd66b19" +checksum = "5e98e2ad1a782e33928b96fc3948e7c355e5af34ba4de7670fe8bac2a3b2006d" dependencies = [ "quote", "syn", @@ -2141,7 +2141,7 @@ dependencies = [ "futures-io", "memchr", "parking", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "waker-fn", ] @@ -2212,7 +2212,7 @@ dependencies = [ "futures-sink", "futures-task", "memchr", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "pin-utils", "proc-macro-hack", "proc-macro-nested", @@ -2422,9 +2422,9 @@ dependencies = [ [[package]] name = "hermit-abi" -version = "0.1.18" +version = "0.1.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "322f4de77956e22ed0e5032c359a0f1273f1f7f0d79bfa3b8ffbc730d7fbcc5c" +checksum = "62b467343b94ba476dcb2500d242dadbb39557df889310ac77c5d99100aaac33" dependencies = [ "libc", ] @@ -2502,9 +2502,9 @@ dependencies = [ [[package]] name = "http" -version = "0.2.3" +version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7245cd7449cc792608c3c8a9eaf69bd4eabbabf802713748fd739c98b82f0747" +checksum = "527e8c9ac747e28542699a951517aa9a6945af506cd1f2e1b53a576c17b6cc11" dependencies = [ "bytes 1.0.1", "fnv", @@ -2513,13 +2513,13 @@ dependencies = [ [[package]] name = "http-body" -version = "0.4.2" +version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "60daa14be0e0786db0f03a9e57cb404c9d756eed2b6c62b9ea98ec5743ec75a9" +checksum = "399c583b2979440c60be0821a6199eca73bc3c8dcd9d070d75ac726e2c6186e5" dependencies = [ "bytes 1.0.1", "http", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", ] [[package]] @@ -2583,8 +2583,8 @@ dependencies = [ "httparse", "httpdate", "itoa", - "pin-project-lite 0.2.6", - "socket2 0.4.0", + "pin-project-lite 0.2.7", + "socket2 0.4.1", "tokio 1.10.0", "tower-service", "tracing", @@ -2722,9 +2722,9 @@ dependencies = [ [[package]] name = "instant" -version = "0.1.9" +version = "0.1.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "61124eeebbd69b8190558df225adf7e4caafce0d743919e5d6b19652314ec5ec" +checksum = "bee0328b1209d157ef001c94dd85b4f8f64139adb0eac2659f4b08382b2f474d" dependencies = [ "cfg-if 1.0.0", "js-sys", @@ -3119,9 +3119,9 @@ checksum = "3576a87f2ba00f6f106fdfcd16db1d698d648a26ad8e0573cad8537c3c362d2a" [[package]] name = "libc" -version = "0.2.95" +version = "0.2.99" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "789da6d93f1b866ffe175afc5322a4d76c038605a1c3319bb57b06967ca98a36" +checksum = "a7f823d141fe0a24df1e23b4af4e3c7ba9e5966ec514ea068c93024aa7deb765" [[package]] name = "libgit2-sys" @@ -3362,7 +3362,7 @@ dependencies = [ "log 0.4.14", "rand 0.8.4", "smallvec 1.6.1", - "socket2 0.4.0", + "socket2 0.4.1", "void", ] @@ -3535,7 +3535,7 @@ dependencies = [ "libc", "libp2p-core", "log 0.4.14", - "socket2 0.4.0", + "socket2 0.4.1", ] [[package]] @@ -3689,9 +3689,9 @@ dependencies = [ [[package]] name = "lock_api" -version = "0.4.2" +version = "0.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd96ffd135b2fd7b973ac026d28085defbe8983df057ced3eb4f2130b0831312" +checksum = "0382880606dff6d15c9476c416d18690b72742aa7b605bb6dd6ec9030fbf07eb" dependencies = [ "scopeguard", ] @@ -3942,7 +3942,7 @@ checksum = "8c2bdb6314ec10835cd3293dd268473a835c02b7b352e788be788b3c6ca6bb16" dependencies = [ "libc", "log 0.4.14", - "miow 0.3.6", + "miow 0.3.7", "ntapi", "winapi 0.3.9", ] @@ -3984,11 +3984,10 @@ dependencies = [ [[package]] name = "miow" -version = "0.3.6" +version = "0.3.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5a33c1b55807fbed163481b5ba66db4b2fa6cde694a5027be10fb724206c5897" +checksum = "b9f1c5b025cda876f66ef43a113f91ebc9f4ccef34843000e0adf6ebbab84e21" dependencies = [ - "socket2 0.3.19", "winapi 0.3.9", ] @@ -4698,9 +4697,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.7.2" +version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af8b08b04175473088b46763e51ee54da5f9a164bc162f615b91bc179dbf15a3" +checksum = "692fcb63b64b1758029e0a96ee63e049ce8c5948587f2f7208df04625e5f6b56" dependencies = [ "parking_lot 0.11.1", ] @@ -5951,7 +5950,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6d7744ac029df22dca6284efe4e898991d28e3085c706c972bcd7da4a27a15eb" dependencies = [ "instant", - "lock_api 0.4.2", + "lock_api 0.4.4", "parking_lot_core 0.8.3", ] @@ -5993,7 +5992,7 @@ dependencies = [ "cfg-if 1.0.0", "instant", "libc", - "redox_syscall 0.2.5", + "redox_syscall 0.2.10", "smallvec 1.6.1", "winapi 0.3.9", ] @@ -6161,9 +6160,9 @@ checksum = "257b64915a082f7811703966789728173279bdebb956b143dbcd23f6f970a777" [[package]] name = "pin-project-lite" -version = "0.2.6" +version = "0.2.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc0e1f259c92177c30a4c9d177246edd0a3568b25756a977d0632cf8fa37e905" +checksum = "8d31d11c69a6b52a174b42bdc0c30e5e11670f90788b2c471c31c1d17d449443" [[package]] name = "pin-utils" @@ -6830,9 +6829,9 @@ checksum = "41cc0f7e4d5d4544e8861606a285bb08d3e70712ccc7d2b84d7c0ccfaf4b05ce" [[package]] name = "redox_syscall" -version = "0.2.5" +version = "0.2.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94341e4e44e24f6b591b59e47a8a027df12e008d73fd5672dbea9cc22f4507d9" +checksum = "8383f39639269cde97d255a32bdb68c047337295414940c68bdd30c2e13203ff" dependencies = [ "bitflags", ] @@ -6855,7 +6854,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "528532f3d801c87aec9def2add9ca802fe569e44a544afe633765267840abe64" dependencies = [ "getrandom 0.2.3", - "redox_syscall 0.2.5", + "redox_syscall 0.2.10", ] [[package]] @@ -8423,9 +8422,9 @@ checksum = "f638d531eccd6e23b980caf34876660d38e265409d8e99b397ab71eb3612fad0" [[package]] name = "serde" -version = "1.0.126" +version = "1.0.127" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec7505abeacaec74ae4778d9d9328fe5a5d04253220a85c4ee022239fc996d03" +checksum = "f03b9878abf6d14e6779d3f24f07b2cfa90352cfec4acc5aab8f1ac7f146fae8" dependencies = [ "serde_derive", ] @@ -8442,9 +8441,9 @@ dependencies = [ [[package]] name = "serde_derive" -version = "1.0.126" +version = "1.0.127" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "963a7dbc9895aeac7ac90e74f34a5d5261828f79df35cbed41e10189d3804d43" +checksum = "a024926d3432516606328597e0f224a51355a493b49fdd67e9209187cbe55ecc" dependencies = [ "proc-macro2", "quote", @@ -8453,9 +8452,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.64" +version = "1.0.66" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "799e97dc9fdae36a5c8b8f2cae9ce2ee9fdce2058c57a93e6099d919fd982f79" +checksum = "336b10da19a12ad094b59d870ebde26a45402e5b470add4b5fd03c5048a32127" dependencies = [ "itoa", "ryu", @@ -8557,9 +8556,9 @@ dependencies = [ [[package]] name = "signal-hook-registry" -version = "1.3.0" +version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "16f1d0fef1604ba8f7a073c7e701f213e056707210e9020af4528e0101ce11a6" +checksum = "e51e73328dc4ac0c7ccbda3a494dfa03df1de2f46018127f60c693f2648455b0" dependencies = [ "libc", ] @@ -8584,9 +8583,9 @@ dependencies = [ [[package]] name = "slab" -version = "0.4.2" +version = "0.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c111b5bd5695e56cffe5129854aa230b39c93a305372fdbb2668ca2394eea9f8" +checksum = "c307a32c1c5c437f38c7fd45d753050587732ba8628319fbdf12a7e289ccc590" [[package]] name = "slog" @@ -8649,9 +8648,9 @@ dependencies = [ [[package]] name = "socket2" -version = "0.4.0" +version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9e3dfc207c526015c632472a77be09cf1b6e46866581aecae5cc38fb4235dea2" +checksum = "765f090f0e423d2b55843402a07915add955e7d60657db13707a159727326cad" dependencies = [ "libc", "winapi 0.3.9", @@ -9847,9 +9846,9 @@ checksum = "1e81da0851ada1f3e9d4312c704aa4f8806f0f9d69faaf8df2f3464b4a9437c2" [[package]] name = "syn" -version = "1.0.69" +version = "1.0.74" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48fe99c6bd8b1cc636890bcc071842de909d902c81ac7dab53ba33c421ab8ffb" +checksum = "1873d832550d4588c3dbc20f01361ab00bfe741048f71e3fecf145a7cc18b29c" dependencies = [ "proc-macro2", "quote", @@ -9900,7 +9899,7 @@ dependencies = [ "cfg-if 1.0.0", "libc", "rand 0.8.4", - "redox_syscall 0.2.5", + "redox_syscall 0.2.10", "remove_dir_all", "winapi 0.3.9", ] @@ -10149,7 +10148,7 @@ dependencies = [ "num_cpus", "once_cell", "parking_lot 0.11.1", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "signal-hook-registry", "tokio-macros 1.3.0", "winapi 0.3.9", @@ -10289,7 +10288,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7b2f3f698253f03119ac0102beaa64f67a67e08074d03a22d18784104543727f" dependencies = [ "futures-core", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "tokio 1.10.0", ] @@ -10415,7 +10414,7 @@ dependencies = [ "futures-core", "futures-sink", "log 0.4.14", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "tokio 1.10.0", ] @@ -10436,12 +10435,12 @@ checksum = "360dfd1d6d30e05fda32ace2c8c70e9c0a9da713275777f5a4dbb8a1893930c6" [[package]] name = "tracing" -version = "0.1.25" +version = "0.1.26" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "01ebdc2bb4498ab1ab5f5b73c5803825e60199229ccba0698170e3be0e7f959f" +checksum = "09adeb8c97449311ccd28a427f96fb563e7fd31aabf994189879d9da2394b89d" dependencies = [ "cfg-if 1.0.0", - "pin-project-lite 0.2.6", + "pin-project-lite 0.2.7", "tracing-attributes", "tracing-core", ] @@ -10459,9 +10458,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.17" +version = "0.1.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f50de3927f93d202783f4513cda820ab47ef17f624b03c096e86ef00c67e6b5f" +checksum = "a9ff14f98b1a4b289c6248a023c1c2fa1491062964e9fed67ab29c4e4da4a052" dependencies = [ "lazy_static", ] @@ -10753,9 +10752,9 @@ checksum = "9337591893a19b88d8d87f2cec1e73fad5cdfd10e5a6f349f498ad6ea2ffb1e3" [[package]] name = "unicode-xid" -version = "0.2.1" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f7fe0bb3479651439c9112f72b6c505038574c9fbb575ed1bf3b797fa39dd564" +checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" [[package]] name = "universal-hash" @@ -10828,11 +10827,12 @@ dependencies = [ [[package]] name = "value-bag" -version = "1.0.0-alpha.6" +version = "1.0.0-alpha.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6b676010e055c99033117c2343b33a40a30b91fecd6c49055ac9cd2d6c305ab1" +checksum = "dd320e1520f94261153e96f7534476ad869c14022aee1e59af7c778075d840ae" dependencies = [ "ctor", + "version_check 0.9.2", ] [[package]] @@ -11277,9 +11277,9 @@ dependencies = [ [[package]] name = "web-sys" -version = "0.3.47" +version = "0.3.50" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c40dc691fc48003eba817c38da7113c15698142da971298003cac3ef175680b3" +checksum = "a905d57e488fec8861446d3393670fb50d27a262344013181c2cdf9fff5481be" dependencies = [ "js-sys", "wasm-bindgen", From 644e3bfb2df9c6f1eabaf444af12f0e5a41378f4 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Mon, 16 Aug 2021 10:00:59 -0700 Subject: [PATCH 19/37] Update frame/bags-list/src/list/tests.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/bags-list/src/list/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 28d3cb031a59f..49efcca8bf416 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -136,8 +136,8 @@ fn migrate_works() { (15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15 (20, vec![711]), (1000, vec![2, 3, 4]), - (10_000, vec![712]), /* nodes in range 1_001 ..= 2_000 move from bag 2_000 - * to bag 10_000 */ + // nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000 + (10_000, vec![712]), ] ); }); From ded3b490764fb947c6667a8f8f50b6c3045c9b11 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 19 Aug 2021 22:25:45 -0700 Subject: [PATCH 20/37] move generate-bag from frame to utils --- Cargo.toml | 4 ++-- {frame => utils/frame}/generate-bags/Cargo.toml | 12 ++++++------ .../frame}/generate-bags/node-runtime/Cargo.toml | 8 ++++---- .../frame}/generate-bags/node-runtime/src/main.rs | 0 {frame => utils/frame}/generate-bags/src/lib.rs | 0 5 files changed, 12 insertions(+), 12 deletions(-) rename {frame => utils/frame}/generate-bags/Cargo.toml (73%) rename {frame => utils/frame}/generate-bags/node-runtime/Cargo.toml (74%) rename {frame => utils/frame}/generate-bags/node-runtime/src/main.rs (100%) rename {frame => utils/frame}/generate-bags/src/lib.rs (100%) diff --git a/Cargo.toml b/Cargo.toml index e9dd034bf5c50..d2232257d879e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -129,8 +129,6 @@ members = [ "frame/utility", "frame/vesting", "frame/bags-list", - "frame/generate-bags", - "frame/generate-bags/node-runtime", "primitives/api", "primitives/api/proc-macro", "primitives/api/test", @@ -202,6 +200,8 @@ members = [ "utils/frame/try-runtime/cli", "utils/frame/rpc/support", "utils/frame/rpc/system", + "utils/frame/generate-bags", + "utils/frame/generate-bags/node-runtime", "utils/prometheus", "utils/wasm-builder", ] diff --git a/frame/generate-bags/Cargo.toml b/utils/frame/generate-bags/Cargo.toml similarity index 73% rename from frame/generate-bags/Cargo.toml rename to utils/frame/generate-bags/Cargo.toml index a2b6c28f9350f..ecbc09b05f7f7 100644 --- a/frame/generate-bags/Cargo.toml +++ b/utils/frame/generate-bags/Cargo.toml @@ -10,16 +10,16 @@ description = "Bag threshold generation script for pallet-bag-list" readme = "README.md" [dependencies] -node-runtime = { version = "3.0.0-dev", path = "../../bin/node/runtime" } +node-runtime = { version = "3.0.0-dev", path = "../../../bin/node/runtime" } # FRAME -frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } -frame-election-provider-support = { version = "4.0.0-dev", path = "../election-provider-support", features = ["runtime-benchmarks"] } -frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } -pallet-staking = { version = "4.0.0-dev", default-features = false, path = "../staking" } +frame-support = { version = "4.0.0-dev", default-features = false, path = "../../../frame/support" } +frame-election-provider-support = { version = "4.0.0-dev", path = "../../../frame/election-provider-support", features = ["runtime-benchmarks"] } +frame-system = { version = "4.0.0-dev", default-features = false, path = "../../../frame/system" } +pallet-staking = { version = "4.0.0-dev", default-features = false, path = "../../../frame/staking" } # primitives -sp-io = { version = "4.0.0-dev", default-features = false, path = "../../primitives/io" } +sp-io = { version = "4.0.0-dev", default-features = false, path = "../../../primitives/io" } # third party chrono = { version = "0.4.19" } diff --git a/frame/generate-bags/node-runtime/Cargo.toml b/utils/frame/generate-bags/node-runtime/Cargo.toml similarity index 74% rename from frame/generate-bags/node-runtime/Cargo.toml rename to utils/frame/generate-bags/node-runtime/Cargo.toml index 0214b668238af..5b517e4162b2a 100644 --- a/frame/generate-bags/node-runtime/Cargo.toml +++ b/utils/frame/generate-bags/node-runtime/Cargo.toml @@ -10,14 +10,14 @@ description = "Bag threshold generation script for pallet-bag-list and node-runt readme = "README.md" [dependencies] -node-runtime = { version = "3.0.0-dev", path = "../../../bin/node/runtime" } +node-runtime = { version = "3.0.0-dev", path = "../../../../bin/node/runtime" } +generate-bags = { version = "3.0.0", path = "../" } # FRAME -frame-support = { version = "4.0.0-dev", default-features = false, path = "../../support" } -generate-bags = { version = "3.0.0", path = "../" } +frame-support = { version = "4.0.0-dev", default-features = false, path = "../../../../frame/support" } # primitives -sp-io = { version = "4.0.0-dev", path = "../../../primitives/io" } +sp-io = { version = "4.0.0-dev", path = "../../../../primitives/io" } # third-party structopt = "0.3.21" diff --git a/frame/generate-bags/node-runtime/src/main.rs b/utils/frame/generate-bags/node-runtime/src/main.rs similarity index 100% rename from frame/generate-bags/node-runtime/src/main.rs rename to utils/frame/generate-bags/node-runtime/src/main.rs diff --git a/frame/generate-bags/src/lib.rs b/utils/frame/generate-bags/src/lib.rs similarity index 100% rename from frame/generate-bags/src/lib.rs rename to utils/frame/generate-bags/src/lib.rs From 624cfe9a9191f4b70ac095c9d836b81ff7c608de Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 23 Aug 2021 18:06:39 -0700 Subject: [PATCH 21/37] wip --- Cargo.lock | 5 ++--- frame/bags-list/src/lib.rs | 6 ++++++ frame/election-provider-support/src/lib.rs | 6 ++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2bf915b439d5f..89db06226d785 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -529,9 +529,9 @@ dependencies = [ [[package]] name = "bitflags" -version = "1.3.1" +version = "1.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2da1976d75adbe5fbc88130ecd119529cf1cc6a93ae1546d8696ee66f0d21af1" +checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] name = "bitvec" @@ -5772,7 +5772,6 @@ dependencies = [ "pallet-staking-reward-curve", "pallet-timestamp", "parity-scale-codec", - "rand 0.8.4", "rand_chacha 0.2.2", "serde", "sp-application-crypto", diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index d53c5866e738f..be4a02001a6a7 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -275,4 +275,10 @@ impl SortedListProvider for Pallet { fn is_bag_head(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().head_id() == Some(id) } + + /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of the worst case + #[cfg(any(feature = "runtime-benchmarks", test))] + fn weight_update_worst_case(_who: &T::AccountId) -> Result { + + } } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index b31b9838dd27d..4632b6828bd75 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -351,6 +351,12 @@ pub trait SortedListProvider { fn is_bag_head(_: &AccountId, _: VoteWeight, mock: bool) -> bool { mock } + + /// If `who` changes by `amount` they are guaranteed to change bags in terms of the worst case + #[cfg(any(feature = "runtime-benchmarks", test))] + fn weight_update_worst_case(_who: &AccountId, _amount: Balance) -> VoteWeight { + VoteWeight::MAX + } } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and From d1f5d449024d0c3b06156acc4ac0cf7ab280c00a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Mon, 23 Aug 2021 21:19:08 -0700 Subject: [PATCH 22/37] refactor WIP --- frame/bags-list/src/lib.rs | 30 +++- frame/bags-list/src/list/mod.rs | 2 +- frame/election-provider-support/src/lib.rs | 18 +- frame/staking/src/benchmarking.rs | 185 ++++++++++----------- 4 files changed, 125 insertions(+), 110 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index be4a02001a6a7..201f9652381b9 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -266,8 +266,9 @@ impl SortedListProvider for Pallet { List::::clear() } + /// `id` is in a position in the list that corresponds to `weight` #[cfg(feature = "runtime-benchmarks")] - fn is_in_bag(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { + fn is_in_pos(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } @@ -276,9 +277,28 @@ impl SortedListProvider for Pallet { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().head_id() == Some(id) } - /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of the worst case - #[cfg(any(feature = "runtime-benchmarks", test))] - fn weight_update_worst_case(_who: &T::AccountId) -> Result { - + /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of + /// the worst case + #[cfg(feature = "runtime-benchmarks")] + fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> VoteWeight { + use frame_support::traits::Get as _; + let thresholds = T::BagThresholds::get(); + let node = list::Node::::get(who).unwrap(); + let current_bag_idx = thresholds + .iter() + .chain(std::iter::once(&VoteWeight::MAX)) + .position(|w| w == &node.bag_upper) + .unwrap(); + + if is_increase { + // +2 helps in some edge cases to ensure threshold are far enough apart + let next_threshold_idx = current_bag_idx + 2; + assert!(thresholds.len() > next_threshold_idx); + thresholds[next_threshold_idx] + } else { + assert!(current_bag_idx != 0); + let prev_threshold_idx = current_bag_idx - 1; + thresholds[prev_threshold_idx] + } } } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 502066936f94f..bf2b83e0a0840 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -606,7 +606,7 @@ pub struct Node { id: T::AccountId, prev: Option, next: Option, - bag_upper: VoteWeight, + pub(crate) bag_upper: VoteWeight, } impl Node { diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 4632b6828bd75..6344e9f147dbb 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -334,12 +334,9 @@ pub trait SortedListProvider { /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; - /// Wether the voter is in the notional bag for the given weight. Only relevant for - /// implementations that use bags. - /// - /// `mock` parameter is for implementations where this method is not relevant. + /// `id` is in a position in the list that corresponds to `weight` #[cfg(any(feature = "runtime-benchmarks", test))] - fn is_in_bag(_: &AccountId, _: VoteWeight, mock: bool) -> bool { + fn is_in_pos(_id: &AccountId, _weight: VoteWeight, mock: bool) -> bool { mock } @@ -352,10 +349,15 @@ pub trait SortedListProvider { mock } - /// If `who` changes by `amount` they are guaranteed to change bags in terms of the worst case + /// If `who` changes by the returned amount they are guaranteed to have a worst case change + /// in their list position. #[cfg(any(feature = "runtime-benchmarks", test))] - fn weight_update_worst_case(_who: &AccountId, _amount: Balance) -> VoteWeight { - VoteWeight::MAX + fn weight_update_worst_case(_who: &AccountId, is_increase: bool) -> VoteWeight { + if is_increase { + VoteWeight::MAX + } else { + 1 + } } } diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 55a764ba2100d..06b8407df856c 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -132,7 +132,7 @@ pub fn create_validator_with_nominators( Ok((v_stash, nominators)) } -struct RebagScenario { +struct ListScenario { dest_stash1: T::AccountId, /// Stash that is expected to be rebagged. origin_stash1: T::AccountId, @@ -141,9 +141,10 @@ struct RebagScenario { origin_stash2: T::AccountId, origin_thresh_as_vote: VoteWeight, dest_thresh_as_vote: VoteWeight, + dest_thresh: BalanceOf, } -impl RebagScenario { +impl ListScenario { /// An expensive rebag scenario: /// /// - the node to be rebagged (r) is the head of a bag that has at least one other node. The bag @@ -151,20 +152,12 @@ impl RebagScenario { /// will need to be read and written as it will need to have its prev pointer updated. /// /// - the destination bag has at least one node, which will need its next pointer updated. - fn new( - origin_bag_thresh: BalanceOf, - dest_bag_thresh: BalanceOf, - ) -> Result { - ensure!( - !origin_bag_thresh.is_zero() && !dest_bag_thresh.is_zero(), - "both thresholds must be greater than 0" - ); + fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { + ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); // create_stash_controller takes a factor, so we compute it. let origin_factor: BalanceOf = - origin_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); - let dest_factor: BalanceOf = - dest_bag_thresh * 10u32.into() / T::Currency::minimum_balance(); + origin_weight * 10u32.into() / T::Currency::minimum_balance(); // create validators to nominate. let validators = create_validators::( @@ -173,7 +166,7 @@ impl RebagScenario { T::MAX_NOMINATIONS * 2, // account seed prevents unintentional account collisions )?; - // create accounts in origin bag + // create accounts with the origin weight let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( USER_SEED + 2, origin_factor, @@ -194,7 +187,14 @@ impl RebagScenario { validators.clone(), )?; - // create an account in the destination bag + // find a destination weight that will trigger the worst case scenario. + let dest_weight = + T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); + let total_issuance = T::Currency::total_issuance(); + let dest_weight_balance = T::CurrencyToVote::to_currency(dest_weight as u128, total_issuance); + let dest_factor: BalanceOf = dest_weight_balance * 10u32.into() / T::Currency::minimum_balance(); + + // create an account with the worst case destination weight let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( USER_SEED + 1, dest_factor, @@ -202,32 +202,31 @@ impl RebagScenario { )?; Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators)?; - let total_issuance = T::Currency::total_issuance(); - let origin_thresh_as_vote = T::CurrencyToVote::to_vote(origin_bag_thresh, total_issuance); - let dest_thresh_as_vote = T::CurrencyToVote::to_vote(dest_bag_thresh, total_issuance); - Ok(RebagScenario { + + Ok(ListScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2, - origin_thresh_as_vote, - dest_thresh_as_vote, + origin_thresh_as_vote: T::CurrencyToVote::to_vote(origin_weight, total_issuance), + dest_thresh_as_vote: dest_weight, + dest_thresh: dest_weight_balance, }) } fn check_preconditions(&self) { - let RebagScenario { dest_stash1, origin_stash2, origin_thresh_as_vote, .. } = self; + let ListScenario { dest_stash1, origin_stash2, origin_thresh_as_vote, .. } = self; // destination stash1 is not in the origin bag. - assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); + assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_thresh_as_vote, false)); // origin stash2 is in the origin bag - assert!(T::SortedListProvider::is_in_bag(&origin_stash2, *origin_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_thresh_as_vote, true)); // this implicitly checks that dest stash1 and origin stash1 are in the correct bags. - self.check_head_preconditions(); + // self.check_head_preconditions(); } fn check_postconditions(&self) { - let RebagScenario { + let ListScenario { dest_stash1, origin_stash1, dest_thresh_as_vote, @@ -235,41 +234,41 @@ impl RebagScenario { .. } = self; // dest stash1 is not in the origin bag - assert!(!T::SortedListProvider::is_in_bag(&dest_stash1, *origin_thresh_as_vote, false)); + assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_thresh_as_vote, false)); // and is in the destination bag. - assert!(T::SortedListProvider::is_in_bag(&dest_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_thresh_as_vote, true)); // origin stash1 is now in the destination bag. - assert!(T::SortedListProvider::is_in_bag(&origin_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_thresh_as_vote, true)); // dest stash1 is the head of the destination bag. - assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); - self.check_origin_head_postconditions(); - } - - fn check_head_preconditions(&self) { - assert!(T::SortedListProvider::is_bag_head( - &self.dest_stash1, - self.dest_thresh_as_vote, - true - )); - self.check_origin_head_preconditions(); + // assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); + // self.check_origin_head_postconditions(); } - // Just checking the origin head is useful for scenarios where we start out with all the nodes - // in the same bag, and thus no destination node is actually ever a head. - fn check_origin_head_preconditions(&self) { - assert!(T::SortedListProvider::is_bag_head( - &self.origin_stash1, - self.origin_thresh_as_vote, - true - )); - } - - // Just checking the origin head is useful for scenarios where we start out with all the nodes - // in the same bag, and thus no destination node is actually ever a head. - fn check_origin_head_postconditions(&self) { - let RebagScenario { origin_stash2, origin_thresh_as_vote, .. } = self; - assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); - } + // fn check_head_preconditions(&self) { + // assert!(T::SortedListProvider::is_bag_head( + // &self.dest_stash1, + // self.dest_thresh_as_vote, + // true + // )); + // self.check_origin_head_preconditions(); + // } + + // // Just checking the origin head is useful for scenarios where we start out with all the + // nodes // in the same bag, and thus no destination node is actually ever a head. + // fn check_origin_head_preconditions(&self) { + // assert!(T::SortedListProvider::is_bag_head( + // &self.origin_stash1, + // self.origin_thresh_as_vote, + // true + // )); + // } + + // // Just checking the origin head is useful for scenarios where we start out with all the + // nodes // in the same bag, and thus no destination node is actually ever a head. + // fn check_origin_head_postconditions(&self) { + // let ListScenario { origin_stash2, origin_thresh_as_vote, .. } = self; + // assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); + // } } const USER_SEED: u32 = 999666; @@ -292,19 +291,15 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); - // The worst case scenario includes the voter changing bags. + // setup the worst case list scenario let total_issuance = T::Currency::total_issuance(); - // the bag the voter will start at + // the weight the voter will start at let origin_bag_thresh = T::CurrencyToVote::to_currency(1u128, total_issuance); - // the bag we will move the voter to - let dest_bag_thresh = - T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); - let scenario - = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + = ListScenario::::new(origin_bag_thresh, true)?; - let max_additional = dest_bag_thresh - origin_bag_thresh; + let max_additional = scenario.dest_thresh.clone() - origin_bag_thresh; let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); @@ -332,15 +327,12 @@ benchmarks! { // the bag the voter will start at let origin_bag_thresh = T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); - // the bag we will move the voter to - let dest_bag_thresh = - T::CurrencyToVote::to_currency(1u128, total_issuance); let scenario - = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + = ListScenario::::new(origin_bag_thresh, false)?; let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); - let amount = origin_bag_thresh - dest_bag_thresh; + let amount = origin_bag_thresh - scenario.dest_thresh.clone(); let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; @@ -383,7 +375,7 @@ benchmarks! { // A worst case scenario includes the voter being a bag head, so we can reuse the rebag // scenario setup, but we don't care about the setup of the destination bag. - let scenario = RebagScenario::::new(One::one(), One::one())?; + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -394,14 +386,16 @@ benchmarks! { Ledger::::insert(&controller, ledger); CurrentEra::::put(EraIndex::max_value()); - scenario.check_origin_head_preconditions(); + // TODO + // scenario.check_origin_head_preconditions(); whitelist_account!(controller); }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { assert!(!Ledger::::contains_key(controller)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // TODO + // scenario.check_origin_head_postconditions(); } validate { @@ -412,12 +406,12 @@ benchmarks! { // a nominator), so we can reuse the rebag scenario setup, but we don't care about the setup // of the destination bag. - let scenario = RebagScenario::::new(One::one(), One::one())?; + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_preconditions(); + // scenario.check_origin_head_preconditions(); todo let prefs = ValidatorPrefs::default(); whitelist_account!(controller); @@ -425,7 +419,7 @@ benchmarks! { verify { assert!(Validators::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // scenario.check_origin_head_postconditions(); todo } kick { @@ -502,7 +496,7 @@ benchmarks! { // the bag itself and the tail.next pointer need to be updated. let threshold = One::one(); // all the voters in the scenario will be in the same bag. - let scenario = RebagScenario::::new(threshold, threshold)?; + let scenario = ListScenario::::new(threshold, true)?; let origin_threshold_factor: BalanceOf = threshold * 10u32.into() / T::Currency::minimum_balance(); let (stash, controller) = create_stash_controller_with_max_free::( @@ -531,18 +525,18 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario - = RebagScenario::::new(One::one(), One::one())?; + = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_preconditions(); + // scenario.check_origin_head_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // scenario.check_origin_head_postconditions(); } set_payee { @@ -603,19 +597,19 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario - = RebagScenario::::new(One::one(), One::one())?; + = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); add_slashing_spans::(&stash, s); - scenario.check_origin_head_preconditions(); + // scenario.check_origin_head_preconditions(); }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // scenario.check_origin_head_postconditions(); } cancel_deferred_slash { @@ -714,21 +708,20 @@ benchmarks! { // the bag the voter will start at let origin_bag_thresh = - T::CurrencyToVote::to_currency(1u128, total_issuance); - // the bag we will move the voter to - let dest_bag_thresh = - T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + T::CurrencyToVote::to_currency(One::one(), total_issuance); let scenario - = RebagScenario::::new(origin_bag_thresh, dest_bag_thresh)?; + = ListScenario::::new(origin_bag_thresh, true)?; + let dest_thresh = scenario.dest_thresh.clone(); + println!("dest: {:#?}. origin: {:#?}", dest_thresh, origin_bag_thresh); // rebond an amount that will put the user into the destination bag - let rebond_amount = dest_bag_thresh - origin_bag_thresh; + let rebond_amount = dest_thresh - origin_bag_thresh; // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); // so the sum of unlocking chunks puts voter into the dest bag - assert!((value * l.into() + origin_bag_thresh) > origin_bag_thresh); - assert!(value * l.into() + origin_bag_thresh <= dest_bag_thresh); + assert!(value * l.into() + origin_bag_thresh > origin_bag_thresh); + assert!(value * l.into() + origin_bag_thresh <= dest_thresh); let unlock_chunk = UnlockChunk::> { value, era: EraIndex::zero(), @@ -782,7 +775,7 @@ benchmarks! { // A worst case scenario includes the voter being a bag head, so we can reuse the rebag // scenario setup, but we don't care about the setup of the destination bag. let scenario - = RebagScenario::::new(One::one(), One::one())?; + = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -791,14 +784,14 @@ benchmarks! { assert!(Bonded::::contains_key(&stash)); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_preconditions(); + // scenario.check_origin_head_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { assert!(!Bonded::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // scenario.check_origin_head_postconditions(); } new_era { @@ -952,7 +945,7 @@ benchmarks! { // thresholds are inconsequential because we are just care that we are removing a head. let scenario - = RebagScenario::::new(One::one(), One::one())?; + = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -966,13 +959,13 @@ benchmarks! { Some(Percent::from_percent(0)) )?; - scenario.check_origin_head_preconditions(); + // scenario.check_origin_head_preconditions(); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_head_postconditions(); + // scenario.check_origin_head_postconditions(); } } From dbb95db20a400465726fc0e1964acbe452e0be47 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 07:43:52 -0700 Subject: [PATCH 23/37] WIP save --- frame/bags-list/src/lib.rs | 4 ++-- frame/staking/src/benchmarking.rs | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 201f9652381b9..0577b5f8f2f14 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -291,8 +291,8 @@ impl SortedListProvider for Pallet { .unwrap(); if is_increase { - // +2 helps in some edge cases to ensure threshold are far enough apart - let next_threshold_idx = current_bag_idx + 2; + // +2 helps in some edge cases to ensure threshold are far enough apart TODO + let next_threshold_idx = current_bag_idx + 1; assert!(thresholds.len() > next_threshold_idx); thresholds[next_threshold_idx] } else { diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 06b8407df856c..8de07861788f9 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -708,17 +708,19 @@ benchmarks! { // the bag the voter will start at let origin_bag_thresh = - T::CurrencyToVote::to_currency(One::one(), total_issuance); + T::CurrencyToVote::to_currency(MAX_UNLOCKING_CHUNKS as u64, total_issuance); let scenario = ListScenario::::new(origin_bag_thresh, true)?; let dest_thresh = scenario.dest_thresh.clone(); println!("dest: {:#?}. origin: {:#?}", dest_thresh, origin_bag_thresh); // rebond an amount that will put the user into the destination bag - let rebond_amount = dest_thresh - origin_bag_thresh; + // TODO + let rebond_amount = dest_thresh - origin_bag_thresh // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); + println!("value: {:#?}. rebond_amount: {:#?}. l: {:#?}.", value, rebond_amount, l); // so the sum of unlocking chunks puts voter into the dest bag assert!(value * l.into() + origin_bag_thresh > origin_bag_thresh); assert!(value * l.into() + origin_bag_thresh <= dest_thresh); From 0cd71037ef51a6c591345aefdc11e20eaecb5ce7 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 09:42:59 -0700 Subject: [PATCH 24/37] Refactor working --- frame/bags-list/src/lib.rs | 9 ++------- frame/staking/src/benchmarking.rs | 7 +++---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 0577b5f8f2f14..16107efaf20c1 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -266,17 +266,12 @@ impl SortedListProvider for Pallet { List::::clear() } - /// `id` is in a position in the list that corresponds to `weight` + #[cfg(feature = "runtime-benchmarks")] fn is_in_pos(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } - #[cfg(feature = "runtime-benchmarks")] - fn is_bag_head(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { - list::Bag::::get(list::notional_bag_for::(weight)).unwrap().head_id() == Some(id) - } - /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of /// the worst case #[cfg(feature = "runtime-benchmarks")] @@ -291,7 +286,7 @@ impl SortedListProvider for Pallet { .unwrap(); if is_increase { - // +2 helps in some edge cases to ensure threshold are far enough apart TODO + // +3 helps in some edge cases to ensure threshold are far enough apart TODO let next_threshold_idx = current_bag_idx + 1; assert!(thresholds.len() > next_threshold_idx); thresholds[next_threshold_idx] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 8de07861788f9..2400d601a44f9 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -708,7 +708,7 @@ benchmarks! { // the bag the voter will start at let origin_bag_thresh = - T::CurrencyToVote::to_currency(MAX_UNLOCKING_CHUNKS as u64, total_issuance); + T::CurrencyToVote::to_currency(100u128, total_issuance); let scenario = ListScenario::::new(origin_bag_thresh, true)?; let dest_thresh = scenario.dest_thresh.clone(); @@ -716,7 +716,7 @@ benchmarks! { println!("dest: {:#?}. origin: {:#?}", dest_thresh, origin_bag_thresh); // rebond an amount that will put the user into the destination bag // TODO - let rebond_amount = dest_thresh - origin_bag_thresh + let rebond_amount = dest_thresh - origin_bag_thresh; // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); @@ -776,8 +776,7 @@ benchmarks! { // A worst case scenario includes the voter being a bag head, so we can reuse the rebag // scenario setup, but we don't care about the setup of the destination bag. - let scenario - = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); From aea555433ea4e9e21a329b4a08d481d3748c79bd Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 10:23:25 -0700 Subject: [PATCH 25/37] some variable renaming --- frame/staking/src/benchmarking.rs | 162 +++++++++++++----------------- 1 file changed, 69 insertions(+), 93 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 2400d601a44f9..5427b9ab8cc16 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -134,14 +134,14 @@ pub fn create_validator_with_nominators( struct ListScenario { dest_stash1: T::AccountId, - /// Stash that is expected to be rebagged. + /// Stash that is expected to be move. origin_stash1: T::AccountId, - /// Controller of the Stash that is expected to be rebagged. + /// Controller of the Stash that is expected to be move. origin_controller1: T::AccountId, origin_stash2: T::AccountId, - origin_thresh_as_vote: VoteWeight, - dest_thresh_as_vote: VoteWeight, - dest_thresh: BalanceOf, + origin_weight_as_vote: VoteWeight, + dest_weight_as_vote: VoteWeight, + dest_weight: BalanceOf, } impl ListScenario { @@ -188,11 +188,11 @@ impl ListScenario { )?; // find a destination weight that will trigger the worst case scenario. - let dest_weight = + let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); let total_issuance = T::Currency::total_issuance(); - let dest_weight_balance = T::CurrencyToVote::to_currency(dest_weight as u128, total_issuance); - let dest_factor: BalanceOf = dest_weight_balance * 10u32.into() / T::Currency::minimum_balance(); + let dest_weight = T::CurrencyToVote::to_currency(dest_weight_as_vote as u128, total_issuance); + let dest_factor: BalanceOf = dest_weight * 10u32.into() / T::Currency::minimum_balance(); // create an account with the worst case destination weight let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( @@ -202,25 +202,23 @@ impl ListScenario { )?; Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators)?; - - Ok(ListScenario { dest_stash1, origin_stash1, origin_controller1, origin_stash2, - origin_thresh_as_vote: T::CurrencyToVote::to_vote(origin_weight, total_issuance), - dest_thresh_as_vote: dest_weight, - dest_thresh: dest_weight_balance, + origin_weight_as_vote: T::CurrencyToVote::to_vote(origin_weight, total_issuance), + dest_weight_as_vote, + dest_weight, }) } fn check_preconditions(&self) { - let ListScenario { dest_stash1, origin_stash2, origin_thresh_as_vote, .. } = self; + let ListScenario { dest_stash1, origin_stash2, origin_weight_as_vote, .. } = self; // destination stash1 is not in the origin bag. - assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_thresh_as_vote, false)); + assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); // origin stash2 is in the origin bag - assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); // this implicitly checks that dest stash1 and origin stash1 are in the correct bags. // self.check_head_preconditions(); } @@ -229,25 +227,25 @@ impl ListScenario { let ListScenario { dest_stash1, origin_stash1, - dest_thresh_as_vote, - origin_thresh_as_vote, + dest_weight_as_vote, + origin_weight_as_vote, .. } = self; // dest stash1 is not in the origin bag - assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_thresh_as_vote, false)); + assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); // and is in the destination bag. - assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); // origin stash1 is now in the destination bag. - assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_thresh_as_vote, true)); + assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_weight_as_vote, true)); // dest stash1 is the head of the destination bag. - // assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_thresh_as_vote, true)); + // assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_weight_as_vote, true)); // self.check_origin_head_postconditions(); } // fn check_head_preconditions(&self) { // assert!(T::SortedListProvider::is_bag_head( // &self.dest_stash1, - // self.dest_thresh_as_vote, + // self.dest_weight_as_vote, // true // )); // self.check_origin_head_preconditions(); @@ -258,7 +256,7 @@ impl ListScenario { // fn check_origin_head_preconditions(&self) { // assert!(T::SortedListProvider::is_bag_head( // &self.origin_stash1, - // self.origin_thresh_as_vote, + // self.origin_weight_as_vote, // true // )); // } @@ -266,8 +264,8 @@ impl ListScenario { // // Just checking the origin head is useful for scenarios where we start out with all the // nodes // in the same bag, and thus no destination node is actually ever a head. // fn check_origin_head_postconditions(&self) { - // let ListScenario { origin_stash2, origin_thresh_as_vote, .. } = self; - // assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_thresh_as_vote, true)); + // let ListScenario { origin_stash2, origin_weight_as_vote, .. } = self; + // assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_weight_as_vote, true)); // } } @@ -288,18 +286,16 @@ benchmarks! { } bond_extra { - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // setup the worst case list scenario + // setup the worst case list scenario. let total_issuance = T::Currency::total_issuance(); - // the weight the voter will start at - let origin_bag_thresh = - T::CurrencyToVote::to_currency(1u128, total_issuance); - let scenario - = ListScenario::::new(origin_bag_thresh, true)?; + // the weight the nominator will start at. + let origin_weight = One::one(); + let scenario = ListScenario::::new(origin_weight, true)?; - let max_additional = scenario.dest_thresh.clone() - origin_bag_thresh; + let max_additional = scenario.dest_weight.clone() - origin_weight; let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); @@ -318,21 +314,18 @@ benchmarks! { } unbond { - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // The worst case scenario includes the voter changing bags. - + // setup the worst case list scenario. let total_issuance = T::Currency::total_issuance(); - // the bag the voter will start at - let origin_bag_thresh = - T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); - let scenario - = ListScenario::::new(origin_bag_thresh, false)?; + // the weight the nominator will start at. + let origin_weight = T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + let scenario = ListScenario::::new(origin_weight, false)?; let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); - let amount = origin_bag_thresh - scenario.dest_thresh.clone(); + let amount = origin_weight - scenario.dest_weight.clone(); let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; @@ -370,11 +363,11 @@ benchmarks! { withdraw_unbonded_kill { // Slashing Spans let s in 0 .. MAX_SPANS; - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head, so we can reuse the rebag - // scenario setup, but we don't care about the setup of the destination bag. + // setup a worst case list scenario. Note that we don't care about the setup of the + // destination bag. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -399,13 +392,11 @@ benchmarks! { } validate { - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head (which implies they are already - // a nominator), so we can reuse the rebag scenario setup, but we don't care about the setup - // of the destination bag. - + // setup a worst case scenario where the user calling validate was formerly a nominator so + // they must be removed from the list. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -489,19 +480,17 @@ benchmarks! { nominate { let n in 1 .. T::MAX_NOMINATIONS; - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // The worst case scenario: a voter is inserted into a bag that already has voters so both - // the bag itself and the tail.next pointer need to be updated. - - let threshold = One::one(); // all the voters in the scenario will be in the same bag. + // setup a worst case list scenario. + let threshold = One::one(); let scenario = ListScenario::::new(threshold, true)?; - let origin_threshold_factor: BalanceOf = + let origin_weight_factor: BalanceOf = threshold * 10u32.into() / T::Currency::minimum_balance(); let (stash, controller) = create_stash_controller_with_max_free::( SEED + T::MAX_NOMINATIONS + 1, // make sure the account does not conflict with others - origin_threshold_factor, // bond an amount that puts them in the bag with nodes. + origin_weight_factor, // bond an amount that puts them in the bag with nodes. Default::default(), )?; @@ -520,10 +509,8 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head, so we can reuse the rebag - // scenario setup, but we don't care about the setup of the destination bag. - - // thresholds are inconsequential because we are just care that we are removing a head. + // setup a worst case list scenario. Note that we don't care about the setup of the + // destination bag. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); @@ -592,10 +579,8 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head, so we can reuse the rebag - // scenario setup, but we don't care about the setup of the destination bag. - - // thresholds are inconsequential because we are just care that we are removing a head. + // setup a worst case list scenario. Note that we don't care about the setup of the + // destination bag. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); @@ -699,31 +684,25 @@ benchmarks! { rebond { let l in 1 .. MAX_UNLOCKING_CHUNKS as u32; - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // The worst case scenario includes the voter changing bags. - - let total_issuance = T::Currency::total_issuance(); + // setup a worst case list scenario. - // the bag the voter will start at - let origin_bag_thresh = - T::CurrencyToVote::to_currency(100u128, total_issuance); - let scenario - = ListScenario::::new(origin_bag_thresh, true)?; - let dest_thresh = scenario.dest_thresh.clone(); + // the weight the nominator will start at. Note we use 100 to play friendly with the bags + // list threshold values in the mock. + let origin_weight = 100u32.into(); + let scenario = ListScenario::::new(origin_weight, true)?; + let dest_weight = scenario.dest_weight.clone(); - println!("dest: {:#?}. origin: {:#?}", dest_thresh, origin_bag_thresh); - // rebond an amount that will put the user into the destination bag - // TODO - let rebond_amount = dest_thresh - origin_bag_thresh; + // rebond an amount that will give the use dest_weight + let rebond_amount = dest_weight - origin_weight; // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); - println!("value: {:#?}. rebond_amount: {:#?}. l: {:#?}.", value, rebond_amount, l); // so the sum of unlocking chunks puts voter into the dest bag - assert!(value * l.into() + origin_bag_thresh > origin_bag_thresh); - assert!(value * l.into() + origin_bag_thresh <= dest_thresh); + assert!(value * l.into() + origin_weight > origin_weight); + assert!(value * l.into() + origin_weight <= dest_weight); let unlock_chunk = UnlockChunk::> { value, era: EraIndex::zero(), @@ -771,11 +750,11 @@ benchmarks! { reap_stash { let s in 1 .. MAX_SPANS; - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head, so we can reuse the rebag - // scenario setup, but we don't care about the setup of the destination bag. + // setup a worst case list scenario. Note that we don't care about the setup of the + // destination bag. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -938,15 +917,12 @@ benchmarks! { } chill_other { - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); - // A worst case scenario includes the voter being a bag head, so we can reuse the rebag - // scenario setup, but we don't care about the setup of the destination bag. - - // thresholds are inconsequential because we are just care that we are removing a head. - let scenario - = ListScenario::::new(One::one(), true)?; + // setup a worst case list scenario. Note that we don't care about the setup of the + // destination bag. + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); From 033b7173c6bf582ba2e3e084f8bc94356ff85f97 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 11:16:55 -0700 Subject: [PATCH 26/37] WIP: prepare to remove head checks --- frame/bags-list/src/lib.rs | 6 +- frame/bags-list/src/list/mod.rs | 7 +- frame/election-provider-support/src/lib.rs | 15 +-- frame/staking/src/benchmarking.rs | 108 ++++++++++----------- 4 files changed, 62 insertions(+), 74 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 16107efaf20c1..a0387534f7439 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -266,12 +266,16 @@ impl SortedListProvider for Pallet { List::::clear() } - #[cfg(feature = "runtime-benchmarks")] fn is_in_pos(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } + #[cfg(feature = "runtime-benchmarks")] + fn is_worst_pos(id: &T::AccountId, _: bool) -> bool { + list::Node::::get(id).unwrap().is_terminal() + } + /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of /// the worst case #[cfg(feature = "runtime-benchmarks")] diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index bf2b83e0a0840..628758a360129 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -591,11 +591,6 @@ impl Bag { pub(crate) fn contains(&self, id: &T::AccountId) -> bool { self.iter().any(|n| n.id() == id) } - - #[cfg(feature = "runtime-benchmarks")] - pub(crate) fn head_id(&self) -> Option<&T::AccountId> { - self.head.as_ref() - } } /// A Node is the fundamental element comprising the doubly-linked list described by `Bag`. @@ -652,7 +647,7 @@ impl Node { } /// `true` when this voter is a bag head or tail. - fn is_terminal(&self) -> bool { + pub(crate) fn is_terminal(&self) -> bool { self.prev.is_none() || self.next.is_none() } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 6344e9f147dbb..d353b0876212a 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -334,24 +334,15 @@ pub trait SortedListProvider { /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; - /// `id` is in a position in the list that corresponds to `weight` - #[cfg(any(feature = "runtime-benchmarks", test))] + /// Whether or not `id` is in a position in the list that corresponds to `weight`. + #[cfg(feature = "runtime-benchmarks")] fn is_in_pos(_id: &AccountId, _weight: VoteWeight, mock: bool) -> bool { mock } - /// Wether the voter is the head of the notional bag for the given weight. Only relevant for - /// implementations that use bags. - /// - /// `mock` parameter is for implementations where this method is not relevant. - #[cfg(any(feature = "runtime-benchmarks", test))] - fn is_bag_head(_: &AccountId, _: VoteWeight, mock: bool) -> bool { - mock - } - /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. - #[cfg(any(feature = "runtime-benchmarks", test))] + #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(_who: &AccountId, is_increase: bool) -> VoteWeight { if is_increase { VoteWeight::MAX diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 5427b9ab8cc16..07f1d034c5d54 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -134,9 +134,9 @@ pub fn create_validator_with_nominators( struct ListScenario { dest_stash1: T::AccountId, - /// Stash that is expected to be move. + /// Stash that is expected to be moved. origin_stash1: T::AccountId, - /// Controller of the Stash that is expected to be move. + /// Controller of the Stash that is expected to be moved. origin_controller1: T::AccountId, origin_stash2: T::AccountId, origin_weight_as_vote: VoteWeight, @@ -145,13 +145,17 @@ struct ListScenario { } impl ListScenario { - /// An expensive rebag scenario: + /// An expensive scenario for bags-list implementation: /// - /// - the node to be rebagged (r) is the head of a bag that has at least one other node. The bag + /// - the node to be updated (r) is the head of a bag that has at least one other node. The bag /// itself will need to be read and written to update its head. The node pointed to by r.next /// will need to be read and written as it will need to have its prev pointer updated. /// /// - the destination bag has at least one node, which will need its next pointer updated. + /// + /// NOTE: while this scenario specifically targets a worst case for the bags-list, it should + /// also elicit a worst case for other known `SortedListProvider` implementations; although + /// this is subject to change. fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); @@ -191,7 +195,8 @@ impl ListScenario { let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); let total_issuance = T::Currency::total_issuance(); - let dest_weight = T::CurrencyToVote::to_currency(dest_weight_as_vote as u128, total_issuance); + let dest_weight = + T::CurrencyToVote::to_currency(dest_weight_as_vote as u128, total_issuance); let dest_factor: BalanceOf = dest_weight * 10u32.into() / T::Currency::minimum_balance(); // create an account with the worst case destination weight @@ -214,13 +219,16 @@ impl ListScenario { } fn check_preconditions(&self) { - let ListScenario { dest_stash1, origin_stash2, origin_weight_as_vote, .. } = self; - // destination stash1 is not in the origin bag. + let ListScenario { dest_stash1, origin_stash2, origin_stash1, origin_weight_as_vote, .. } + = self; + // destination stash1 is not in the origin pos. assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); - // origin stash2 is in the origin bag + // origin stash1 is in the origin pos. + assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *origin_weight_as_vote, true)); + // origin stash2 is in the origin pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); - // this implicitly checks that dest stash1 and origin stash1 are in the correct bags. - // self.check_head_preconditions(); + + self.check_worst_removal_pos_preconditions(); } fn check_postconditions(&self) { @@ -231,42 +239,34 @@ impl ListScenario { origin_weight_as_vote, .. } = self; - // dest stash1 is not in the origin bag + // dest stash1 is not in the origin pos. assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); - // and is in the destination bag. + // and is in the destination pos. assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); - // origin stash1 is now in the destination bag. + // origin stash1 is now in the destination pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_weight_as_vote, true)); - // dest stash1 is the head of the destination bag. - // assert!(T::SortedListProvider::is_bag_head(&dest_stash1, *dest_weight_as_vote, true)); - // self.check_origin_head_postconditions(); + // dest stash1 is in a worst case removal position. + assert!(T::SortedListProvider::is_worst_pos(&dest_stash1, true)); + self.check_origin_worst_removal_pos_postconditions(); + } + + fn check_worst_removal_pos_preconditions(&self) { + assert!(T::SortedListProvider::is_worst_pos(&self.dest_stash1, true)); + self.check_origin_worst_removal_pos_preconditions(); } - // fn check_head_preconditions(&self) { - // assert!(T::SortedListProvider::is_bag_head( - // &self.dest_stash1, - // self.dest_weight_as_vote, - // true - // )); - // self.check_origin_head_preconditions(); - // } - - // // Just checking the origin head is useful for scenarios where we start out with all the - // nodes // in the same bag, and thus no destination node is actually ever a head. - // fn check_origin_head_preconditions(&self) { - // assert!(T::SortedListProvider::is_bag_head( - // &self.origin_stash1, - // self.origin_weight_as_vote, - // true - // )); - // } - - // // Just checking the origin head is useful for scenarios where we start out with all the - // nodes // in the same bag, and thus no destination node is actually ever a head. - // fn check_origin_head_postconditions(&self) { - // let ListScenario { origin_stash2, origin_weight_as_vote, .. } = self; - // assert!(T::SortedListProvider::is_bag_head(&origin_stash2, *origin_weight_as_vote, true)); - // } + // Just checking the origin head is useful for scenarios where we start out with all the + // nodes in the same pos, and thus no destination node is actually ever a head. + fn check_origin_worst_removal_pos_preconditions(&self) { + assert!(T::SortedListProvider::is_worst_pos(&self.origin_stash1, true)); + } + + // Just checking the origin head is useful for scenarios where we start out with all the + // nodes in the same bag, and thus no destination node is actually ever a head. + fn check_origin_worst_removal_pos_postconditions(&self) { + let ListScenario { origin_stash2, .. } = self; + assert!(T::SortedListProvider::is_worst_pos(&origin_stash2, true)); + } } const USER_SEED: u32 = 999666; @@ -379,16 +379,14 @@ benchmarks! { Ledger::::insert(&controller, ledger); CurrentEra::::put(EraIndex::max_value()); - // TODO - // scenario.check_origin_head_preconditions(); + scenario.check_preconditions(); whitelist_account!(controller); }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { assert!(!Ledger::::contains_key(controller)); assert!(!T::SortedListProvider::contains(&stash)); - // TODO - // scenario.check_origin_head_postconditions(); + scenario.check_origin_worst_removal_pos_postconditions(); } validate { @@ -402,7 +400,7 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_preconditions(); todo + scenario.check_preconditions(); let prefs = ValidatorPrefs::default(); whitelist_account!(controller); @@ -410,7 +408,7 @@ benchmarks! { verify { assert!(Validators::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_postconditions(); todo + scenario.check_origin_worst_removal_pos_postconditions(); } kick { @@ -517,13 +515,13 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_preconditions(); + scenario.check_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { assert!(!T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_postconditions(); + scenario.check_origin_worst_removal_pos_postconditions(); } set_payee { @@ -588,13 +586,13 @@ benchmarks! { assert!(T::SortedListProvider::contains(&stash)); add_slashing_spans::(&stash, s); - // scenario.check_origin_head_preconditions(); + scenario.check_preconditions(); }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); assert!(!T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_postconditions(); + scenario.check_origin_worst_removal_pos_postconditions(); } cancel_deferred_slash { @@ -764,14 +762,14 @@ benchmarks! { assert!(Bonded::::contains_key(&stash)); assert!(T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_preconditions(); + scenario.check_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) verify { assert!(!Bonded::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_postconditions(); + scenario.check_origin_worst_removal_pos_postconditions(); } new_era { @@ -936,13 +934,13 @@ benchmarks! { Some(Percent::from_percent(0)) )?; - // scenario.check_origin_head_preconditions(); + scenario.check_preconditions(); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { assert!(!T::SortedListProvider::contains(&stash)); - // scenario.check_origin_head_postconditions(); + scenario.check_origin_worst_removal_pos_postconditions(); } } From d08d85866146ea3b6fe2173fb49c3000d4c9998b Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 11:32:54 -0700 Subject: [PATCH 27/37] Finish MvP refactor --- frame/bags-list/src/lib.rs | 8 ------ frame/election-provider-support/src/lib.rs | 6 +---- frame/staking/src/benchmarking.rs | 29 ---------------------- 3 files changed, 1 insertion(+), 42 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index a0387534f7439..518146747e0a2 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -271,13 +271,6 @@ impl SortedListProvider for Pallet { list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) } - #[cfg(feature = "runtime-benchmarks")] - fn is_worst_pos(id: &T::AccountId, _: bool) -> bool { - list::Node::::get(id).unwrap().is_terminal() - } - - /// If `who`'s bond becomes the returned value they are guaranteed to change bags in terms of - /// the worst case #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> VoteWeight { use frame_support::traits::Get as _; @@ -290,7 +283,6 @@ impl SortedListProvider for Pallet { .unwrap(); if is_increase { - // +3 helps in some edge cases to ensure threshold are far enough apart TODO let next_threshold_idx = current_bag_idx + 1; assert!(thresholds.len() > next_threshold_idx); thresholds[next_threshold_idx] diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index d353b0876212a..43bde378f8f94 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -344,11 +344,7 @@ pub trait SortedListProvider { /// in their list position. #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(_who: &AccountId, is_increase: bool) -> VoteWeight { - if is_increase { - VoteWeight::MAX - } else { - 1 - } + VoteWeight::MAX } } diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 07f1d034c5d54..b03db207854a5 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -227,8 +227,6 @@ impl ListScenario { assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *origin_weight_as_vote, true)); // origin stash2 is in the origin pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); - - self.check_worst_removal_pos_preconditions(); } fn check_postconditions(&self) { @@ -245,27 +243,6 @@ impl ListScenario { assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); // origin stash1 is now in the destination pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_weight_as_vote, true)); - // dest stash1 is in a worst case removal position. - assert!(T::SortedListProvider::is_worst_pos(&dest_stash1, true)); - self.check_origin_worst_removal_pos_postconditions(); - } - - fn check_worst_removal_pos_preconditions(&self) { - assert!(T::SortedListProvider::is_worst_pos(&self.dest_stash1, true)); - self.check_origin_worst_removal_pos_preconditions(); - } - - // Just checking the origin head is useful for scenarios where we start out with all the - // nodes in the same pos, and thus no destination node is actually ever a head. - fn check_origin_worst_removal_pos_preconditions(&self) { - assert!(T::SortedListProvider::is_worst_pos(&self.origin_stash1, true)); - } - - // Just checking the origin head is useful for scenarios where we start out with all the - // nodes in the same bag, and thus no destination node is actually ever a head. - fn check_origin_worst_removal_pos_postconditions(&self) { - let ListScenario { origin_stash2, .. } = self; - assert!(T::SortedListProvider::is_worst_pos(&origin_stash2, true)); } } @@ -386,7 +363,6 @@ benchmarks! { verify { assert!(!Ledger::::contains_key(controller)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } validate { @@ -408,7 +384,6 @@ benchmarks! { verify { assert!(Validators::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } kick { @@ -521,7 +496,6 @@ benchmarks! { }: _(RawOrigin::Signed(controller)) verify { assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } set_payee { @@ -592,7 +566,6 @@ benchmarks! { verify { assert!(!Ledger::::contains_key(&controller)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } cancel_deferred_slash { @@ -769,7 +742,6 @@ benchmarks! { verify { assert!(!Bonded::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } new_era { @@ -940,7 +912,6 @@ benchmarks! { }: _(RawOrigin::Signed(caller), controller.clone()) verify { assert!(!T::SortedListProvider::contains(&stash)); - scenario.check_origin_worst_removal_pos_postconditions(); } } From 8cacd0ca377752dcf1bf6a87742d2b3c05cc88e0 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 12:28:40 -0700 Subject: [PATCH 28/37] Some cleanup --- frame/bags-list/src/list/mod.rs | 2 +- frame/election-provider-support/src/lib.rs | 2 +- frame/staking/Cargo.toml | 2 -- frame/staking/src/benchmarking.rs | 7 +++++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 628758a360129..11c0f413c593e 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -647,7 +647,7 @@ impl Node { } /// `true` when this voter is a bag head or tail. - pub(crate) fn is_terminal(&self) -> bool { + fn is_terminal(&self) -> bool { self.prev.is_none() || self.next.is_none() } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 43bde378f8f94..8ff4ce46c120b 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -343,7 +343,7 @@ pub trait SortedListProvider { /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. #[cfg(feature = "runtime-benchmarks")] - fn weight_update_worst_case(_who: &AccountId, is_increase: bool) -> VoteWeight { + fn weight_update_worst_case(_who: &AccountId, _is_increase: bool) -> VoteWeight { VoteWeight::MAX } } diff --git a/frame/staking/Cargo.toml b/frame/staking/Cargo.toml index eb507e958a778..18fcd46ecc5f2 100644 --- a/frame/staking/Cargo.toml +++ b/frame/staking/Cargo.toml @@ -39,7 +39,6 @@ log = { version = "0.4.14", default-features = false } # Optional imports for benchmarking frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true } rand_chacha = { version = "0.2", default-features = false, optional = true } -pallet-bags-list = { version = "4.0.0-dev", path = "../bags-list", optional = true } [dev-dependencies] sp-tracing = { version = "4.0.0-dev", path = "../../primitives/tracing" } @@ -75,6 +74,5 @@ runtime-benchmarks = [ "frame-benchmarking", "frame-election-provider-support/runtime-benchmarks", "rand_chacha", - "pallet-bags-list/runtime-benchmarks", ] try-runtime = ["frame-support/try-runtime"] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index b03db207854a5..92337b89b5fb2 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -149,13 +149,16 @@ impl ListScenario { /// /// - the node to be updated (r) is the head of a bag that has at least one other node. The bag /// itself will need to be read and written to update its head. The node pointed to by r.next - /// will need to be read and written as it will need to have its prev pointer updated. + /// will need to be read and written as it will need to have its prev pointer updated. Note + /// that there are two other worst case scenarios for bag removal: 1) the node is a tail and + /// 2) the node is a middle node with prev and next; all scenarios end up with the same number + /// of storage reads and writes. /// /// - the destination bag has at least one node, which will need its next pointer updated. /// /// NOTE: while this scenario specifically targets a worst case for the bags-list, it should /// also elicit a worst case for other known `SortedListProvider` implementations; although - /// this is subject to change. + /// this may not be true against unknown `SortedListProvider` implementations. fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); From 5aa980c1b889269deaa7fff852235c95460ebfac Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 24 Aug 2021 13:04:06 -0700 Subject: [PATCH 29/37] Soem more cleanup --- frame/staking/src/benchmarking.rs | 51 ++++++++++++++++++------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 92337b89b5fb2..eeb46cc93f7de 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -162,11 +162,11 @@ impl ListScenario { fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); - // create_stash_controller takes a factor, so we compute it. + // create_stash_controller takes a factor, so we compute it let origin_factor: BalanceOf = origin_weight * 10u32.into() / T::Currency::minimum_balance(); - // create validators to nominate. + // create validators to nominate let validators = create_validators::( T::MAX_NOMINATIONS, 100, @@ -194,7 +194,7 @@ impl ListScenario { validators.clone(), )?; - // find a destination weight that will trigger the worst case scenario. + // find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); let total_issuance = T::Currency::total_issuance(); @@ -222,10 +222,18 @@ impl ListScenario { } fn check_preconditions(&self) { - let ListScenario { dest_stash1, origin_stash2, origin_stash1, origin_weight_as_vote, .. } - = self; - // destination stash1 is not in the origin pos. + let ListScenario { + dest_stash1, + origin_stash2, + origin_stash1, + origin_weight_as_vote, + dest_weight_as_vote, + .. + } = self; + // destination stash1 is not in the origin pos, assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); + // and is in the destination pos. + assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); // origin stash1 is in the origin pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *origin_weight_as_vote, true)); // origin stash2 is in the origin pos. @@ -238,14 +246,17 @@ impl ListScenario { origin_stash1, dest_weight_as_vote, origin_weight_as_vote, + origin_stash2, .. } = self; - // dest stash1 is not in the origin pos. + // dest stash1 is not in the origin pos, assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); - // and is in the destination pos. + // and is still in the destination pos. assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); // origin stash1 is now in the destination pos. assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_weight_as_vote, true)); + // origin stash2 is still in the origin pos. + assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); } } @@ -285,7 +296,7 @@ benchmarks! { scenario.check_preconditions(); whitelist_account!(stash); - }: _(RawOrigin::Signed(stash.clone()), max_additional) + }: _(RawOrigin::Signed(stash), max_additional) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; @@ -347,7 +358,7 @@ benchmarks! { clear_validators_and_nominators::(); // setup a worst case list scenario. Note that we don't care about the setup of the - // destination bag. + // destination position because we are doing a removal from the list but no insert. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -482,13 +493,12 @@ benchmarks! { } chill { - // Clean up any existing state. + // clean up any existing state. clear_validators_and_nominators::(); // setup a worst case list scenario. Note that we don't care about the setup of the - // destination bag. - let scenario - = ListScenario::::new(One::one(), true)?; + // destination position because we are doing a removal from the list but no insert. + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -555,9 +565,8 @@ benchmarks! { clear_validators_and_nominators::(); // setup a worst case list scenario. Note that we don't care about the setup of the - // destination bag. - let scenario - = ListScenario::::new(One::one(), true)?; + // destination position because we are doing a removal from the list but no insert. + let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -663,7 +672,7 @@ benchmarks! { // setup a worst case list scenario. - // the weight the nominator will start at. Note we use 100 to play friendly with the bags + // the weight the nominator will start at. Note we use 100 to play friendly with the bags, // list threshold values in the mock. let origin_weight = 100u32.into(); let scenario = ListScenario::::new(origin_weight, true)?; @@ -674,7 +683,7 @@ benchmarks! { // spread that amount to rebond across `l` unlocking chunks, let value = rebond_amount / l.into(); - // so the sum of unlocking chunks puts voter into the dest bag + // so the sum of unlocking chunks puts voter into the dest bag. assert!(value * l.into() + origin_weight > origin_weight); assert!(value * l.into() + origin_weight <= dest_weight); let unlock_chunk = UnlockChunk::> { @@ -728,7 +737,7 @@ benchmarks! { clear_validators_and_nominators::(); // setup a worst case list scenario. Note that we don't care about the setup of the - // destination bag. + // destination position because we are doing a removal from the list but no insert. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -894,7 +903,7 @@ benchmarks! { clear_validators_and_nominators::(); // setup a worst case list scenario. Note that we don't care about the setup of the - // destination bag. + // destination position because we are doing a removal from the list but no insert. let scenario = ListScenario::::new(One::one(), true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); From efbf22d2747596ac04755c33b85e825e833eddff Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Thu, 26 Aug 2021 14:37:51 +0200 Subject: [PATCH 30/37] save --- client/db/src/bench.rs | 2 +- frame/bags-list/src/lib.rs | 4 +- frame/staking/src/benchmarking.rs | 75 ++++++++++++++++++++++-------- frame/staking/src/mock.rs | 3 +- frame/staking/src/pallet/impls.rs | 4 +- frame/staking/src/pallet/mod.rs | 1 + frame/staking/src/testing_utils.rs | 19 +++++--- frame/support/src/traits/voting.rs | 3 ++ 8 files changed, 81 insertions(+), 30 deletions(-) diff --git a/client/db/src/bench.rs b/client/db/src/bench.rs index a4b8f6696ea6a..bc429e2df4c97 100644 --- a/client/db/src/bench.rs +++ b/client/db/src/bench.rs @@ -544,7 +544,7 @@ impl StateBackend> for BenchmarkingState { if tracker.writes > 0 { writes += 1; - repeat_writes += tracker.reads - 1; + repeat_writes += tracker.reads.saturating_sub(1); } } }); diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 518146747e0a2..d8afbd864d399 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -278,15 +278,17 @@ impl SortedListProvider for Pallet { let node = list::Node::::get(who).unwrap(); let current_bag_idx = thresholds .iter() - .chain(std::iter::once(&VoteWeight::MAX)) + .chain(sp_std::iter::once(&VoteWeight::MAX)) .position(|w| w == &node.bag_upper) .unwrap(); if is_increase { let next_threshold_idx = current_bag_idx + 1; + log!(info, "next_threshold_idx {:#?}", next_threshold_idx); assert!(thresholds.len() > next_threshold_idx); thresholds[next_threshold_idx] } else { + log!(info, "node bag upper {:#?}", node.bag_upper); assert!(current_bag_idx != 0); let prev_threshold_idx = current_bag_idx - 1; thresholds[prev_threshold_idx] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index eeb46cc93f7de..07709c647a087 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -162,9 +162,7 @@ impl ListScenario { fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); - // create_stash_controller takes a factor, so we compute it - let origin_factor: BalanceOf = - origin_weight * 10u32.into() / T::Currency::minimum_balance(); + log!(info, "scenario origin_weight {:#?}", origin_weight); // create validators to nominate let validators = create_validators::( @@ -174,6 +172,11 @@ impl ListScenario { )?; // create accounts with the origin weight + + // create_stash_controller takes a factor, so we compute it + let origin_factor: BalanceOf = + origin_weight * 10u32.into() / T::Currency::minimum_balance(); + let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( USER_SEED + 2, origin_factor, @@ -184,6 +187,12 @@ impl ListScenario { validators.clone(), )?; + log!( + info, + "A origin1 ledger active {:#?}", + Ledger::::get(&origin_controller1).ok_or("ledger not created before")?.active + ); + let (origin_stash2, origin_controller2) = create_stash_controller_with_max_free::( USER_SEED + 3, origin_factor, @@ -194,6 +203,12 @@ impl ListScenario { validators.clone(), )?; + log!( + info, + "B origin1 ledger active {:#?}", + Ledger::::get(&origin_controller1).ok_or("ledger not created before")?.active + ); + // find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); @@ -280,19 +295,25 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup the worst case list scenario. - let total_issuance = T::Currency::total_issuance(); + // the weight the nominator will start at. - let origin_weight = One::one(); let scenario = ListScenario::::new(origin_weight, true)?; let max_additional = scenario.dest_weight.clone() - origin_weight; + log!(info, "dest_weight {:#?}, origin_weight {:#?}", scenario.dest_weight.clone(), origin_weight); + log!(info, "max additional in benchmark to try {:#?}", max_additional); let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); - let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; + let mut ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let original_bonded: BalanceOf = ledger.active; + // ledger.total = BalanceOf::::max_value(); + // Ledger::::insert(&controller, &ledger); + scenario.check_preconditions(); whitelist_account!(stash); @@ -311,7 +332,7 @@ benchmarks! { // setup the worst case list scenario. let total_issuance = T::Currency::total_issuance(); // the weight the nominator will start at. - let origin_weight = T::CurrencyToVote::to_currency(VoteWeight::MAX as u128, total_issuance); + let origin_weight = T::CurrencyToVote::to_currency(106_282_535_907_434u128, total_issuance); let scenario = ListScenario::::new(origin_weight, false)?; let stash = scenario.origin_stash1.clone(); @@ -357,9 +378,11 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. Note that we don't care about the setup of the // destination position because we are doing a removal from the list but no insert. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -383,9 +406,11 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case scenario where the user calling validate was formerly a nominator so // they must be removed from the list. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -470,11 +495,12 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. - let threshold = One::one(); - let scenario = ListScenario::::new(threshold, true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let origin_weight_factor: BalanceOf = - threshold * 10u32.into() / T::Currency::minimum_balance(); + origin_weight * 10u32.into() / T::Currency::minimum_balance(); let (stash, controller) = create_stash_controller_with_max_free::( SEED + T::MAX_NOMINATIONS + 1, // make sure the account does not conflict with others origin_weight_factor, // bond an amount that puts them in the bag with nodes. @@ -496,9 +522,11 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. Note that we don't care about the setup of the // destination position because we are doing a removal from the list but no insert. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -564,9 +592,11 @@ benchmarks! { // Clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. Note that we don't care about the setup of the // destination position because we are doing a removal from the list but no insert. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); @@ -670,11 +700,12 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); - // setup a worst case list scenario. + let origin_weight = MinNominatorBond::::get() + .max(T::Currency::minimum_balance()) + // we use 100 to play friendly with the bags, list threshold values in the mock. + .max(100u32.into()); - // the weight the nominator will start at. Note we use 100 to play friendly with the bags, - // list threshold values in the mock. - let origin_weight = 100u32.into(); + // setup a worst case list scenario. let scenario = ListScenario::::new(origin_weight, true)?; let dest_weight = scenario.dest_weight.clone(); @@ -736,9 +767,11 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. Note that we don't care about the setup of the // destination position because we are doing a removal from the list but no insert. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); @@ -902,9 +935,11 @@ benchmarks! { // clean up any existing state. clear_validators_and_nominators::(); + let origin_weight = MinNominatorBond::::get().max(T::Currency::minimum_balance()); + // setup a worst case list scenario. Note that we don't care about the setup of the // destination position because we are doing a removal from the list but no insert. - let scenario = ListScenario::::new(One::one(), true)?; + let scenario = ListScenario::::new(origin_weight, true)?; let controller = scenario.origin_controller1.clone(); let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index fcd6d21732785..3a96ebe4645b5 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -267,7 +267,8 @@ impl crate::pallet::pallet::Config for Test { const MAX_NOMINATIONS: u32 = 16; type Currency = Balances; type UnixTime = Timestamp; - type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; + // type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; + type CurrencyToVote = frame_support::traits::U128CurrencyToVote; type RewardRemainder = RewardRemainderMock; type Event = Event; type Slash = (); diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index b6df94adb3b73..5d04304dc1427 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -746,7 +746,9 @@ impl Pallet { CounterForNominators::::mutate(|x| x.saturating_inc()); // maybe update sorted list. Defensive-only: this should never fail. - if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() { + let weight = Self::weight_of(who); + log!(info, "weight of nominator {}", weight); + if T::SortedListProvider::on_insert(who.clone(), weight).is_err() { log!(warn, "attempt to insert duplicate nominator ({:#?})", who); debug_assert!(false, "attempt to insert duplicate nominator"); }; diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 695a57c06a58f..8daaffe200fa0 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -760,6 +760,7 @@ pub mod pallet { let stash_balance = T::Currency::free_balance(&stash); if let Some(extra) = stash_balance.checked_sub(&ledger.total) { let extra = extra.min(max_additional); + log!(info, "extra to bond_extra {:#?}", extra); ledger.total += extra; ledger.active += extra; // Last check: the new active amount of ledger must be more than ED. diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 75660208b8d88..2ff24434f5e9f 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -60,13 +60,19 @@ pub fn create_funded_user( } /// Grab a funded user with max Balance. -pub fn create_funded_user_with_max(string: &'static str, n: u32) -> T::AccountId { +pub fn create_funded_user_with_max( + string: &'static str, + n: u32, + balance_factor: BalanceOf, +) -> T::AccountId { use sp_runtime::traits::Bounded; let user = account(string, n, SEED); - let balance = BalanceOf::::max_value(); - T::Currency::make_free_balance_be(&user, balance); + let base = T::Currency::minimum_balance() * balance_factor; + let balance = base.saturating_mul(20u32.into()); + let imbalance = T::Currency::make_free_balance_be(&user, balance); // ensure T::CurrencyToVote will work correctly. - T::Currency::issue(balance); // TODO I don't get this .. will drop NegativeImbalance which cancels itself out + // T::Currency::issue(balance); + // cancels itself out user } @@ -96,11 +102,12 @@ pub fn create_stash_controller_with_max_free( balance_factor: crate::BalanceOf, destination: RewardDestination, ) -> Result<(T::AccountId, T::AccountId), &'static str> { - let stash = create_funded_user_with_max::("stash", n); - let controller = create_funded_user_with_max::("controller", n); + let stash = create_funded_user_with_max::("stash", n, balance_factor); + let controller = create_funded_user_with_max::("controller", n, balance_factor); let controller_lookup: ::Source = T::Lookup::unlookup(controller.clone()); let amount = T::Currency::minimum_balance() * (balance_factor / 10u32.into()).max(1u32.into()); + Staking::::bond( RawOrigin::Signed(stash.clone()).into(), controller_lookup, diff --git a/frame/support/src/traits/voting.rs b/frame/support/src/traits/voting.rs index 62c6217ad59bc..d59bc46620c43 100644 --- a/frame/support/src/traits/voting.rs +++ b/frame/support/src/traits/voting.rs @@ -61,6 +61,9 @@ impl U128CurrencyToVote { impl CurrencyToVote for U128CurrencyToVote { fn to_vote(value: u128, issuance: u128) -> u64 { + sp_std::if_std! { + println!("issuance factor {:#?}. issuance {:#?}", Self::factor(issuance), issuance); + } (value / Self::factor(issuance)).saturated_into() } From 532ffffb2c09e274eca8153aa1a302898c2f8164 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 26 Aug 2021 19:38:22 +0200 Subject: [PATCH 31/37] fix a lot of stuff --- frame/staking/src/benchmarking.rs | 59 +++++++++++++----------------- frame/staking/src/mock.rs | 3 +- frame/staking/src/pallet/impls.rs | 1 - frame/staking/src/pallet/mod.rs | 4 +- frame/staking/src/testing_utils.rs | 28 ++++++-------- frame/support/src/traits/voting.rs | 3 -- 6 files changed, 41 insertions(+), 57 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 07709c647a087..a9ec7e9670077 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -162,29 +162,23 @@ impl ListScenario { fn new(origin_weight: BalanceOf, is_increase: bool) -> Result { ensure!(!origin_weight.is_zero(), "origin weight must be greater than 0"); - log!(info, "scenario origin_weight {:#?}", origin_weight); + // burn the entire issuance. + let i = T::Currency::burn(T::Currency::total_issuance()); + sp_std::mem::forget(i); - // create validators to nominate - let validators = create_validators::( - T::MAX_NOMINATIONS, - 100, - T::MAX_NOMINATIONS * 2, // account seed prevents unintentional account collisions - )?; + log!(info, "scenario origin_weight {:#?}", origin_weight); // create accounts with the origin weight - // create_stash_controller takes a factor, so we compute it - let origin_factor: BalanceOf = - origin_weight * 10u32.into() / T::Currency::minimum_balance(); - - let (origin_stash1, origin_controller1) = create_stash_controller_with_max_free::( + let (origin_stash1, origin_controller1) = create_stash_controller_with_balance::( USER_SEED + 2, - origin_factor, + origin_weight, Default::default(), )?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), - validators.clone(), + // NOTE: these don't really need to be validators. + vec![T::Lookup::unlookup(account("random_validator", 0, SEED))], )?; log!( @@ -193,14 +187,14 @@ impl ListScenario { Ledger::::get(&origin_controller1).ok_or("ledger not created before")?.active ); - let (origin_stash2, origin_controller2) = create_stash_controller_with_max_free::( + let (origin_stash2, origin_controller2) = create_stash_controller_with_balance::( USER_SEED + 3, - origin_factor, + origin_weight, Default::default(), )?; Staking::::nominate( RawOrigin::Signed(origin_controller2.clone()).into(), - validators.clone(), + vec![T::Lookup::unlookup(account("random_validator", 0, SEED))].clone(), )?; log!( @@ -212,18 +206,24 @@ impl ListScenario { // find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); + let total_issuance = T::Currency::total_issuance(); + let dest_weight = T::CurrencyToVote::to_currency(dest_weight_as_vote as u128, total_issuance); + let dest_factor: BalanceOf = dest_weight * 10u32.into() / T::Currency::minimum_balance(); // create an account with the worst case destination weight - let (dest_stash1, dest_controller1) = create_stash_controller_with_max_free::( + let (dest_stash1, dest_controller1) = create_stash_controller_with_balance::( USER_SEED + 1, - dest_factor, + dest_weight, Default::default(), )?; - Staking::::nominate(RawOrigin::Signed(dest_controller1).into(), validators)?; + Staking::::nominate( + RawOrigin::Signed(dest_controller1).into(), + vec![T::Lookup::unlookup(account("random_validator", 0, SEED))], + )?; Ok(ListScenario { dest_stash1, @@ -303,17 +303,12 @@ benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let max_additional = scenario.dest_weight.clone() - origin_weight; - log!(info, "dest_weight {:#?}, origin_weight {:#?}", scenario.dest_weight.clone(), origin_weight); - log!(info, "max additional in benchmark to try {:#?}", max_additional); let stash = scenario.origin_stash1.clone(); let controller = scenario.origin_controller1.clone(); - let mut ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; - let original_bonded: BalanceOf = ledger.active; - - // ledger.total = BalanceOf::::max_value(); - // Ledger::::insert(&controller, &ledger); + let original_bonded: BalanceOf = Ledger::::get(&controller).map(|l| l.active).ok_or("ledger not created after")?; + T::Currency::deposit_into_existing(&stash, max_additional); scenario.check_preconditions(); whitelist_account!(stash); @@ -499,18 +494,16 @@ benchmarks! { // setup a worst case list scenario. let scenario = ListScenario::::new(origin_weight, true)?; - let origin_weight_factor: BalanceOf = - origin_weight * 10u32.into() / T::Currency::minimum_balance(); - let (stash, controller) = create_stash_controller_with_max_free::( + let (stash, controller) = create_stash_controller_with_balance::( SEED + T::MAX_NOMINATIONS + 1, // make sure the account does not conflict with others - origin_weight_factor, // bond an amount that puts them in the bag with nodes. + origin_weight, Default::default(), - )?; + ).unwrap(); assert!(!Nominators::::contains_key(&stash)); assert!(!T::SortedListProvider::contains(&stash)); - let validators = create_validators::(n, 100, 0)?; + let validators = create_validators::(n, 100, 0).unwrap(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), validators) verify { diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3a96ebe4645b5..fcd6d21732785 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -267,8 +267,7 @@ impl crate::pallet::pallet::Config for Test { const MAX_NOMINATIONS: u32 = 16; type Currency = Balances; type UnixTime = Timestamp; - // type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; - type CurrencyToVote = frame_support::traits::U128CurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = RewardRemainderMock; type Event = Event; type Slash = (); diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 5d04304dc1427..80b2206495e34 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -747,7 +747,6 @@ impl Pallet { // maybe update sorted list. Defensive-only: this should never fail. let weight = Self::weight_of(who); - log!(info, "weight of nominator {}", weight); if T::SortedListProvider::on_insert(who.clone(), weight).is_err() { log!(warn, "attempt to insert duplicate nominator ({:#?})", who); debug_assert!(false, "attempt to insert duplicate nominator"); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 8daaffe200fa0..acbc23d2181ea 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -760,7 +760,9 @@ pub mod pallet { let stash_balance = T::Currency::free_balance(&stash); if let Some(extra) = stash_balance.checked_sub(&ledger.total) { let extra = extra.min(max_additional); - log!(info, "extra to bond_extra {:#?}", extra); + sp_std::if_std! { + println!("extra to bond_extra {:#?}", extra); + } ledger.total += extra; ledger.active += extra; // Last check: the new active amount of ledger must be more than ED. diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 2ff24434f5e9f..b4f105fe1d7fe 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -53,26 +53,21 @@ pub fn create_funded_user( ) -> T::AccountId { let user = account(string, n, SEED); let balance = T::Currency::minimum_balance() * balance_factor.into(); - T::Currency::make_free_balance_be(&user, balance); + let _ = T::Currency::make_free_balance_be(&user, balance); // ensure T::CurrencyToVote will work correctly. - T::Currency::issue(balance); + // T::Currency::issue(balance); user } /// Grab a funded user with max Balance. -pub fn create_funded_user_with_max( +pub fn create_funded_user_with_balance( string: &'static str, n: u32, - balance_factor: BalanceOf, + balance: BalanceOf, ) -> T::AccountId { use sp_runtime::traits::Bounded; let user = account(string, n, SEED); - let base = T::Currency::minimum_balance() * balance_factor; - let balance = base.saturating_mul(20u32.into()); - let imbalance = T::Currency::make_free_balance_be(&user, balance); - // ensure T::CurrencyToVote will work correctly. - // T::Currency::issue(balance); - // cancels itself out + let _ = T::Currency::make_free_balance_be(&user, balance); user } @@ -96,22 +91,21 @@ pub fn create_stash_controller( return Ok((stash, controller)) } -/// Create a stash and controller pair with max free balance. -pub fn create_stash_controller_with_max_free( +/// Create a stash and controller pair with fixed balance. +pub fn create_stash_controller_with_balance( n: u32, - balance_factor: crate::BalanceOf, + balance: crate::BalanceOf, destination: RewardDestination, ) -> Result<(T::AccountId, T::AccountId), &'static str> { - let stash = create_funded_user_with_max::("stash", n, balance_factor); - let controller = create_funded_user_with_max::("controller", n, balance_factor); + let stash = create_funded_user_with_balance::("stash", n, balance); + let controller = create_funded_user_with_balance::("controller", n, balance); let controller_lookup: ::Source = T::Lookup::unlookup(controller.clone()); - let amount = T::Currency::minimum_balance() * (balance_factor / 10u32.into()).max(1u32.into()); Staking::::bond( RawOrigin::Signed(stash.clone()).into(), controller_lookup, - amount, + balance, destination, )?; Ok((stash, controller)) diff --git a/frame/support/src/traits/voting.rs b/frame/support/src/traits/voting.rs index d59bc46620c43..62c6217ad59bc 100644 --- a/frame/support/src/traits/voting.rs +++ b/frame/support/src/traits/voting.rs @@ -61,9 +61,6 @@ impl U128CurrencyToVote { impl CurrencyToVote for U128CurrencyToVote { fn to_vote(value: u128, issuance: u128) -> u64 { - sp_std::if_std! { - println!("issuance factor {:#?}. issuance {:#?}", Self::factor(issuance), issuance); - } (value / Self::factor(issuance)).saturated_into() } From 24c2767f56a1f86c9c68235ec66f217c39fa17b6 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Thu, 26 Aug 2021 21:05:53 +0200 Subject: [PATCH 32/37] Update client/db/src/bench.rs Co-authored-by: Shawn Tabrizi --- client/db/src/bench.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/db/src/bench.rs b/client/db/src/bench.rs index bc429e2df4c97..5f5cbae12749c 100644 --- a/client/db/src/bench.rs +++ b/client/db/src/bench.rs @@ -544,7 +544,7 @@ impl StateBackend> for BenchmarkingState { if tracker.writes > 0 { writes += 1; - repeat_writes += tracker.reads.saturating_sub(1); + repeat_writes += tracker.writes - 1; } } }); From 6b9137177482d96d710726646774128a6a6ec715 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Thu, 26 Aug 2021 21:59:39 +0200 Subject: [PATCH 33/37] Apply suggestions from code review --- frame/staking/src/testing_utils.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index b4f105fe1d7fe..d25a3c61e9ede 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -55,7 +55,6 @@ pub fn create_funded_user( let balance = T::Currency::minimum_balance() * balance_factor.into(); let _ = T::Currency::make_free_balance_be(&user, balance); // ensure T::CurrencyToVote will work correctly. - // T::Currency::issue(balance); user } From 0317ea19f12ac839f64c2dbf57d4419235ca33c0 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Thu, 26 Aug 2021 22:05:41 +0200 Subject: [PATCH 34/37] Apply suggestions from code review --- frame/staking/src/benchmarking.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index a9ec7e9670077..4aaab93ff303e 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -181,11 +181,6 @@ impl ListScenario { vec![T::Lookup::unlookup(account("random_validator", 0, SEED))], )?; - log!( - info, - "A origin1 ledger active {:#?}", - Ledger::::get(&origin_controller1).ok_or("ledger not created before")?.active - ); let (origin_stash2, origin_controller2) = create_stash_controller_with_balance::( USER_SEED + 3, @@ -197,12 +192,6 @@ impl ListScenario { vec![T::Lookup::unlookup(account("random_validator", 0, SEED))].clone(), )?; - log!( - info, - "B origin1 ledger active {:#?}", - Ledger::::get(&origin_controller1).ok_or("ledger not created before")?.active - ); - // find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = T::SortedListProvider::weight_update_worst_case(&origin_stash1, is_increase); From 4faf565b102e9f43b1b7f0659dadc7fe609ca2f8 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 27 Aug 2021 08:25:53 +0200 Subject: [PATCH 35/37] Fix some issues that came from trying to merge comments on github --- frame/bags-list/src/lib.rs | 2 -- frame/staking/src/benchmarking.rs | 14 +++++++------- frame/staking/src/pallet/mod.rs | 3 --- frame/staking/src/testing_utils.rs | 1 - 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index d8afbd864d399..316771179d1ac 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -284,11 +284,9 @@ impl SortedListProvider for Pallet { if is_increase { let next_threshold_idx = current_bag_idx + 1; - log!(info, "next_threshold_idx {:#?}", next_threshold_idx); assert!(thresholds.len() > next_threshold_idx); thresholds[next_threshold_idx] } else { - log!(info, "node bag upper {:#?}", node.bag_upper); assert!(current_bag_idx != 0); let prev_threshold_idx = current_bag_idx - 1; thresholds[prev_threshold_idx] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 4aaab93ff303e..dfd745ef899f6 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -166,8 +166,6 @@ impl ListScenario { let i = T::Currency::burn(T::Currency::total_issuance()); sp_std::mem::forget(i); - log!(info, "scenario origin_weight {:#?}", origin_weight); - // create accounts with the origin weight let (origin_stash1, origin_controller1) = create_stash_controller_with_balance::( @@ -201,8 +199,6 @@ impl ListScenario { let dest_weight = T::CurrencyToVote::to_currency(dest_weight_as_vote as u128, total_issuance); - let dest_factor: BalanceOf = dest_weight * 10u32.into() / T::Currency::minimum_balance(); - // create an account with the worst case destination weight let (dest_stash1, dest_controller1) = create_stash_controller_with_balance::( USER_SEED + 1, @@ -297,7 +293,7 @@ benchmarks! { let controller = scenario.origin_controller1.clone(); let original_bonded: BalanceOf = Ledger::::get(&controller).map(|l| l.active).ok_or("ledger not created after")?; - T::Currency::deposit_into_existing(&stash, max_additional); + T::Currency::deposit_into_existing(&stash, max_additional).unwrap(); scenario.check_preconditions(); whitelist_account!(stash); @@ -310,13 +306,17 @@ benchmarks! { } unbond { + use sp_std::convert::TryFrom; // clean up any existing state. clear_validators_and_nominators::(); // setup the worst case list scenario. let total_issuance = T::Currency::total_issuance(); - // the weight the nominator will start at. - let origin_weight = T::CurrencyToVote::to_currency(106_282_535_907_434u128, total_issuance); + // the weight the nominator will start at. The value used here is expected to be + // significantly higher than the first position in a list (e.g. the first bag threshold). + let origin_weight = BalanceOf::::try_from(952_994_955_240_703u128) + .map_err(|_| "balance expected to be a u128") + .unwrap(); let scenario = ListScenario::::new(origin_weight, false)?; let stash = scenario.origin_stash1.clone(); diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index acbc23d2181ea..695a57c06a58f 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -760,9 +760,6 @@ pub mod pallet { let stash_balance = T::Currency::free_balance(&stash); if let Some(extra) = stash_balance.checked_sub(&ledger.total) { let extra = extra.min(max_additional); - sp_std::if_std! { - println!("extra to bond_extra {:#?}", extra); - } ledger.total += extra; ledger.active += extra; // Last check: the new active amount of ledger must be more than ED. diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index d25a3c61e9ede..6c5e5c37597c6 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -64,7 +64,6 @@ pub fn create_funded_user_with_balance( n: u32, balance: BalanceOf, ) -> T::AccountId { - use sp_runtime::traits::Bounded; let user = account(string, n, SEED); let _ = T::Currency::make_free_balance_be(&user, balance); user From 41a50dbef79fa00449fd0106c16529268a7db5fe Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 27 Aug 2021 08:32:48 +0200 Subject: [PATCH 36/37] some small changes --- frame/staking/src/benchmarking.rs | 1 - frame/staking/src/pallet/impls.rs | 3 +-- frame/staking/src/testing_utils.rs | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index dfd745ef899f6..da4387353621a 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -179,7 +179,6 @@ impl ListScenario { vec![T::Lookup::unlookup(account("random_validator", 0, SEED))], )?; - let (origin_stash2, origin_controller2) = create_stash_controller_with_balance::( USER_SEED + 3, origin_weight, diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 80b2206495e34..b6df94adb3b73 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -746,8 +746,7 @@ impl Pallet { CounterForNominators::::mutate(|x| x.saturating_inc()); // maybe update sorted list. Defensive-only: this should never fail. - let weight = Self::weight_of(who); - if T::SortedListProvider::on_insert(who.clone(), weight).is_err() { + if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() { log!(warn, "attempt to insert duplicate nominator ({:#?})", who); debug_assert!(false, "attempt to insert duplicate nominator"); }; diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 6c5e5c37597c6..2e57931c6f3cf 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -54,7 +54,6 @@ pub fn create_funded_user( let user = account(string, n, SEED); let balance = T::Currency::minimum_balance() * balance_factor.into(); let _ = T::Currency::make_free_balance_be(&user, balance); - // ensure T::CurrencyToVote will work correctly. user } From 6277454b59ee5b669ed166caf84735ced7d2cca5 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 27 Aug 2021 11:40:01 +0200 Subject: [PATCH 37/37] simplify it --- frame/bags-list/src/lib.rs | 5 -- frame/election-provider-support/src/lib.rs | 6 --- frame/staking/src/benchmarking.rs | 58 ---------------------- frame/staking/src/pallet/impls.rs | 1 - 4 files changed, 70 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 316771179d1ac..677bd0ee8c956 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -266,11 +266,6 @@ impl SortedListProvider for Pallet { List::::clear() } - #[cfg(feature = "runtime-benchmarks")] - fn is_in_pos(id: &T::AccountId, weight: VoteWeight, _: bool) -> bool { - list::Bag::::get(list::notional_bag_for::(weight)).unwrap().contains(id) - } - #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> VoteWeight { use frame_support::traits::Get as _; diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 8ff4ce46c120b..9eae6fc636a88 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -334,12 +334,6 @@ pub trait SortedListProvider { /// Sanity check internal state of list. Only meant for debug compilation. fn sanity_check() -> Result<(), &'static str>; - /// Whether or not `id` is in a position in the list that corresponds to `weight`. - #[cfg(feature = "runtime-benchmarks")] - fn is_in_pos(_id: &AccountId, _weight: VoteWeight, mock: bool) -> bool { - mock - } - /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. #[cfg(feature = "runtime-benchmarks")] diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index da4387353621a..233c628f2f925 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -219,44 +219,6 @@ impl ListScenario { dest_weight, }) } - - fn check_preconditions(&self) { - let ListScenario { - dest_stash1, - origin_stash2, - origin_stash1, - origin_weight_as_vote, - dest_weight_as_vote, - .. - } = self; - // destination stash1 is not in the origin pos, - assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); - // and is in the destination pos. - assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); - // origin stash1 is in the origin pos. - assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *origin_weight_as_vote, true)); - // origin stash2 is in the origin pos. - assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); - } - - fn check_postconditions(&self) { - let ListScenario { - dest_stash1, - origin_stash1, - dest_weight_as_vote, - origin_weight_as_vote, - origin_stash2, - .. - } = self; - // dest stash1 is not in the origin pos, - assert!(!T::SortedListProvider::is_in_pos(&dest_stash1, *origin_weight_as_vote, false)); - // and is still in the destination pos. - assert!(T::SortedListProvider::is_in_pos(&dest_stash1, *dest_weight_as_vote, true)); - // origin stash1 is now in the destination pos. - assert!(T::SortedListProvider::is_in_pos(&origin_stash1, *dest_weight_as_vote, true)); - // origin stash2 is still in the origin pos. - assert!(T::SortedListProvider::is_in_pos(&origin_stash2, *origin_weight_as_vote, true)); - } } const USER_SEED: u32 = 999666; @@ -293,7 +255,6 @@ benchmarks! { let original_bonded: BalanceOf = Ledger::::get(&controller).map(|l| l.active).ok_or("ledger not created after")?; T::Currency::deposit_into_existing(&stash, max_additional).unwrap(); - scenario.check_preconditions(); whitelist_account!(stash); }: _(RawOrigin::Signed(stash), max_additional) @@ -301,7 +262,6 @@ benchmarks! { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); - scenario.check_postconditions(); } unbond { @@ -324,15 +284,12 @@ benchmarks! { let ledger = Ledger::::get(&controller).ok_or("ledger not created before")?; let original_bonded: BalanceOf = ledger.active; - scenario.check_preconditions(); - whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), amount) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded > new_bonded); - scenario.check_postconditions(); } // Withdraw only updates the ledger @@ -376,8 +333,6 @@ benchmarks! { Ledger::::insert(&controller, ledger); CurrentEra::::put(EraIndex::max_value()); - scenario.check_preconditions(); - whitelist_account!(controller); }: withdraw_unbonded(RawOrigin::Signed(controller.clone()), s) verify { @@ -398,8 +353,6 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_preconditions(); - let prefs = ValidatorPrefs::default(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), prefs) @@ -512,8 +465,6 @@ benchmarks! { let stash = scenario.origin_stash1.clone(); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_preconditions(); - whitelist_account!(controller); }: _(RawOrigin::Signed(controller)) verify { @@ -583,8 +534,6 @@ benchmarks! { assert!(T::SortedListProvider::contains(&stash)); add_slashing_spans::(&stash, s); - scenario.check_preconditions(); - }: _(RawOrigin::Root, stash.clone(), s) verify { assert!(!Ledger::::contains_key(&controller)); @@ -713,16 +662,12 @@ benchmarks! { Ledger::::insert(controller.clone(), staking_ledger.clone()); let original_bonded: BalanceOf = staking_ledger.active; - scenario.check_preconditions(); - whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), rebond_amount) verify { let ledger = Ledger::::get(&controller).ok_or("ledger not created after")?; let new_bonded: BalanceOf = ledger.active; assert!(original_bonded < new_bonded); - - scenario.check_postconditions(); } set_history_depth { @@ -761,7 +706,6 @@ benchmarks! { assert!(Bonded::::contains_key(&stash)); assert!(T::SortedListProvider::contains(&stash)); - scenario.check_preconditions(); whitelist_account!(controller); }: _(RawOrigin::Signed(controller), stash.clone(), s) @@ -934,8 +878,6 @@ benchmarks! { Some(Percent::from_percent(0)) )?; - scenario.check_preconditions(); - let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), controller.clone()) verify { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index b6df94adb3b73..bd971e15ae45b 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1273,7 +1273,6 @@ impl SortedListProvider for UseNominatorsMap { fn sanity_check() -> Result<(), &'static str> { Ok(()) } - fn clear() { Nominators::::remove_all(None); CounterForNominators::::kill();