From 7b4254b0f3c8a54b8a36a5925af18c5ed60a0010 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 21 Aug 2024 15:33:48 -0400 Subject: [PATCH] Suggestions for `new_session` benches (#1019) * Format benchmarking code * Update `*_less_then_signers` bench to scale signers in storage Since we end up reading from this storage vec we want to make sure it is populated with up to `MAX_SIGNERS` for the purposes of benchmarking. * Change benchmarking for session handler to only use two benches We add an extra storage read each time, but we can simplify the benchmarking a bit * Always update session weight If the signer length was more than the total signers we wouldn't update the session weight, even though we would still do a few storage operations. * Remove `expect` by using `match` statement * Remove unused `new_session_not_adding_new_signer` bench * Update the name for one of the benches * Update benchmark results * Add missing parameter to base bench --- pallets/staking/src/benchmarking.rs | 70 ++++++++-------- pallets/staking/src/lib.rs | 48 ++++++----- pallets/staking/src/weights.rs | 73 ++++------------- .../src/weights/pallet_staking_extension.rs | 80 ++++++++----------- 4 files changed, 117 insertions(+), 154 deletions(-) diff --git a/pallets/staking/src/benchmarking.rs b/pallets/staking/src/benchmarking.rs index 5d4053930..5fce1157b 100644 --- a/pallets/staking/src/benchmarking.rs +++ b/pallets/staking/src/benchmarking.rs @@ -228,35 +228,32 @@ benchmarks! { assert_last_event::(Event::::SignersRotation(signers.clone()).into()); } - new_session_validators_less_then_signers { - let caller: T::AccountId = whitelisted_caller(); - let validator_id_res = ::ValidatorId::try_from(caller.clone()).or(Err(Error::::InvalidValidatorId)).unwrap(); - Signers::::put(vec![validator_id_res.clone(), validator_id_res.clone()]); - - }: { - let _ = Staking::::new_session_handler(&vec![validator_id_res]); - } - verify { - assert!(NextSigners::::get().is_none()); - } + new_session_base_weight { + let s in 2 .. MAX_SIGNERS as u32; - new_session_not_adding_new_signer { let caller: T::AccountId = whitelisted_caller(); - let validator_id_res = ::ValidatorId::try_from(caller.clone()).or(Err(Error::::InvalidValidatorId)).unwrap(); - let second_signer: T::AccountId = account("second_signer", 0, SEED); - let second_signer_id = ::ValidatorId::try_from(second_signer.clone()).or(Err(Error::::InvalidValidatorId)).unwrap(); - // full signer list leaving room for one extra validator - let mut signers = vec![second_signer_id.clone(); 5]; - Signers::::put(signers.clone()); - signers.push(validator_id_res.clone()); + // For the purpose of the bench these values don't actually matter, we just care that there's a + // storage entry available + SignersInfo::::put(SignersSize { + total_signers: MAX_SIGNERS, + threshold: 3, + last_session_change: 0, + }); + let validator_id = ::ValidatorId::try_from(caller.clone()) + .or(Err(Error::::InvalidValidatorId)) + .unwrap(); + let signers = vec![validator_id.clone(); s as usize]; + Signers::::put(signers); }: { - let _ = Staking::::new_session_handler(&signers); + // Note that here we only add one validator, where as `Signers` already contains two as a + // minimum. + let _ = Staking::::new_session_handler(&vec![validator_id]); } verify { - assert_eq!(NextSigners::::get().unwrap().next_signers.len(), signers.len() - 2); + assert!(NextSigners::::get().is_none()); } new_session { @@ -264,25 +261,34 @@ benchmarks! { let l in 0 .. MAX_SIGNERS as u32; let caller: T::AccountId = whitelisted_caller(); - let validator_id_res = ::ValidatorId::try_from(caller.clone()).or(Err(Error::::InvalidValidatorId)).unwrap(); + + // For the purpose of the bench these values don't actually matter, we just care that there's a + // storage entry available + SignersInfo::::put(SignersSize { + total_signers: MAX_SIGNERS, + threshold: 3, + last_session_change: 0, + }); + + let validator_id = ::ValidatorId::try_from(caller.clone()) + .or(Err(Error::::InvalidValidatorId)) + .unwrap(); let second_signer: T::AccountId = account("second_signer", 0, SEED); - let second_signer_id = ::ValidatorId::try_from(second_signer.clone()).or(Err(Error::::InvalidValidatorId)).unwrap(); + let second_signer_id = + ::ValidatorId::try_from(second_signer.clone()) + .or(Err(Error::::InvalidValidatorId)) + .unwrap(); + // full signer list leaving room for one extra validator let mut signers = vec![second_signer_id.clone(); c as usize]; Signers::::put(signers.clone()); signers.push(second_signer_id.clone()); - // place new signer in the signers struct in different locations to calculate random selection re-run - signers[l as usize % c as usize] = validator_id_res.clone(); - - SignersInfo::::put(SignersSize { - total_signers: MAX_SIGNERS, - threshold: 3, - last_session_change: 0 - }); - + // place new signer in the signers struct in different locations to calculate random selection + // re-run + signers[l as usize % c as usize] = validator_id.clone(); }: { let _ = Staking::::new_session_handler(&signers); } diff --git a/pallets/staking/src/lib.rs b/pallets/staking/src/lib.rs index 7e3a5619f..76af74d3b 100644 --- a/pallets/staking/src/lib.rs +++ b/pallets/staking/src/lib.rs @@ -541,31 +541,35 @@ pub mod pallet { ) -> Result { let mut current_signers = Self::signers(); let current_signers_length = current_signers.len(); + let signers_info = pallet_parameters::Pallet::::signers_info(); + + let mut weight: Weight = + ::WeightInfo::new_session_base_weight(current_signers_length as u32); + // Since not enough validators do not allow rotation // TODO: https://github.com/entropyxyz/entropy-core/issues/943 if validators.len() <= current_signers_length { - return Ok(::WeightInfo::new_session_validators_less_then_signers()); + return Ok(weight); } - let signers_info = pallet_parameters::Pallet::::signers_info(); let mut new_signer = vec![]; - let mut weight: Weight = ::WeightInfo::new_session_not_adding_new_signer(); + let mut count = 0u32; if current_signers_length <= signers_info.total_signers as usize { let mut randomness = Self::get_randomness(); // grab a current signer to initiate value let mut next_signer_up = ¤t_signers[0].clone(); let mut index; - let mut count = 0u32; + // loops to find signer in validator that is not already signer while current_signers.contains(next_signer_up) { index = randomness.next_u32() % validators.len() as u32; next_signer_up = &validators[index as usize]; count += 1; } + current_signers.push(next_signer_up.clone()); new_signer = next_signer_up.encode(); - weight = ::WeightInfo::new_session(current_signers.len() as u32, count) } // removes first signer and pushes new signer to back if total signers not increased @@ -574,20 +578,24 @@ pub mod pallet { } NextSigners::::put(NextSignerInfo { - next_signers: current_signers, + next_signers: current_signers.clone(), confirmations: vec![], }); + // trigger reshare at next block let current_block_number = >::block_number(); let reshare_info = ReshareInfo { block_number: current_block_number + sp_runtime::traits::One::one(), new_signer, }; + ReshareData::::put(reshare_info); JumpStartProgress::::mutate(|jump_start_details| { jump_start_details.parent_key_threshold = signers_info.threshold }); + weight = ::WeightInfo::new_session(current_signers.len() as u32, count); + Ok(weight) } } @@ -605,18 +613,22 @@ pub mod pallet { fn new_session(new_index: SessionIndex) -> Option> { let new_session = I::new_session(new_index); if let Some(validators) = &new_session { - let result_weight = Pallet::::new_session_handler(validators); - if let Err(why) = result_weight { - log::warn!( - "Error splitting validators, Session: {:?}, reason: {:?}", - new_index, - why - ) - } else { - frame_system::Pallet::::register_extra_weight_unchecked( - result_weight.expect("Error unwraping non error value"), - DispatchClass::Mandatory, - ); + let weight = Pallet::::new_session_handler(validators); + + match weight { + Ok(weight) => { + frame_system::Pallet::::register_extra_weight_unchecked( + weight, + DispatchClass::Mandatory, + ); + }, + Err(why) => { + log::warn!( + "Error splitting validators, Session: {:?}, reason: {:?}", + new_index, + why + ) + }, } } new_session diff --git a/pallets/staking/src/weights.rs b/pallets/staking/src/weights.rs index ba51fd542..25be1c558 100644 --- a/pallets/staking/src/weights.rs +++ b/pallets/staking/src/weights.rs @@ -59,8 +59,7 @@ pub trait WeightInfo { fn declare_synced() -> Weight; fn confirm_key_reshare_confirmed(c: u32) -> Weight; fn confirm_key_reshare_completed() -> Weight; - fn new_session_validators_less_then_signers() -> Weight; - fn new_session_not_adding_new_signer() -> Weight; + fn new_session_base_weight(s: u32) -> Weight; fn new_session(c: u32, l: u32) -> Weight; } @@ -195,14 +194,15 @@ impl WeightInfo for SubstrateWeight { } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_validators_less_then_signers() -> Weight { + fn new_session_base_weight(s: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `234` - // Estimated: `1719` - // Minimum execution time: 4_000_000 picoseconds. - Weight::from_parts(4_000_000, 0) - .saturating_add(Weight::from_parts(0, 1719)) - .saturating_add(T::DbWeight::get().reads(1)) + // Measured: `254 + s * (32 ±0)` + // Estimated: `1739 + s * (32 ±0)` + // Minimum execution time: 7_000_000 picoseconds. + Weight::from_parts(7_682_879, 0) + .saturating_add(Weight::from_parts(0, 1739)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(Weight::from_parts(0, 32).saturating_mul(s.into())) } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) @@ -235,26 +235,6 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes(3)) .saturating_add(Weight::from_parts(0, 32).saturating_mul(c.into())) } - /// Storage: `StakingExtension::Signers` (r:1 w:0) - /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `Parameters::SignersInfo` (r:1 w:0) - /// Proof: `Parameters::SignersInfo` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::JumpStartProgress` (r:1 w:1) - /// Proof: `StakingExtension::JumpStartProgress` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::ReshareData` (r:0 w:1) - /// Proof: `StakingExtension::ReshareData` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::NextSigners` (r:0 w:1) - /// Proof: `StakingExtension::NextSigners` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_not_adding_new_signer() -> Weight { - // Proof Size summary in bytes: - // Measured: `439` - // Estimated: `1924` - // Minimum execution time: 9_000_000 picoseconds. - Weight::from_parts(10_000_000, 0) - .saturating_add(Weight::from_parts(0, 1924)) - .saturating_add(T::DbWeight::get().reads(3)) - .saturating_add(T::DbWeight::get().writes(3)) - } } // For backwards compatibility and tests @@ -387,14 +367,15 @@ impl WeightInfo for () { } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_validators_less_then_signers() -> Weight { + fn new_session_base_weight(s: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `234` - // Estimated: `1719` - // Minimum execution time: 4_000_000 picoseconds. - Weight::from_parts(4_000_000, 0) - .saturating_add(Weight::from_parts(0, 1719)) - .saturating_add(RocksDbWeight::get().reads(1)) + // Measured: `254 + s * (32 ±0)` + // Estimated: `1739 + s * (32 ±0)` + // Minimum execution time: 7_000_000 picoseconds. + Weight::from_parts(7_682_879, 0) + .saturating_add(Weight::from_parts(0, 1739)) + .saturating_add(RocksDbWeight::get().reads(2)) + .saturating_add(Weight::from_parts(0, 32).saturating_mul(s.into())) } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) @@ -427,24 +408,4 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes(3)) .saturating_add(Weight::from_parts(0, 32).saturating_mul(c.into())) } - /// Storage: `StakingExtension::Signers` (r:1 w:0) - /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `Parameters::SignersInfo` (r:1 w:0) - /// Proof: `Parameters::SignersInfo` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::JumpStartProgress` (r:1 w:1) - /// Proof: `StakingExtension::JumpStartProgress` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::ReshareData` (r:0 w:1) - /// Proof: `StakingExtension::ReshareData` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::NextSigners` (r:0 w:1) - /// Proof: `StakingExtension::NextSigners` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_not_adding_new_signer() -> Weight { - // Proof Size summary in bytes: - // Measured: `439` - // Estimated: `1924` - // Minimum execution time: 9_000_000 picoseconds. - Weight::from_parts(10_000_000, 0) - .saturating_add(Weight::from_parts(0, 1924)) - .saturating_add(RocksDbWeight::get().reads(3)) - .saturating_add(RocksDbWeight::get().writes(3)) - } } diff --git a/runtime/src/weights/pallet_staking_extension.rs b/runtime/src/weights/pallet_staking_extension.rs index 4029ab5c6..f3232611b 100644 --- a/runtime/src/weights/pallet_staking_extension.rs +++ b/runtime/src/weights/pallet_staking_extension.rs @@ -16,9 +16,9 @@ //! Autogenerated weights for `pallet_staking_extension` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 33.0.0 -//! DATE: 2024-08-20, STEPS: `5`, REPEAT: `2`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2024-08-21, STEPS: `5`, REPEAT: `2`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `Jesses-MacBook-Pro.local`, CPU: `` +//! HOSTNAME: `hcastano`, CPU: `` //! WASM-EXECUTION: `Compiled`, CHAIN: `Some("dev")`, DB CACHE: 1024 // Executed Command: @@ -55,8 +55,8 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `1309` // Estimated: `4774` - // Minimum execution time: 23_000_000 picoseconds. - Weight::from_parts(24_000_000, 0) + // Minimum execution time: 28_000_000 picoseconds. + Weight::from_parts(30_000_000, 0) .saturating_add(Weight::from_parts(0, 4774)) .saturating_add(T::DbWeight::get().reads(3)) .saturating_add(T::DbWeight::get().writes(1)) @@ -73,8 +73,8 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `1430` // Estimated: `4895` - // Minimum execution time: 26_000_000 picoseconds. - Weight::from_parts(26_000_000, 0) + // Minimum execution time: 31_000_000 picoseconds. + Weight::from_parts(32_000_000, 0) .saturating_add(Weight::from_parts(0, 4895)) .saturating_add(T::DbWeight::get().reads(4)) .saturating_add(T::DbWeight::get().writes(2)) @@ -95,8 +95,8 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `1262` // Estimated: `4764` - // Minimum execution time: 38_000_000 picoseconds. - Weight::from_parts(39_000_000, 0) + // Minimum execution time: 45_000_000 picoseconds. + Weight::from_parts(48_000_000, 0) .saturating_add(Weight::from_parts(0, 4764)) .saturating_add(T::DbWeight::get().reads(6)) .saturating_add(T::DbWeight::get().writes(3)) @@ -131,8 +131,8 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `1918` // Estimated: `6248` - // Minimum execution time: 59_000_000 picoseconds. - Weight::from_parts(62_000_000, 0) + // Minimum execution time: 76_000_000 picoseconds. + Weight::from_parts(184_000_000, 0) .saturating_add(Weight::from_parts(0, 6248)) .saturating_add(T::DbWeight::get().reads(13)) .saturating_add(T::DbWeight::get().writes(8)) @@ -145,8 +145,8 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `320` // Estimated: `3785` - // Minimum execution time: 9_000_000 picoseconds. - Weight::from_parts(10_000_000, 0) + // Minimum execution time: 11_000_000 picoseconds. + Weight::from_parts(13_000_000, 0) .saturating_add(Weight::from_parts(0, 3785)) .saturating_add(T::DbWeight::get().reads(1)) .saturating_add(T::DbWeight::get().writes(1)) @@ -160,9 +160,11 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `797 + c * (32 ±0)` // Estimated: `4298 + c * (29 ±1)` - // Minimum execution time: 11_000_000 picoseconds. - Weight::from_parts(11_500_000, 0) + // Minimum execution time: 13_000_000 picoseconds. + Weight::from_parts(14_248_618, 0) .saturating_add(Weight::from_parts(0, 4298)) + // Standard Error: 122_082 + .saturating_add(Weight::from_parts(90_469, 0).saturating_mul(c.into())) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(1)) .saturating_add(Weight::from_parts(0, 29).saturating_mul(c.into())) @@ -177,42 +179,26 @@ impl pallet_staking_extension::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `1309` // Estimated: `4774` - // Minimum execution time: 11_000_000 picoseconds. - Weight::from_parts(12_000_000, 0) + // Minimum execution time: 14_000_000 picoseconds. + Weight::from_parts(14_000_000, 0) .saturating_add(Weight::from_parts(0, 4774)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_validators_less_then_signers() -> Weight { - // Proof Size summary in bytes: - // Measured: `234` - // Estimated: `1719` - // Minimum execution time: 3_000_000 picoseconds. - Weight::from_parts(3_000_000, 0) - .saturating_add(Weight::from_parts(0, 1719)) - .saturating_add(T::DbWeight::get().reads(1)) - } - /// Storage: `StakingExtension::Signers` (r:1 w:0) - /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) /// Storage: `Parameters::SignersInfo` (r:1 w:0) /// Proof: `Parameters::SignersInfo` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::JumpStartProgress` (r:1 w:1) - /// Proof: `StakingExtension::JumpStartProgress` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::ReshareData` (r:0 w:1) - /// Proof: `StakingExtension::ReshareData` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `StakingExtension::NextSigners` (r:0 w:1) - /// Proof: `StakingExtension::NextSigners` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn new_session_not_adding_new_signer() -> Weight { + /// The range of component `s` is `[2, 15]`. + fn new_session_base_weight(s: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `439` - // Estimated: `1924` - // Minimum execution time: 9_000_000 picoseconds. - Weight::from_parts(9_000_000, 0) - .saturating_add(Weight::from_parts(0, 1924)) - .saturating_add(T::DbWeight::get().reads(3)) - .saturating_add(T::DbWeight::get().writes(3)) + // Measured: `254 + s * (32 ±0)` + // Estimated: `1739 + s * (32 ±0)` + // Minimum execution time: 7_000_000 picoseconds. + Weight::from_parts(7_682_879, 0) + .saturating_add(Weight::from_parts(0, 1739)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(Weight::from_parts(0, 32).saturating_mul(s.into())) } /// Storage: `StakingExtension::Signers` (r:1 w:0) /// Proof: `StakingExtension::Signers` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) @@ -230,17 +216,15 @@ impl pallet_staking_extension::WeightInfo for WeightInf /// Proof: `StakingExtension::NextSigners` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) /// The range of component `c` is `[1, 14]`. /// The range of component `l` is `[0, 15]`. - fn new_session(c: u32, l: u32, ) -> Weight { + fn new_session(c: u32, _l: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `482 + c * (32 ±0)` // Estimated: `1966 + c * (32 ±0)` - // Minimum execution time: 13_000_000 picoseconds. - Weight::from_parts(12_683_761, 0) + // Minimum execution time: 15_000_000 picoseconds. + Weight::from_parts(16_252_259, 0) .saturating_add(Weight::from_parts(0, 1966)) - // Standard Error: 28_030 - .saturating_add(Weight::from_parts(45_131, 0).saturating_mul(c.into())) - // Standard Error: 24_017 - .saturating_add(Weight::from_parts(12_438, 0).saturating_mul(l.into())) + // Standard Error: 101_760 + .saturating_add(Weight::from_parts(74_486, 0).saturating_mul(c.into())) .saturating_add(T::DbWeight::get().reads(5)) .saturating_add(T::DbWeight::get().writes(3)) .saturating_add(Weight::from_parts(0, 32).saturating_mul(c.into()))