From 8061c87007ffff007464d3ae62c6c1f4f394dcd3 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Wed, 4 Dec 2024 04:02:12 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20=F0=9F=A9=B9=20Amend=20wasmtime=20precom?= =?UTF-8?q?piled=20PR=20review=20(#13)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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: :adhesive_bandage: Fix doc for `wasmtime-precompiled` argument * fix: :adhesive_bandage: Avoid looping through precompiles directory and search directly for filename * docs: :bulb: Comment fixes and clarifying which WASM code is precompiled * fix: :adhesive_bandage: Use configured hasher instead of blake2 always * fix: :adhesive_bandage: Error out if chainspec does not contain `:code` storage element * feat: :sparkles: Add parser to CLI path arguments to check that they are existing directories * fix: :adhesive_bandage: Change `with_wasmtime_precompiled_path` to `with_optional_wasmtime_precompiled_path` to handle optional inside * fix: :bulb: Improve doc comments * fix: :adhesive_bandage: Write directly to file instead of creating it and then writing * style: :art: Apply `cargo fmt` --- Cargo.lock | 3 +- .../parachain-template/node/src/service.rs | 12 +- substrate/bin/node/cli/src/command.rs | 2 +- .../cli/src/commands/precompile_wasm_cmd.rs | 17 ++- .../client/cli/src/params/import_params.rs | 24 +++- substrate/client/executor/Cargo.toml | 1 - substrate/client/executor/src/executor.rs | 19 +-- substrate/client/executor/src/wasm_runtime.rs | 119 +++++++----------- substrate/client/executor/wasmtime/Cargo.toml | 2 +- .../client/executor/wasmtime/src/imports.rs | 2 +- .../executor/wasmtime/src/instance_wrapper.rs | 2 +- .../client/executor/wasmtime/src/runtime.rs | 4 +- substrate/client/service/src/builder.rs | 12 +- 13 files changed, 108 insertions(+), 111 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fdbae9851682..fb04c72a735c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15104,7 +15104,6 @@ dependencies = [ "assert_matches", "criterion 0.4.0", "env_logger 0.9.3", - "log", "num_cpus", "parity-scale-codec", "parking_lot 0.12.1", @@ -15155,7 +15154,6 @@ dependencies = [ "cargo_metadata", "cfg-if", "libc", - "log", "parity-scale-codec", "paste", "rustix 0.36.15", @@ -15167,6 +15165,7 @@ dependencies = [ "sp-runtime-interface", "sp-wasm-interface", "tempfile", + "tracing", "wasmtime", "wat", ] diff --git a/cumulus/parachain-template/node/src/service.rs b/cumulus/parachain-template/node/src/service.rs index c64be23ded7b..29cb22953a63 100644 --- a/cumulus/parachain-template/node/src/service.rs +++ b/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); diff --git a/substrate/bin/node/cli/src/command.rs b/substrate/bin/node/cli/src/command.rs index 8b652483cbf8..7a8e4f344a9c 100644 --- a/substrate/bin/node/cli/src/command.rs +++ b/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)) }) }, } diff --git a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs index 1ab0a876d26a..a3b7a8af25a8 100644 --- a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs +++ b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs @@ -33,11 +33,16 @@ 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)] @@ -45,11 +50,12 @@ pub struct PrecompileWasmCmd { 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, - /// 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: <::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"))); } } diff --git a/substrate/client/cli/src/params/import_params.rs b/substrate/client/cli/src/params/import_params.rs index b47b9729e614..1bce14271536 100644 --- a/substrate/client/cli/src/params/import_params.rs +++ b/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, /// 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, #[allow(missing_docs)] @@ -126,6 +134,16 @@ impl ImportParams { pub fn wasm_runtime_overrides(&self) -> Option { self.wasm_runtime_overrides.clone() } + + /// Validate that the path is a valid directory. + fn validate_existing_directory(path: &str) -> Result { + 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. diff --git a/substrate/client/executor/Cargo.toml b/substrate/client/executor/Cargo.toml index a05e0eda2890..9f41b7423737 100644 --- a/substrate/client/executor/Cargo.toml +++ b/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" } diff --git a/substrate/client/executor/src/executor.rs b/substrate/client/executor/src/executor.rs index c6831f25a0e5..b9bccd3905df 100644 --- a/substrate/client/executor/src/executor.rs +++ b/substrate/client/executor/src/executor.rs @@ -195,17 +195,20 @@ impl WasmExecutorBuilder { 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, + maybe_wasmtime_precompiled_path: Option>, ) -> 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 { cache_path: Option, /// 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, phantom: PhantomData, } diff --git a/substrate/client/executor/src/wasm_runtime.rs b/substrate/client/executor/src/wasm_runtime.rs index 138356cf7147..96cb01a0ad5c 100644 --- a/substrate/client/executor/src/wasm_runtime.rs +++ b/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,58 +320,39 @@ 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, @@ -378,9 +360,11 @@ where // 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::( - &compiled_artifact_path, + &file_path, module_version_strategy, sc_executor_wasmtime::Config { allow_missing_func_imports, @@ -390,28 +374,24 @@ where ) } .map(|runtime| -> Box { Box::new(runtime) }) - } else { - sc_executor_wasmtime::create_runtime::( - blob, - sc_executor_wasmtime::Config { - allow_missing_func_imports, - cache_path: cache_path.map(ToOwned::to_owned), - semantics, - }, - ) - .map(|runtime| -> Box { Box::new(runtime) }) } - } else { - sc_executor_wasmtime::create_runtime::( - blob, - sc_executor_wasmtime::Config { - allow_missing_func_imports, - cache_path: cache_path.map(ToOwned::to_owned), - semantics, - }, - ) - .map(|runtime| -> Box { 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::( + blob, + sc_executor_wasmtime::Config { + allow_missing_func_imports, + cache_path: cache_path.map(ToOwned::to_owned), + semantics, + }, + ) + .map(|runtime| -> Box { 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(); diff --git a/substrate/client/executor/wasmtime/Cargo.toml b/substrate/client/executor/wasmtime/Cargo.toml index 3aaa1424bcf6..6f448911a267 100644 --- a/substrate/client/executor/wasmtime/Cargo.toml +++ b/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" diff --git a/substrate/client/executor/wasmtime/src/imports.rs b/substrate/client/executor/wasmtime/src/imports.rs index fccc121fa088..d4dc16239f07 100644 --- a/substrate/client/executor/wasmtime/src/imports.rs +++ b/substrate/client/executor/wasmtime/src/imports.rs @@ -64,7 +64,7 @@ where if allow_missing_func_imports { for (name, (import_ty, func_ty)) in registry.pending_func_imports { let error = format!("call to a missing function {}:{}", import_ty.module(), name); - log::debug!("Missing import: '{}' {:?}", name, func_ty); + tracing::debug!("Missing import: '{}' {:?}", name, func_ty); linker .func_new("env", &name, func_ty.clone(), move |_, _, _| { Err(anyhow::Error::msg(error.clone())) diff --git a/substrate/client/executor/wasmtime/src/instance_wrapper.rs b/substrate/client/executor/wasmtime/src/instance_wrapper.rs index acc799061c27..e1dd80d8313c 100644 --- a/substrate/client/executor/wasmtime/src/instance_wrapper.rs +++ b/substrate/client/executor/wasmtime/src/instance_wrapper.rs @@ -128,7 +128,7 @@ impl sc_allocator::Memory for MemoryWrapper<'_, C> { self.0 .grow(&mut self.1, additional as u64) .map_err(|e| { - log::error!( + tracing::error!( target: "wasm-executor", "Failed to grow memory by {} pages: {}", additional, diff --git a/substrate/client/executor/wasmtime/src/runtime.rs b/substrate/client/executor/wasmtime/src/runtime.rs index 6389d4642cf9..d3227ff6eaa9 100644 --- a/substrate/client/executor/wasmtime/src/runtime.rs +++ b/substrate/client/executor/wasmtime/src/runtime.rs @@ -205,7 +205,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result(config: &Configuration) -> WasmExecut let strategy = config .default_heap_pages .map_or(DEFAULT_HEAP_ALLOC_STRATEGY, |p| HeapAllocStrategy::Static { extra_pages: p as _ }); - let mut wasm_builder = WasmExecutor::::builder() + WasmExecutor::::builder() .with_execution_method(config.wasm_method) .with_onchain_heap_alloc_strategy(strategy) .with_offchain_heap_alloc_strategy(strategy) .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); - } - - wasm_builder.build() + .with_runtime_cache_size(config.runtime_cache_size) + .with_optional_wasmtime_precompiled_path(config.wasmtime_precompiled.as_ref()) + .build() } /// Create an instance of default DB-backend backend.