From dbd93c7f534b91bd50dd230ca69b1285efd8ae86 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 7 Feb 2023 15:20:58 +0300 Subject: [PATCH 1/2] MaxValues limit for storage maps in the pallet-bridge-grandpa --- modules/grandpa/src/benchmarking.rs | 24 +++---- modules/grandpa/src/lib.rs | 107 +++++++++++++--------------- modules/grandpa/src/weights.rs | 50 ++++++------- 3 files changed, 88 insertions(+), 93 deletions(-) diff --git a/modules/grandpa/src/benchmarking.rs b/modules/grandpa/src/benchmarking.rs index 84a80eace2..710a7e0952 100644 --- a/modules/grandpa/src/benchmarking.rs +++ b/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/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index 493475cf4e..8fd0cd3142 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -44,7 +44,7 @@ use bp_header_chain::{ }; use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderId, HeaderOf, OwnedBridgeModule}; use finality_grandpa::voter_set::VoterSet; -use frame_support::{ensure, fail}; +use frame_support::{dispatch::PostDispatchInfo, ensure, fail}; use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; use sp_runtime::traits::{Header as HeaderT, Zero}; use sp_std::{boxed::Box, convert::TryInto}; @@ -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/modules/grandpa/src/weights.rs b/modules/grandpa/src/weights.rs index e8971d69b4..44b4ebd37f 100644 --- a/modules/grandpa/src/weights.rs +++ b/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)) } From 74697183a45a99db943c7abf571950c0c5b209e3 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 7 Feb 2023 15:38:27 +0300 Subject: [PATCH 2/2] remove use from the future PR --- modules/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index 8fd0cd3142..6128030d09 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -44,7 +44,7 @@ use bp_header_chain::{ }; use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderId, HeaderOf, OwnedBridgeModule}; use finality_grandpa::voter_set::VoterSet; -use frame_support::{dispatch::PostDispatchInfo, ensure, fail}; +use frame_support::{ensure, fail}; use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; use sp_runtime::traits::{Header as HeaderT, Zero}; use sp_std::{boxed::Box, convert::TryInto};