Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

☭ Liberate workers! ☭ #7570

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
26 changes: 21 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 4 additions & 12 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,6 @@
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."
Expand Down Expand Up @@ -38,24 +30,24 @@ 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"] }
tempfile = "3.2.0"
tokio = "1.24.2"
substrate-rpc-client = { git = "https://github.com/paritytech/substrate", branch = "master" }
polkadot-core-primitives = { path = "core-primitives" }
polkadot-execute-worker = { path = "bin/execute-worker" }
polkadot-prepare-worker = { path = "bin/prepare-worker" }

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }

[workspace]
members = [
"cli",
"bin/execute-worker",
"bin/prepare-worker",
"core-primitives",
"erasure-coding",
"erasure-coding/fuzzer",
Expand Down
16 changes: 16 additions & 0 deletions bin/execute-worker/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[package]
name = "polkadot-execute-worker"
version.workspace = true
authors.workspace = true
edition.workspace = true

[[bin]]
name = "polkadot-execute-worker"
path = "src/main.rs"

[dependencies]
polkadot-node-core-pvf-common = { path = "../../node/core/pvf/common" }
polkadot-node-core-pvf-execute-worker = { path = "../../node/core/pvf/execute-worker" }

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
22 changes: 22 additions & 0 deletions bin/execute-worker/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

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();
}
18 changes: 18 additions & 0 deletions bin/execute-worker/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

/// Execute worker binary name
pub const BINARY_NAME: &str = "polkadot-execute-worker";
33 changes: 33 additions & 0 deletions bin/execute-worker/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Execute worker.

#[cfg(test)]
use polkadot_execute_worker::BINARY_NAME;

polkadot_node_core_pvf_common::decl_worker_main!(
"execute-worker",
polkadot_node_core_pvf_execute_worker::worker_entrypoint,
env!("SUBSTRATE_CLI_IMPL_VERSION")
);

#[test]
fn name_test() {
// If binary name ever needs to change, it must be changed in the package manifest
// as well as in the constant value
assert_eq!(env!("CARGO_BIN_NAME"), BINARY_NAME);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Execute worker.
//! Dummy integration test needed for the binary to get build when other intergation
//! tests are run

polkadot_node_core_pvf_common::decl_worker_main!(
"execute-worker",
polkadot_node_core_pvf_execute_worker::worker_entrypoint,
env!("SUBSTRATE_CLI_IMPL_VERSION")
);
#[test]
fn dummy_test() {
assert!(true);
}
16 changes: 16 additions & 0 deletions bin/prepare-worker/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[package]
name = "polkadot-prepare-worker"
version.workspace = true
authors.workspace = true
edition.workspace = true

[[bin]]
name = "polkadot-prepare-worker"
path = "src/main.rs"

[dependencies]
polkadot-node-core-pvf-common = { path = "../../node/core/pvf/common" }
polkadot-node-core-pvf-prepare-worker = { path = "../../node/core/pvf/prepare-worker" }

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
22 changes: 22 additions & 0 deletions bin/prepare-worker/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

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();
}
18 changes: 18 additions & 0 deletions bin/prepare-worker/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

/// Prepare worker binary name
pub const BINARY_NAME: &str = "polkadot-prepare-worker";
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same as PREPARE_BINARY_NAME in pvf-host. Can we move it to the pvf/prepare-worker crate, and use that here, for less duplication?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm there is an issue with the PVF host depending on the worker binaries in this way. The worker binaries will be recompiled after every commit which will be annoying when hacking on pvf crate. I think we can move the binary names into pvf-common.

33 changes: 33 additions & 0 deletions bin/prepare-worker/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot 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.

// Polkadot 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 Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Prepare worker.

#[cfg(test)]
use polkadot_prepare_worker::BINARY_NAME;

polkadot_node_core_pvf_common::decl_worker_main!(
"prepare-worker",
polkadot_node_core_pvf_prepare_worker::worker_entrypoint,
env!("SUBSTRATE_CLI_IMPL_VERSION")
);

#[test]
fn name_test() {
// If binary name ever needs to change, it must be changed in the package manifest
// as well as in the constant value
assert_eq!(env!("CARGO_BIN_NAME"), BINARY_NAME);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Prepare worker.
//! Dummy integration test needed for the binary to get build when other intergation
//! tests are run

polkadot_node_core_pvf_common::decl_worker_main!(
"prepare-worker",
polkadot_node_core_pvf_prepare_worker::worker_entrypoint,
env!("SUBSTRATE_CLI_IMPL_VERSION")
);
#[test]
fn dummy_test() {
assert!(true);
}
52 changes: 52 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,59 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

//! Build script to ensure that PVF worker binaries are always built whenever polkadot is built.
//!
//! This is needed because `default-members` does the same thing, but only for `cargo build` -- it
//! does not work for `cargo run`.

use std::{env::var, path::Path, process::Command};

use polkadot_node_core_pvf::{EXECUTE_BINARY_NAME, PREPARE_BINARY_NAME};

// TODO: unskip
#[rustfmt::skip]
fn main() {
// Build additional artifacts if a single package build is explicitly requested.
{
let cargo = dbg!(var("CARGO").expect("`CARGO` env variable is always set by cargo"));
let target = dbg!(var("TARGET").expect("`TARGET` env variable is always set by cargo"));
let profile = dbg!(var("PROFILE").expect("`PROFILE` env variable is always set by cargo"));
let out_dir = dbg!(var("OUT_DIR").expect("`OUT_DIR` env variable is always set by cargo"));
let target_dir = format!("{}/workers", out_dir);

// assert!(false);

// TODO: opt-level, debug, features, etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this build.rs script to fix the issue where cargo run does not build the worker binaries. This is still WIP but it needs to get out soon, ideally before the next release. I didn't open a separate PR but tagged this onto yours because it depends on the workers being their own crates.

But how do we forward everything from the cargo command that was called. For features we will have to iterate over all env vars to get them and pass them on. For things like opt-level, lto etc. (there are more) I don't see how to pass them on, but I think the child cargo process inherits it from env vars, but am not sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered that we eventually want to build these workers with the musl target, so we'll need this script anyways.

Also, I realized we don't need support for features, the workers don't use any.

Copy link
Contributor

@mrcnski mrcnski Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into whether profile settings are passed along, and they are! Env vars like CARGO_CFG_OVERFLOW_CHECKS (see https://doc.rust-lang.org/cargo/reference/environment-variables.html#configuration-environment-variables) are passed along, confirmed by logging all env vars from execute-worker's build script. So this
script is basically good-to-go, modulo a couple of small TODOs.

Actually they are not. I ran --profile=testnet, and CARGO_CFG_OVERFLOW_CHECKS and CARGO_CFG_PANIC were set correctly (I guess they are inherited from release), but DEBUG was not set.

We should be able to forward them with rustc -C flags. See here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should undo the change removing default-members. I thought it wouldn't be necessary anymore with the build script, but I realized that cargo install will not work for the worker binaries without it.

Copy link
Contributor

@mrcnski mrcnski Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can experiment with moving the build script to cli crate or something, that way it will also work for malus which depends on that crate. Might make things cleaner. But since we should get this PR in ASAP (hard deadline by Aug 18 before the switch to monorepo), it can be a followup.

Edit: everything in above comments has been done except this one!

let mut args = vec![
"build",
"-p",
EXECUTE_BINARY_NAME,
"-p",
PREPARE_BINARY_NAME,
"--target",
&target,
"--target-dir",
&target_dir,
];
if profile != "debug" {
args.push("--profile");
args.push(&profile);
}

Command::new(cargo).args(&args).status().unwrap();
std::fs::rename(
Path::new(&format!("{target_dir}/{target}/{profile}/{EXECUTE_BINARY_NAME}")),
Path::new(&format!("{target_dir}/../../../../{EXECUTE_BINARY_NAME}")),
)
.unwrap();
std::fs::rename(
Path::new(&format!("{target_dir}/{target}/{profile}/{PREPARE_BINARY_NAME}")),
Path::new(&format!("{target_dir}/../../../../{PREPARE_BINARY_NAME}")),
)
.unwrap();
}

// TODO: is this needed here?
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.
Expand Down
3 changes: 3 additions & 0 deletions node/core/pvf/prepare-worker/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ sp-io = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-maybe-compressed-blob = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" }

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }

[target.'cfg(target_os = "linux")'.dependencies]
tikv-jemalloc-ctl = "0.5.0"

Expand Down
Loading