From a52c6660780edaf0ed1811dc5d10392d8cc2441b Mon Sep 17 00:00:00 2001 From: Andrei Eres Date: Thu, 19 Jun 2025 13:25:02 +0200 Subject: [PATCH 1/4] Refactor execute request handling --- Cargo.lock | 1 + polkadot/node/core/pvf/common/Cargo.toml | 1 + polkadot/node/core/pvf/common/src/execute.rs | 18 ++++++++-- .../node/core/pvf/execute-worker/src/lib.rs | 33 +++---------------- .../core/pvf/src/execute/worker_interface.rs | 11 ++++--- 5 files changed, 30 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1cb70f27c00e0..5bef8e8396ebb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15452,6 +15452,7 @@ dependencies = [ "libc", "nix 0.29.0", "parity-scale-codec", + "polkadot-node-primitives", "polkadot-parachain-primitives", "polkadot-primitives", "sc-executor 0.32.0", diff --git a/polkadot/node/core/pvf/common/Cargo.toml b/polkadot/node/core/pvf/common/Cargo.toml index 1606aceb58c27..afacb217dc188 100644 --- a/polkadot/node/core/pvf/common/Cargo.toml +++ b/polkadot/node/core/pvf/common/Cargo.toml @@ -21,6 +21,7 @@ thiserror = { workspace = true } codec = { features = ["derive"], workspace = true } +polkadot-node-primitives = { workspace = true, default-features = true } polkadot-parachain-primitives = { workspace = true, default-features = true } polkadot-primitives = { workspace = true, default-features = true } diff --git a/polkadot/node/core/pvf/common/src/execute.rs b/polkadot/node/core/pvf/common/src/execute.rs index cf5ae19897a17..a8ac6f3cdb18d 100644 --- a/polkadot/node/core/pvf/common/src/execute.rs +++ b/polkadot/node/core/pvf/common/src/execute.rs @@ -14,10 +14,11 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::error::InternalValidationError; +use crate::{error::InternalValidationError, ArtifactChecksum}; use codec::{Decode, Encode}; +use polkadot_node_primitives::PoV; use polkadot_parachain_primitives::primitives::ValidationResult; -use polkadot_primitives::ExecutorParams; +use polkadot_primitives::{ExecutorParams, PersistedValidationData}; use std::time::Duration; /// The payload of the one-time handshake that is done when a worker process is created. Carries @@ -28,6 +29,19 @@ pub struct Handshake { pub executor_params: ExecutorParams, } +/// A request to execute a PVF. Contains all the data needed for validation. +#[derive(Encode, Decode)] +pub struct ExecuteRequest { + /// The persisted validation data. + pub pvd: PersistedValidationData, + /// The proof-of-validity. + pub pov: PoV, + /// The execution timeout. + pub execution_timeout: Duration, + /// The checksum of the artifact to execute. + pub artifact_checksum: ArtifactChecksum, +} + /// The response from the execution worker. #[derive(Debug, Encode, Decode)] pub struct WorkerResponse { diff --git a/polkadot/node/core/pvf/execute-worker/src/lib.rs b/polkadot/node/core/pvf/execute-worker/src/lib.rs index 88a3af985bb5d..03c58de3f8840 100644 --- a/polkadot/node/core/pvf/execute-worker/src/lib.rs +++ b/polkadot/node/core/pvf/execute-worker/src/lib.rs @@ -91,40 +91,17 @@ fn recv_execute_handshake(stream: &mut UnixStream) -> io::Result { fn recv_request( stream: &mut UnixStream, ) -> io::Result<(PersistedValidationData, PoV, Duration, ArtifactChecksum)> { - let pvd = framed_recv_blocking(stream)?; - let pvd = PersistedValidationData::decode(&mut &pvd[..]).map_err(|_| { - io::Error::new( - io::ErrorKind::Other, - "execute pvf recv_request: failed to decode persisted validation data".to_string(), - ) - })?; - - let pov = framed_recv_blocking(stream)?; - let pov = PoV::decode(&mut &pov[..]).map_err(|_| { - io::Error::new( - io::ErrorKind::Other, - "execute pvf recv_request: failed to decode PoV".to_string(), - ) - })?; + use polkadot_node_core_pvf_common::execute::ExecuteRequest; - let execution_timeout = framed_recv_blocking(stream)?; - let execution_timeout = Duration::decode(&mut &execution_timeout[..]).map_err(|_| { + let request_bytes = framed_recv_blocking(stream)?; + let request = ExecuteRequest::decode(&mut &request_bytes[..]).map_err(|_| { io::Error::new( io::ErrorKind::Other, - "execute pvf recv_request: failed to decode duration".to_string(), + "execute pvf recv_request: failed to decode ExecuteRequest".to_string(), ) })?; - let artifact_checksum = framed_recv_blocking(stream)?; - let artifact_checksum = - ArtifactChecksum::decode(&mut &artifact_checksum[..]).map_err(|_| { - io::Error::new( - io::ErrorKind::Other, - "execute pvf recv_request: failed to decode artifact checksum".to_string(), - ) - })?; - - Ok((pvd, pov, execution_timeout, artifact_checksum)) + Ok((request.pvd, request.pov, request.execution_timeout, request.artifact_checksum)) } /// Sends an error to the host and returns the original error wrapped in `io::Error`. diff --git a/polkadot/node/core/pvf/src/execute/worker_interface.rs b/polkadot/node/core/pvf/src/execute/worker_interface.rs index 545d7c6f568bb..6be6c24e5e77a 100644 --- a/polkadot/node/core/pvf/src/execute/worker_interface.rs +++ b/polkadot/node/core/pvf/src/execute/worker_interface.rs @@ -295,10 +295,13 @@ async fn send_request( execution_timeout: Duration, artifact_checksum: ArtifactChecksum, ) -> io::Result<()> { - framed_send(stream, &pvd.encode()).await?; - framed_send(stream, &pov.encode()).await?; - framed_send(stream, &execution_timeout.encode()).await?; - framed_send(stream, &artifact_checksum.encode()).await + let request = polkadot_node_core_pvf_common::execute::ExecuteRequest { + pvd: (*pvd).clone(), + pov: (*pov).clone(), + execution_timeout, + artifact_checksum, + }; + framed_send(stream, &request.encode()).await } async fn recv_result(stream: &mut UnixStream) -> io::Result> { From 987bc927e46febe906eb8db3e21c5dab1328a061 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 19 Jun 2025 11:39:24 +0000 Subject: [PATCH 2/4] Update from github-actions[bot] running command 'prdoc --audience node_dev --bump patch' --- prdoc/pr_8908.prdoc | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 prdoc/pr_8908.prdoc diff --git a/prdoc/pr_8908.prdoc b/prdoc/pr_8908.prdoc new file mode 100644 index 0000000000000..9abe9e82bd891 --- /dev/null +++ b/prdoc/pr_8908.prdoc @@ -0,0 +1,22 @@ +title: '[pvf-worker] Refactor execute request handling' +doc: +- audience: Node Dev + description: |- + # Description + + Fixes https://github.com/paritytech/polkadot-sdk/issues/8886 + + PVF execution worker communication was organized into a single ExecuteRequest struct. This should improve performance: one encode/decode operation instead of four. Also, no more chance of ordering mistakes. + + + + ## Integration + + This is an internal refactoring of the PVF execution worker. Downstream projects will not need any code changes. +crates: +- name: polkadot-node-core-pvf-common + bump: patch +- name: polkadot-node-core-pvf-execute-worker + bump: patch +- name: polkadot-node-core-pvf + bump: patch From 2c82b9b67118f9873fe100a0419ee493f3b4a2bd Mon Sep 17 00:00:00 2001 From: Andrei Eres Date: Thu, 19 Jun 2025 13:53:25 +0200 Subject: [PATCH 3/4] Update docs --- polkadot/node/core/pvf/common/src/execute.rs | 10 +++++----- prdoc/pr_8908.prdoc | 11 +---------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/polkadot/node/core/pvf/common/src/execute.rs b/polkadot/node/core/pvf/common/src/execute.rs index a8ac6f3cdb18d..9704184013cd2 100644 --- a/polkadot/node/core/pvf/common/src/execute.rs +++ b/polkadot/node/core/pvf/common/src/execute.rs @@ -29,16 +29,16 @@ pub struct Handshake { pub executor_params: ExecutorParams, } -/// A request to execute a PVF. Contains all the data needed for validation. +/// A request to execute a PVF #[derive(Encode, Decode)] pub struct ExecuteRequest { - /// The persisted validation data. + /// Persisted validation data. pub pvd: PersistedValidationData, - /// The proof-of-validity. + /// Proof-of-validity. pub pov: PoV, - /// The execution timeout. + /// Execution timeout. pub execution_timeout: Duration, - /// The checksum of the artifact to execute. + /// Checksum of the artifact to execute. pub artifact_checksum: ArtifactChecksum, } diff --git a/prdoc/pr_8908.prdoc b/prdoc/pr_8908.prdoc index 9abe9e82bd891..fb589a79d99e0 100644 --- a/prdoc/pr_8908.prdoc +++ b/prdoc/pr_8908.prdoc @@ -2,20 +2,11 @@ title: '[pvf-worker] Refactor execute request handling' doc: - audience: Node Dev description: |- - # Description - - Fixes https://github.com/paritytech/polkadot-sdk/issues/8886 - PVF execution worker communication was organized into a single ExecuteRequest struct. This should improve performance: one encode/decode operation instead of four. Also, no more chance of ordering mistakes. - - - ## Integration - - This is an internal refactoring of the PVF execution worker. Downstream projects will not need any code changes. crates: - name: polkadot-node-core-pvf-common - bump: patch + bump: minor - name: polkadot-node-core-pvf-execute-worker bump: patch - name: polkadot-node-core-pvf From 3aea49dd41ea9d7eca1f0bbb7d925e2c0cb12367 Mon Sep 17 00:00:00 2001 From: Andrei Eres Date: Wed, 25 Jun 2025 09:42:06 +0200 Subject: [PATCH 4/4] Move import to the top --- polkadot/node/core/pvf/execute-worker/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/polkadot/node/core/pvf/execute-worker/src/lib.rs b/polkadot/node/core/pvf/execute-worker/src/lib.rs index 03c58de3f8840..9cec00a5a8de8 100644 --- a/polkadot/node/core/pvf/execute-worker/src/lib.rs +++ b/polkadot/node/core/pvf/execute-worker/src/lib.rs @@ -41,7 +41,9 @@ use nix::{ use polkadot_node_core_pvf_common::{ compute_checksum, error::InternalValidationError, - execute::{Handshake, JobError, JobResponse, JobResult, WorkerError, WorkerResponse}, + execute::{ + ExecuteRequest, Handshake, JobError, JobResponse, JobResult, WorkerError, WorkerResponse, + }, executor_interface::params_to_wasmtime_semantics, framed_recv_blocking, framed_send_blocking, worker::{ @@ -91,8 +93,6 @@ fn recv_execute_handshake(stream: &mut UnixStream) -> io::Result { fn recv_request( stream: &mut UnixStream, ) -> io::Result<(PersistedValidationData, PoV, Duration, ArtifactChecksum)> { - use polkadot_node_core_pvf_common::execute::ExecuteRequest; - let request_bytes = framed_recv_blocking(stream)?; let request = ExecuteRequest::decode(&mut &request_bytes[..]).map_err(|_| { io::Error::new(