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
+401 −73
Open

☭ Liberate workers! ☭ #7570

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cc8e1fd
Move PVF worker CLI binaries to separate packages
s0me0ne-unkn0wn Aug 1, 2023
3c43f00
Move worker CLIs closer to the root package
s0me0ne-unkn0wn Aug 2, 2023
25f950c
Rename crates
s0me0ne-unkn0wn Aug 2, 2023
53d035d
Fix malus
s0me0ne-unkn0wn Aug 2, 2023
5ae83f3
Fix tests
s0me0ne-unkn0wn Aug 8, 2023
c9329ca
`Cargo.lock` & `cargo fmt`
s0me0ne-unkn0wn Aug 8, 2023
dcbd057
Merge remote-tracking branch 'origin/master' into s0me0ne/liberate-wo…
s0me0ne-unkn0wn Aug 8, 2023
8d50a4c
Try to fix `cargo run` not building worker binaries
mrcnski Aug 9, 2023
3ab5657
Resolve some TODOs, make output nicer
mrcnski Aug 9, 2023
96d2a90
Fix filename typo
s0me0ne-unkn0wn Aug 10, 2023
63ee515
Remove duplicated constants
s0me0ne-unkn0wn Aug 10, 2023
c172aab
`cargo fmt`
s0me0ne-unkn0wn Aug 10, 2023
85f0a50
Fix build script
mrcnski Aug 11, 2023
c4d8844
Update build script
mrcnski Aug 11, 2023
a8c98c1
Update comment
mrcnski Aug 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions Cargo.lock
View file
Open in desktop

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

20 changes: 8 additions & 12 deletions Cargo.toml
View file
Open in desktop
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 @@ -37,10 +29,6 @@ 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"] }
Expand All @@ -55,6 +43,8 @@ substrate-build-script-utils = { git = "https://github.com/paritytech/substrate"
[workspace]
members = [
"cli",
"bin/execute-worker",
"bin/prepare-worker",
"core-primitives",
"erasure-coding",
"erasure-coding/fuzzer",
Expand Down Expand Up @@ -141,6 +131,12 @@ members = [
"utils/generate-bags",
]

default-members = [
".",
"bin/execute-worker",
"bin/prepare-worker",
]

[badges]
maintenance = { status = "actively-developed" }

Expand Down
16 changes: 16 additions & 0 deletions bin/execute-worker/Cargo.toml
View file
Open in desktop
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
View file
Open in desktop
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();
}
0 src/bin/execute-worker.rs → bin/execute-worker/src/main.rs
View file
Open in desktop
File renamed without changes.
16 changes: 16 additions & 0 deletions bin/prepare-worker/Cargo.toml
View file
Open in desktop
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
View file
Open in desktop
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();
}
0 src/bin/prepare-worker.rs → bin/prepare-worker/src/main.rs
View file
Open in desktop
File renamed without changes.
3 changes: 3 additions & 0 deletions node/core/pvf/prepare-worker/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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
18 changes: 0 additions & 18 deletions node/malus/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,6 @@ publish = false
name = "malus"
path = "src/malus.rs"

# Use artifact dependencies once stable.
# See https://github.com/rust-lang/cargo/issues/9096.
[[bin]]
name = "polkadot-execute-worker"
path = "../../src/bin/execute-worker.rs"
# Prevent rustdoc error. Already documented from top-level Cargo.toml.
doc = false
[[bin]]
name = "polkadot-prepare-worker"
path = "../../src/bin/prepare-worker.rs"
# Prevent rustdoc error. Already documented from top-level Cargo.toml.
doc = false

[dependencies]
polkadot-cli = { path = "../../cli", features = [ "malus", "rococo-native", "kusama-native", "westend-native", "polkadot-native" ] }
polkadot-node-subsystem = { path = "../subsystem" }
Expand All @@ -47,11 +34,6 @@ gum = { package = "tracing-gum", path = "../gum/" }
erasure = { package = "polkadot-erasure-coding", path = "../../erasure-coding" }
rand = "0.8.5"

# Required for worker binaries to build.
polkadot-node-core-pvf-common = { path = "../core/pvf/common" }
polkadot-node-core-pvf-execute-worker = { path = "../core/pvf/execute-worker" }
polkadot-node-core-pvf-prepare-worker = { path = "../core/pvf/prepare-worker" }

[dev-dependencies]
polkadot-node-subsystem-test-helpers = { path = "../subsystem-test-helpers" }
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
Expand Down
2 changes: 1 addition & 1 deletion scripts/ci/gitlab/pipeline/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ build-malus:
- .compiler-info
- .collect-artifacts
script:
- time cargo build --locked --profile testnet --verbose -p polkadot-test-malus
- time cargo build --locked --profile testnet --verbose -p polkadot-test-malus -p polkadot-execute-worker -p polkadot-prepare-worker
Copy link
Contributor Author

@s0me0ne-unkn0wn s0me0ne-unkn0wn Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried different approaches and ended up explicitly building workers for malus.

This one even works but has some flaws and looks too hacky to end up in the production code.

Any better ideas are appreciated.

Copy link
Contributor

@mrcnski mrcnski Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to have a separate workspace for malus, then build it the exact same way we do polkadot? i.e. if we list the workers as default-members, we don't have to specify them with -p for each build command.

I think this would work if the polkadot workspace didn't list malus, and the malus Cargo.toml depended on polkadot and listed the workers as workspace members and default-members. Worth a try at least.

This one even works but has some flaws and looks too hacky to end up in the production code.

Yeah that looks too hacky to me, but -p .. is fine if nothing else works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to have a separate workspace for malus

Unfortunately, no, nested workspaces are proposed but not supported yet. I went through a lot of interesting proposals while I was researching possible solutions, like intersecting workspaces and binary dependencies, but neither of them is implemented now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went through a lot of interesting proposals while I was researching possible solutions ... but neither of them is implemented now.

Yeah I know the feeling. I think -p .. is fine, maybe just update the readme with build instructions like we did for node metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found out that cargo test --workspace --profile testnet triggers building some auxiliary binaries like adder-collator and undying, which are required for tests residing in tests/. But I still need to understand what mechanism triggers those builds. Probably it can be used for malus as well.

# pack artifacts
- mkdir -p ./artifacts
- mv ./target/testnet/malus ./artifacts/.
Expand Down
4 changes: 2 additions & 2 deletions tests/workers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
use polkadot_cli::NODE_VERSION;
use std::process::Command;

const PREPARE_WORKER_EXE: &str = env!("CARGO_BIN_EXE_polkadot-prepare-worker");
const EXECUTE_WORKER_EXE: &str = env!("CARGO_BIN_EXE_polkadot-execute-worker");
const PREPARE_WORKER_EXE: &str = "polkadot-prepare-worker";
const EXECUTE_WORKER_EXE: &str = "polkadot-execute-worker";

#[test]
fn worker_binaries_have_same_version_as_node() {
Expand Down