Sub-commands for benchmark (#11164)

* Restructure benchmark commands

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add benchmark block test

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fixup imports

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* CI

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Review fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Extend error message

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Apply suggestions from code review

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* Review fixes

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add commands to node-template

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

substrate/bin/node-template/node/Cargo.toml

@@ -36,6 +36,10 @@ sp-finality-grandpa = { version = "4.0.0-dev", path = "../../../primitives/final
 sc-client-api = { version = "4.0.0-dev", path = "../../../client/api" }
 sp-runtime = { version = "6.0.0", path = "../../../primitives/runtime" }
 sp-timestamp = { version = "4.0.0-dev", path = "../../../primitives/timestamp" }
+sp-inherents = { version = "4.0.0-dev", path = "../../../primitives/inherents" }
+sp-keyring = { version = "6.0.0", path = "../../../primitives/keyring" }
+frame-system = { version = "4.0.0-dev", path = "../../../frame/system" }
+pallet-transaction-payment = { version = "4.0.0-dev", default-features = false, path = "../../../frame/transaction-payment" }
 
 # These dependencies are used for the node template's RPCs
 jsonrpc-core = "18.0.0"

substrate/bin/node-template/node/src/cli.rs

@@ -36,8 +36,8 @@ pub enum Subcommand {
 	/// Revert the chain to a previous state.
 	Revert(sc_cli::RevertCmd),
 
-	/// The custom benchmark subcommand benchmarking runtime pallets.
-	#[clap(name = "benchmark", about = "Benchmark runtime pallets.")]
+	/// Sub-commands concerned with benchmarking.
+	#[clap(subcommand)]
 	Benchmark(frame_benchmarking_cli::BenchmarkCmd),
 
 	/// Try some command against runtime state.

substrate/bin/node-template/node/src/command.rs

@@ -1,11 +1,14 @@
 use crate::{
 	chain_spec,
 	cli::{Cli, Subcommand},
+	command_helper::{inherent_benchmark_data, BenchmarkExtrinsicBuilder},
 	service,
 };
+use frame_benchmarking_cli::BenchmarkCmd;
 use node_template_runtime::Block;
 use sc_cli::{ChainSpec, RuntimeVersion, SubstrateCli};
 use sc_service::PartialComponents;
+use std::sync::Arc;
 
 impl SubstrateCli for Cli {
 	fn impl_name() -> String {
@@ -102,16 +105,41 @@ pub fn run() -> sc_cli::Result<()> {
 				Ok((cmd.run(client, backend, Some(aux_revert)), task_manager))
 			})
 		},
-		Some(Subcommand::Benchmark(cmd)) =>
-			if cfg!(feature = "runtime-benchmarks") {
-				let runner = cli.create_runner(cmd)?;
+		Some(Subcommand::Benchmark(cmd)) => {
+			let runner = cli.create_runner(cmd)?;
 
-				runner.sync_run(|config| cmd.run::<Block, service::ExecutorDispatch>(config))
-			} else {
-				Err("Benchmarking wasn't enabled when building the node. You can enable it with \
-				     `--features runtime-benchmarks`."
-					.into())
-			},
+			runner.sync_run(|config| {
+				let PartialComponents { client, backend, .. } = service::new_partial(&config)?;
+
+				// This switch needs to be in the client, since the client decides
+				// which sub-commands it wants to support.
+				match cmd {
+					BenchmarkCmd::Pallet(cmd) => {
+						if !cfg!(feature = "runtime-benchmarks") {
+							return Err(
+								"Runtime benchmarking wasn't enabled when building the node. \
+							You can enable it with `--features runtime-benchmarks`."
+									.into(),
+							)
+						}
+
+						cmd.run::<Block, service::ExecutorDispatch>(config)
+					},
+					BenchmarkCmd::Block(cmd) => cmd.run(client),
+					BenchmarkCmd::Storage(cmd) => {
+						let db = backend.expose_db();
+						let storage = backend.expose_storage();
+
+						cmd.run(config, client, db, storage)
+					},
+					BenchmarkCmd::Overhead(cmd) => {
+						let ext_builder = BenchmarkExtrinsicBuilder::new(client.clone());
+
+						cmd.run(config, client, inherent_benchmark_data()?, Arc::new(ext_builder))
+					},
+				}
+			})
+		},
 		#[cfg(feature = "try-runtime")]
 		Some(Subcommand::TryRuntime(cmd)) => {
 			let runner = cli.create_runner(cmd)?;

substrate/bin/node-template/node/src/command_helper.rs

@@ -0,0 +1,131 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2022 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
+
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+//! Contains code to setup the command invocations in [`super::command`] which would
+//! otherwise bloat that module.
+
+use crate::service::FullClient;
+
+use node_template_runtime as runtime;
+use runtime::SystemCall;
+use sc_cli::Result;
+use sc_client_api::BlockBackend;
+use sp_core::{Encode, Pair};
+use sp_inherents::{InherentData, InherentDataProvider};
+use sp_keyring::Sr25519Keyring;
+use sp_runtime::{OpaqueExtrinsic, SaturatedConversion};
+
+use std::{sync::Arc, time::Duration};
+
+/// Generates extrinsics for the `benchmark overhead` command.
+///
+/// Note: Should only be used for benchmarking.
+pub struct BenchmarkExtrinsicBuilder {
+	client: Arc<FullClient>,
+}
+
+impl BenchmarkExtrinsicBuilder {
+	/// Creates a new [`Self`] from the given client.
+	pub fn new(client: Arc<FullClient>) -> Self {
+		Self { client }
+	}
+}
+
+impl frame_benchmarking_cli::ExtrinsicBuilder for BenchmarkExtrinsicBuilder {
+	fn remark(&self, nonce: u32) -> std::result::Result<OpaqueExtrinsic, &'static str> {
+		let acc = Sr25519Keyring::Bob.pair();
+		let extrinsic: OpaqueExtrinsic = create_benchmark_extrinsic(
+			self.client.as_ref(),
+			acc,
+			SystemCall::remark { remark: vec![] }.into(),
+			nonce,
+		)
+		.into();
+
+		Ok(extrinsic)
+	}
+}
+
+/// Create a transaction using the given `call`.
+///
+/// Note: Should only be used for benchmarking.
+pub fn create_benchmark_extrinsic(
+	client: &FullClient,
+	sender: sp_core::sr25519::Pair,
+	call: runtime::Call,
+	nonce: u32,
+) -> runtime::UncheckedExtrinsic {
+	let genesis_hash = client.block_hash(0).ok().flatten().expect("Genesis block exists; qed");
+	let best_hash = client.chain_info().best_hash;
+	let best_block = client.chain_info().best_number;
+
+	let period = runtime::BlockHashCount::get()
+		.checked_next_power_of_two()
+		.map(|c| c / 2)
+		.unwrap_or(2) as u64;
+	let extra: runtime::SignedExtra = (
+		frame_system::CheckNonZeroSender::<runtime::Runtime>::new(),
+		frame_system::CheckSpecVersion::<runtime::Runtime>::new(),
+		frame_system::CheckTxVersion::<runtime::Runtime>::new(),
+		frame_system::CheckGenesis::<runtime::Runtime>::new(),
+		frame_system::CheckEra::<runtime::Runtime>::from(sp_runtime::generic::Era::mortal(
+			period,
+			best_block.saturated_into(),
+		)),
+		frame_system::CheckNonce::<runtime::Runtime>::from(nonce),
+		frame_system::CheckWeight::<runtime::Runtime>::new(),
+		pallet_transaction_payment::ChargeTransactionPayment::<runtime::Runtime>::from(0),
+	);
+
+	let raw_payload = runtime::SignedPayload::from_raw(
+		call.clone(),
+		extra.clone(),
+		(
+			(),
+			runtime::VERSION.spec_version,
+			runtime::VERSION.transaction_version,
+			genesis_hash,
+			best_hash,
+			(),
+			(),
+			(),
+		),
+	);
+	let signature = raw_payload.using_encoded(|e| sender.sign(e));
+
+	runtime::UncheckedExtrinsic::new_signed(
+		call.clone(),
+		sp_runtime::AccountId32::from(sender.public()).into(),
+		runtime::Signature::Sr25519(signature.clone()),
+		extra.clone(),
+	)
+}
+
+/// Generates inherent data for the `benchmark overhead` command.
+///
+/// Note: Should only be used for benchmarking.
+pub fn inherent_benchmark_data() -> Result<InherentData> {
+	let mut inherent_data = InherentData::new();
+	let d = Duration::from_millis(0);
+	let timestamp = sp_timestamp::InherentDataProvider::new(d.into());
+
+	timestamp
+		.provide_inherent_data(&mut inherent_data)
+		.map_err(|e| format!("creating inherent data: {:?}", e))?;
+	Ok(inherent_data)
+}

substrate/bin/node-template/node/src/main.rs

@@ -6,6 +6,7 @@ mod chain_spec;
 mod service;
 mod cli;
 mod command;
+mod command_helper;
 mod rpc;
 
 fn main() -> sc_cli::Result<()> {

substrate/bin/node-template/node/src/service.rs

@@ -31,7 +31,7 @@ impl sc_executor::NativeExecutionDispatch for ExecutorDispatch {
 	}
 }
 
-type FullClient =
+pub(crate) type FullClient =
 	sc_service::TFullClient<Block, RuntimeApi, NativeElseWasmExecutor<ExecutorDispatch>>;
 type FullBackend = sc_service::TFullBackend<Block>;
 type FullSelectChain = sc_consensus::LongestChain<FullBackend, Block>;

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

@@ -33,6 +33,7 @@ pub use frame_support::{
 	},
 	StorageValue,
 };
+pub use frame_system::Call as SystemCall;
 pub use pallet_balances::Call as BalancesCall;
 pub use pallet_timestamp::Call as TimestampCall;
 use pallet_transaction_payment::CurrencyAdapter;
@@ -306,6 +307,8 @@ pub type SignedExtra = (
 );
 /// Unchecked extrinsic type as expected by this runtime.
 pub type UncheckedExtrinsic = generic::UncheckedExtrinsic<Address, Call, Signature, SignedExtra>;
+/// The payload being signed in transactions.
+pub type SignedPayload = generic::SignedPayload<Call, SignedExtra>;
 /// Executive: handles dispatch to the various modules.
 pub type Executive = frame_executive::Executive<
 	Runtime,

substrate/bin/node/cli/src/cli.rs

@@ -38,28 +38,11 @@ pub enum Subcommand {
 	)]
 	Inspect(node_inspect::cli::InspectCmd),
 
-	/// The custom benchmark subcommmand benchmarking runtime pallets.
-	#[clap(name = "benchmark", about = "Benchmark runtime pallets.")]
+	/// Sub-commands concerned with benchmarking.
+	/// The pallet benchmarking moved to the `pallet` sub-command.
+	#[clap(subcommand)]
 	Benchmark(frame_benchmarking_cli::BenchmarkCmd),
 
-	/// Benchmark the execution time of historic blocks and compare it to their consumed weight.
-	#[clap(
-		name = "benchmark-block",
-		about = "Benchmark the execution time of historic blocks and compare it to their consumed weight."
-	)]
-	BenchmarkBlock(frame_benchmarking_cli::BlockCmd),
-
-	/// Sub command for benchmarking the per-block and per-extrinsic execution overhead.
-	#[clap(
-		name = "benchmark-overhead",
-		about = "Benchmark the per-block and per-extrinsic execution overhead."
-	)]
-	BenchmarkOverhead(frame_benchmarking_cli::OverheadCmd),
-
-	/// Sub command for benchmarking the storage speed.
-	#[clap(name = "benchmark-storage", about = "Benchmark storage speed.")]
-	BenchmarkStorage(frame_benchmarking_cli::StorageCmd),
-
 	/// Try some command against runtime state.
 	#[cfg(feature = "try-runtime")]
 	TryRuntime(try_runtime_cli::TryRuntimeCmd),

substrate/bin/node/cli/src/command.rs

@@ -16,11 +16,13 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
+use super::command_helper::{inherent_benchmark_data, BenchmarkExtrinsicBuilder};
 use crate::{
 	chain_spec, service,
 	service::{new_partial, FullClient},
 	Cli, Subcommand,
 };
+use frame_benchmarking_cli::*;
 use node_executor::ExecutorDispatch;
 use node_primitives::Block;
 use node_runtime::RuntimeApi;
@@ -92,45 +94,39 @@ pub fn run() -> Result<()> {
 
 			runner.sync_run(|config| cmd.run::<Block, RuntimeApi, ExecutorDispatch>(config))
 		},
-		Some(Subcommand::Benchmark(cmd)) =>
-			if cfg!(feature = "runtime-benchmarks") {
-				let runner = cli.create_runner(cmd)?;
-
-				runner.sync_run(|config| cmd.run::<Block, ExecutorDispatch>(config))
-			} else {
-				Err("Benchmarking wasn't enabled when building the node. \
-				You can enable it with `--features runtime-benchmarks`."
-					.into())
-			},
-		Some(Subcommand::BenchmarkBlock(cmd)) => {
+		Some(Subcommand::Benchmark(cmd)) => {
 			let runner = cli.create_runner(cmd)?;
-			runner.async_run(|config| {
-				let PartialComponents { client, task_manager, .. } = new_partial(&config)?;
-				Ok((cmd.run(client), task_manager))
-			})
-		},
-		Some(Subcommand::BenchmarkOverhead(cmd)) => {
-			let runner = cli.create_runner(cmd)?;
-			runner.async_run(|mut config| {
-				use super::command_helper::{inherent_data, ExtrinsicBuilder};
-				// We don't use the authority role since that would start producing blocks
-				// in the background which would mess with our benchmark.
-				config.role = sc_service::Role::Full;
 
-				let PartialComponents { client, task_manager, .. } = new_partial(&config)?;
-				let ext_builder = ExtrinsicBuilder::new(client.clone());
+			runner.sync_run(|config| {
+				let PartialComponents { client, backend, .. } = new_partial(&config)?;
 
-				Ok((cmd.run(config, client, inherent_data()?, Arc::new(ext_builder)), task_manager))
-			})
-		},
-		Some(Subcommand::BenchmarkStorage(cmd)) => {
-			let runner = cli.create_runner(cmd)?;
-			runner.async_run(|config| {
-				let PartialComponents { client, task_manager, backend, .. } = new_partial(&config)?;
-				let db = backend.expose_db();
-				let storage = backend.expose_storage();
+				// This switch needs to be in the client, since the client decides
+				// which sub-commands it wants to support.
+				match cmd {
+					BenchmarkCmd::Pallet(cmd) => {
+						if !cfg!(feature = "runtime-benchmarks") {
+							return Err(
+								"Runtime benchmarking wasn't enabled when building the node. \
+							You can enable it with `--features runtime-benchmarks`."
+									.into(),
+							)
+						}
 
-				Ok((cmd.run(config, client, db, storage), task_manager))
+						cmd.run::<Block, ExecutorDispatch>(config)
+					},
+					BenchmarkCmd::Block(cmd) => cmd.run(client),
+					BenchmarkCmd::Storage(cmd) => {
+						let db = backend.expose_db();
+						let storage = backend.expose_storage();
+
+						cmd.run(config, client, db, storage)
+					},
+					BenchmarkCmd::Overhead(cmd) => {
+						let ext_builder = BenchmarkExtrinsicBuilder::new(client.clone());
+
+						cmd.run(config, client, inherent_benchmark_data()?, Arc::new(ext_builder))
+					},
+				}
 			})
 		},
 		Some(Subcommand::Key(cmd)) => cmd.run(&cli),

substrate/bin/node/cli/src/command_helper.rs

@@ -29,19 +29,19 @@ use sp_runtime::OpaqueExtrinsic;
 
 use std::{sync::Arc, time::Duration};
 
-/// Generates extrinsics for the `benchmark-overhead` command.
-pub struct ExtrinsicBuilder {
+/// Generates extrinsics for the `benchmark overhead` command.
+pub struct BenchmarkExtrinsicBuilder {
 	client: Arc<FullClient>,
 }
 
-impl ExtrinsicBuilder {
+impl BenchmarkExtrinsicBuilder {
 	/// Creates a new [`Self`] from the given client.
 	pub fn new(client: Arc<FullClient>) -> Self {
 		Self { client }
 	}
 }
 
-impl frame_benchmarking_cli::ExtrinsicBuilder for ExtrinsicBuilder {
+impl frame_benchmarking_cli::ExtrinsicBuilder for BenchmarkExtrinsicBuilder {
 	fn remark(&self, nonce: u32) -> std::result::Result<OpaqueExtrinsic, &'static str> {
 		let acc = Sr25519Keyring::Bob.pair();
 		let extrinsic: OpaqueExtrinsic = create_extrinsic(
@@ -56,8 +56,8 @@ impl frame_benchmarking_cli::ExtrinsicBuilder for ExtrinsicBuilder {
 	}
 }
 
-/// Generates inherent data for the `benchmark-overhead` command.
-pub fn inherent_data() -> Result<InherentData> {
+/// Generates inherent data for the `benchmark overhead` command.
+pub fn inherent_benchmark_data() -> Result<InherentData> {
 	let mut inherent_data = InherentData::new();
 	let d = Duration::from_millis(0);
 	let timestamp = sp_timestamp::InherentDataProvider::new(d.into());

substrate/bin/node/cli/tests/benchmark_block_works.rs

@@ -0,0 +1,48 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2022 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
+
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+// Unix only since it uses signals from [`common::run_node_for_a_while`].
+#![cfg(unix)]
+
+use assert_cmd::cargo::cargo_bin;
+use std::process::Command;
+use tempfile::tempdir;
+
+pub mod common;
+
+/// `benchmark block` works for the dev runtime using the wasm executor.
+#[tokio::test]
+async fn benchmark_block_works() {
+	let base_dir = tempdir().expect("could not create a temp dir");
+
+	common::run_node_for_a_while(base_dir.path(), &["--dev"]).await;
+
+	// Invoke `benchmark block` with all options to make sure that they are valid.
+	let status = Command::new(cargo_bin("substrate"))
+		.args(["benchmark", "block", "--dev"])
+		.arg("-d")
+		.arg(base_dir.path())
+		.args(["--pruning", "archive"])
+		.args(["--from", "1", "--to", "1"])
+		.args(["--repeat", "1"])
+		.args(["--execution", "wasm", "--wasm-execution", "compiled"])
+		.status()
+		.unwrap();
+
+	assert!(status.success())
+}

substrate/bin/node/cli/tests/benchmark_overhead_works.rs

@@ -20,7 +20,7 @@ use assert_cmd::cargo::cargo_bin;
 use std::process::Command;
 use tempfile::tempdir;
 
-/// Tests that the `benchmark-overhead` command works for the substrate dev runtime.
+/// Tests that the `benchmark overhead` command works for the substrate dev runtime.
 #[test]
 fn benchmark_overhead_works() {
 	let tmp_dir = tempdir().expect("could not create a temp dir");
@@ -29,7 +29,7 @@ fn benchmark_overhead_works() {
 	// Only put 10 extrinsics into the block otherwise it takes forever to build it
 	// especially for a non-release build.
 	let status = Command::new(cargo_bin("substrate"))
-		.args(&["benchmark-overhead", "--dev", "-d"])
+		.args(&["benchmark", "overhead", "--dev", "-d"])
 		.arg(base_path)
 		.arg("--weight-path")
 		.arg(base_path)

substrate/bin/node/cli/tests/benchmark_storage_works.rs

@@ -23,7 +23,7 @@ use std::{
 };
 use tempfile::tempdir;
 
-/// Tests that the `benchmark-storage` command works for the dev runtime.
+/// Tests that the `benchmark storage` command works for the dev runtime.
 #[test]
 fn benchmark_storage_works() {
 	let tmp_dir = tempdir().expect("could not create a temp dir");
@@ -39,7 +39,7 @@ fn benchmark_storage_works() {
 
 fn benchmark_storage(db: &str, base_path: &Path) -> ExitStatus {
 	Command::new(cargo_bin("substrate"))
-		.args(&["benchmark-storage", "--dev"])
+		.args(&["benchmark", "storage", "--dev"])
 		.arg("--db")
 		.arg(db)
 		.arg("--weight-path")