From d89714d2fe0869162134c050e8e8739a364e6bbd Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 6 Feb 2023 15:56:34 +0300 Subject: [PATCH] Fix `HeadersToKeep` and `MaxBridgedAuthorities` in Millau benchmarks (#1851) * fix `HeadersToKeep` and `MaxBridgedAuthorities` in Millau benchmarks * typo * impl review suggestion --- bridges/bin/millau/runtime/src/lib.rs | 22 +--------- .../bin/rialto-parachain/runtime/src/lib.rs | 4 +- bridges/bin/rialto/runtime/src/lib.rs | 4 +- bridges/modules/grandpa/src/benchmarking.rs | 33 +++++++------- bridges/modules/grandpa/src/lib.rs | 21 ++++++++- bridges/modules/grandpa/src/weights.rs | 44 +++++++++---------- 6 files changed, 65 insertions(+), 63 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index b590a2399d09a..2119fa04610be 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -387,30 +387,12 @@ impl pallet_bridge_relayers::Config for Runtime { type WeightInfo = (); } -#[cfg(feature = "runtime-benchmarks")] -parameter_types! { - /// Number of headers to keep in benchmarks. - /// - /// In benchmarks we always populate with full number of `HeadersToKeep` to make sure that - /// pruning is taken into account. - /// - /// Note: This is lower than regular value, to speed up benchmarking setup. - pub const HeadersToKeep: u32 = 1024; - /// Maximal number of authorities at Rialto. - /// - /// In benchmarks we're using sets of up to `1024` authorities to prepare for possible - /// upgrades in the future and see if performance degrades when number of authorities - /// grow. - pub const MaxAuthoritiesAtRialto: u32 = pallet_bridge_grandpa::benchmarking::MAX_VALIDATOR_SET_SIZE; -} - -#[cfg(not(feature = "runtime-benchmarks"))] parameter_types! { /// Number of headers to keep. /// /// Assuming the worst case of every header being finalized, we will keep headers at least for a - /// week. - pub const HeadersToKeep: u32 = 7 * bp_rialto::DAYS; + /// day. + pub const HeadersToKeep: u32 = bp_rialto::DAYS; /// Maximal number of authorities at Rialto. pub const MaxAuthoritiesAtRialto: u32 = bp_rialto::MAX_AUTHORITIES_COUNT; } diff --git a/bridges/bin/rialto-parachain/runtime/src/lib.rs b/bridges/bin/rialto-parachain/runtime/src/lib.rs index 6fd6c24ebeb17..be457d950415f 100644 --- a/bridges/bin/rialto-parachain/runtime/src/lib.rs +++ b/bridges/bin/rialto-parachain/runtime/src/lib.rs @@ -533,8 +533,8 @@ parameter_types! { /// Number of headers to keep. /// /// Assuming the worst case of every header being finalized, we will keep headers at least for a - /// week. - pub const HeadersToKeep: u32 = 7 * bp_millau::DAYS as u32; + /// day. + pub const HeadersToKeep: u32 = bp_millau::DAYS as u32; /// Maximal number of authorities at Millau. pub const MaxAuthoritiesAtMillau: u32 = bp_millau::MAX_AUTHORITIES_COUNT; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 5ead9b32dc13f..cb3b6d604866b 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -401,8 +401,8 @@ parameter_types! { /// Number of headers to keep. /// /// Assuming the worst case of every header being finalized, we will keep headers at least for a - /// week. - pub const HeadersToKeep: u32 = 7 * bp_rialto::DAYS; + /// day. + pub const HeadersToKeep: u32 = bp_rialto::DAYS; /// Maximal number of authorities at Millau. pub const MaxAuthoritiesAtMillau: u32 = bp_millau::MAX_AUTHORITIES_COUNT; diff --git a/bridges/modules/grandpa/src/benchmarking.rs b/bridges/modules/grandpa/src/benchmarking.rs index e937f7a0bf414..84a80eace2458 100644 --- a/bridges/modules/grandpa/src/benchmarking.rs +++ b/bridges/modules/grandpa/src/benchmarking.rs @@ -53,26 +53,29 @@ use sp_finality_grandpa::AuthorityId; use sp_runtime::traits::Zero; use sp_std::vec::Vec; -// The maximum number of vote ancestries to include in a justification. -// -// In practice this would be limited by the session length (number of blocks a single authority set -// can produce) of a given chain. +/// The maximum number of vote ancestries to include in a justification. +/// +/// In practice this would be limited by the session length (number of blocks a single authority set +/// can produce) of a given chain. const MAX_VOTE_ANCESTRIES: u32 = 1000; -// The maximum number of pre-commits to include in a justification. In practice this scales with the -// number of validators. -pub const MAX_VALIDATOR_SET_SIZE: u32 = 1024; - -// `1..MAX_VALIDATOR_SET_SIZE` and `1..MAX_VOTE_ANCESTRIES` are too large && benchmarks are -// running for almost 40m (steps=50, repeat=20) on a decent laptop, which is too much. Since -// we're building linear function here, let's just select some limited subrange for benchmarking. -const VALIDATOR_SET_SIZE_RANGE_BEGIN: u32 = MAX_VALIDATOR_SET_SIZE / 20; -const VALIDATOR_SET_SIZE_RANGE_END: u32 = - VALIDATOR_SET_SIZE_RANGE_BEGIN + VALIDATOR_SET_SIZE_RANGE_BEGIN; +// `1..MAX_VOTE_ANCESTRIES` is too large && benchmarks are running for almost 40m (steps=50, +// repeat=20) on a decent laptop, which is too much. Since we're building linear function here, +// let's just select some limited subrange for benchmarking. const MAX_VOTE_ANCESTRIES_RANGE_BEGIN: u32 = MAX_VOTE_ANCESTRIES / 20; const MAX_VOTE_ANCESTRIES_RANGE_END: u32 = MAX_VOTE_ANCESTRIES_RANGE_BEGIN + MAX_VOTE_ANCESTRIES_RANGE_BEGIN; +// the same with validators - if there are too much validators, let's run benchmarks on subrange +fn validator_set_range_end, I: 'static>() -> u32 { + let max_bridged_authorities = T::MaxBridgedAuthorities::get(); + if max_bridged_authorities > 128 { + sp_std::cmp::max(128, max_bridged_authorities / 5) + } else { + max_bridged_authorities + } +} + /// Returns number of first header to be imported. /// /// Since we bootstrap the pallet with `HeadersToKeep` already imported headers, @@ -117,7 +120,7 @@ benchmarks_instance_pallet! { // This is the "gold standard" benchmark for this extrinsic, and it's what should be used to // annotate the weight in the pallet. submit_finality_proof { - let p in VALIDATOR_SET_SIZE_RANGE_BEGIN..VALIDATOR_SET_SIZE_RANGE_END; + let p in 1 .. validator_set_range_end::(); let v in MAX_VOTE_ANCESTRIES_RANGE_BEGIN..MAX_VOTE_ANCESTRIES_RANGE_END; let caller: T::AccountId = whitelisted_caller(); let (header, justification) = prepare_benchmark_data::(p, v); diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 3cf67349b474a..493475cf4ed39 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -104,6 +104,9 @@ pub mod pallet { #[pallet::constant] type MaxRequests: Get; + // Avoid using `HeadersToKeep` directly in the pallet code. Use `headers_to_keep` function + // instead. + /// Maximal number of finalized headers to keep in the storage. /// /// The setting is there to prevent growing the on-chain state indefinitely. Note @@ -464,6 +467,20 @@ pub mod pallet { })?) } + /// Return number of headers to keep in the runtime storage. + #[cfg(not(feature = "runtime-benchmarks"))] + pub(crate) fn headers_to_keep, I: 'static>() -> u32 { + T::HeadersToKeep::get() + } + + /// Return number of headers to keep in the runtime storage. + #[cfg(feature = "runtime-benchmarks")] + pub(crate) fn headers_to_keep, I: 'static>() -> u32 { + // every db operation (significantly) slows down benchmarks, so let's keep as min as + // possible + 2 + } + /// Import a previously verified header to the storage. /// /// Note this function solely takes care of updating the storage and pruning old entries, @@ -479,7 +496,7 @@ pub mod pallet { >::insert(index, hash); // Update ring buffer pointer and remove old header. - >::put((index + 1) % T::HeadersToKeep::get()); + >::put((index + 1) % headers_to_keep::()); if let Ok(hash) = pruning { log::debug!(target: LOG_TARGET, "Pruning old header: {:?}.", hash); >::remove(hash); @@ -523,7 +540,7 @@ pub mod pallet { init_params: super::InitializationData>, ) { let start_number = *init_params.header.number(); - let end_number = start_number + T::HeadersToKeep::get().into(); + let end_number = start_number + headers_to_keep::().into(); initialize_bridge::(init_params).expect("benchmarks are correct"); let mut number = start_number; diff --git a/bridges/modules/grandpa/src/weights.rs b/bridges/modules/grandpa/src/weights.rs index 3c3a62083832d..e8971d69b484b 100644 --- a/bridges/modules/grandpa/src/weights.rs +++ b/bridges/modules/grandpa/src/weights.rs @@ -75,8 +75,8 @@ impl WeightInfo for BridgeWeight { /// /// Storage: BridgeRialtoGrandpa CurrentAuthoritySet (r:1 w:0) /// - /// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(40970), - /// added: 41465, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(209), + /// added: 704, mode: MaxEncodedLen) /// /// Storage: BridgeRialtoGrandpa ImportedHashesPointer (r:1 w:1) /// @@ -93,19 +93,19 @@ impl WeightInfo for BridgeWeight { /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added: /// 2543, mode: MaxEncodedLen) /// - /// The range of component `p` is `[51, 102]`. + /// The range of component `p` is `[1, 5]`. /// /// The range of component `v` is `[50, 100]`. fn submit_finality_proof(p: u32, v: u32) -> Weight { // Proof Size summary in bytes: - // Measured: `2524 + p * (40 ±0)` - // Estimated: `46001` - // Minimum execution time: 2_282_140 nanoseconds. - Weight::from_parts(142_496_714, 46001) - // Standard Error: 32_796 - .saturating_add(Weight::from_ref_time(40_232_935).saturating_mul(p.into())) - // Standard Error: 33_574 - .saturating_add(Weight::from_ref_time(1_185_407).saturating_mul(v.into())) + // Measured: `459 + p * (40 ±0)` + // Estimated: `5240` + // Minimum execution time: 368_734 nanoseconds. + Weight::from_parts(64_214_587, 5240) + // Standard Error: 226_504 + .saturating_add(Weight::from_ref_time(41_231_918).saturating_mul(p.into())) + // Standard Error: 20_667 + .saturating_add(Weight::from_ref_time(2_770_962).saturating_mul(v.into())) .saturating_add(T::DbWeight::get().reads(6_u64)) .saturating_add(T::DbWeight::get().writes(6_u64)) } @@ -130,8 +130,8 @@ impl WeightInfo for () { /// /// Storage: BridgeRialtoGrandpa CurrentAuthoritySet (r:1 w:0) /// - /// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(40970), - /// added: 41465, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(209), + /// added: 704, mode: MaxEncodedLen) /// /// Storage: BridgeRialtoGrandpa ImportedHashesPointer (r:1 w:1) /// @@ -148,19 +148,19 @@ impl WeightInfo for () { /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added: /// 2543, mode: MaxEncodedLen) /// - /// The range of component `p` is `[51, 102]`. + /// The range of component `p` is `[1, 5]`. /// /// The range of component `v` is `[50, 100]`. fn submit_finality_proof(p: u32, v: u32) -> Weight { // Proof Size summary in bytes: - // Measured: `2524 + p * (40 ±0)` - // Estimated: `46001` - // Minimum execution time: 2_282_140 nanoseconds. - Weight::from_parts(142_496_714, 46001) - // Standard Error: 32_796 - .saturating_add(Weight::from_ref_time(40_232_935).saturating_mul(p.into())) - // Standard Error: 33_574 - .saturating_add(Weight::from_ref_time(1_185_407).saturating_mul(v.into())) + // Measured: `459 + p * (40 ±0)` + // Estimated: `5240` + // Minimum execution time: 368_734 nanoseconds. + Weight::from_parts(64_214_587, 5240) + // Standard Error: 226_504 + .saturating_add(Weight::from_ref_time(41_231_918).saturating_mul(p.into())) + // Standard Error: 20_667 + .saturating_add(Weight::from_ref_time(2_770_962).saturating_mul(v.into())) .saturating_add(RocksDbWeight::get().reads(6_u64)) .saturating_add(RocksDbWeight::get().writes(6_u64)) }