fix: 🩹 Amend wasmtime precompiled PR review (#13)

* fix: ➖ Remove use of `log` in favour of `tracing` in `sc-executor`

* fix: 🚨 Fix `node-cli` build for missing parameter added to `cmd.run()`

* fix: 🩹 Fix doc for `wasmtime-precompiled` argument

* fix: 🩹 Avoid looping through precompiles directory and search directly for filename

* docs: 💡 Comment fixes and clarifying which WASM code is precompiled

* fix: 🩹 Use configured hasher instead of blake2 always

* fix: 🩹 Error out if chainspec does not contain `:code` storage element

* feat: ✨ Add parser to CLI path arguments to check that they are existing directories

* fix: 🩹 Change `with_wasmtime_precompiled_path` to `with_optional_wasmtime_precompiled_path` to handle optional inside

* fix: 💡 Improve doc comments

* fix: 🩹 Write directly to file instead of creating it and then writing

* style: 🎨 Apply `cargo fmt`

cumulus/parachain-template/node/src/service.rs

@@ -91,18 +91,14 @@ pub fn new_partial(
 		.default_heap_pages
 		.map_or(DEFAULT_HEAP_ALLOC_STRATEGY, |h| HeapAllocStrategy::Static { extra_pages: h as _ });
 
-	let mut wasm_builder = WasmExecutor::builder()
+	let wasm = WasmExecutor::builder()
 		.with_execution_method(config.wasm_method)
 		.with_onchain_heap_alloc_strategy(heap_pages)
 		.with_offchain_heap_alloc_strategy(heap_pages)
 		.with_max_runtime_instances(config.max_runtime_instances)
-		.with_runtime_cache_size(config.runtime_cache_size);
-
-	if let Some(ref wasmtime_precompiled_path) = config.wasmtime_precompiled {
-		wasm_builder = wasm_builder.with_wasmtime_precompiled_path(wasmtime_precompiled_path);
-	}
-
-	let wasm = wasm_builder.build();
+		.with_runtime_cache_size(config.runtime_cache_size)
+		.with_optional_wasmtime_precompiled_path(config.wasmtime_precompiled.as_ref())
+		.build();
 
 	let executor = ParachainExecutor::new_with_wasm_executor(wasm);
 

substrate/bin/node/cli/src/command.rs

@@ -235,7 +235,7 @@ pub fn run() -> Result<()> {
 			runner.async_run(|config| {
 				let PartialComponents { task_manager, backend, .. } =
 					service::new_partial(&config)?;
-				Ok((cmd.run(backend), task_manager))
+				Ok((cmd.run(backend, config.chain_spec), task_manager))
 			})
 		},
 	}

substrate/client/cli/src/commands/precompile_wasm_cmd.rs

@@ -33,23 +33,29 @@ use sc_executor::{
 };
 use sc_service::ChainSpec;
 use sp_core::traits::RuntimeCode;
-use sp_runtime::traits::Block as BlockT;
+use sp_runtime::traits::{Block as BlockT, Hash, Header};
 use sp_state_machine::backend::BackendRuntimeCode;
 use std::{fmt::Debug, path::PathBuf, sync::Arc};
 
 /// The `precompile-wasm` command used to serialize a precompiled WASM module.
+///
+/// The WASM code precompiled will be the one used at the latest finalized block
+/// this node is aware of, if this node has the state for that finalized block in
+/// its storage. If that's not the case, it will use the WASM code from the chain
+/// spec passed as parameter when running the node.
 #[derive(Debug, Parser)]
 pub struct PrecompileWasmCmd {
 	#[allow(missing_docs)]
 	#[clap(flatten)]
 	pub database_params: DatabaseParams,
 
 	/// The default number of 64KB pages to ever allocate for Wasm execution.
+	///  
 	/// Don't alter this unless you know what you're doing.
 	#[arg(long, value_name = "COUNT")]
 	pub default_heap_pages: Option<u32>,
 
-	/// path to the directory where precompiled artifact will be written
+	/// Path to the directory where precompiled artifact will be written.
 	#[arg()]
 	pub output_dir: PathBuf,
 
@@ -62,6 +68,7 @@ pub struct PrecompileWasmCmd {
 	pub shared_params: SharedParams,
 
 	/// The WASM instantiation method to use.
+	///
 	/// Only has an effect when `wasm-execution` is set to `compiled`.
 	/// The copy-on-write strategies are only supported on Linux.
 	/// If the copy-on-write variant of a strategy is unsupported
@@ -110,7 +117,9 @@ impl PrecompileWasmCmd {
 					code_fetcher: &sp_core::traits::WrappedRuntimeCode(
 						wasm_bytecode.as_slice().into(),
 					),
-					hash: sp_core::blake2_256(&wasm_bytecode).to_vec(),
+					hash: <<B::Header as Header>::Hashing as Hash>::hash(&wasm_bytecode)
+						.as_ref()
+						.to_vec(),
 					heap_pages: Some(heap_pages as u64),
 				};
 				precompile_and_serialize_versioned_wasm_runtime(
@@ -123,6 +132,8 @@ impl PrecompileWasmCmd {
 					&self.output_dir,
 				)
 				.map_err(|e| Error::Application(Box::new(e)))?;
+			} else {
+				return Err(Error::Input(format!("The chain spec used does not contain a wasm bytecode in the `:code` storage key")));
 			}
 		}
 

substrate/client/cli/src/params/import_params.rs

@@ -67,16 +67,24 @@ pub struct ImportParams {
 	/// Specify the path where local precompiled WASM runtimes are stored.
 	/// Only has an effect when `wasm-execution` is set to `compiled`.
 	///
-	/// The precompiled runtimes must have been generated using the `precompile-runtimes`
+	/// The precompiled runtimes must have been generated using the `precompile-wasm`
 	/// subcommand with the same version of wasmtime and the exact same configuration.
 	/// The file name must end with the hash of the configuration. This hash must match, otherwise
 	/// the runtime will be recompiled.
-	#[arg(long, value_name = "PATH")]
+	#[arg(
+        long,
+        value_name = "PATH",
+        value_parser = ImportParams::validate_existing_directory,
+    )]
 	pub wasmtime_precompiled: Option<PathBuf>,
 
 	/// Specify the path where local WASM runtimes are stored.
 	/// These runtimes will override on-chain runtimes when the version matches.
-	#[arg(long, value_name = "PATH")]
+	#[arg(
+        long,
+        value_name = "PATH",
+        value_parser = ImportParams::validate_existing_directory,
+    )]
 	pub wasm_runtime_overrides: Option<PathBuf>,
 
 	#[allow(missing_docs)]
@@ -126,6 +134,16 @@ impl ImportParams {
 	pub fn wasm_runtime_overrides(&self) -> Option<PathBuf> {
 		self.wasm_runtime_overrides.clone()
 	}
+
+	/// Validate that the path is a valid directory.
+	fn validate_existing_directory(path: &str) -> Result<PathBuf, String> {
+		let path_buf = PathBuf::from(path);
+		if path_buf.is_dir() {
+			Ok(path_buf)
+		} else {
+			Err(format!("The path '{}' is not a valid directory", path))
+		}
+	}
 }
 
 /// Execution strategies parameters.

substrate/client/executor/Cargo.toml

@@ -17,7 +17,6 @@ targets = ["x86_64-unknown-linux-gnu"]
 parking_lot = "0.12.1"
 schnellru = "0.2.1"
 tracing = "0.1.29"
-log = "0.4.17"
 
 codec = { package = "parity-scale-codec", version = "3.6.1" }
 sc-executor-common = { path = "common" }

substrate/client/executor/src/executor.rs

@@ -195,17 +195,20 @@ impl<H> WasmExecutorBuilder<H> {
 		self
 	}
 
-	/// Create the wasm executor with the given `wasmtime_precompiled_path`.
+	/// Create the wasm executor with the given `maybe_wasmtime_precompiled_path`, provided that it
+	/// is `Some`.
 	///
-	/// The `wasmtime_precompiled_path` is a path to a directory where the executor load precompiled
-	/// wasmtime modules.
+	/// The `maybe_wasmtime_precompiled_path` is an optional which (if `Some`) its inner value is a
+	/// path to a directory where the executor loads precompiled wasmtime modules.
 	///
-	/// By default there is no `wasmtime_precompiled_path` given.
-	pub fn with_wasmtime_precompiled_path(
+	/// If `None`, calling this function will have no impact on the wasm executor being built.
+	pub fn with_optional_wasmtime_precompiled_path(
 		mut self,
-		wasmtime_precompiled_path: impl Into<PathBuf>,
+		maybe_wasmtime_precompiled_path: Option<impl Into<PathBuf>>,
 	) -> Self {
-		self.wasmtime_precompiled_path = Some(wasmtime_precompiled_path.into());
+		if let Some(wasmtime_precompiled_path) = maybe_wasmtime_precompiled_path {
+			self.wasmtime_precompiled_path = Some(wasmtime_precompiled_path.into());
+		}
 		self
 	}
 
@@ -251,7 +254,7 @@ pub struct WasmExecutor<H> {
 	cache_path: Option<PathBuf>,
 	/// Ignore missing function imports.
 	allow_missing_host_functions: bool,
-	/// TODO
+	/// Optional path to a directory where the executor can find precompiled wasmtime modules.
 	wasmtime_precompiled_path: Option<PathBuf>,
 	phantom: PhantomData<H>,
 }

substrate/client/executor/src/wasm_runtime.rs

@@ -35,12 +35,13 @@ use sp_version::RuntimeVersion;
 use sp_wasm_interface::HostFunctions;
 
 use std::{
-	io::Write,
 	panic::AssertUnwindSafe,
 	path::{Path, PathBuf},
 	sync::Arc,
 };
 
+const PRECOM_FILENAME_PREFIX: &str = "precompiled_wasm_";
+
 /// Specification of different methods of executing the runtime Wasm code.
 #[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)]
 pub enum WasmExecutionMethod {
@@ -319,68 +320,51 @@ where
 			if let Some(wasmtime_precompiled_dir) = wasmtime_precompiled_path {
 				if !wasmtime_precompiled_dir.is_dir() {
 					return Err(WasmError::Instantiation(format!(
-						"--wasmtime-precompiled is not a directory: {}",
+						"Wasmtime precompiled is not a directory: {}",
 						wasmtime_precompiled_dir.display()
 					)))
 				}
-				let handle_err = |e: std::io::Error| -> WasmError {
-					return WasmError::Instantiation(format!(
-						"Io error when loading wasmtime precompiled folder ({}): {}",
-						wasmtime_precompiled_dir.display(),
-						e
-					))
-				};
-				let mut maybe_compiled_artifact = None;
 
 				let artifact_version =
 					compute_artifact_version(allow_missing_func_imports, code_hash, &semantics);
-				log::debug!(
+				tracing::debug!(
 					target: "wasmtime-runtime",
 					"Searching for wasm hash: {}",
 					artifact_version.clone()
 				);
 
-				for entry in std::fs::read_dir(wasmtime_precompiled_dir).map_err(handle_err)? {
-					let entry = entry.map_err(handle_err)?;
-					if let Some(file_name) = entry.file_name().to_str() {
-						// We check that the artifact was generated for this specific artifact
-						// version and with the same wasm interface version and configuration.
-						if file_name.contains(&artifact_version.clone()) {
-							log::info!(
-								target: "wasmtime-runtime",
-								"Found precompiled wasm: {}",
-								file_name
-							);
-							// We change the version check strategy to make sure that the file
-							// content was serialized with the exact same config as well
-							maybe_compiled_artifact = Some((
-								entry.path(),
-								sc_executor_wasmtime::ModuleVersionStrategy::Custom(
-									artifact_version.clone(),
-								),
-							));
-						}
-					} else {
-						return Err(WasmError::Instantiation(
-							"wasmtime precompiled folder contain a file with invalid utf8 name"
-								.to_owned(),
-						))
-					}
-				}
+				let file_name = format!("{}{}", PRECOM_FILENAME_PREFIX, &artifact_version);
+				let file_path = std::path::Path::new(wasmtime_precompiled_dir).join(&file_name);
+
+				// If the file exists, and it hasn't been tempered with, the name is known.
+				// And it is in itself a check for the artifact version, wasm interface
+				// version and configuration.
+				if file_path.is_file() {
+					tracing::info!(
+						target: "wasmtime-runtime",
+						"🔎 Found precompiled wasm: {}",
+						file_name
+					);
+
+					// We change the version check strategy to make sure that the file
+					// content was serialized with the exact same config as well
+					let module_version_strategy =
+						sc_executor_wasmtime::ModuleVersionStrategy::Custom(
+							artifact_version.clone(),
+						);
 
-				if let Some((compiled_artifact_path, module_version_strategy)) =
-					maybe_compiled_artifact
-				{
 					// # Safety
 					//
 					// The file name of the artifact was checked before,
 					// so if the user has not renamed nor modified the file,
 					// it's certain that the file has been generated by
 					// `prepare_runtime_artifact` and with the same wasmtime
 					// version and configuration.
-					unsafe {
+					//
+					// Return early if the expected precompiled artifact exists.
+					return unsafe {
 						sc_executor_wasmtime::create_runtime_from_artifact::<H>(
-							&compiled_artifact_path,
+							&file_path,
 							module_version_strategy,
 							sc_executor_wasmtime::Config {
 								allow_missing_func_imports,
@@ -390,28 +374,24 @@ where
 						)
 					}
 					.map(|runtime| -> Box<dyn WasmModule> { Box::new(runtime) })
-				} else {
-					sc_executor_wasmtime::create_runtime::<H>(
-						blob,
-						sc_executor_wasmtime::Config {
-							allow_missing_func_imports,
-							cache_path: cache_path.map(ToOwned::to_owned),
-							semantics,
-						},
-					)
-					.map(|runtime| -> Box<dyn WasmModule> { Box::new(runtime) })
 				}
-			} else {
-				sc_executor_wasmtime::create_runtime::<H>(
-					blob,
-					sc_executor_wasmtime::Config {
-						allow_missing_func_imports,
-						cache_path: cache_path.map(ToOwned::to_owned),
-						semantics,
-					},
-				)
-				.map(|runtime| -> Box<dyn WasmModule> { Box::new(runtime) })
+
+				// If the expected precompiled artifact doesn't exist, we default to compiling it.
+				tracing::warn!(
+					"❗️ Precompiled wasm with name '{}' doesn't exist, compiling it.",
+					file_name
+				);
 			}
+
+			sc_executor_wasmtime::create_runtime::<H>(
+				blob,
+				sc_executor_wasmtime::Config {
+					allow_missing_func_imports,
+					cache_path: cache_path.map(ToOwned::to_owned),
+					semantics,
+				},
+			)
+			.map(|runtime| -> Box<dyn WasmModule> { Box::new(runtime) })
 		},
 	}
 }
@@ -460,16 +440,11 @@ pub fn precompile_and_serialize_versioned_wasm_runtime<'c>(
 	)?;
 
 	// Write in a file
-	let mut file = std::fs::File::create(
-		wasmtime_precompiled_path.join(format!("precompiled_wasm_{}", &artifact_version)),
+	std::fs::write(
+		wasmtime_precompiled_path.join(format!("{PRECOM_FILENAME_PREFIX}{artifact_version}")),
+		&serialized_precompiled_wasm,
 	)
 	.map_err(|e| {
-		WasmError::Other(format!(
-			"Fail to create file 'precompiled_wasm_0x{}', I/O Error: {}",
-			&artifact_version, e
-		))
-	})?;
-	file.write_all(&serialized_precompiled_wasm).map_err(|e| {
 		WasmError::Other(format!("Fail to write precompiled artifact, I/O Error: {}", e))
 	})?;
 
@@ -487,7 +462,7 @@ fn compute_artifact_version(
 		target: "wasmtime-runtime",
 		allow_missing_func_imports,
 		code_hash = sp_core::bytes::to_hex(&code_hash, false),
-		?semantics, 
+		?semantics,
 		"Computing wasm runtime hash",
 	);
 	let mut buffer = Vec::new();

substrate/client/executor/wasmtime/Cargo.toml

@@ -13,7 +13,7 @@ readme = "README.md"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
-log = "0.4.17"
+tracing = "0.1.29"
 cfg-if = "1.0"
 libc = "0.2.121"