omni-node: fix benchmark pallet to work with --runtime (#8594)

# Description

The PR fixes the logic so that `polkadot-omni-node benchmark pallet`
subcommand pulls in the appropriate arguments, while not needing to
instatiate the full node configuration, which requires always passing
`--chain` flag.

## Integration

Developers will be able to use `polkadot-omni-node benchmark pallet`
with `--runtime` flag, same as `frame-omni-bencher`.

## Review Notes

Did a few things in this PR:
- made `polkadot-omni-node benchmark pallet` not require always the
`--chain` flag
- removed the deprecated `run` method for `benchmark pallet` from
`benchmarking-cli`.
- removed the check that disallows the chain flag from
`frame-omni-bencher` since it is allowed and usable

TODO:
- [x] a few more testing of the commands with both `--runtime` and
`--chain`.
- [x] remove `runtime-benchmarks` feature guard for benchmark cmds

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: 2 people

cumulus/polkadot-omni-node/lib/src/command.rs

@@ -227,38 +227,61 @@ where
 			})
 		},
 		Some(Subcommand::Benchmark(cmd)) => {
-			let runner = cli.create_runner(cmd)?;
-
 			// Switch on the concrete benchmark sub-command-
 			match cmd {
 				#[cfg(feature = "runtime-benchmarks")]
-				BenchmarkCmd::Pallet(cmd) => runner.sync_run(|config| {
-					cmd.run_with_spec::<HashingFor<Block<u32>>, ReclaimHostFunctions>(Some(
-						config.chain_spec,
-					))
-				}),
-				BenchmarkCmd::Block(cmd) => runner.sync_run(|config| {
-					let node = new_node_spec(
-						&config,
-						&cmd_config.runtime_resolver,
-						&cli.node_extra_args(),
-					)?;
-					node.run_benchmark_block_cmd(config, cmd)
-				}),
+				BenchmarkCmd::Pallet(cmd) => {
+					let chain = cmd
+						.shared_params
+						.chain
+						.as_ref()
+						.map(|chain| cli.load_spec(&chain))
+						.transpose()?;
+					cmd.run_with_spec::<HashingFor<Block<u32>>, ReclaimHostFunctions>(chain)
+				},
+				BenchmarkCmd::Block(cmd) => {
+					// The command needs the full node configuration because it uses the node
+					// client and the database source, which in its turn has a dependency on the
+					// chain spec, given via the `--chain` flag.
+					let runner = cli.create_runner(cmd)?;
+					runner.sync_run(|config| {
+						let node = new_node_spec(
+							&config,
+							&cmd_config.runtime_resolver,
+							&cli.node_extra_args(),
+						)?;
+						node.run_benchmark_block_cmd(config, cmd)
+					})
+				},
 				#[cfg(feature = "runtime-benchmarks")]
-				BenchmarkCmd::Storage(cmd) => runner.sync_run(|config| {
-					let node = new_node_spec(
-						&config,
-						&cmd_config.runtime_resolver,
-						&cli.node_extra_args(),
-					)?;
-					node.run_benchmark_storage_cmd(config, cmd)
-				}),
-				BenchmarkCmd::Machine(cmd) =>
-					runner.sync_run(|config| cmd.run(&config, SUBSTRATE_REFERENCE_HARDWARE.clone())),
+				BenchmarkCmd::Storage(cmd) => {
+					// The command needs the full node configuration because it uses the node
+					// client and the database API, storage and shared_trie_cache. It requires
+					// the `--chain` flag to be passed.
+					let runner = cli.create_runner(cmd)?;
+					runner.sync_run(|config| {
+						let node = new_node_spec(
+							&config,
+							&cmd_config.runtime_resolver,
+							&cli.node_extra_args(),
+						)?;
+						node.run_benchmark_storage_cmd(config, cmd)
+					})
+				},
+				BenchmarkCmd::Machine(cmd) => {
+					// The command needs the full node configuration, and implicitly a chain
+					// spec to be passed, even if it doesn't use it directly. The `--chain` flag is
+					// relevant in determining the database path, which is used for the disk
+					// benchmark.
+					//
+					// TODO: change `machine` subcommand to take instead a disk path we want to
+					// benchmark?.
+					let runner = cli.create_runner(cmd)?;
+					runner.sync_run(|config| cmd.run(&config, SUBSTRATE_REFERENCE_HARDWARE.clone()))
+				},
 				#[allow(unreachable_patterns)]
 				_ => Err("Benchmarking sub-command unsupported or compilation feature missing. \
-					Make sure to compile with --features=runtime-benchmarks \
+					Make sure to compile omni-node with --features=runtime-benchmarks \
 					to enable all supported benchmarks."
 					.into()),
 			}

prdoc/pr_8594.prdoc

@@ -0,0 +1,19 @@
+title: 'omni-node: fix `benchmark pallet` to work with `--runtime`'
+doc:
+- audience: Node Dev 
+  description: |
+    `polkadot-omni-node benchmark pallet` can use one of `--runtime` or `--chain` now, like `frame-omni-bencher` does.
+
+crates:
+- name: polkadot-omni-node-lib
+  bump: patch
+- name: frame-benchmarking-cli
+  bump: major
+- name: frame-omni-bencher
+  bump: patch
+- name: polkadot-sdk
+  bump: patch
+- name: polkadot-omni-node
+  bump: patch
+- name: polkadot-parachain-bin
+  bump: patch

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

@@ -159,21 +159,6 @@ This could mean that you either did not build the node correctly with the \
 not created by a node that was compiled with the flag";
 
 impl PalletCmd {
-	/// Runs the command and benchmarks a pallet.
-	#[deprecated(
-		note = "`run` will be removed after December 2024. Use `run_with_spec` instead or \
-	completely remove the code and use the `frame-benchmarking-cli` instead (see \
-	https://github.com/paritytech/polkadot-sdk/pull/3512)."
-	)]
-	pub fn run<Hasher, ExtraHostFunctions>(&self, config: sc_service::Configuration) -> Result<()>
-	where
-		Hasher: Hash,
-		<Hasher as Hash>::Output: DecodeWithMemTracking,
-		ExtraHostFunctions: HostFunctions,
-	{
-		self.run_with_spec::<Hasher, ExtraHostFunctions>(Some(config.chain_spec))
-	}
-
 	fn state_handler_from_cli<HF: HostFunctions>(
 		&self,
 		chain_spec_from_api: Option<Box<dyn ChainSpec>>,

substrate/utils/frame/omni-bencher/src/command.rs

@@ -134,14 +134,6 @@ impl V1SubCommand {
 		match self {
 			V1SubCommand::Benchmark(V1BenchmarkCommand { sub }) => match sub {
 				BenchmarkCmd::Pallet(pallet) => {
-					if let Some(spec) = pallet.shared_params.chain {
-						return Err(format!(
-							"Chain specs are not supported. Please remove `--chain={spec}` and use \
-				`--runtime=<PATH>` instead"
-						)
-						.into());
-					}
-
 					pallet.run_with_spec::<BlakeTwo256, HostFunctions>(None)
 				},
 				BenchmarkCmd::Overhead(overhead_cmd) =>