98.6% OF DEVELOPERS CANNOT REVIEW THIS PR! [read more...] (#7337)

* [WIP] PVF: Split out worker binaries

* Address compilation problems and re-design a bit

* Reorganize once more, fix tests

* Reformat with new nightly to make `cargo fmt` test happy

* Address `clippy` warnings

* Add temporary trace to debug zombienet tests

* Fix zombienet node upgrade test

* Fix malus and its CI

* Fix building worker binaries with malus

* More fixes for malus

* Remove unneeded cli subcommands

* Support placing auxiliary binaries to `/usr/libexec`

* Fix spelling

* Spelling

Co-authored-by: Marcin S. <[email protected]>

* Implement review comments (mostly nits)

* Fix worker node version flag

* Rework getting the worker paths

* Address a couple of review comments

* Minor restructuring

* Fix CI error

* Add tests for worker binaries detection

* Improve tests; try to fix CI

* Move workers module into separate file

* Try to fix failing test and workers not printing latest version

- Tests were not finding the worker binaries
- Workers were not being rebuilt when the version changed
- Made some errors easier to read

* Make a bunch of fixes

* Rebuild nodes on version change

* Fix more issues

* Fix tests

* Pass node version from node into dependencies to avoid recompiles

- [X] get version in CLI
- [X] pass it in to service
- [X] pass version along to PVF
- [X] remove rerun from service
- [X] add rerun to CLI

- [X] don’t rerun pvf/worker’s (these should be built by nodes which have rerun enabled)

* Some more improvements for smoother tests

- [X] Fix tests
- [X] Make puppet workers pass None for version and remove rerun
- [X] Make test collators self-contained

* Add back rerun to PVF workers

* Move worker binaries into files in cli crate

As a final optimization I've separated out each worker binary from its own crate
into the CLI crate. Before, the worker bin shared a crate with the worker lib,
so when the binaries got recompiled so did the libs and everything transitively
depending on the libs. This commit fixes this regression that was causing
recompiles after every commit.

* Fix bug (was passing worker version for node version)

* Move workers out of cli into root src/bin/ dir

- [X] Pass in node version from top-level (polkadot)
- [X] Add build.rs with rerun-git-head to root dir

* Add some sanity checks for workers to dockerfiles

* Update malus

  + [X] Make it self-contained
  + [X] Undo multiple binary changes

* Try to fix clippy errors

* Address `cargo run` issue

- [X] Add default-run for polkadot
- [X] Add note about installation to error

* Update readme (installation instructions)

* Allow disabling external workers for local/testing setups

  + [X] cli flag to enable single-binary mode
  + [X] Add message to error

* Revert unnecessary Cargo.lock changes

* Remove unnecessary build scripts from collators

* Add back missing malus commands (should fix failing ZN job)

* Some minor fixes

* Update Cargo.lock

* Fix some build errors

* Undo self-contained binaries; cli flag to disable version check

  + [X] Remove --dont-run-external-workers
  + [X] Add --disable-worker-version-check
  + [X] Remove PVF subcommands
  + [X] Redo malus changes

* Try to fix failing job and add some docs for local tests

---------

Co-authored-by: Dmitry Sinyavin <[email protected]>
Co-authored-by: s0me0ne-unkn0wn <[email protected]>
Co-authored-by: parity-processbot <>

Author: mrcnski

Cargo.lock

@@ -2,12 +2,21 @@
 name = "polkadot"
 path = "src/main.rs"
 
+[[bin]]
+name = "polkadot-execute-worker"
+path = "src/bin/execute-worker.rs"
+
+[[bin]]
+name = "polkadot-prepare-worker"
+path = "src/bin/prepare-worker.rs"
+
 [package]
 name = "polkadot"
 description = "Implementation of a `https://polkadot.network` node in Rust based on the Substrate framework."
 license = "GPL-3.0-only"
 rust-version = "1.64.0" # workspace properties
 readme = "README.md"
+default-run = "polkadot"
 authors.workspace = true
 edition.workspace = true
 version.workspace = true
@@ -28,6 +37,10 @@ polkadot-node-core-pvf = { path = "node/core/pvf" }
 polkadot-node-core-pvf-prepare-worker = { path = "node/core/pvf/prepare-worker" }
 polkadot-overseer = { path = "node/overseer" }
 
+# Needed for worker binaries.
+polkadot-node-core-pvf-common = { path = "node/core/pvf/common" }
+polkadot-node-core-pvf-execute-worker = { path = "node/core/pvf/execute-worker" }
+
 [dev-dependencies]
 assert_cmd = "2.0.4"
 nix = { version = "0.26.1", features = ["signal"] }
@@ -36,6 +49,9 @@ tokio = "1.24.2"
 substrate-rpc-client = { git = "https://github.com/paritytech/substrate", branch = "master" }
 polkadot-core-primitives = { path = "core-primitives" }
 
+[build-dependencies]
+substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
+
 [workspace]
 members = [
 	"cli",
@@ -226,6 +242,8 @@ license-file = ["LICENSE", "0"]
 maintainer-scripts = "scripts/packaging/deb-maintainer-scripts"
 assets = [
 	["target/release/polkadot", "/usr/bin/", "755"],
+	["target/release/polkadot-prepare-worker", "/usr/lib/polkadot/", "755"],
+	["target/release/polkadot-execute-worker", "/usr/lib/polkadot/", "755"],
 	["scripts/packaging/polkadot.service", "/lib/systemd/system/", "644"]
 ]
 conf-files = [

Cargo.toml

@@ -91,7 +91,9 @@ git checkout <latest tagged release>
 cargo build --release
 ```
 
-Note that compilation is a memory intensive process. We recommend having 4 GiB of physical RAM or swap available (keep in mind that if a build hits swap it tends to be very slow).
+**Note:** compilation is a memory intensive process. We recommend having 4 GiB of physical RAM or swap available (keep in mind that if a build hits swap it tends to be very slow).
+
+**Note:** if you want to move the built `polkadot` binary somewhere (e.g. into $PATH) you will also need to move `polkadot-execute-worker` and `polkadot-prepare-worker`. You can let cargo do all this for you by running `cargo install --path .`.
 
 #### Build from Source with Docker
 

README.md

@@ -16,4 +16,7 @@
 
 fn main() {
 	substrate_build_script_utils::generate_cargo_keys();
+	// For the node/worker version check, make sure we always rebuild the node and binary workers
+	// when the version changes.
+	substrate_build_script_utils::rerun_if_git_head_changed();
 }

node/core/pvf/build.rs build.rsnode/core/pvf/build.rs renamed to build.rs

@@ -22,8 +22,6 @@ pyro = { package = "pyroscope", version = "0.5.3", optional = true }
 pyroscope_pprofrs = { version = "0.2", optional = true }
 
 service = { package = "polkadot-service", path = "../node/service", default-features = false, optional = true }
-polkadot-node-core-pvf-execute-worker = { path = "../node/core/pvf/execute-worker", optional = true }
-polkadot-node-core-pvf-prepare-worker = { path = "../node/core/pvf/prepare-worker", optional = true }
 polkadot-performance-test = { path = "../node/test/performance-test", optional = true }
 
 sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
@@ -53,8 +51,6 @@ cli = [
 	"sc-tracing",
 	"frame-benchmarking-cli",
 	"try-runtime-cli",
-	"polkadot-node-core-pvf-execute-worker",
-	"polkadot-node-core-pvf-prepare-worker",
 	"service",
 ]
 runtime-benchmarks = [

cli/Cargo.toml

@@ -19,4 +19,7 @@ fn main() {
 		println!("cargo:rustc-cfg=build_type=\"{}\"", profile);
 	}
 	substrate_build_script_utils::generate_cargo_keys();
+	// For the node/worker version check, make sure we always rebuild the node when the version
+	// changes.
+	substrate_build_script_utils::rerun_if_git_head_changed();
 }

cli/build.rs

@@ -17,6 +17,10 @@
 //! Polkadot CLI library.
 
 use clap::Parser;
+use std::path::PathBuf;
+
+/// The version of the node. The passed-in version of the workers should match this.
+pub const NODE_VERSION: &'static str = env!("SUBSTRATE_CLI_IMPL_VERSION");
 
 #[allow(missing_docs)]
 #[derive(Debug, Parser)]
@@ -42,14 +46,6 @@ pub enum Subcommand {
 	/// Revert the chain to a previous state.
 	Revert(sc_cli::RevertCmd),
 
-	#[allow(missing_docs)]
-	#[command(name = "prepare-worker", hide = true)]
-	PvfPrepareWorker(ValidationWorkerCommand),
-
-	#[allow(missing_docs)]
-	#[command(name = "execute-worker", hide = true)]
-	PvfExecuteWorker(ValidationWorkerCommand),
-
 	/// Sub-commands concerned with benchmarking.
 	/// The pallet benchmarking moved to the `pallet` sub-command.
 	#[command(subcommand)]
@@ -148,6 +144,17 @@ pub struct RunCmd {
 	/// **Dangerous!** Do not touch unless explicitly adviced to.
 	#[arg(long)]
 	pub overseer_channel_capacity_override: Option<usize>,
+
+	/// Path to the directory where auxiliary worker binaries reside. If not specified, the main
+	/// binary's directory is searched first, then `/usr/lib/polkadot` is searched. TESTING ONLY: if
+	/// the path points to an executable rather then directory, that executable is used both as
+	/// preparation and execution worker.
+	#[arg(long, value_name = "PATH")]
+	pub workers_path: Option<PathBuf>,
+
+	/// TESTING ONLY: disable the version check between nodes and workers.
+	#[arg(long, hide = true)]
+	pub disable_worker_version_check: bool,
 }
 
 #[allow(missing_docs)]

cli/src/cli.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use crate::cli::{Cli, Subcommand};
+use crate::cli::{Cli, Subcommand, NODE_VERSION};
 use frame_benchmarking_cli::{BenchmarkCmd, ExtrinsicFactory, SUBSTRATE_REFERENCE_HARDWARE};
 use futures::future::TryFutureExt;
 use log::info;
@@ -55,7 +55,7 @@ impl SubstrateCli for Cli {
 	}
 
 	fn impl_version() -> String {
-		env!("SUBSTRATE_CLI_IMPL_VERSION").into()
+		NODE_VERSION.into()
 	}
 
 	fn description() -> String {
@@ -272,6 +272,9 @@ where
 		None
 	};
 
+	let node_version =
+		if cli.run.disable_worker_version_check { None } else { Some(NODE_VERSION.to_string()) };
+
 	runner.run_node_until_exit(move |config| async move {
 		let hwbench = (!cli.run.no_hardware_benchmarks)
 			.then_some(config.database.path().map(|database_path| {
@@ -283,16 +286,23 @@ where
 		let database_source = config.database.clone();
 		let task_manager = service::build_full(
 			config,
-			service::IsCollator::No,
-			grandpa_pause,
-			enable_beefy,
-			jaeger_agent,
-			None,
-			false,
-			overseer_gen,
-			cli.run.overseer_channel_capacity_override,
-			maybe_malus_finality_delay,
-			hwbench,
+			service::NewFullParams {
+				is_collator: service::IsCollator::No,
+				grandpa_pause,
+				enable_beefy,
+				jaeger_agent,
+				telemetry_worker_handle: None,
+				node_version,
+				workers_path: cli.run.workers_path,
+				workers_names: None,
+				overseer_enable_anyways: false,
+				overseer_gen,
+				overseer_message_channel_capacity_override: cli
+					.run
+					.overseer_channel_capacity_override,
+				malus_finality_delay: maybe_malus_finality_delay,
+				hwbench,
+			},
 		)
 		.map(|full| full.task_manager)?;
 
@@ -419,50 +429,6 @@ pub fn run() -> Result<()> {
 				))
 			})?)
 		},
-		Some(Subcommand::PvfPrepareWorker(cmd)) => {
-			let mut builder = sc_cli::LoggerBuilder::new("");
-			builder.with_colors(false);
-			let _ = builder.init();
-
-			#[cfg(target_os = "android")]
-			{
-				return Err(sc_cli::Error::Input(
-					"PVF preparation workers are not supported under this platform".into(),
-				)
-				.into())
-			}
-
-			#[cfg(not(target_os = "android"))]
-			{
-				polkadot_node_core_pvf_prepare_worker::worker_entrypoint(
-					&cmd.socket_path,
-					Some(&cmd.node_impl_version),
-				);
-				Ok(())
-			}
-		},
-		Some(Subcommand::PvfExecuteWorker(cmd)) => {
-			let mut builder = sc_cli::LoggerBuilder::new("");
-			builder.with_colors(false);
-			let _ = builder.init();
-
-			#[cfg(target_os = "android")]
-			{
-				return Err(sc_cli::Error::Input(
-					"PVF execution workers are not supported under this platform".into(),
-				)
-				.into())
-			}
-
-			#[cfg(not(target_os = "android"))]
-			{
-				polkadot_node_core_pvf_execute_worker::worker_entrypoint(
-					&cmd.socket_path,
-					Some(&cmd.node_impl_version),
-				);
-				Ok(())
-			}
-		},
 		Some(Subcommand::Benchmark(cmd)) => {
 			let runner = cli.create_runner(cmd)?;
 			let chain_spec = &runner.config().chain_spec;