MaxValues limit for storage maps in the pallet-bridge-grandpa (#1861)

svyatonik · bkchr · commit e75d872aa78d · 2024-04-10T10:28:37.000+02:00
* MaxValues limit for storage maps in the pallet-bridge-grandpa

* remove use from the future PR
diff --git 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<T: Config<I>, 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<T: Config<I>, I: 'static, N: From<u32>>() -> N {
-	(T::HeadersToKeep::get() + 1).into()
-}
-
 /// Prepare header and its justification to submit using `submit_finality_proof`.
 fn prepare_benchmark_data<T: Config<I>, I: 'static>(
 	precommits: u32,
@@ -94,16 +86,19 @@ fn prepare_benchmark_data<T: Config<I>, I: 'static>(
 		.map(|id| (AuthorityId::from(*id), 1))
 		.collect::<Vec<_>>();
 
+	let genesis_header: BridgedHeader<T, I> = 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::<T, I>(init_data);
+	assert!(<ImportedHeaders<T, I>>::contains_key(genesis_hash));
 
-	let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
+	let header: BridgedHeader<T, I> = 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::<T, I>(p, v);
 	}: submit_finality_proof(RawOrigin::Signed(caller), Box::new(header), justification)
 	verify {
-		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
+		let genesis_header: BridgedHeader<T, I> = bp_test_utils::test_header(Zero::zero());
+		let header: BridgedHeader<T, I> = bp_test_utils::test_header(One::one());
 		let expected_hash = header.hash();
 
+		// check that the header#1 has been inserted
 		assert_eq!(<BestFinalized<T, I>>::get().unwrap().1, expected_hash);
 		assert!(<ImportedHeaders<T, I>>::contains_key(expected_hash));
+
+		// check that the header#0 has been pruned
+		assert!(!<ImportedHeaders<T, I>>::contains_key(genesis_header.hash()));
 	}
 }
diff --git 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<u32>;
 
-		// 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<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Identity, u32, BridgedBlockHash<T, I>>;
+	pub(super) type ImportedHashes<T: Config<I>, I: 'static = ()> = StorageMap<
+		Hasher = Identity,
+		Key = u32,
+		Value = BridgedBlockHash<T, I>,
+		QueryKind = OptionQuery,
+		OnEmpty = GetDefault,
+		MaxValues = MaybeHeadersToKeep<T, I>,
+	>;
 
 	/// Current ring buffer position.
 	#[pallet::storage]
@@ -302,8 +305,14 @@ pub mod pallet {
 
 	/// Relevant fields of imported headers.
 	#[pallet::storage]
-	pub type ImportedHeaders<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Identity, BridgedBlockHash<T, I>, BridgedStoredHeaderData<T, I>>;
+	pub type ImportedHeaders<T: Config<I>, I: 'static = ()> = StorageMap<
+		Hasher = Identity,
+		Key = BridgedBlockHash<T, I>,
+		Value = BridgedStoredHeaderData<T, I>,
+		QueryKind = OptionQuery,
+		OnEmpty = GetDefault,
+		MaxValues = MaybeHeadersToKeep<T, I>,
+	>;
 
 	/// 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<T: Config<I>, 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<T: Config<I>, 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 {
 		<ImportedHashes<T, I>>::insert(index, hash);
 
 		// Update ring buffer pointer and remove old header.
-		<ImportedHashesPointer<T, I>>::put((index + 1) % headers_to_keep::<T, I>());
+		<ImportedHashesPointer<T, I>>::put((index + 1) % T::HeadersToKeep::get());
 		if let Ok(hash) = pruning {
 			log::debug!(target: LOG_TARGET, "Pruning old header: {:?}.", hash);
 			<ImportedHeaders<T, I>>::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<T, I>(PhantomData<(T, I)>);
+
+	// this implementation is required to use the struct as `MaxValues`
+	impl<T: Config<I>, I: 'static> Get<Option<u32>> for MaybeHeadersToKeep<T, I> {
+		fn get() -> Option<u32> {
+			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<T: Config<I>, I: 'static>(
 		init_params: super::InitializationData<BridgedHeader<T, I>>,
-	) {
-		let start_number = *init_params.header.number();
-		let end_number = start_number + headers_to_keep::<T, I>().into();
+	) -> BridgedHeader<T, I> {
+		let start_header = init_params.header.clone();
 		initialize_bridge::<T, I>(init_params).expect("benchmarks are correct");
 
-		let mut number = start_number;
-		while number < end_number {
-			number = number + sp_runtime::traits::One::one();
-			let header = <BridgedHeader<T, I>>::new(
-				number,
-				Default::default(),
-				Default::default(),
-				Default::default(),
-				Default::default(),
-			);
-			let hash = header.hash();
-			insert_header::<T, I>(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::<T, I>::get(), 1);
+		ImportedHashesPointer::<T, I>::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!(<BestFinalized<TestRuntime>>::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::<TestRuntime>::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::<TestRuntime>::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!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());
diff --git 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<T: frame_system::Config> WeightInfo for BridgeWeight<T> {
 	///
 	/// 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))
 	}
