From d219260cac0491b611dd4689e7e6ce5c3a1b679b Mon Sep 17 00:00:00 2001
From: Marcin S <marcin@realemail.net>
Date: Tue, 23 May 2023 14:56:02 -0400
Subject: [PATCH] PVF: instantiate runtime from bytes (#7270)
* PVF: instantiate runtime from bytes
* Update naming
---
polkadot/node/core/pvf/worker/Cargo.toml | 1 -
polkadot/node/core/pvf/worker/src/execute.rs | 34 +++++++++++++------
.../node/core/pvf/worker/src/executor_intf.rs | 34 +++++++++++++------
polkadot/node/core/pvf/worker/src/testing.rs | 7 ++--
4 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/polkadot/node/core/pvf/worker/Cargo.toml b/polkadot/node/core/pvf/worker/Cargo.toml
index 260c6217eb6..53d548dbac6 100644
--- a/polkadot/node/core/pvf/worker/Cargo.toml
+++ b/polkadot/node/core/pvf/worker/Cargo.toml
@@ -43,7 +43,6 @@ substrate-build-script-utils = { git = "https://github.com/paritytech/substrate"
[dev-dependencies]
adder = { package = "test-parachain-adder", path = "../../../../parachain/test-parachains/adder" }
halt = { package = "test-parachain-halt", path = "../../../../parachain/test-parachains/halt" }
-tempfile = "3.3.0"
[features]
jemalloc-allocator = ["dep:tikv-jemalloc-ctl"]
diff --git a/polkadot/node/core/pvf/worker/src/execute.rs b/polkadot/node/core/pvf/worker/src/execute.rs
index 997613fe7bc..c5b8ddc9dd1 100644
--- a/polkadot/node/core/pvf/worker/src/execute.rs
+++ b/polkadot/node/core/pvf/worker/src/execute.rs
@@ -31,7 +31,7 @@ use polkadot_node_core_pvf::{
};
use polkadot_parachain::primitives::ValidationResult;
use std::{
- path::{Path, PathBuf},
+ path::PathBuf,
sync::{mpsc::channel, Arc},
time::Duration,
};
@@ -97,6 +97,20 @@ pub fn worker_entrypoint(socket_path: &str, node_version: Option<&str>) {
artifact_path.display(),
);
+ // Get the artifact bytes.
+ //
+ // We do this outside the thread so that we can lock down filesystem access there.
+ let compiled_artifact_blob = match std::fs::read(artifact_path) {
+ Ok(bytes) => bytes,
+ Err(err) => {
+ let response = Response::InternalError(
+ InternalValidationError::CouldNotOpenFile(err.to_string()),
+ );
+ send_response(&mut stream, response).await?;
+ continue
+ },
+ };
+
// Conditional variable to notify us when a thread is done.
let condvar = thread::get_condvar();
@@ -116,7 +130,12 @@ pub fn worker_entrypoint(socket_path: &str, node_version: Option<&str>) {
let execute_thread = thread::spawn_worker_thread_with_stack_size(
"execute thread",
move || {
- validate_using_artifact(&artifact_path, ¶ms, executor_2, cpu_time_start)
+ validate_using_artifact(
+ &compiled_artifact_blob,
+ ¶ms,
+ executor_2,
+ cpu_time_start,
+ )
},
Arc::clone(&condvar),
WaitOutcome::Finished,
@@ -167,23 +186,16 @@ pub fn worker_entrypoint(socket_path: &str, node_version: Option<&str>) {
}
fn validate_using_artifact(
- artifact_path: &Path,
+ compiled_artifact_blob: &[u8],
params: &[u8],
executor: Executor,
cpu_time_start: ProcessTime,
) -> Response {
- // Check here if the file exists, because the error from Substrate is not match-able.
- // TODO: Re-evaluate after <https://github.com/paritytech/substrate/issues/13860>.
- let file_metadata = std::fs::metadata(artifact_path);
- if let Err(err) = file_metadata {
- return Response::InternalError(InternalValidationError::CouldNotOpenFile(err.to_string()))
- }
-
let descriptor_bytes = match unsafe {
// SAFETY: this should be safe since the compiled artifact passed here comes from the
// file created by the prepare workers. These files are obtained by calling
// [`executor_intf::prepare`].
- executor.execute(artifact_path.as_ref(), params)
+ executor.execute(compiled_artifact_blob, params)
} {
Err(err) => return Response::format_invalid("execute", &err),
Ok(d) => d,
diff --git a/polkadot/node/core/pvf/worker/src/executor_intf.rs b/polkadot/node/core/pvf/worker/src/executor_intf.rs
index 05d3b41c762..ff286dd74d6 100644
--- a/polkadot/node/core/pvf/worker/src/executor_intf.rs
+++ b/polkadot/node/core/pvf/worker/src/executor_intf.rs
@@ -18,16 +18,14 @@
use polkadot_primitives::{ExecutorParam, ExecutorParams};
use sc_executor_common::{
+ error::WasmError,
runtime_blob::RuntimeBlob,
wasm_runtime::{HeapAllocStrategy, InvokeMethod, WasmModule as _},
};
-use sc_executor_wasmtime::{Config, DeterministicStackLimit, Semantics};
+use sc_executor_wasmtime::{Config, DeterministicStackLimit, Semantics, WasmtimeRuntime};
use sp_core::storage::{ChildInfo, TrackedStorageKey};
use sp_externalities::MultiRemovalResults;
-use std::{
- any::{Any, TypeId},
- path::Path,
-};
+use std::any::{Any, TypeId};
// Wasmtime powers the Substrate Executor. It compiles the wasm bytecode into native code.
// That native code does not create any stacks and just reuses the stack of the thread that
@@ -206,7 +204,7 @@ impl Executor {
/// Failure to adhere to these requirements might lead to crashes and arbitrary code execution.
pub unsafe fn execute(
&self,
- compiled_artifact_path: &Path,
+ compiled_artifact_blob: &[u8],
params: &[u8],
) -> Result<Vec<u8>, String> {
let mut extensions = sp_externalities::Extensions::new();
@@ -216,10 +214,7 @@ impl Executor {
let mut ext = ValidationExternalities(extensions);
match sc_executor::with_externalities_safe(&mut ext, || {
- let runtime = sc_executor_wasmtime::create_runtime_from_artifact::<HostFunctions>(
- compiled_artifact_path,
- self.config.clone(),
- )?;
+ let runtime = self.create_runtime_from_bytes(compiled_artifact_blob)?;
runtime.new_instance()?.call(InvokeMethod::Export("validate_block"), params)
}) {
Ok(Ok(ok)) => Ok(ok),
@@ -227,6 +222,25 @@ impl Executor {
}
.map_err(|err| format!("execute error: {:?}", err))
}
+
+ /// Constructs the runtime for the given PVF, given the artifact bytes.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that the compiled artifact passed here was:
+ /// 1) produced by [`prepare`],
+ /// 2) was not modified,
+ ///
+ /// Failure to adhere to these requirements might lead to crashes and arbitrary code execution.
+ pub unsafe fn create_runtime_from_bytes(
+ &self,
+ compiled_artifact_blob: &[u8],
+ ) -> Result<WasmtimeRuntime, WasmError> {
+ sc_executor_wasmtime::create_runtime_from_artifact_bytes::<HostFunctions>(
+ compiled_artifact_blob,
+ self.config.clone(),
+ )
+ }
}
type HostFunctions = (
diff --git a/polkadot/node/core/pvf/worker/src/testing.rs b/polkadot/node/core/pvf/worker/src/testing.rs
index d09b68bf8b3..7497d4aed31 100644
--- a/polkadot/node/core/pvf/worker/src/testing.rs
+++ b/polkadot/node/core/pvf/worker/src/testing.rs
@@ -33,16 +33,13 @@ pub fn validate_candidate(
.expect("Decompressing code failed");
let blob = prevalidate(&code)?;
- let artifact = prepare(blob, &ExecutorParams::default())?;
- let tmpdir = tempfile::tempdir()?;
- let artifact_path = tmpdir.path().join("blob");
- std::fs::write(&artifact_path, &artifact)?;
+ let compiled_artifact_blob = prepare(blob, &ExecutorParams::default())?;
let executor = Executor::new(ExecutorParams::default())?;
let result = unsafe {
// SAFETY: This is trivially safe since the artifact is obtained by calling `prepare`
// and is written into a temporary directory in an unmodified state.
- executor.execute(&artifact_path, params)?
+ executor.execute(&compiled_artifact_blob, params)?
};
Ok(result)
--
GitLab