From b7047c066bad192632841b219199c491369d355e Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 8 Feb 2023 09:16:37 +0300 Subject: [PATCH] MaxValues limit for storage maps in the pallet-bridge-grandpa (#1861) * MaxValues limit for storage maps in the pallet-bridge-grandpa * remove use from the future PR --- bridges/modules/grandpa/src/benchmarking.rs | 24 ++--- bridges/modules/grandpa/src/lib.rs | 105 ++++++++++---------- bridges/modules/grandpa/src/weights.rs | 50 +++++----- 3 files changed, 87 insertions(+), 92 deletions(-) diff --git a/bridges/modules/grandpa/src/benchmarking.rs b/bridges/modules/grandpa/src/benchmarking.rs index 84a80eace24..710a7e0952c 100644 --- a/bridges/modules/grandpa/src/benchmarking.rs +++ b/bridges/modules/grandpa/src/benchmarking.rs @@ -50,7 +50,7 @@ use frame_benchmarking::{benchmarks_instance_pallet, whitelisted_caller}; use frame_support::traits::Get; use frame_system::RawOrigin; use sp_finality_grandpa::AuthorityId; -use sp_runtime::traits::Zero; +use sp_runtime::traits::{One, Zero}; use sp_std::vec::Vec; /// The maximum number of vote ancestries to include in a justification. @@ -76,14 +76,6 @@ fn validator_set_range_end, I: 'static>() -> u32 { } } -/// Returns number of first header to be imported. -/// -/// Since we bootstrap the pallet with `HeadersToKeep` already imported headers, -/// this function computes the next expected header number to import. -fn header_number, I: 'static, N: From>() -> N { - (T::HeadersToKeep::get() + 1).into() -} - /// Prepare header and its justification to submit using `submit_finality_proof`. fn prepare_benchmark_data, I: 'static>( precommits: u32, @@ -94,16 +86,19 @@ fn prepare_benchmark_data, I: 'static>( .map(|id| (AuthorityId::from(*id), 1)) .collect::>(); + let genesis_header: BridgedHeader = bp_test_utils::test_header(Zero::zero()); + let genesis_hash = genesis_header.hash(); let init_data = InitializationData { - header: Box::new(bp_test_utils::test_header(Zero::zero())), + header: Box::new(genesis_header), authority_list, set_id: TEST_GRANDPA_SET_ID, operating_mode: BasicOperatingMode::Normal, }; bootstrap_bridge::(init_data); + assert!(>::contains_key(genesis_hash)); - let header: BridgedHeader = bp_test_utils::test_header(header_number::()); + let header: BridgedHeader = bp_test_utils::test_header(One::one()); let params = JustificationGeneratorParams { header: header.clone(), round: TEST_GRANDPA_ROUND, @@ -126,10 +121,15 @@ benchmarks_instance_pallet! { let (header, justification) = prepare_benchmark_data::(p, v); }: submit_finality_proof(RawOrigin::Signed(caller), Box::new(header), justification) verify { - let header: BridgedHeader = bp_test_utils::test_header(header_number::()); + let genesis_header: BridgedHeader = bp_test_utils::test_header(Zero::zero()); + let header: BridgedHeader = bp_test_utils::test_header(One::one()); let expected_hash = header.hash(); + // check that the header#1 has been inserted assert_eq!(>::get().unwrap().1, expected_hash); assert!(>::contains_key(expected_hash)); + + // check that the header#0 has been pruned + assert!(!>::contains_key(genesis_header.hash())); } } diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 493475cf4ed..6128030d090 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -104,9 +104,6 @@ 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 @@ -292,8 +289,14 @@ pub mod pallet { /// A ring buffer of imported hashes. Ordered by the insertion time. #[pallet::storage] - pub(super) type ImportedHashes, I: 'static = ()> = - StorageMap<_, Identity, u32, BridgedBlockHash>; + pub(super) type ImportedHashes, I: 'static = ()> = StorageMap< + Hasher = Identity, + Key = u32, + Value = BridgedBlockHash, + QueryKind = OptionQuery, + OnEmpty = GetDefault, + MaxValues = MaybeHeadersToKeep, + >; /// Current ring buffer position. #[pallet::storage] @@ -302,8 +305,14 @@ pub mod pallet { /// Relevant fields of imported headers. #[pallet::storage] - pub type ImportedHeaders, I: 'static = ()> = - StorageMap<_, Identity, BridgedBlockHash, BridgedStoredHeaderData>; + pub type ImportedHeaders, I: 'static = ()> = StorageMap< + Hasher = Identity, + Key = BridgedBlockHash, + Value = BridgedStoredHeaderData, + QueryKind = OptionQuery, + OnEmpty = GetDefault, + MaxValues = MaybeHeadersToKeep, + >; /// The current GRANDPA Authority set. #[pallet::storage] @@ -467,20 +476,6 @@ 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, @@ -496,7 +491,7 @@ pub mod pallet { >::insert(index, hash); // Update ring buffer pointer and remove old header. - >::put((index + 1) % headers_to_keep::()); + >::put((index + 1) % T::HeadersToKeep::get()); if let Ok(hash) = pruning { log::debug!(target: LOG_TARGET, "Pruning old header: {:?}.", hash); >::remove(hash); @@ -535,27 +530,35 @@ pub mod pallet { Ok(()) } + /// Adapter for using `Config::HeadersToKeep` as `MaxValues` bound in our storage maps. + pub struct MaybeHeadersToKeep(PhantomData<(T, I)>); + + // this implementation is required to use the struct as `MaxValues` + impl, I: 'static> Get> for MaybeHeadersToKeep { + fn get() -> Option { + Some(T::HeadersToKeep::get()) + } + } + + /// Initialize pallet so that it is ready for inserting new header. + /// + /// The function makes sure that the new insertion will cause the pruning of some old header. + /// + /// Returns parent header for the new header. #[cfg(feature = "runtime-benchmarks")] pub(crate) fn bootstrap_bridge, I: 'static>( init_params: super::InitializationData>, - ) { - let start_number = *init_params.header.number(); - let end_number = start_number + headers_to_keep::().into(); + ) -> BridgedHeader { + let start_header = init_params.header.clone(); initialize_bridge::(init_params).expect("benchmarks are correct"); - let mut number = start_number; - while number < end_number { - number = number + sp_runtime::traits::One::one(); - let header = >::new( - number, - Default::default(), - Default::default(), - Default::default(), - Default::default(), - ); - let hash = header.hash(); - insert_header::(header, hash); - } + // the most obvious way to cause pruning during next insertion would be to insert + // `HeadersToKeep` headers. But it'll make our benchmarks slow. So we will just play with + // our pruning ring-buffer. + assert_eq!(ImportedHashesPointer::::get(), 1); + ImportedHashesPointer::::put(0); + + *start_header } } @@ -816,13 +819,9 @@ mod tests { fn succesfully_imports_header_with_valid_finality() { run_test(|| { initialize_substrate_bridge(); - assert_ok!( - submit_finality_proof(1), - PostDispatchInfo { - actual_weight: None, - pays_fee: frame_support::dispatch::Pays::Yes, - }, - ); + let result = submit_finality_proof(1); + assert_ok!(result); + assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); let header = test_header(1); assert_eq!(>::get().unwrap().1, header.hash()); @@ -929,17 +928,13 @@ mod tests { let justification = make_default_justification(&header); // Let's import our test header - assert_ok!( - Pallet::::submit_finality_proof( - RuntimeOrigin::signed(1), - Box::new(header.clone()), - justification - ), - PostDispatchInfo { - actual_weight: None, - pays_fee: frame_support::dispatch::Pays::No, - }, + let result = Pallet::::submit_finality_proof( + RuntimeOrigin::signed(1), + Box::new(header.clone()), + justification, ); + assert_ok!(result); + assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::No); // Make sure that our header is the best finalized assert_eq!(>::get().unwrap().1, header.hash()); diff --git a/bridges/modules/grandpa/src/weights.rs b/bridges/modules/grandpa/src/weights.rs index e8971d69b48..44b4ebd37fb 100644 --- a/bridges/modules/grandpa/src/weights.rs +++ b/bridges/modules/grandpa/src/weights.rs @@ -17,7 +17,7 @@ //! Autogenerated weights for pallet_bridge_grandpa //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-02-06, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2023-02-07, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` //! HOSTNAME: `covid`, CPU: `11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 1024 @@ -85,27 +85,27 @@ impl WeightInfo for BridgeWeight { /// /// Storage: BridgeRialtoGrandpa ImportedHashes (r:1 w:1) /// - /// Proof: BridgeRialtoGrandpa ImportedHashes (max_values: None, max_size: Some(36), added: - /// 2511, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa ImportedHashes (max_values: Some(14400), max_size: Some(36), + /// added: 2016, mode: MaxEncodedLen) /// /// Storage: BridgeRialtoGrandpa ImportedHeaders (r:0 w:2) /// - /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added: - /// 2543, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: Some(14400), max_size: Some(68), + /// added: 2048, mode: MaxEncodedLen) /// /// 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: `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())) + // Measured: `416 + p * (40 ±0)` + // Estimated: `4745` + // Minimum execution time: 221_703 nanoseconds. + Weight::from_parts(39_358_497, 4745) + // Standard Error: 85_573 + .saturating_add(Weight::from_ref_time(40_593_280).saturating_mul(p.into())) + // Standard Error: 7_808 + .saturating_add(Weight::from_ref_time(1_529_400).saturating_mul(v.into())) .saturating_add(T::DbWeight::get().reads(6_u64)) .saturating_add(T::DbWeight::get().writes(6_u64)) } @@ -140,27 +140,27 @@ impl WeightInfo for () { /// /// Storage: BridgeRialtoGrandpa ImportedHashes (r:1 w:1) /// - /// Proof: BridgeRialtoGrandpa ImportedHashes (max_values: None, max_size: Some(36), added: - /// 2511, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa ImportedHashes (max_values: Some(14400), max_size: Some(36), + /// added: 2016, mode: MaxEncodedLen) /// /// Storage: BridgeRialtoGrandpa ImportedHeaders (r:0 w:2) /// - /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added: - /// 2543, mode: MaxEncodedLen) + /// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: Some(14400), max_size: Some(68), + /// added: 2048, mode: MaxEncodedLen) /// /// 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: `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())) + // Measured: `416 + p * (40 ±0)` + // Estimated: `4745` + // Minimum execution time: 221_703 nanoseconds. + Weight::from_parts(39_358_497, 4745) + // Standard Error: 85_573 + .saturating_add(Weight::from_ref_time(40_593_280).saturating_mul(p.into())) + // Standard Error: 7_808 + .saturating_add(Weight::from_ref_time(1_529_400).saturating_mul(v.into())) .saturating_add(RocksDbWeight::get().reads(6_u64)) .saturating_add(RocksDbWeight::get().writes(6_u64)) }