Remove PVF memory reuse (#2142)

* Remove initial memory constant

* Remove module instance caching for single and multi process pvf execution 

* Remove runtime memory memset

.githooks/pre-commit

@@ -14,7 +14,7 @@ fi
 
 # check clang-format binary
 CLANG_FORMAT_ENABLED=1
-CLANG_FORMAT=$(which clang-format-15)
+CLANG_FORMAT=$(which clang-format-15 2>/dev/null)
 if [ -z "${CLANG_FORMAT}" ]; then
   CLANG_FORMAT=$(which clang-format)
   if [ -z "${CLANG_FORMAT}" ]; then

CMakePresets.json

@@ -3,7 +3,7 @@
     "configurePresets": [
         {
             "name": "base",
-            "toolchainFile": "${sourceDir}/cmake/toolchain/clang-15_cxx20.cmake",
+            "toolchainFile": "${sourceDir}/cmake/toolchain/clang-16_cxx20.cmake",
             "generator": "Ninja",
             "hidden": true
         },

core/parachain/pvf/kagome_pvf_worker.cpp

@@ -12,6 +12,7 @@
 #include <string>
 #include <system_error>
 #include <vector>
+#include "runtime/wabt/instrument.hpp"
 
 #ifdef __linux__
 #include <linux/landlock.h>
@@ -338,20 +339,21 @@ namespace kagome::parachain {
 #endif
     auto injector = pvf_worker_injector(input_config);
     OUTCOME_TRY(factory, createModuleFactory(injector, input_config.engine));
-    std::shared_ptr<runtime::ModuleInstance> instance;
+    std::shared_ptr<runtime::Module> module;
     while (true) {
       OUTCOME_TRY(input, decodeInput<PvfWorkerInput>());
       if (auto *code = std::get_if<PvfWorkerInputCode>(&input)) {
         OUTCOME_TRY(path, chroot_path(*code));
-        OUTCOME_TRY(module, factory->loadCompiled(path));
-        BOOST_OUTCOME_TRY(instance, module->instantiate());
+        BOOST_OUTCOME_TRY(module, factory->loadCompiled(path));
         continue;
       }
       auto &input_args = std::get<PvfWorkerInputArgs>(input);
-      if (not instance) {
+      if (not module) {
         SL_ERROR(logger, "PvfWorkerInputCode expected");
         return std::errc::invalid_argument;
       }
+      OUTCOME_TRY(instance, module->instantiate());
+
       OUTCOME_TRY(ctx, runtime::RuntimeContextFactory::stateless(instance));
       OUTCOME_TRY(
           result,

core/parachain/pvf/pool.cpp

@@ -44,12 +44,12 @@ namespace kagome::parachain {
 
   PvfPool::PvfPool(const application::AppConfiguration &app_config,
                    std::shared_ptr<runtime::ModuleFactory> module_factory,
-                   std::shared_ptr<runtime::InstrumentWasm> instrument)
+                   std::shared_ptr<runtime::WasmInstrumenter> instrument)
       : pool_{std::make_shared<runtime::RuntimeInstancesPoolImpl>(
-          app_config,
-          std::move(module_factory),
-          std::move(instrument),
-          app_config.parachainRuntimeInstanceCacheSize())} {}
+            app_config,
+            std::move(module_factory),
+            std::move(instrument),
+            app_config.parachainRuntimeInstanceCacheSize())} {}
 
   outcome::result<void> PvfPool::precompile(
       const Hash256 &code_hash,
@@ -65,4 +65,17 @@ namespace kagome::parachain {
         },
         config);
   }
+
+  std::optional<std::shared_ptr<const runtime::Module>> PvfPool::getModule(
+      const Hash256 &code_hash,
+      const runtime::RuntimeContext::ContextParams &config) const {
+    return pool_->getModule(code_hash, config);
+  }
+
+  std::filesystem::path PvfPool::getCachePath(
+      const common::Hash256 &code_hash,
+      const runtime::RuntimeContext::ContextParams &config) const {
+    return pool_->getCachePath(code_hash, config);
+  }
+
 }  // namespace kagome::parachain

core/parachain/pvf/pool.hpp

@@ -6,17 +6,20 @@
 
 #pragma once
 
+#include <filesystem>
 #include <memory>
 
+#include "common/optref.hpp"
 #include "runtime/runtime_context.hpp"
 
 namespace kagome::application {
   class AppConfiguration;
 }  // namespace kagome::application
 
 namespace kagome::runtime {
-  class InstrumentWasm;
+  class WasmInstrumenter;
   class ModuleFactory;
+  class Module;
   class RuntimeInstancesPoolImpl;
 }  // namespace kagome::runtime
 
@@ -28,11 +31,15 @@ namespace kagome::parachain {
    public:
     PvfPool(const application::AppConfiguration &app_config,
             std::shared_ptr<runtime::ModuleFactory> module_factory,
-            std::shared_ptr<runtime::InstrumentWasm> instrument);
+            std::shared_ptr<runtime::WasmInstrumenter> instrument);
 
-    auto &pool() const {
-      return pool_;
-    }
+    std::optional<std::shared_ptr<const runtime::Module>> getModule(
+        const Hash256 &code_hash,
+        const runtime::RuntimeContext::ContextParams &config) const;
+
+    std::filesystem::path getCachePath(
+        const common::Hash256 &code_hash,
+        const runtime::RuntimeContext::ContextParams &config) const;
 
     /**
      * Measures `kagome_parachain_candidate_validation_code_size` and

core/parachain/pvf/pvf_impl.cpp

@@ -12,6 +12,7 @@
 #include "application/app_state_manager.hpp"
 #include "blockchain/block_tree.hpp"
 #include "common/visitor.hpp"
+#include "log/profiling_logger.hpp"
 #include "metrics/histogram_timer.hpp"
 #include "parachain/pvf/module_precompiler.hpp"
 #include "parachain/pvf/pool.hpp"
@@ -26,6 +27,7 @@
 #include "runtime/module.hpp"
 #include "runtime/module_repository.hpp"
 #include "runtime/runtime_code_provider.hpp"
+#include "runtime/runtime_context.hpp"
 #include "runtime/runtime_instances_pool.hpp"
 #include "scale/std_variant.hpp"
 
@@ -44,7 +46,7 @@ OUTCOME_CPP_DEFINE_CATEGORY(kagome::parachain, PvfError, e) {
   using kagome::parachain::PvfError;
   switch (e) {
     case PvfError::PERSISTED_DATA_HASH:
-      return "Incorrect Perssted Data hash";
+      return "Incorrect Persisted Data hash";
     case PvfError::NO_CODE:
       return "No code";
     case PvfError::NO_PERSISTED_DATA:
@@ -63,6 +65,8 @@ OUTCOME_CPP_DEFINE_CATEGORY(kagome::parachain, PvfError, e) {
       return "Commitments hash mismatch";
     case PvfError::OUTPUTS:
       return "ValidationResult is invalid";
+    case PvfError::COMPILATION_ERROR:
+      return "PVF code failed to compile";
   }
   return "unknown error (kagome::parachain::PvfError)";
 }
@@ -318,15 +322,27 @@ namespace kagome::parachain {
     constexpr auto name = "validate_block";
     CB_TRYV(pvf_pool_->precompile(code_hash, code_zstd, executor_params));
     if (not app_configuration_->usePvfSubprocess()) {
-      CB_TRY(
-          auto instance,
-          pvf_pool_->pool()->instantiateFromCode(
-              code_hash, [&] { return PvfError::NO_CODE; }, executor_params));
-      CB_TRY(auto ctx, ctx_factory_->stateless(instance));
+      // Reusing instances for PVF calls doesn't work, runtime calls start to
+      // crash on access out of memory bounds
+      KAGOME_PROFILE_START_L(log_, single_process_runtime_instantitation);
+      auto module_opt = pvf_pool_->getModule(code_hash, executor_params);
+      if (!module_opt) {
+        SL_ERROR(log_,
+                 "Runtime module supposed to be precompiled for parachain ID "
+                 "{}, but it's not. This indicates a bug.",
+                 receipt.descriptor.para_id);
+        cb(PvfError::NO_CODE);
+        return;
+      }
+      auto &wasm_module = module_opt.value();
+      CB_TRY(auto instance, wasm_module->instantiate());
+      CB_TRY(auto ctx, runtime::RuntimeContextFactory::stateless(instance));
+      KAGOME_PROFILE_END_L(log_, single_process_runtime_instantitation);
+      KAGOME_PROFILE_START_L(log_, single_process_runtime_call);
       return cb(executor_->call<ValidationResult>(ctx, name, params));
     }
     workers_->execute({
-        pvf_pool_->pool()->cachePath(code_hash, executor_params),
+        pvf_pool_->getCachePath(code_hash, executor_params),
         scale::encode(params).value(),
         [cb{std::move(cb)}](outcome::result<common::Buffer> r) {
           if (r.has_error()) {

core/parachain/pvf/pvf_impl.hpp

@@ -13,6 +13,7 @@
 #include "crypto/sr25519_provider.hpp"
 #include "log/logger.hpp"
 #include "runtime/runtime_api/parachain_host.hpp"
+#include "runtime/wabt/instrument.hpp"
 
 namespace kagome {
   class PoolHandler;
@@ -46,6 +47,7 @@ namespace kagome::parachain {
     OUTPUTS,
     PERSISTED_DATA_HASH,
     NO_CODE,
+    COMPILATION_ERROR
   };
 }  // namespace kagome::parachain
 

core/parachain/pvf/workers.cpp

@@ -145,7 +145,9 @@ namespace kagome::parachain {
               return job.cb(r.error());
             }
             self->writeCode(
-                std::move(job), {std::move(process)}, std::move(used));
+                std::move(job),
+                {.process = std::move(process), .code = std::nullopt},
+                std::move(used));
           });
       return;
     }

core/runtime/binaryen/instance_environment_factory.cpp

@@ -9,6 +9,7 @@
 #include "host_api/host_api_factory.hpp"
 #include "runtime/binaryen/binaryen_memory_provider.hpp"
 #include "runtime/common/trie_storage_provider_impl.hpp"
+#include "runtime/wabt/instrument.hpp"
 
 namespace kagome::runtime::binaryen {
 

core/runtime/binaryen/memory_impl.hpp

@@ -9,7 +9,6 @@
 #include <binaryen/shell-interface.h>
 
 #include <array>
-#include <cstring>  // for std::memset in gcc
 #include <memory>
 #include <optional>
 #include <unordered_map>

core/runtime/common/core_api_factory_impl.cpp

@@ -30,9 +30,8 @@ namespace kagome::runtime {
 
   CoreApiFactoryImpl::CoreApiFactoryImpl(
       std::shared_ptr<crypto::Hasher> hasher,
-      LazySPtr<RuntimeInstancesPool> module_factory)
-      : hasher_{std::move(hasher)},
-        module_factory_{std::move(module_factory)} {}
+      LazySPtr<RuntimeInstancesPool> instance_pool)
+      : hasher_{std::move(hasher)}, instance_pool_{std::move(instance_pool)} {}
 
   outcome::result<std::unique_ptr<RestrictedCore>> CoreApiFactoryImpl::make(
       BufferView code_zstd,
@@ -43,15 +42,15 @@ namespace kagome::runtime {
     if (version) {
       return std::make_unique<GetVersion>(*version);
     }
-    if (not module_factory_.get()) {
+    if (not instance_pool_.get()) {
       return std::errc::not_supported;
     }
     MemoryLimits config;
     BOOST_OUTCOME_TRY(config.heap_alloc_strategy,
                       heapAllocStrategyHeappagesDefault(
                           *storage_provider->getCurrentBatch()));
     OUTCOME_TRY(instance,
-                module_factory_.get()->instantiateFromCode(
+                instance_pool_.get()->instantiateFromCode(
                     code_hash,
                     [&] { return std::make_shared<Buffer>(code); },
                     {config}));

core/runtime/common/core_api_factory_impl.hpp

@@ -35,15 +35,15 @@ namespace kagome::runtime {
         public std::enable_shared_from_this<CoreApiFactoryImpl> {
    public:
     explicit CoreApiFactoryImpl(std::shared_ptr<crypto::Hasher> hasher,
-                                LazySPtr<RuntimeInstancesPool> module_factory);
+                                LazySPtr<RuntimeInstancesPool> instance_pool);
 
     outcome::result<std::unique_ptr<RestrictedCore>> make(
         BufferView code,
         std::shared_ptr<TrieStorageProvider> storage_provider) const override;
 
    private:
     std::shared_ptr<crypto::Hasher> hasher_;
-    LazySPtr<RuntimeInstancesPool> module_factory_;
+    LazySPtr<RuntimeInstancesPool> instance_pool_;
   };
 
 }  // namespace kagome::runtime

core/runtime/common/module_instance.cpp

@@ -61,9 +61,6 @@ namespace kagome::runtime {
           "__heap_base too low, allocations will overwrite wasm data segments");
     }
 
-    auto memory_size = memory.memory()->size();
-    memset(memory.view(0, memory_size).value().data(), 0, memory_size);
-
     forDataSegment([&](auto offset, auto segment) {
       memory.storeBuffer(offset, segment);
     });

core/runtime/common/runtime_instances_pool.cpp

@@ -83,7 +83,7 @@ namespace kagome::runtime {
   RuntimeInstancesPoolImpl::RuntimeInstancesPoolImpl(
       const application::AppConfiguration &app_config,
       std::shared_ptr<ModuleFactory> module_factory,
-      std::shared_ptr<InstrumentWasm> instrument,
+      std::shared_ptr<WasmInstrumenter> instrument,
       size_t capacity)
       : cache_dir_{app_config.runtimeCacheDirPath()},
         module_factory_{std::move(module_factory)},
@@ -98,14 +98,14 @@ namespace kagome::runtime {
       const GetCode &get_code,
       const RuntimeContext::ContextParams &config) {
     std::unique_lock lock{pools_mtx_};
-    OUTCOME_TRY(module, getModule(lock, code_hash, get_code, config));
+    OUTCOME_TRY(module, getPool(lock, code_hash, get_code, config));
     OUTCOME_TRY(instance, module.get().instantiate(lock));
     BOOST_ASSERT(shared_from_this());
     return std::make_shared<BorrowedInstance>(
         weak_from_this(), code_hash, config, std::move(instance));
   }
 
-  std::filesystem::path RuntimeInstancesPoolImpl::cachePath(
+  std::filesystem::path RuntimeInstancesPoolImpl::getCachePath(
       const CodeHash &code_hash,
       const RuntimeContext::ContextParams &config) const {
     std::string name;
@@ -140,13 +140,25 @@ namespace kagome::runtime {
       const GetCode &get_code,
       const RuntimeContext::ContextParams &config) {
     std::unique_lock lock{pools_mtx_};
-    OUTCOME_TRY(getModule(lock, code_hash, get_code, config));
+    OUTCOME_TRY(getPool(lock, code_hash, get_code, config));
     return outcome::success();
   }
 
+  std::optional<std::shared_ptr<const Module>>
+  RuntimeInstancesPoolImpl::getModule(
+      const CodeHash &code_hash, const RuntimeContext::ContextParams &config) {
+    std::unique_lock lock{pools_mtx_};
+    Key key{code_hash, config};
+    auto pool_opt = pools_.get(key);
+    if (!pool_opt) {
+      return std::nullopt;
+    }
+    return pool_opt->get().module;
+  }
+
   outcome::result<
       std::reference_wrapper<RuntimeInstancesPoolImpl::InstancePool>>
-  RuntimeInstancesPoolImpl::getModule(
+  RuntimeInstancesPoolImpl::getPool(
       std::unique_lock<std::mutex> &lock,
       const CodeHash &code_hash,
       const GetCode &get_code,
@@ -186,7 +198,7 @@ namespace kagome::runtime {
     BOOST_ASSERT(is_inserted);
     BOOST_ASSERT(iter != compiling_modules_.end());
     l.unlock();
-    auto path = cachePath(code_hash, config);
+    auto path = getCachePath(code_hash, config);
     auto res = [&]() -> CompilationResult {
       std::error_code ec;
       if (not std::filesystem::exists(path, ec)) {