Discard Executor (#1855)

closes #622 

Pros:
* simpler interface, just functions:
`create_runtime_from_artifact_bytes()` and `execute_artifact()`

Cons:
* extra overhead of constructing executor semantics each time

I could make it a combination of
* `create_runtime_config(params)` (such that we could clone the
constructed semantics)
* `create_runtime(blob, config)`
* `execute_artifact(blob, config, params)`

Not sure if it's worth it though.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>

polkadot/node/core/pvf/common/src/executor_intf.rs

@@ -95,6 +95,60 @@ pub const DEFAULT_CONFIG: Config = Config {
 	},
 };
 
+/// Executes the given PVF in the form of a compiled artifact and returns the result of
+/// execution upon success.
+///
+/// # 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 execute_artifact(
+	compiled_artifact_blob: &[u8],
+	executor_params: &ExecutorParams,
+	params: &[u8],
+) -> Result<Vec<u8>, String> {
+	let mut extensions = sp_externalities::Extensions::new();
+
+	extensions.register(sp_core::traits::ReadRuntimeVersionExt::new(ReadRuntimeVersion));
+
+	let mut ext = ValidationExternalities(extensions);
+
+	match sc_executor::with_externalities_safe(&mut ext, || {
+		let runtime = create_runtime_from_artifact_bytes(compiled_artifact_blob, executor_params)?;
+		runtime.new_instance()?.call(InvokeMethod::Export("validate_block"), params)
+	}) {
+		Ok(Ok(ok)) => Ok(ok),
+		Ok(Err(err)) | Err(err) => Err(err),
+	}
+	.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_artifact_bytes(
+	compiled_artifact_blob: &[u8],
+	executor_params: &ExecutorParams,
+) -> Result<WasmtimeRuntime, WasmError> {
+	let mut config = DEFAULT_CONFIG.clone();
+	config.semantics =
+		params_to_wasmtime_semantics(executor_params).map_err(|err| WasmError::Other(err))?;
+
+	sc_executor_wasmtime::create_runtime_from_artifact_bytes::<HostFunctions>(
+		compiled_artifact_blob,
+		config,
+	)
+}
+
 pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result<Semantics, String> {
 	let mut sem = DEFAULT_CONFIG.semantics.clone();
 	let mut stack_limit = if let Some(stack_limit) = sem.deterministic_stack_limit.clone() {
@@ -121,72 +175,6 @@ pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result<Semantics, S
 	Ok(sem)
 }
 
-/// A WASM executor with a given configuration. It is instantiated once per execute worker and is
-/// specific to that worker.
-#[derive(Clone)]
-pub struct Executor {
-	config: Config,
-}
-
-impl Executor {
-	pub fn new(params: ExecutorParams) -> Result<Self, String> {
-		let mut config = DEFAULT_CONFIG.clone();
-		config.semantics = params_to_wasmtime_semantics(&params)?;
-
-		Ok(Self { config })
-	}
-
-	/// Executes the given PVF in the form of a compiled artifact and returns the result of
-	/// execution upon success.
-	///
-	/// # 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 execute(
-		&self,
-		compiled_artifact_blob: &[u8],
-		params: &[u8],
-	) -> Result<Vec<u8>, String> {
-		let mut extensions = sp_externalities::Extensions::new();
-
-		extensions.register(sp_core::traits::ReadRuntimeVersionExt::new(ReadRuntimeVersion));
-
-		let mut ext = ValidationExternalities(extensions);
-
-		match sc_executor::with_externalities_safe(&mut ext, || {
-			let runtime = self.create_runtime_from_bytes(compiled_artifact_blob)?;
-			runtime.new_instance()?.call(InvokeMethod::Export("validate_block"), params)
-		}) {
-			Ok(Ok(ok)) => Ok(ok),
-			Ok(Err(err)) | Err(err) => Err(err),
-		}
-		.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(),
-		)
-	}
-}
-
 /// Available host functions. We leave out:
 ///
 /// 1. storage related stuff (PVF doesn't have a notion of a persistent storage/trie)

polkadot/node/core/pvf/execute-worker/src/lib.rs

@@ -16,7 +16,9 @@
 
 //! Contains the logic for executing PVFs. Used by the polkadot-execute-worker binary.
 
-pub use polkadot_node_core_pvf_common::{executor_intf::Executor, worker_dir, SecurityStatus};
+pub use polkadot_node_core_pvf_common::{
+	executor_intf::execute_artifact, worker_dir, SecurityStatus,
+};
 
 // NOTE: Initializing logging in e.g. tests will not have an effect in the workers, as they are
 //       separate spawned processes. Run with e.g. `RUST_LOG=parachain::pvf-execute-worker=trace`.
@@ -35,7 +37,7 @@ use polkadot_node_core_pvf_common::{
 	},
 };
 use polkadot_parachain_primitives::primitives::ValidationResult;
-use polkadot_primitives::executor_params::DEFAULT_NATIVE_STACK_MAX;
+use polkadot_primitives::{executor_params::DEFAULT_NATIVE_STACK_MAX, ExecutorParams};
 use std::{
 	os::unix::net::UnixStream,
 	path::PathBuf,
@@ -141,9 +143,6 @@ pub fn worker_entrypoint(
 			let artifact_path = worker_dir::execute_artifact(&worker_dir_path);
 
 			let Handshake { executor_params } = recv_handshake(&mut stream)?;
-			let executor = Executor::new(executor_params).map_err(|e| {
-				io::Error::new(io::ErrorKind::Other, format!("cannot create executor: {}", e))
-			})?;
 
 			loop {
 				let (params, execution_timeout) = recv_request(&mut stream)?;
@@ -185,14 +184,15 @@ pub fn worker_entrypoint(
 					Arc::clone(&condvar),
 					WaitOutcome::TimedOut,
 				)?;
-				let executor_2 = executor.clone();
+
+				let executor_params_2 = executor_params.clone();
 				let execute_thread = thread::spawn_worker_thread_with_stack_size(
 					"execute thread",
 					move || {
 						validate_using_artifact(
 							&compiled_artifact_blob,
+							&executor_params_2,
 							&params,
-							executor_2,
 							cpu_time_start,
 						)
 					},
@@ -257,15 +257,15 @@ pub fn worker_entrypoint(
 
 fn validate_using_artifact(
 	compiled_artifact_blob: &[u8],
+	executor_params: &ExecutorParams,
 	params: &[u8],
-	executor: Executor,
 	cpu_time_start: ProcessTime,
 ) -> Response {
 	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(compiled_artifact_blob, params)
+		execute_artifact(compiled_artifact_blob, executor_params, params)
 	} {
 		Err(err) => return Response::format_invalid("execute", &err),
 		Ok(d) => d,

polkadot/node/core/pvf/prepare-worker/src/lib.rs

@@ -32,7 +32,7 @@ use crate::memory_stats::memory_tracker::{get_memory_tracker_loop_stats, memory_
 use parity_scale_codec::{Decode, Encode};
 use polkadot_node_core_pvf_common::{
 	error::{PrepareError, PrepareResult},
-	executor_intf::Executor,
+	executor_intf::create_runtime_from_artifact_bytes,
 	framed_recv_blocking, framed_send_blocking,
 	prepare::{MemoryStats, PrepareJobKind, PrepareStats},
 	pvf::PvfPrepData,
@@ -145,7 +145,7 @@ pub fn worker_entrypoint(
 
 				let preparation_timeout = pvf.prep_timeout();
 				let prepare_job_kind = pvf.prep_kind();
-				let executor_params = (*pvf.executor_params()).clone();
+				let executor_params = pvf.executor_params();
 
 				// Conditional variable to notify us when a thread is done.
 				let condvar = thread::get_condvar();
@@ -191,7 +191,10 @@ pub fn worker_entrypoint(
 						// anyway.
 						if let PrepareJobKind::Prechecking = prepare_job_kind {
 							result = result.and_then(|output| {
-								runtime_construction_check(output.0.as_ref(), executor_params)?;
+								runtime_construction_check(
+									output.0.as_ref(),
+									executor_params.as_ref(),
+								)?;
 								Ok(output)
 							});
 						}
@@ -317,13 +320,10 @@ fn prepare_artifact(
 /// Try constructing the runtime to catch any instantiation errors during pre-checking.
 fn runtime_construction_check(
 	artifact_bytes: &[u8],
-	executor_params: ExecutorParams,
+	executor_params: &ExecutorParams,
 ) -> Result<(), PrepareError> {
-	let executor = Executor::new(executor_params)
-		.map_err(|e| PrepareError::RuntimeConstruction(format!("cannot create executor: {}", e)))?;
-
 	// SAFETY: We just compiled this artifact.
-	let result = unsafe { executor.create_runtime_from_bytes(&artifact_bytes) };
+	let result = unsafe { create_runtime_from_artifact_bytes(artifact_bytes, executor_params) };
 	result
 		.map(|_runtime| ())
 		.map_err(|err| PrepareError::RuntimeConstruction(format!("{:?}", err)))

polkadot/node/core/pvf/src/testing.rs

@@ -29,20 +29,20 @@ pub fn validate_candidate(
 	code: &[u8],
 	params: &[u8],
 ) -> Result<Vec<u8>, Box<dyn std::error::Error>> {
-	use polkadot_node_core_pvf_execute_worker::Executor;
+	use polkadot_node_core_pvf_execute_worker::execute_artifact;
 	use polkadot_node_core_pvf_prepare_worker::{prepare, prevalidate};
 
 	let code = sp_maybe_compressed_blob::decompress(code, 10 * 1024 * 1024)
 		.expect("Decompressing code failed");
 
 	let blob = prevalidate(&code)?;
-	let compiled_artifact_blob = prepare(blob, &ExecutorParams::default())?;
+	let executor_params = ExecutorParams::default();
+	let compiled_artifact_blob = prepare(blob, &executor_params)?;
 
-	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(&compiled_artifact_blob, params)?
+		execute_artifact(&compiled_artifact_blob, &executor_params, params)?
 	};
 
 	Ok(result)

substrate/frame/support/procedural/src/pallet/expand/warnings.rs

@@ -33,6 +33,7 @@ pub(crate) fn weight_witness_warning(
 	if dev_mode {
 		return
 	}
+
 	let CallWeightDef::Immediate(w) = &method.weight else { return };
 
 	let partial_warning = Warning::new_deprecated("UncheckedWeightWitness")
@@ -64,6 +65,7 @@ pub(crate) fn weight_constant_warning(
 	if dev_mode {
 		return
 	}
+
 	let syn::Expr::Lit(lit) = weight else { return };
 
 	let warning = Warning::new_deprecated("ConstantWeight")