pallet scheduler: fix weight and add safety checks

prdoc/pr_7785.prdoc

@@ -0,0 +1,22 @@
+title: 'pallet scheduler: fix weight and add safety checks'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Changes:
+    - Add runtime integrity test for scheduler pallet to ensure that lookups use sensible weights
+    - Check all passed storage names in the omni bencher to be known by FRAME metadata
+    - Trim storage names in omni bencher to fix V1 bench syntax bug
+    - Fix V1 bench syntax storage name sanitization for specific Rust versions
+
+    I re-ran the benchmarks with the omni-bencher modifications and it did not change the [proof size](https://weights.tasty.limo/compare?repo=polkadot-sdk&threshold=1&path_pattern=substrate%2Fframe%2F**%2Fsrc%2Fweights.rs%2Cpolkadot%2Fruntime%2F*%2Fsrc%2Fweights%2F**%2F*.rs%2Cpolkadot%2Fbridges%2Fmodules%2F*%2Fsrc%2Fweights.rs%2Ccumulus%2F**%2Fweights%2F*.rs%2Ccumulus%2F**%2Fweights%2Fxcm%2F*.rs%2Ccumulus%2F**%2Fsrc%2Fweights.rs&method=asymptotic&ignore_errors=true&unit=proof&old=cc0142510b81dcf1c1a22f7dc164c453c25287e6&new=bb19d78821eaeaf2262f6a23ee45f83dd4f94d29). I reverted [the commit](https://github.com/paritytech/polkadot-sdk/pull/7785/commits/bb19d78821eaeaf2262f6a23ee45f83dd4f94d29) afterwards to reduce the noise for reviewers.
+crates:
+- name: frame-benchmarking-cli
+  bump: minor
+- name: frame-benchmarking
+  bump: minor
+- name: pallet-scheduler
+  bump: minor
+- name: asset-hub-westend-runtime
+  bump: minor
+- name: westend-runtime
+  bump: minor

substrate/frame/benchmarking/src/v1.rs

@@ -1008,7 +1008,8 @@ macro_rules! impl_benchmark {
 					$(
 						(stringify!($pov_name).as_bytes().to_vec(),
 						$crate::__private::vec![
-							$( ( stringify!($storage).as_bytes().to_vec(),
+							// Stringify sometimes includes spaces, depending on the Rust version.
+							$( ( stringify!($storage).replace(" ", "").as_bytes().to_vec(),
 								 stringify!($pov_mode).as_bytes().to_vec() ), )*
 						]),
 					)*

substrate/frame/scheduler/src/lib.rs

@@ -415,6 +415,29 @@ pub mod pallet {
 			Self::service_agendas(&mut weight_counter, now, u32::MAX);
 			weight_counter.consumed()
 		}
+
+		#[cfg(feature = "std")]
+		fn integrity_test() {
+			fn lookup_weight<T: Config>(s: u32) -> Weight {
+				T::WeightInfo::service_agendas_base() +
+					T::WeightInfo::service_agenda_base(T::MaxScheduledPerBlock::get()) +
+					T::WeightInfo::service_task_base() +
+					T::WeightInfo::service_task_fetched(s) +
+					T::WeightInfo::service_task_named() +
+					T::WeightInfo::service_task_periodic()
+			}
+
+			let limit = sp_runtime::Perbill::from_percent(90) * T::MaximumWeight::get();
+
+			let small_lookup = lookup_weight::<T>(128);
+			assert!(small_lookup.all_lte(limit), "Must be possible to submit a small lookup");
+
+			let medium_lookup = lookup_weight::<T>(1024);
+			assert!(medium_lookup.all_lte(limit), "Must be possible to submit a medium lookup");
+
+			let large_lookup = lookup_weight::<T>(1024 * 1024);
+			assert!(large_lookup.all_lte(limit), "Must be possible to submit a large lookup");
+		}
 	}
 
 	#[pallet::call]

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

@@ -365,7 +365,8 @@
 		let mut timer = time::SystemTime::now();
 		// Maps (pallet, extrinsic) to its component ranges.
 		let mut component_ranges = HashMap::<(String, String), Vec<ComponentRange>>::new();
-		let pov_modes = Self::parse_pov_modes(&benchmarks_to_run)?;
+		let pov_modes =
+			Self::parse_pov_modes(&benchmarks_to_run, &storage_info, self.ignore_unknown_pov_mode)?;
 		let mut failed = Vec::<(String, String)>::new();
 
 		'outer: for (i, SelectedBenchmark { pallet, instance, extrinsic, components, .. }) in
@@ -912,22 +913,27 @@
 	}
 
 	/// Parses the PoV modes per benchmark that were specified by the `#[pov_mode]` attribute.
-	fn parse_pov_modes(benchmarks: &Vec<SelectedBenchmark>) -> Result<PovModesMap> {
+	fn parse_pov_modes(
+		benchmarks: &Vec<SelectedBenchmark>,
+		storage_info: &[StorageInfo],
+		ignore_unknown_pov_mode: bool,
+	) -> Result<PovModesMap> {
 		use std::collections::hash_map::Entry;
 		let mut parsed = PovModesMap::new();
 
 		for SelectedBenchmark { pallet, extrinsic, pov_modes, .. } in benchmarks {
 			for (pallet_storage, mode) in pov_modes {
 				let mode = PovEstimationMode::from_str(&mode)?;
-				let splits = pallet_storage.split("::").collect::<Vec<_>>();
+				let splits = pallet_storage.replace(" ", "").split("::").collect::<Vec<_>>();
 				if splits.is_empty() || splits.len() > 2 {
 					return Err(format!(
 						"Expected 'Pallet::Storage' as storage name but got: {}",
 						pallet_storage
 					)
 					.into())
 				}
-				let (pov_pallet, pov_storage) = (splits[0], splits.get(1).unwrap_or(&"ALL"));
+				let (pov_pallet, pov_storage) =
+					(splits[0].trim(), splits.get(1).unwrap_or(&"ALL").trim());
 
 				match parsed
 					.entry((pallet.clone(), extrinsic.clone()))
@@ -946,9 +952,43 @@
 				}
 			}
 		}
+		log::debug!("Parsed PoV modes: {:?}", parsed);
+		Self::check_pov_modes(&parsed, storage_info, ignore_unknown_pov_mode)?;
+
 		Ok(parsed)
 	}
 
+	fn check_pov_modes(
+		pov_modes: &PovModesMap,
+		storage_info: &[StorageInfo],
+		ignore_unknown_pov_mode: bool,
+	) -> Result<()> {
+		// Check that all PoV modes are valid pallet storage keys
+		for (pallet, storage) in pov_modes.values().map(|i| i.keys()).flatten() {
+			let (mut found_pallet, mut found_storage) = (false, false);
+
+			for info in storage_info {
+				if pallet == "ALL" || info.pallet_name == pallet.as_bytes() {
+					found_pallet = true;
+				}
+				if storage == "ALL" || info.storage_name == storage.as_bytes() {
+					found_storage = true;
+				}
+			}
+			if !found_pallet || !found_storage {
+				let err = format!("The PoV mode references an unknown storage item or pallet: `{}::{}`. You can ignore this warning by specifying `--ignore-unknown-pov-mode`", pallet, storage);
+
+				if ignore_unknown_pov_mode {
+					log::warn!(target: LOG_TARGET, "Error demoted to warning due to `--ignore-unknown-pov-mode`: {}", err);
+				} else {
+					return Err(err.into());
+				}
+			}
+		}
+
+		Ok(())
+	}
+
 	/// Sanity check the CLI arguments.
 	fn check_args(
 		&self,

substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs

@@ -133,6 +133,10 @@ pub struct PalletCmd {
 	#[arg(long, default_value("max-encoded-len"), value_enum)]
 	pub default_pov_mode: command::PovEstimationMode,
 
+	/// Ignore the error when PoV modes reference unknown storage items or pallets.
+	#[arg(long)]
+	pub ignore_unknown_pov_mode: bool,
+
 	/// Set the heap pages while running benchmarks. If not set, the default value from the client
 	/// is used.
 	#[arg(long)]