Enable verification logic when executing benchmarks (#6929)

* Add `--verify` flag to benchmark execution

* make it so `--verify` can be used for getting the actual benchmarks

* undo manual testing

* oops

* use benchmark config struct

* verify is default on, docs update

* remove clone

* improve formatting

* fix test

* bump impl for ci

substrate/bin/node/runtime/src/lib.rs

@@ -1134,13 +1134,7 @@ impl_runtime_apis! {
 	#[cfg(feature = "runtime-benchmarks")]
 	impl frame_benchmarking::Benchmark<Block> for Runtime {
 		fn dispatch_benchmark(
-			pallet: Vec<u8>,
-			benchmark: Vec<u8>,
-			lowest_range_values: Vec<u32>,
-			highest_range_values: Vec<u32>,
-			steps: Vec<u32>,
-			repeat: u32,
-			extra: bool,
+			config: frame_benchmarking::BenchmarkConfig
 		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
 			use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark, TrackedStorageKey};
 			// Trying to add benchmarks directly to the Session Pallet caused cyclic dependency issues.
@@ -1170,7 +1164,7 @@ impl_runtime_apis! {
 			];
 
 			let mut batches = Vec::<BenchmarkBatch>::new();
-			let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist, extra);
+			let params = (&config, &whitelist);
 
 			add_benchmark!(params, batches, pallet_babe, Babe);
 			add_benchmark!(params, batches, pallet_balances, Balances);

substrate/frame/benchmarking/src/lib.rs

@@ -648,9 +648,11 @@ macro_rules! benchmark_backend {
 				]
 			}
 
-			fn instance(&self, components: &[($crate::BenchmarkParameter, u32)])
-				-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
-			{
+			fn instance(
+				&self,
+				components: &[($crate::BenchmarkParameter, u32)],
+				verify: bool
+			) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
 				$(
 					let $common = $common_from;
 				)*
@@ -666,28 +668,13 @@ macro_rules! benchmark_backend {
 				$( $param_instancer ; )*
 				$( $post )*
 
-				Ok(Box::new(move || -> Result<(), &'static str> { $eval; Ok(()) }))
-			}
-
-			fn verify(&self, components: &[($crate::BenchmarkParameter, u32)])
-				-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
-			{
-				$(
-					let $common = $common_from;
-				)*
-				$(
-					// Prepare instance
-					let $param = components.iter()
-						.find(|&c| c.0 == $crate::BenchmarkParameter::$param)
-						.unwrap().1;
-				)*
-				$(
-					let $pre_id : $pre_ty = $pre_ex;
-				)*
-				$( $param_instancer ; )*
-				$( $post )*
-
-				Ok(Box::new(move || -> Result<(), &'static str> { $eval; $postcode; Ok(()) }))
+				Ok(Box::new(move || -> Result<(), &'static str> {
+					$eval;
+					if verify {
+						$postcode;
+					}
+					Ok(())
+				}))
 			}
 		}
 	};
@@ -736,26 +723,16 @@ macro_rules! selected_benchmark {
 				}
 			}
 
-			fn instance(&self, components: &[($crate::BenchmarkParameter, u32)])
-				-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
-			{
+			fn instance(
+				&self,
+				components: &[($crate::BenchmarkParameter, u32)],
+				verify: bool
+			) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
 				match self {
 					$(
 						Self::$bench => <
 							$bench as $crate::BenchmarkingSetup<T $(, $bench_inst)? >
-						>::instance(&$bench, components),
-					)*
-				}
-			}
-
-			fn verify(&self, components: &[($crate::BenchmarkParameter, u32)])
-				-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
-			{
-				match self {
-					$(
-						Self::$bench => <
-							$bench as $crate::BenchmarkingSetup<T $(, $bench_inst)? >
-						>::verify(&$bench, components),
+						>::instance(&$bench, components, verify),
 					)*
 				}
 			}
@@ -791,7 +768,8 @@ macro_rules! impl_benchmark {
 				highest_range_values: &[u32],
 				steps: &[u32],
 				repeat: u32,
-				whitelist: &[$crate::TrackedStorageKey]
+				whitelist: &[$crate::TrackedStorageKey],
+				verify: bool,
 			) -> Result<Vec<$crate::BenchmarkResults>, &'static str> {
 				// Map the input to the selected benchmark.
 				let extrinsic = sp_std::str::from_utf8(extrinsic)
@@ -826,6 +804,7 @@ macro_rules! impl_benchmark {
 					repeat: u32,
 					c: Vec<($crate::BenchmarkParameter, u32)>,
 					results: &mut Vec<$crate::BenchmarkResults>,
+					verify: bool,
 				| -> Result<(), &'static str> {
 					// Run the benchmark `repeat` times.
 					for _ in 0..repeat {
@@ -833,7 +812,7 @@ macro_rules! impl_benchmark {
 						// benchmark.
 						let closure_to_benchmark = <
 							SelectedBenchmark as $crate::BenchmarkingSetup<T $(, $instance)?>
-						>::instance(&selected_benchmark, &c)?;
+						>::instance(&selected_benchmark, &c, verify)?;
 
 						// Set the block number to at least 1 so events are deposited.
 						if $crate::Zero::is_zero(&frame_system::Module::<T>::block_number()) {
@@ -847,43 +826,49 @@ macro_rules! impl_benchmark {
 						// Reset the read/write counter so we don't count operations in the setup process.
 						$crate::benchmarking::reset_read_write_count();
 
-						// Time the extrinsic logic.
-						frame_support::debug::trace!(
-							target: "benchmark",
-							"Start Benchmark: {:?}", c
-						);
+						if verify {
+							closure_to_benchmark()?;
+						} else {
+							// Time the extrinsic logic.
+							frame_support::debug::trace!(
+								target: "benchmark",
+								"Start Benchmark: {:?}", c
+							);
 
-						let start_extrinsic = $crate::benchmarking::current_time();
-						closure_to_benchmark()?;
-						let finish_extrinsic = $crate::benchmarking::current_time();
-						let elapsed_extrinsic = finish_extrinsic - start_extrinsic;
-						// Commit the changes to get proper write count
-						$crate::benchmarking::commit_db();
-						frame_support::debug::trace!(
-							target: "benchmark",
-							"End Benchmark: {} ns", elapsed_extrinsic
-						);
-						let read_write_count = $crate::benchmarking::read_write_count();
-						frame_support::debug::trace!(
-							target: "benchmark",
-							"Read/Write Count {:?}", read_write_count
-						);
+							let start_extrinsic = $crate::benchmarking::current_time();
 
-						// Time the storage root recalculation.
-						let start_storage_root = $crate::benchmarking::current_time();
-						$crate::storage_root();
-						let finish_storage_root = $crate::benchmarking::current_time();
-						let elapsed_storage_root = finish_storage_root - start_storage_root;
+							closure_to_benchmark()?;
 
-						results.push($crate::BenchmarkResults {
-							components: c.clone(),
-							extrinsic_time: elapsed_extrinsic,
-							storage_root_time: elapsed_storage_root,
-							reads: read_write_count.0,
-							repeat_reads: read_write_count.1,
-							writes: read_write_count.2,
-							repeat_writes: read_write_count.3,
-						});
+							let finish_extrinsic = $crate::benchmarking::current_time();
+							let elapsed_extrinsic = finish_extrinsic - start_extrinsic;
+							// Commit the changes to get proper write count
+							$crate::benchmarking::commit_db();
+							frame_support::debug::trace!(
+								target: "benchmark",
+								"End Benchmark: {} ns", elapsed_extrinsic
+							);
+							let read_write_count = $crate::benchmarking::read_write_count();
+							frame_support::debug::trace!(
+								target: "benchmark",
+								"Read/Write Count {:?}", read_write_count
+							);
+
+							// Time the storage root recalculation.
+							let start_storage_root = $crate::benchmarking::current_time();
+							$crate::storage_root();
+							let finish_storage_root = $crate::benchmarking::current_time();
+							let elapsed_storage_root = finish_storage_root - start_storage_root;
+
+							results.push($crate::BenchmarkResults {
+								components: c.clone(),
+								extrinsic_time: elapsed_extrinsic,
+								storage_root_time: elapsed_storage_root,
+								reads: read_write_count.0,
+								repeat_reads: read_write_count.1,
+								writes: read_write_count.2,
+								repeat_writes: read_write_count.3,
+							});
+						}
 
 						// Wipe the DB back to the genesis state.
 						$crate::benchmarking::wipe_db();
@@ -893,7 +878,11 @@ macro_rules! impl_benchmark {
 				};
 
 				if components.is_empty() {
-					repeat_benchmark(repeat, Default::default(), &mut results)?;
+					if verify {
+						// If `--verify` is used, run the benchmark once to verify it would complete.
+						repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?;
+					}
+					repeat_benchmark(repeat, Default::default(), &mut results, false)?;
 				} else {
 					// Select the component we will be benchmarking. Each component will be benchmarked.
 					for (idx, (name, low, high)) in components.iter().enumerate() {
@@ -929,7 +918,11 @@ macro_rules! impl_benchmark {
 								)
 								.collect();
 
-							repeat_benchmark(repeat, c, &mut results)?;
+							if verify {
+								// If `--verify` is used, run the benchmark once to verify it would complete.
+								repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?;
+							}
+							repeat_benchmark(repeat, c, &mut results, false)?;
 						}
 					}
 				}
@@ -962,17 +955,17 @@ macro_rules! impl_benchmark_test {
 				let execute_benchmark = |
 					c: Vec<($crate::BenchmarkParameter, u32)>
 				| -> Result<(), &'static str> {
-					// Set up the verification state
+					// Set up the benchmark, return execution + verification function.
 					let closure_to_verify = <
 						SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
-					>::verify(&selected_benchmark, &c)?;
+					>::instance(&selected_benchmark, &c, true)?;
 
 					// Set the block number to at least 1 so events are deposited.
 					if $crate::Zero::is_zero(&frame_system::Module::<T>::block_number()) {
 						frame_system::Module::<T>::set_block_number(1.into());
 					}
 
-					// Run verification
+					// Run execution + verification
 					closure_to_verify()?;
 
 					// Reset the state
@@ -1015,7 +1008,7 @@ macro_rules! impl_benchmark_test {
 /// First create an object that holds in the input parameters for the benchmark:
 ///
 /// ```ignore
-/// let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist);
+/// let params = (&config, &whitelist);
 /// ```
 ///
 /// The `whitelist` is a parameter you pass to control the DB read/write tracking.
@@ -1059,18 +1052,29 @@ macro_rules! impl_benchmark_test {
 macro_rules! add_benchmark {
 	( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => (
 		let name_string = stringify!($name).as_bytes();
-		let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist, extra) = $params;
+		let (config, whitelist) = $params;
+		let $crate::BenchmarkConfig {
+			pallet,
+			benchmark,
+			lowest_range_values,
+			highest_range_values,
+			steps,
+			repeat,
+			verify,
+			extra,
+		} = config;
 		if &pallet[..] == &name_string[..] || &pallet[..] == &b"*"[..] {
 			if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] {
-				for benchmark in $( $location )*::benchmarks(extra).into_iter() {
+				for benchmark in $( $location )*::benchmarks(*extra).into_iter() {
 					$batches.push($crate::BenchmarkBatch {
 						results: $( $location )*::run_benchmark(
 							benchmark,
 							&lowest_range_values[..],
 							&highest_range_values[..],
 							&steps[..],
-							repeat,
+							*repeat,
 							whitelist,
+							*verify,
 						)?,
 						pallet: name_string.to_vec(),
 						benchmark: benchmark.to_vec(),
@@ -1083,8 +1087,9 @@ macro_rules! add_benchmark {
 						&lowest_range_values[..],
 						&highest_range_values[..],
 						&steps[..],
-						repeat,
+						*repeat,
 						whitelist,
+						*verify,
 					)?,
 					pallet: name_string.to_vec(),
 					benchmark: benchmark.clone(),

substrate/frame/benchmarking/src/tests.rs

@@ -176,10 +176,11 @@ fn benchmarks_macro_works() {
 	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
 		&selected,
 		&[(BenchmarkParameter::b, 1)],
+		true,
 	).expect("failed to create closure");
 
 	new_test_ext().execute_with(|| {
-		assert_eq!(closure(), Ok(()));
+		assert_ok!(closure());
 	});
 }
 
@@ -193,6 +194,7 @@ fn benchmarks_macro_rename_works() {
 	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
 		&selected,
 		&[(BenchmarkParameter::b, 1)],
+		true,
 	).expect("failed to create closure");
 
 	new_test_ext().execute_with(|| {
@@ -210,9 +212,10 @@ fn benchmarks_macro_works_for_non_dispatchable() {
 	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
 		&selected,
 		&[(BenchmarkParameter::x, 1)],
+		true,
 	).expect("failed to create closure");
 
-	assert_eq!(closure(), Ok(()));
+	assert_ok!(closure());
 }
 
 #[test]
@@ -220,14 +223,28 @@ fn benchmarks_macro_verify_works() {
 	// Check postcondition for benchmark `set_value` is valid.
 	let selected = SelectedBenchmark::set_value;
 
-	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::verify(
+	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
 		&selected,
 		&[(BenchmarkParameter::b, 1)],
+		true,
 	).expect("failed to create closure");
 
 	new_test_ext().execute_with(|| {
 		assert_ok!(closure());
 	});
+
+	// Check postcondition for benchmark `bad_verify` is invalid.
+	let selected = SelectedBenchmark::bad_verify;
+
+	let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
+		&selected,
+		&[(BenchmarkParameter::x, 10000)],
+		true,
+	).expect("failed to create closure");
+
+	new_test_ext().execute_with(|| {
+		assert_err!(closure(), "You forgot to sort!");
+	});
 }
 
 #[test]

substrate/frame/benchmarking/src/utils.rs

@@ -63,19 +63,32 @@ pub struct BenchmarkResults {
 	pub repeat_writes: u32,
 }
 
+/// Configuration used to setup and run runtime benchmarks.
+#[derive(Encode, Decode, Default, Clone, PartialEq, Debug)]
+pub struct BenchmarkConfig {
+	/// The encoded name of the pallet to benchmark.
+	pub pallet: Vec<u8>,
+	/// The encoded name of the benchmark/extrinsic to run.
+	pub benchmark: Vec<u8>,
+	/// An optional manual override to the lowest values used in the `steps` range.
+	pub lowest_range_values: Vec<u32>,
+	/// An optional manual override to the highest values used in the `steps` range.
+	pub highest_range_values: Vec<u32>,
+	/// The number of samples to take across the range of values for components.
+	pub steps: Vec<u32>,
+	/// The number of times to repeat a benchmark.
+	pub repeat: u32,
+	/// Enable an extra benchmark iteration which runs the verification logic for a benchmark.
+	pub verify: bool,
+	/// Enable benchmarking of "extra" extrinsics, i.e. those that are not directly used in a pallet.
+	pub extra: bool,
+}
+
 sp_api::decl_runtime_apis! {
 	/// Runtime api for benchmarking a FRAME runtime.
 	pub trait Benchmark {
 		/// Dispatch the given benchmark.
-		fn dispatch_benchmark(
-			pallet: Vec<u8>,
-			benchmark: Vec<u8>,
-			lowest_range_values: Vec<u32>,
-			highest_range_values: Vec<u32>,
-			steps: Vec<u32>,
-			repeat: u32,
-			extra: bool,
-		) -> Result<Vec<BenchmarkBatch>, RuntimeString>;
+		fn dispatch_benchmark(config: BenchmarkConfig) -> Result<Vec<BenchmarkBatch>, RuntimeString>;
 	}
 }
 
@@ -175,7 +188,8 @@ pub trait Benchmarking<T> {
 		highest_range_values: &[u32],
 		steps: &[u32],
 		repeat: u32,
-		whitelist: &[TrackedStorageKey]
+		whitelist: &[TrackedStorageKey],
+		verify: bool,
 	) -> Result<Vec<T>, &'static str>;
 }
 
@@ -188,10 +202,11 @@ pub trait BenchmarkingSetup<T, I = ()> {
 	fn components(&self) -> Vec<(BenchmarkParameter, u32, u32)>;
 
 	/// Set up the storage, and prepare a closure to run the benchmark.
-	fn instance(&self, components: &[(BenchmarkParameter, u32)]) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>;
-
-	/// Set up the storage, and prepare a closure to test and verify the benchmark
-	fn verify(&self, components: &[(BenchmarkParameter, u32)]) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>;
+	fn instance(
+		&self,
+		components: &[(BenchmarkParameter, u32)],
+		verify: bool
+	) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>;
 }
 
 /// Grab an account, seeded by a name and index.

substrate/frame/staking/src/benchmarking.rs

@@ -715,7 +715,8 @@ mod tests {
 			let closure_to_benchmark =
 				<SelectedBenchmark as frame_benchmarking::BenchmarkingSetup<Test>>::instance(
 					&selected_benchmark,
-					&c
+					&c,
+					true
 				).unwrap();
 
 			assert_ok!(closure_to_benchmark());

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

@@ -75,6 +75,7 @@ impl BenchmarkCmd {
 				self.highest_range_values.clone(),
 				self.steps.clone(),
 				self.repeat,
+				!self.no_verify,
 				self.extra,
 			).encode(),
 			extensions,

substrate/utils/frame/benchmarking-cli/src/lib.rs

@@ -72,6 +72,10 @@ pub struct BenchmarkCmd {
 	#[structopt(long)]
 	pub heap_pages: Option<u64>,
 
+	/// Disable verification logic when running benchmarks.
+	#[structopt(long)]
+	pub no_verify: bool,
+
 	/// Display and run extra benchmarks that would otherwise not be needed for weight construction.
 	#[structopt(long)]
 	pub extra: bool,