feat(runtime): support more than one VM. #2853

olonho · 2020-06-15T17:07:57Z

Added support for basic work with Wasmtime 0.17. Error handling and
gas metering works the same way on all tests.

Test plan

Made most contract test to execute with two VMs and then
cargo test --all

gitpod-io · 2020-06-15T17:08:01Z

SkidanovAlex · 2020-06-15T17:58:58Z

That's a pretty lengthy change. Do we want to postpone it post-phase-I? (we already have a branch for such changes, it is called post-phase-1).

olonho · 2020-06-16T08:24:04Z

Yeah, we could, although if not implemented, phase-1 will be Wasmer only.

frol

This looks good to me, and I don't think it is too big, and it adds redundancy to catch errors, so I consider this to be a safe change if it does not affect performance too much.

frol · 2020-06-16T13:21:28Z

runtime/near-vm-runner/src/wasmtime_runner.rs

+
+    fn read_memory(&self, offset: u64, buffer: &mut [u8]) {
+        let offset = offset as usize;
+        unsafe {


Is it enforced by MemoryLike trait contract that we can use unsafe here? Can we have comments around every unsafe usage on why it is safe and minimize the block to the specific unsafe expression, so it is easier to see which method is exactly unsafe?

Yeah, added comments.

frol · 2020-06-16T13:24:36Z

runtime/near-vm-runner/src/wasmer_runner.rs

+    ) {
+        Ok(x) => x,
+        Err(_err) => panic!("Cannot create memory for a contract call"),
+    };


You can replace this match with .expect() method.

Sure, it was just moving existing code, although.

runtime/near-vm-logic/src/logic.rs

MaksymZavershynskyi · 2020-06-16T20:42:15Z

That's a pretty lengthy change. Do we want to postpone it post-phase-I? (we already have a branch for such changes, it is called post-phase-1).

It adds a lot of code, but it is not invasive to our current code -- me and @olonho discussed hiding Wasmtime behind the compilation flag for now and not add it to the genesis config, and not expose the environment variable. We have several goals with this code:

Verify Wasmer correctness by comparing it against Wasmtime in our tests -- something that is very important to do ASAP, because we had encountered Wasmer bugs in the past;
Have Wasmtime as a backup runtime -- if we find some critical issue with Wasmer, we would like to be able to switch to Wasmtime without being delayed by R&D;
In a long-run we want to run Wasmer and Wasmtime side-by-side in production, and if node encounters a situation when they disagree it should panic. This will add a significant level of safety to our runtime.

The first bullet point is something we would like to do ASAP, which we can achieve by hiding Wasmtime behind a compilation flag.

SkidanovAlex · 2020-06-16T22:24:54Z

Cool, I have no concerns pushing this to master.

runtime/near-vm-logic/src/config.rs

MaksymZavershynskyi

Looks good, let's replace env variable with compilation flag and get rid of anyhow.

MaksymZavershynskyi · 2020-06-17T16:46:41Z

runtime/near-vm-runner/src/wasmtime_runner.rs

+
+impl IntoVMError for anyhow::Error {
+    fn into_vm_error(self) -> VMError {
+        // TODO: incorrect


Could you please elaborate this comment?

Removed obsolete comment.

MaksymZavershynskyi · 2020-06-17T16:49:15Z

runtime/near-vm-runner/Cargo.toml

@@ -16,6 +16,8 @@ This crate implements the specification of the interface that Near blockchain ex
 cached = "0.12"
 wasmer-runtime = { version = "=0.17.0", features = ["default-backend-singlepass"], default-features = false }
 wasmer-runtime-core = { version = "=0.17.0" }
+wasmtime = "0.17.0"
+anyhow = "1.0.19"


In general, we try to avoid crates unless they are necessary. anyhow is a nice create but a renown author, but it is not necessary for here. Also, Rust community is still figuring out how to deal with errors nicely, so anyhow but become unpopular as quickly as it became popular. I suggest we don't us it here to avoid pulling in more dependencies.

This crate as needed, IMO, as otherwise I cannot implement IntoVMError for anyhow::Error, and this is an error returned by wasmtime::Linker::instantiate and function runner.

Sounds bad that wasmtime doesn't wrap and export it's own error type

I see, my original comment is probably wrong anyway, since wasmtime pulls in this dependency anyway.

MaksymZavershynskyi · 2020-06-17T16:50:12Z

runtime/near-vm-logic/src/config.rs

+
+impl Default for VMKind {
+    fn default() -> Self {
+        match env::var("NEAR_VM") {


I suggest we don't use env variables, but use a conditional compilation here. Then we can have two different implementations of Default depending whether feature is on or off.

Could you please elaborate why you find compilation option preferable here? IMO runtime flag gives us more flexibility, and I'd love to keep it, if possible.

I think @nearmax means a combination of feature flag + env var. Compiled with both feature flag gives you ability to "override runtime with env var", but default feature flag: default_runtime_is_wasmer, another feature flag: default_runtime_is_wasmtime

The problem is that we have validators that run our code and overall experiment with it. The general principle that we hold is that it should not be easy to run a node in a not-safe mode, just to avoid potential human error. Therefore various code parameters that are considered to be dangerous we hide behind compilation flags. In the case of wasmtime we don't want some validator to turn it on, because they think it is a safe configuration parameter, like timeout on our RPC, cache size, etc. There were cases in the blockchain world, even with some renowned blockchains, where validators would accidentally run a misconfigured node and get slashed, i.e. lose a lot of assets. Since wasmtime was not thoroughly studied by us, and it uses optimized compilation it is not safe to be used. We were even thinking going as far as protecting these kinds of features by three factors: compilation flag, command line argument, and special marker file -- meaning one would have to provide all three to enable it. Additionally, environment variables is not something that we use in nearcore, env variables make configuration dependencies hard to track (i.e. one cannot tell from the signature of the module's functions/structs what is the input, since env variable is a global statically accessible input).

Makes sense. Fixed the default selection to be compilation feature.

MaksymZavershynskyi · 2020-06-17T16:51:55Z

runtime/near-vm-runner/src/imports.rs

+            type VMResult<T> = ::std::result::Result<T, Trap>;
+            $(
+                #[allow(unused_parens)]
+                pub fn $func( $( $arg_name: rust2wasm!($arg_type) ),* ) -> VMResult<($( rust2wasm!($returns)),*)> {


Cool trick.

MaksymZavershynskyi · 2020-06-17T17:17:27Z

runtime/near-vm-runner/src/wasmtime_runner.rs

+            None => panic!("Error is not properly set"),
+        }
+    } else {
+        // VMError::FunctionCallError(LinkError { msg: format!("{:#?}", trap) })


Should we remove it here?

MaksymZavershynskyi · 2020-06-17T17:20:34Z

runtime/near-vm-runner/src/wasmtime_runner.rs

+        if self.i32_exit_status() == Some(239) {
+            trap_to_error(&self)
+        } else {
+            // VMError::FunctionCallError(LinkError { msg: format!("{:#?}", self) })


Should we remove it?

MaksymZavershynskyi · 2020-06-17T17:27:26Z

runtime/near-vm-runner/tests/test_error_cases.rs

+                )))
+            )
+        );
+    });


It would add safety if tests here were testing for all variants of FunctionCallError enum. It might go into a different PR.

Agree, we generally need to think about better coverage.

MaksymZavershynskyi · 2020-06-22T00:38:30Z

runtime/near-vm-logic/src/config.rs

+
+impl Default for VMKind {
+    fn default() -> Self {
+        match env::var("NEAR_VM") {


The problem is that we have validators that run our code and overall experiment with it. The general principle that we hold is that it should not be easy to run a node in a not-safe mode, just to avoid potential human error. Therefore various code parameters that are considered to be dangerous we hide behind compilation flags. In the case of wasmtime we don't want some validator to turn it on, because they think it is a safe configuration parameter, like timeout on our RPC, cache size, etc. There were cases in the blockchain world, even with some renowned blockchains, where validators would accidentally run a misconfigured node and get slashed, i.e. lose a lot of assets. Since wasmtime was not thoroughly studied by us, and it uses optimized compilation it is not safe to be used. We were even thinking going as far as protecting these kinds of features by three factors: compilation flag, command line argument, and special marker file -- meaning one would have to provide all three to enable it. Additionally, environment variables is not something that we use in nearcore, env variables make configuration dependencies hard to track (i.e. one cannot tell from the signature of the module's functions/structs what is the input, since env variable is a global statically accessible input).

MaksymZavershynskyi · 2020-06-22T00:39:24Z

runtime/near-vm-runner/Cargo.toml

@@ -16,6 +16,8 @@ This crate implements the specification of the interface that Near blockchain ex
 cached = "0.12"
 wasmer-runtime = { version = "=0.17.0", features = ["default-backend-singlepass"], default-features = false }
 wasmer-runtime-core = { version = "=0.17.0" }
+wasmtime = "0.17.0"
+anyhow = "1.0.19"


I see, my original comment is probably wrong anyway, since wasmtime pulls in this dependency anyway.

olonho · 2020-06-22T12:57:51Z

runtime/near-vm-runner/Cargo.toml

@@ -16,6 +16,8 @@ This crate implements the specification of the interface that Near blockchain ex
 cached = "0.12"
 wasmer-runtime = { version = "=0.17.0", features = ["default-backend-singlepass"], default-features = false }
 wasmer-runtime-core = { version = "=0.17.0" }
+wasmtime = { version = "0.17.0", features = ["lightbeam"], default-features = false }


Also note, that I tried to use not so optimizing lightbeam backend.

deny.toml

runtime/near-vm-runner/src/imports.rs

runtime/near-vm-runner/src/runner.rs

Test plan --------- NEAR_VM=wasmtime cargo test --package near-vm-runner --test test_error_cases -- --nocapture NEAR_VM=wasmer cargo test --package near-vm-runner --test test_error_cases -- --nocapture

olonho requested review from evgenykuzyakov, frol and MaksymZavershynskyi as code owners June 15, 2020 17:07

frol approved these changes Jun 16, 2020

View reviewed changes

olonho force-pushed the wasmtime branch 3 times, most recently from e8474df to 3cd17b2 Compare June 16, 2020 15:57

olonho requested review from bowenwang1996 and SkidanovAlex as code owners June 16, 2020 15:57

bowenwang1996 reviewed Jun 16, 2020

View reviewed changes

runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved

ailisp reviewed Jun 17, 2020

View reviewed changes

runtime/near-vm-logic/src/config.rs Show resolved Hide resolved

MaksymZavershynskyi requested a review from ailisp June 17, 2020 17:27

MaksymZavershynskyi assigned ailisp Jun 17, 2020

MaksymZavershynskyi suggested changes Jun 17, 2020

View reviewed changes

olonho force-pushed the wasmtime branch 3 times, most recently from 9c3b840 to 5f27d5c Compare June 20, 2020 08:41

olonho requested a review from MaksymZavershynskyi June 20, 2020 13:15

MaksymZavershynskyi suggested changes Jun 22, 2020

View reviewed changes

olonho force-pushed the wasmtime branch from 178506c to 0fef4a1 Compare June 22, 2020 10:40

olonho requested a review from MaksymZavershynskyi June 22, 2020 12:50

olonho commented Jun 22, 2020

View reviewed changes

frol reviewed Jun 22, 2020

View reviewed changes

deny.toml Show resolved Hide resolved

runtime/near-vm-runner/src/imports.rs Show resolved Hide resolved

runtime/near-vm-runner/src/runner.rs Show resolved Hide resolved

MaksymZavershynskyi approved these changes Jun 22, 2020

View reviewed changes

feat(runtime): support more than one VM.

c1dd289

Test plan --------- NEAR_VM=wasmtime cargo test --package near-vm-runner --test test_error_cases -- --nocapture NEAR_VM=wasmer cargo test --package near-vm-runner --test test_error_cases -- --nocapture

olonho added 11 commits June 23, 2020 17:06

Fix formatting.

ec50522

Error processing.

568d3c3

Simplify VM selection.

6312bdf

Make VM selection more explicit, run tests with both.

f1bc5ed

Review feedback.

0c79af1

Typo.

d5468a2

Update Cargo.lock.

41a4e3d

Use lightbeam.

d6d52ab

Whitelist wasmtime deps.

b5e5910

Run more tests on all VMs.

e559194

More test to multi VM.

8eaf46e

olonho force-pushed the wasmtime branch from a8e8af1 to 8eaf46e Compare June 23, 2020 14:36

Merge branch 'master' into wasmtime

526ab2e

olonho merged commit 3e5cf61 into master Jun 24, 2020

olonho deleted the wasmtime branch June 24, 2020 07:29

MaksymZavershynskyi mentioned this pull request Jun 24, 2020

Fix Wasmtime usage #2895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(runtime): support more than one VM. #2853

feat(runtime): support more than one VM. #2853

olonho commented Jun 15, 2020 •

edited

gitpod-io bot commented Jun 15, 2020 •

edited

SkidanovAlex commented Jun 15, 2020

olonho commented Jun 16, 2020

frol left a comment

frol Jun 16, 2020

olonho Jun 17, 2020

frol Jun 16, 2020

olonho Jun 17, 2020

MaksymZavershynskyi commented Jun 16, 2020

SkidanovAlex commented Jun 16, 2020

MaksymZavershynskyi left a comment

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

ailisp Jun 18, 2020

MaksymZavershynskyi Jun 22, 2020

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

ailisp Jun 18, 2020

MaksymZavershynskyi Jun 22, 2020

olonho Jun 22, 2020

MaksymZavershynskyi Jun 17, 2020

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

MaksymZavershynskyi Jun 17, 2020

olonho Jun 18, 2020

MaksymZavershynskyi Jun 22, 2020

MaksymZavershynskyi Jun 22, 2020

olonho Jun 22, 2020

feat(runtime): support more than one VM. #2853

feat(runtime): support more than one VM. #2853

Conversation

olonho commented Jun 15, 2020 • edited

Test plan

gitpod-io bot commented Jun 15, 2020 • edited

SkidanovAlex commented Jun 15, 2020

olonho commented Jun 16, 2020

frol left a 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

MaksymZavershynskyi commented Jun 16, 2020

SkidanovAlex commented Jun 16, 2020

MaksymZavershynskyi left a 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

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

olonho commented Jun 15, 2020 •

edited

gitpod-io bot commented Jun 15, 2020 •

edited