PVF Worker Process #1964

igor-egorov · 2024-01-30T10:47:57Z

Referenced issues

LimeChain/polkadot-conformance#26
#1800
https://github.com/qdrvm/KAGOME-audit/issues/23 (CPU time limit)

Description of the Change

Execute PVF call to runtime in a separate process thus it could be terminated on deadline.
All the configurations are supported:

WasmEdge Interpreted
WasmEdge Compiled
Binaryen
WAVM

Many thanks to @turuslan for help!

Benefits

Correct behavior.

Possible Drawbacks

Memory-related issues have to be addressed separately.
Further development could be continued to dedicate "prepare worker" too (now "execute worker" is separated only).

Usage Examples or Tests

New CLI flags introduced:

Mention --parachain-single-process to employ the former behavior which would not spawn additional child processes.
Set --parachain-check-deadline=5000 to use 5 seconds deadline for PVF blocks execution (if the first flag is not set).

Alternate Designs

Use fork? - not reasonable. Current approach is not going to spend double size of memory too, but much easier to handle control flow.

Signed-off-by: Igor Egorov <igor@qdrvm.io>

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Signed-off-by: Igor Egorov <igor@qdrvm.io> # Conflicts: # core/parachain/pvf/pvf_impl.cpp # core/parachain/pvf/pvf_impl.hpp # core/runtime/binaryen/core_api_factory_impl.cpp # core/runtime/binaryen/instance_environment_factory.cpp # core/runtime/common/module_instance.cpp # core/runtime/wavm/core_api_factory_impl.cpp # core/runtime/wavm/instance_environment_factory.cpp

Signed-off-by: Igor Egorov <igor@qdrvm.io>

@turuslan

Thanks to @turuslan! Signed-off-by: Igor Egorov <igor@qdrvm.io>

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Signed-off-by: Igor Egorov <igor@qdrvm.io>

kamilsa · 2024-02-07T09:01:26Z

core/runtime/common/module_instance.cpp

-            log,
-            "Creating wasm module with non-default :heappages value set to {}",
-            pages);
+    // TODO: should pvf use ":heappages" from relay chain?


Replace with issue link

":heappages" must limit memory #1962

kamilsa · 2024-02-07T09:01:54Z

core/parachain/pvf/run_worker.hpp

+  // run(scale::encode())
+  // run(make_shared<Buffer>(scale::encode()))


Should be removed?

kamilsa · 2024-02-07T09:02:01Z

core/parachain/pvf/run_worker.hpp

+                exe,
+                bp::args({"pvf-worker"}),
+                bp::std_out > pipe_stdout,
+                // bp::std_err > bp::null,


Harrm · 2024-02-08T12:23:49Z

core/parachain/pvf/kagome_pvf_worker.cpp

+            std::make_shared<libp2p::log::Configurator>()));
+    auto r = logging_system->configure();
+    if (not r.message.empty()) {
+      (r.has_error ? std::cerr : std::cout) << r.message << std::endl;


Why print warnings to cout? Especialy if it's used for IPC.

Harrm · 2024-02-08T12:24:08Z

core/parachain/pvf/kagome_pvf_worker.cpp

+      (r.has_error ? std::cerr : std::cout) << r.message << std::endl;
+    }
+    if (r.has_error) {
+      exit(EXIT_FAILURE);


Why is here it exit() but below it's return?

Harrm · 2024-02-08T12:26:04Z

core/parachain/pvf/pvf_impl.cpp

+              argv0().value(),
+              common::Buffer{scale::encode(input).value()},
+              cb);
+    // this will go away as soon as callWasm will have an async form


Is there an issue for it?

Harrm · 2024-02-08T12:27:07Z

core/parachain/pvf/run_worker.hpp

+  void runWorker(boost::asio::io_context &io_context,
+                 std::shared_ptr<libp2p::basic::Scheduler> scheduler,
+                 std::chrono::milliseconds timeout,
+                 const std::string &exe,


Should it be filesystem::path?

Not really needed as we don't perform any manipulations over the value and use/pass it as is.

Harrm · 2024-02-08T12:27:30Z

core/parachain/pvf/run_worker.hpp

+                 std::chrono::milliseconds timeout,
+                 const std::string &exe,
+                 common::Buffer input_,
+                 Cb cb_) {


From the signature and name it's not quite clear what is this callback. Please comment.

Harrm · 2024-02-08T12:29:09Z

core/utils/argv0.hpp

+#include <string>
+
+namespace kagome {
+  inline auto &argv0() {


There's "/proc/self/exe", but also why not call this function 'get_executable_name' or something similar?

Do you want LINUX ? read("/proc/self/exe") : APPLE ? _dyld_get_image_name(0) : ERROR?

I found argv0 the simplest and safe enough way to implement this. Anyways, thanks for the pointing the possible way too!

Harrm · 2024-02-08T12:30:10Z

node/main.cpp

+
+  if (argc > 1) {
+    std::string_view name{argv[1]};
+    if (name == "pvf-worker") {


This should be added to the output of kagome --help

I prefer to leave it undocumented as it is not supposed for manual use (at least the node run with the flag expects binary input).

Harrm · 2024-02-08T12:48:22Z

core/parachain/pvf/run_worker.hpp

+    };
+
+    io_context.post([scheduler, timeout, cb] {
+      scheduler->schedule([cb]() mutable { cb(std::errc::timed_out); },


Is it going to be called in a separate thread, or how is it guaranteed that the thread where it's scheduled to execute will be available?

Here we use the "main" io_context, thus this surely outlive the executor.

Signed-off-by: Igor Egorov <igor@qdrvm.io>

This reverts commit 5d419f2.

Signed-off-by: Igor Egorov <igor@qdrvm.io> Signed-off-by: turuslan <turuslan.devbox@gmail.com> Co-authored-by: turuslan <turuslan.devbox@gmail.com>

igor-egorov and others added 12 commits November 17, 2023 16:12

DRAFT

9520c1d

Signed-off-by: Igor Egorov <igor@qdrvm.io>

tmp

cce134e

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

DRAFT WasmEdge includes, can be built

3b9045c

Signed-off-by: Igor Egorov <igor@qdrvm.io>

DRAFT compiles and passes the test w WasmEdge

76e741c

Signed-off-by: Igor Egorov <igor@qdrvm.io>

DRAFT WAVM compiles and test passes

cdade3d

Signed-off-by: Igor Egorov <igor@qdrvm.io>

Pvf in child process: WasmEdge, Binaryen, WAVM - everything works

089fd8c

Signed-off-by: Igor Egorov <igor@qdrvm.io>

Pvf check in a separate process

f6c1d77

Thanks to @turuslan! Signed-off-by: Igor Egorov <igor@qdrvm.io>

fix build

23cde8e

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

fix test

0e03d11

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Merge branch 'master' into feature/pvf_worker_process

6e5db26

Remove unnecessary assertions

5012c55

Signed-off-by: Igor Egorov <igor@qdrvm.io>

igor-egorov requested review from Harrm and iceseer January 30, 2024 10:47

igor-egorov added 5 commits January 30, 2024 13:49

Merge branch 'master' into feature/pvf_worker_process

7f56bea

Remove unnecessary assertions

acc35f6

Signed-off-by: Igor Egorov <igor@qdrvm.io>

Merge branch 'master' into feature/pvf_worker_process

3bb655a

Merge branch 'master' into feature/pvf_worker_process

0051447

Merge branch 'master' into feature/pvf_worker_process

c27aa03

kamilsa requested review from kamilsa and removed request for iceseer February 2, 2024 09:13

kamilsa approved these changes Feb 7, 2024

View reviewed changes

Harrm reviewed Feb 8, 2024

View reviewed changes

Fix merge conflicts

c7cb0e2

Signed-off-by: Igor Egorov <igor@qdrvm.io>

igor-egorov requested a review from Harrm February 20, 2024 11:04

Harrm approved these changes Feb 20, 2024

View reviewed changes

igor-egorov merged commit 5d419f2 into master Feb 20, 2024
12 of 13 checks passed

igor-egorov deleted the feature/pvf_worker_process branch February 20, 2024 11:42

iceseer added a commit that referenced this pull request Feb 23, 2024

Revert "PVF Worker Process (#1964)"

ef8f1c3

This reverts commit 5d419f2.

iceseer pushed a commit that referenced this pull request Feb 23, 2024

PVF Worker Process (#1964)

7b650ea

Signed-off-by: Igor Egorov <igor@qdrvm.io> Signed-off-by: turuslan <turuslan.devbox@gmail.com> Co-authored-by: turuslan <turuslan.devbox@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PVF Worker Process #1964

PVF Worker Process #1964

igor-egorov commented Jan 30, 2024 •

edited

Loading

kamilsa Feb 7, 2024

turuslan Feb 8, 2024

kamilsa Feb 7, 2024

kamilsa Feb 7, 2024

Harrm Feb 8, 2024

Harrm Feb 8, 2024

Harrm Feb 8, 2024

Harrm Feb 8, 2024

igor-egorov Feb 20, 2024

Harrm Feb 8, 2024

Harrm Feb 8, 2024

turuslan Feb 8, 2024

igor-egorov Feb 20, 2024

Harrm Feb 8, 2024

igor-egorov Feb 20, 2024

Harrm Feb 8, 2024

igor-egorov Feb 20, 2024

		// run(scale::encode())
		// run(make_shared<Buffer>(scale::encode()))

PVF Worker Process #1964

PVF Worker Process #1964

Conversation

igor-egorov commented Jan 30, 2024 • edited Loading

Referenced issues

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Alternate Designs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-egorov commented Jan 30, 2024 •

edited

Loading