From b5fd3576c8e2974b1921b8b05da666cca364a601 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:30:10 -0300 Subject: [PATCH 01/12] =?UTF-8?q?fix:=20=E2=9E=96=20Remove=20use=20of=20`l?= =?UTF-8?q?og`=20in=20favour=20of=20`tracing`=20in=20`sc-executor`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 3 +-- substrate/client/executor/Cargo.toml | 1 - substrate/client/executor/src/wasm_runtime.rs | 8 ++++---- substrate/client/executor/wasmtime/Cargo.toml | 2 +- substrate/client/executor/wasmtime/src/imports.rs | 2 +- .../client/executor/wasmtime/src/instance_wrapper.rs | 2 +- substrate/client/executor/wasmtime/src/runtime.rs | 4 ++-- 7 files changed, 10 insertions(+), 12 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/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/wasm_runtime.rs b/substrate/client/executor/src/wasm_runtime.rs index 138356cf7147..9f924ccea937 100644 --- a/substrate/client/executor/src/wasm_runtime.rs +++ b/substrate/client/executor/src/wasm_runtime.rs @@ -319,7 +319,7 @@ 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() ))) } @@ -334,7 +334,7 @@ where 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() @@ -346,7 +346,7 @@ where // 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!( + tracing::info!( target: "wasmtime-runtime", "Found precompiled wasm: {}", file_name @@ -465,7 +465,7 @@ pub fn precompile_and_serialize_versioned_wasm_runtime<'c>( ) .map_err(|e| { WasmError::Other(format!( - "Fail to create file 'precompiled_wasm_0x{}', I/O Error: {}", + "Fail to create file 'precompiled_wasm_{}', I/O Error: {}", &artifact_version, e )) })?; 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 Date: Tue, 3 Dec 2024 10:30:38 -0300 Subject: [PATCH 02/12] =?UTF-8?q?fix:=20=F0=9F=9A=A8=20Fix=20`node-cli`=20?= =?UTF-8?q?build=20for=20missing=20parameter=20added=20to=20`cmd.run()`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- substrate/bin/node/cli/src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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)) }) }, } From acb95bdcf47323ffd69475189d3124f449d8a2af Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:30:49 -0300 Subject: [PATCH 03/12] fix: :adhesive_bandage: Fix doc for `wasmtime-precompiled` argument --- substrate/client/cli/src/params/import_params.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/cli/src/params/import_params.rs b/substrate/client/cli/src/params/import_params.rs index b47b9729e614..76bd7347cdc2 100644 --- a/substrate/client/cli/src/params/import_params.rs +++ b/substrate/client/cli/src/params/import_params.rs @@ -67,7 +67,7 @@ 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. From 8fe1a28f2b4b47032e64f507746555220e48309a Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 16:55:53 -0300 Subject: [PATCH 04/12] fix: :adhesive_bandage: Avoid looping through precompiles directory and search directly for filename --- substrate/client/executor/src/wasm_runtime.rs | 103 +++++++----------- 1 file changed, 40 insertions(+), 63 deletions(-) diff --git a/substrate/client/executor/src/wasm_runtime.rs b/substrate/client/executor/src/wasm_runtime.rs index 9f924ccea937..5693ecd22533 100644 --- a/substrate/client/executor/src/wasm_runtime.rs +++ b/substrate/client/executor/src/wasm_runtime.rs @@ -41,6 +41,8 @@ use std::{ 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 { @@ -323,14 +325,6 @@ where 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); @@ -340,37 +334,25 @@ where 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()) { - 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 - 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,21 @@ 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) }) }, } } @@ -461,12 +438,12 @@ 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)), + wasmtime_precompiled_path.join(format!("{}{}", PRECOM_FILENAME_PREFIX, &artifact_version)), ) .map_err(|e| { WasmError::Other(format!( - "Fail to create file 'precompiled_wasm_{}', I/O Error: {}", - &artifact_version, e + "Fail to create file '{}{}', I/O Error: {}", + PRECOM_FILENAME_PREFIX, &artifact_version, e )) })?; file.write_all(&serialized_precompiled_wasm).map_err(|e| { From 266c4a2e7dd4a4c3d75c57e5de56ed55b80ca119 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 17:20:54 -0300 Subject: [PATCH 05/12] docs: :bulb: Comment fixes and clarifying which WASM code is precompiled --- substrate/client/cli/src/commands/precompile_wasm_cmd.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs index 1ab0a876d26a..92da8fff2b6c 100644 --- a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs +++ b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs @@ -38,6 +38,11 @@ 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 From 1bdf7a7ddf1940461428aa76db9c6b34d059a766 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 17:21:54 -0300 Subject: [PATCH 06/12] fix: :adhesive_bandage: Use configured hasher instead of blake2 always --- substrate/client/cli/src/commands/precompile_wasm_cmd.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs index 92da8fff2b6c..b356cece1b2b 100644 --- a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs +++ b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs @@ -33,7 +33,7 @@ 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, Header, Hash}; use sp_state_machine::backend::BackendRuntimeCode; use std::{fmt::Debug, path::PathBuf, sync::Arc}; @@ -117,7 +117,7 @@ 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( From 1535d1a61eb5d5b406f9c5bc06660b8d298c2ad2 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 19:02:06 -0300 Subject: [PATCH 07/12] fix: :adhesive_bandage: Error out if chainspec does not contain `:code` storage element --- substrate/client/cli/src/commands/precompile_wasm_cmd.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs index b356cece1b2b..af836c7c6af5 100644 --- a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs +++ b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs @@ -130,6 +130,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"))); } } From 5d36bd4f6072b3fdc4441afab73fd7cd1a582358 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 20:28:56 -0300 Subject: [PATCH 08/12] feat: :sparkles: Add parser to CLI path arguments to check that they are existing directories --- .../client/cli/src/params/import_params.rs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/substrate/client/cli/src/params/import_params.rs b/substrate/client/cli/src/params/import_params.rs index 76bd7347cdc2..1bce14271536 100644 --- a/substrate/client/cli/src/params/import_params.rs +++ b/substrate/client/cli/src/params/import_params.rs @@ -71,12 +71,20 @@ pub struct ImportParams { /// 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. From d45574ed392bfba26d3b93c4e4a2f0513b8798af Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 20:43:21 -0300 Subject: [PATCH 09/12] fix: :adhesive_bandage: Change `with_wasmtime_precompiled_path` to `with_optional_wasmtime_precompiled_path` to handle optional inside --- cumulus/parachain-template/node/src/service.rs | 12 ++++-------- substrate/client/executor/src/executor.rs | 16 ++++++++-------- substrate/client/service/src/builder.rs | 12 ++++-------- 3 files changed, 16 insertions(+), 24 deletions(-) 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/client/executor/src/executor.rs b/substrate/client/executor/src/executor.rs index c6831f25a0e5..dcce592286ac 100644 --- a/substrate/client/executor/src/executor.rs +++ b/substrate/client/executor/src/executor.rs @@ -195,17 +195,17 @@ 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. - /// - /// By default there is no `wasmtime_precompiled_path` given. - pub fn with_wasmtime_precompiled_path( + /// The `maybe_wasmtime_precompiled_path` is an optional whose inner value (if `Some`) is a path to a + /// directory where the executor load precompiled wasmtime modules. + 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 } diff --git a/substrate/client/service/src/builder.rs b/substrate/client/service/src/builder.rs index 31ce9755503a..f5b76518a9f4 100644 --- a/substrate/client/service/src/builder.rs +++ b/substrate/client/service/src/builder.rs @@ -247,18 +247,14 @@ pub fn new_wasm_executor(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. From 8a7442da488bc5b7e8c97cdee933a4bdb1683574 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 20:49:33 -0300 Subject: [PATCH 10/12] fix: :bulb: Improve doc comments --- substrate/client/executor/src/executor.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/substrate/client/executor/src/executor.rs b/substrate/client/executor/src/executor.rs index dcce592286ac..a50fa5230efc 100644 --- a/substrate/client/executor/src/executor.rs +++ b/substrate/client/executor/src/executor.rs @@ -195,10 +195,12 @@ impl WasmExecutorBuilder { self } - /// Create the wasm executor with the given `maybe_wasmtime_precompiled_path`, provided that it is `Some` + /// Create the wasm executor with the given `maybe_wasmtime_precompiled_path`, provided that it is `Some`. /// - /// The `maybe_wasmtime_precompiled_path` is an optional whose inner value (if `Some`) 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. + /// + /// If `None`, calling this function will have no impact on the wasm executor being built. pub fn with_optional_wasmtime_precompiled_path( mut self, maybe_wasmtime_precompiled_path: Option>, @@ -251,7 +253,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, } From 9a4b02bd619592fd6841371782be2044f41ce477 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 21:07:50 -0300 Subject: [PATCH 11/12] fix: :adhesive_bandage: Write directly to file instead of creating it and then writing --- substrate/client/executor/src/wasm_runtime.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/substrate/client/executor/src/wasm_runtime.rs b/substrate/client/executor/src/wasm_runtime.rs index 5693ecd22533..df77cdce0cc3 100644 --- a/substrate/client/executor/src/wasm_runtime.rs +++ b/substrate/client/executor/src/wasm_runtime.rs @@ -35,7 +35,6 @@ use sp_version::RuntimeVersion; use sp_wasm_interface::HostFunctions; use std::{ - io::Write, panic::AssertUnwindSafe, path::{Path, PathBuf}, sync::Arc, @@ -437,18 +436,9 @@ 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!("{}{}", PRECOM_FILENAME_PREFIX, &artifact_version)), - ) - .map_err(|e| { - WasmError::Other(format!( - "Fail to create file '{}{}', I/O Error: {}", - PRECOM_FILENAME_PREFIX, &artifact_version, e - )) - })?; - file.write_all(&serialized_precompiled_wasm).map_err(|e| { - WasmError::Other(format!("Fail to write precompiled artifact, I/O Error: {}", e)) - })?; + std::fs::write(wasmtime_precompiled_path.join(format!("{PRECOM_FILENAME_PREFIX}{artifact_version}")), &serialized_precompiled_wasm).map_err(|e| { + WasmError::Other(format!("Fail to write precompiled artifact, I/O Error: {}", e)) + })?; Ok(()) } From 57392f04b475f3cad29a48a6378ed479ced858f2 Mon Sep 17 00:00:00 2001 From: Facundo Farall <37149322+ffarall@users.noreply.github.com> Date: Tue, 3 Dec 2024 21:17:14 -0300 Subject: [PATCH 12/12] style: :art: Apply `cargo fmt` --- .../cli/src/commands/precompile_wasm_cmd.rs | 14 ++++++---- substrate/client/executor/src/executor.rs | 9 +++--- substrate/client/executor/src/wasm_runtime.rs | 28 ++++++++++++------- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs index af836c7c6af5..a3b7a8af25a8 100644 --- a/substrate/client/cli/src/commands/precompile_wasm_cmd.rs +++ b/substrate/client/cli/src/commands/precompile_wasm_cmd.rs @@ -33,14 +33,14 @@ use sc_executor::{ }; use sc_service::ChainSpec; use sp_core::traits::RuntimeCode; -use sp_runtime::traits::{Block as BlockT, Header, Hash}; +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 +/// 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)] @@ -50,7 +50,7 @@ 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, @@ -68,7 +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 @@ -117,7 +117,9 @@ impl PrecompileWasmCmd { code_fetcher: &sp_core::traits::WrappedRuntimeCode( wasm_bytecode.as_slice().into(), ), - hash: <::Hashing as Hash>::hash(&wasm_bytecode).as_ref().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( diff --git a/substrate/client/executor/src/executor.rs b/substrate/client/executor/src/executor.rs index a50fa5230efc..b9bccd3905df 100644 --- a/substrate/client/executor/src/executor.rs +++ b/substrate/client/executor/src/executor.rs @@ -195,11 +195,12 @@ impl WasmExecutorBuilder { self } - /// Create the wasm executor with the given `maybe_wasmtime_precompiled_path`, provided that it is `Some`. + /// Create the wasm executor with the given `maybe_wasmtime_precompiled_path`, provided that it + /// is `Some`. + /// + /// 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. /// - /// 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. - /// /// If `None`, calling this function will have no impact on the wasm executor being built. pub fn with_optional_wasmtime_precompiled_path( mut self, diff --git a/substrate/client/executor/src/wasm_runtime.rs b/substrate/client/executor/src/wasm_runtime.rs index df77cdce0cc3..96cb01a0ad5c 100644 --- a/substrate/client/executor/src/wasm_runtime.rs +++ b/substrate/client/executor/src/wasm_runtime.rs @@ -336,8 +336,8 @@ where 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 + // 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!( @@ -348,9 +348,10 @@ where // 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(), - ); + let module_version_strategy = + sc_executor_wasmtime::ModuleVersionStrategy::Custom( + artifact_version.clone(), + ); // # Safety // @@ -376,7 +377,10 @@ where } // 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); + tracing::warn!( + "❗️ Precompiled wasm with name '{}' doesn't exist, compiling it.", + file_name + ); } sc_executor_wasmtime::create_runtime::( @@ -436,9 +440,13 @@ pub fn precompile_and_serialize_versioned_wasm_runtime<'c>( )?; // Write in a file - std::fs::write(wasmtime_precompiled_path.join(format!("{PRECOM_FILENAME_PREFIX}{artifact_version}")), &serialized_precompiled_wasm).map_err(|e| { - WasmError::Other(format!("Fail to write precompiled artifact, I/O Error: {}", e)) - })?; + std::fs::write( + wasmtime_precompiled_path.join(format!("{PRECOM_FILENAME_PREFIX}{artifact_version}")), + &serialized_precompiled_wasm, + ) + .map_err(|e| { + WasmError::Other(format!("Fail to write precompiled artifact, I/O Error: {}", e)) + })?; Ok(()) } @@ -454,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();