feat: rework contract runtime error processing #4181

olonho · 2021-03-31T06:09:12Z

Make sure errors processed in the similar manner between Wasmer 0.x and Wasmer 1.x.
Introduced explicit non-deterministic error.

Test plan

Reworked error cases tests to make them uniform, then cargo test.

matklad · 2021-03-31T10:35:12Z

runtime/near-vm-errors/src/lib.rs

@@ -389,6 +383,9 @@ impl fmt::Display for WasmTrap {
            WasmTrap::GenericTrap => write!(f, "Generic trap."),
            WasmTrap::BreakpointTrap => write!(f, "Breakpoint trap."),
            WasmTrap::StackOverflow => write!(f, "Stack overflow."),
+            WasmTrap::NondeterministicTrap(message) => {
+                write!(f, "Non-deterministic trap: {}", message)
+            }


Undelated to PR, but

The error message given by the Display representation of an error type should be lowercase without trailing punctuation, and typically concise.

https://rust-lang.github.io/api-guidelines/interoperability.html?highlight=Error#error-types-are-meaningful-and-well-behaved-c-good-err

Reworked, but requires looking on that in separate PR.

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

bowenwang1996 · 2021-03-31T17:39:53Z

runtime/near-vm-errors/src/lib.rs

-    /// It can be convert back by wasmer_vm::TrapCode::from_str
-    Wasmer1Trap(String),
+    /// Non-deterministic error.
+    Nondeterministic(String),


Is this error deterministic, i.e, is it guaranteed that a nondeterministic error will be wrapped in this variant?

Yes, now it is.

bowenwang1996 · 2021-03-31T17:40:47Z

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

-                    VMError::FunctionCallError(FunctionCallError::WasmerRuntimeError(error_msg))
-                }
-            }
+            Ok(e) => return (&e).into(),


can probably just do an if let here

runtime/near-vm-runner/Cargo.toml

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

matklad · 2021-03-31T18:17:54Z

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

+                InvokeError::TrapCode { code, srcloc } => {
+                    VMError::FunctionCallError(FunctionCallError::Nondeterministic(format!(
+                        "Wasmer trap {} at {}",
+                        *code as u32, srcloc
+                    )))
                }


Hm, from the light reading of the code, I am not sure that this should always be nondeterministic. It seems that, eg, division by zero generates this trap, no? Do we have a test for division by zero?

Correct, reworked that piece.

matklad · 2021-03-31T18:20:18Z

runtime/runtime/src/actions.rs

+            FunctionCallError::CompilationError(_)
+            | FunctionCallError::LinkError { msg: _ }


Unrelated to the PR, but:

LinkError should probably be a tuple -- it's the only one with msg field, which feels inconsistent

I am not entirely sure we want to distinguish between CompilationError and LinkError, they feel sufficiently similar to me in the context of wasm execution.

Guess so, but this PR already become bigger than I want it to be.

Interesting, I, actually, prefer it to be structured (and update the rest of variants as well if necessary) since String does not capture the meaning of the text value 😐

The intention here is to have error separated between contract executor and the rest of logic. We can make it a enum, but the only purpose of this error is to show error to humans, that's why I made this change.

ailisp · 2021-04-01T20:40:32Z

runtime/near-vm-errors/src/lib.rs

-    /// It can be convert back by wasmer_vm::TrapCode::from_str
-    Wasmer1Trap(String),
+    /// Non-deterministic error.
+    Nondeterministic(String),


I doubt of still keep it WasmUnkownError is a good for wasmer 1. The internal of it is really unknown in wasmer 0.x (UnknownTrap etc.) but it's indeed known in wasmer 1. I suggest name it as WasmerRuntimeError or WasmerInternalError

Reworked, so that Nondeterministic is properly returned.

ailisp · 2021-04-01T20:48:00Z

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

+            Some(TrapCode::VMOutOfMemory) => {
+                FunctionCallError::Nondeterministic("Wasmer out of memory".to_string())
+            }
+            None => FunctionCallError::Nondeterministic(error_msg),


This is incorrect, None only happens when wasmer returns a Generic Error or User Error that failed to downcast to VMLogicError: https://github.com/wasmerio/wasmer/blob/9e0122e1f7bdb6fedf5fc17cc9a3a0fda30031a0/lib/engine/src/trap/error.rs#L18
And Generic Error is constructed by wasmer RuntimeError::new: https://github.com/wasmerio/wasmer/blob/9e0122e1f7bdb6fedf5fc17cc9a3a0fda30031a0/lib/engine/src/trap/error.rs#L61
User Error is any struct that has Error trait.

But both of them must come from constructing such structs in wasm file manually. We only consturct VMLogicError in near-vm-logic/src/logic.rs so other User Error and GenericError should not happen. So it is not an Nondeterministic, should be debug_assert!(false) or unreachable!()

Reworked code around this place.

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

ailisp · 2021-04-01T21:09:28Z

Overall looks good, this specific one looks questionable: #4181 (comment)

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

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

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

evgenykuzyakov · 2021-04-05T20:38:16Z

Adding @volovyk-s to check on error reporting, since it affects near-api-js

MaksymZavershynskyi · 2021-04-05T20:48:51Z

runtime/runtime/src/actions.rs

+            // TODO: shall we abort on unknown errors also?
+            | FunctionCallError::WasmUnknownError(_)
+            | FunctionCallError::HostError(_)
+            | FunctionCallError::EvmError(_) => {


@artob @frankbraun @joshuajbouw Given that EVM execution now does not differ from any other Wasm contract execution, why do we still have a separate EvmError?

MaksymZavershynskyi · 2021-04-05T21:00:54Z

runtime/runtime/src/actions.rs

-            result.result = Err(ActionErrorKind::FunctionCallError(err).into());
-            false
-        }
+        Some(VMError::FunctionCallError(err)) => match err {


It is unexpected that we have structured FunctionCallError information, but then we convert it to string here. What is the reason for doing it? This structure propagates structured errors all the way into RPC (CC @frol ) and we prefer to have error in RPC as structured as possible, so that we avoid bad JS code that parses error strings to understand whether there is a compilation error (which is something devtools would want to differentiate from execution errors).

We would need to cut a release for near-api-js when Wasm error structure changes due to Wasmer upgrade independently on whether we are returning it as string or strictly-typed structure. With strictly typed structure we just going to have less glue-code.

That's intentional, and for that particular distinction we can introduce CompilationError and ExecutionError, if this difference is indeed important. Currently, the only use of the error is showing it to developers in logs.

Currently, the only use of the error is showing it to developers in logs.

This is exactly what we want to improve by introducing sane granularity.

That's intentional, and for that particular distinction we can introduce CompilationError and ExecutionError, if this difference is indeed important.

Yes, that would be ideal. I think end-users care about three types of errors: UnknownMethod, CompilationError, ExecutionError.

Sounds good, please introduce these 3 levels of errors.

Cool, let's do that!

@volovyk-s BadArgument is a contract-specific error (because each contract decides to interpret it differently). We cannot differentiate it on a blockchain-level.

@olonho It seems like we are still combining all three kinds of errors into ActionErrorKind::FunctionCallError with string. Did you forget to push the commit?

and we prefer to have error in RPC as structured as possible

I want to push back a little bit against "as structured as possible errors". This is not strictly related to the PR, just the pattern I observer in Rust's error handling.

The issue with maximally stuctured errors is that they increase coupling, and sometimes it's better to stop at the goldilocks errors, which are structured just enough.

In the context of this PR, if we were to make error maximally structured, than we would need to expose wasmer 0.17 and wasmer 1.0 as is. And when we remove support for wasmer 0.17 or upgrade to (hypothetical) wasmer 2.0, we'd have to change the definition of the error type as well, and update the call-sites. This PR in some sense is the consequence of the fact that we did change error definition when upgrading to wasmer 1.0. This is a code smell, pointing out that the original error definition probably wasn't encapsulated enough.

But, clearly, exposing just the error message goes too far in the opposite direction, as it forces the clients to parse out the messages.

There's a goldilocks error pattern -- ErrorKind. The best example of it is std::io::Error:

The Error type itself is an opaque struct, which can be converted to string via Display. There's a .kind() method, which exposes non-exhaustive, field-less enum. This structure allows the client to behave differently for different sorts of errors, but allows the implementor to modify the errors without changes to the client.

To re-iterate, this is definitely not something we should do in the scope of this PR, and it's not necessary something we want to do even eventually (serializability of errors add an extra design dimension here which might change the tradeoffs). But it's useful to keep this bit of info in mind -- just exposing errors as is is the path of least resistance, but often not something you want to have at the API boundary.

References:

https://github.com/rust-lang-nursery/failure/blob/master/book/src/error-errorkind.md

https://matklad.github.io/2020/10/15/study-of-std-io-error.html

https://blog.burntsushi.net/rust-error-handling/

Exactly, it was intentional "right" coercion.

@matklad thanks for the detailed overview. I believe after some discussions we already arrived at the middle ground of introducing exactly the necessary variants that the consumer cares about 👍

frol · 2021-04-05T08:12:10Z

core/primitives/src/errors.rs

+    /// An error occurred during a `FunctionCall` Action, parameter is debug message.
+    FunctionCallError(String),


@olonho We try to bring more structured errors into the codebase, and this change goes in an opposite direction, so I wonder about the motivation behind this change 🤔

@bowenwang1996 I assume changes to this enum require a protocol upgrade (since the errors are part of the proof), a db migration (since the values are serialized and stored as borsh values), and a network upgrade (?)

P.S. If we still want to change it, I suggest capturing the logic in the code instead of comments:

Suggested change

/// An error occurred during a `FunctionCall` Action, parameter is debug message.

FunctionCallError(String),

/// An error occurred during a `FunctionCall` Action.

FunctionCallError { debug_message: String },

Do not think, that protocol changes in any way, as for protocol it's important presence of FunctionCallError error, not exact type of it.

I think for proof it is important to have an identical hash for the execution outcome, so it will require a protocol change unless I misunderstand something.

IIRC, when computing hash we explicitly ignore error messages, @matklad shown me the place where it's done.

@matklad shown me the place where it's done.

Over here:

nearcore/core/primitives/src/transaction.rs

Line 324 in b95eedf

ExecutionStatus::Failure(_) => PartialExecutionStatus::Failure,

Naturally, I might be missing something, but it seems resonable that, from protocol's point of view, we remeber only a single bit: was there some error?

Great, we don't need a protocol upgrade!

What about database migration?

I also see various ExecutionOutcome types used in chain/network. (ExecutionOutcome uses TxExecutionError which uses ActionError which uses ActionErrorKind and all of that gets borsh-serialized and should be compatible with the existing running nodes, right?)

Also do not think we shall store anything related to exact error kind in DB. If we do - guess it's a bug.

@olonho we definitely store the errors in the storage since we receive them from RPC: https://explorer.near.org/transactions/GcRiQb5PFwMAogtu8te6wDKMRg9pefbWV4nJx3iqijny

http post https://archival-rpc.mainnet.near.org/ jsonrpc=2.0 method=tx params:='["GcRiQb5PFwMAogtu8te6wDKMRg9pefbWV4nJx3iqijny", "tom.zest.near"]' id=dontcare

We do store them in database and if we've changed how the error serializes to borsh we should do a migration

Right, so could we do migration in the followup CL? And would be amazing, if we could cooperate on the migration effort.

frol · 2021-04-05T08:14:07Z

runtime/near-vm-errors/src/lib.rs

@@ -31,15 +29,11 @@ pub enum FunctionCallError {
    MethodResolveError(MethodResolveError),
    /// A trap happened during execution of a binary
    WasmTrap(WasmTrap),
-    WasmUnknownError,
+    WasmUnknownError(String),


Consider adding a description the the string value:

Suggested change

WasmUnknownError(String),

WasmUnknownError { debug_message: String },

frol · 2021-04-05T22:50:00Z

runtime/runtime/src/actions.rs

+            FunctionCallError::CompilationError(_)
+            | FunctionCallError::LinkError { msg: _ }


Interesting, I, actually, prefer it to be structured (and update the rest of variants as well if necessary) since String does not capture the meaning of the text value 😐

olonho · 2021-04-06T17:32:07Z

PTAL, merged suggestions.

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

runtime/runtime/src/actions.rs

MaksymZavershynskyi · 2021-04-06T23:18:35Z

runtime/runtime/src/actions.rs

-            result.result = Err(ActionErrorKind::FunctionCallError(err).into());
-            false
-        }
+        Some(VMError::FunctionCallError(err)) => match err {


@olonho It seems like we are still combining all three kinds of errors into ActionErrorKind::FunctionCallError with string. Did you forget to push the commit?

olonho · 2021-04-14T10:09:50Z

Addresses Max's request.

Max request was fixed.

…inistic

…tic (#4278) * fix: revert part of #4181 to classify FailedWithNoError as deterministic * bump version

Initially, we treated this error case as deterministic (so, we stored this error in our state, etc.) Then, in #4181 (comment) we reasoned that this error actually happens non-deterministically, so it's better to panic in this case. However, when rolling this out, we noticed that this error happens deterministically for at least one contract. So here we roll this back to a previous behavior and emit some deterministic error, which won't cause the node to panic.

olonho requested review from ailisp, bowenwang1996 and evgenykuzyakov as code owners March 31, 2021 06:09

matklad reviewed Mar 31, 2021

View reviewed changes

olonho requested review from frol, MaksymZavershynskyi and SkidanovAlex as code owners March 31, 2021 13:27

olonho changed the title ~~feat: rework Wasmer1 error processing~~ feat: rework contract runtime error processing Mar 31, 2021

bowenwang1996 reviewed Mar 31, 2021

View reviewed changes

matklad reviewed Mar 31, 2021

View reviewed changes

ailisp reviewed Apr 1, 2021

View reviewed changes

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

olonho force-pushed the wasmer1_errors branch from 6959896 to a2cc2f0 Compare April 2, 2021 14:21

matklad reviewed Apr 2, 2021

View reviewed changes

olonho force-pushed the wasmer1_errors branch from a2cc2f0 to e65354e Compare April 2, 2021 15:14

matklad mentioned this pull request Apr 2, 2021

Fully switch to wasmer 1 by default #4076

Closed

matklad approved these changes Apr 2, 2021

View reviewed changes

ailisp approved these changes Apr 2, 2021

View reviewed changes

bowenwang1996 reviewed Apr 2, 2021

View reviewed changes

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

matklad approved these changes Apr 5, 2021

View reviewed changes

evgenykuzyakov requested a review from volovyks April 5, 2021 20:37

MaksymZavershynskyi suggested changes Apr 5, 2021

View reviewed changes

frol requested changes Apr 5, 2021

View reviewed changes

olonho requested review from frol and MaksymZavershynskyi April 6, 2021 17:43

MaksymZavershynskyi previously requested changes Apr 6, 2021

View reviewed changes

olonho added 11 commits April 13, 2021 18:37

Review feedback.

60e6a2a

Fix EVM error message.

549f582

Review feedback: less coerse ActionErrorKind::FunctionCallError

b67513d

Explicit filed name for WasmUnknownError.

28c2d52

Further review feedback.

bdc1bce

Few final comments.

5656615

Ensure serialization format is compatible.

1541dcf

Better docs for ContractCallError.

882eb2e

Into -> From

1273bf2

Review feedback, add more tests.

0b350e6

Typo.

55d6801

olonho force-pushed the wasmer1_errors branch from 67dbcbb to 55d6801 Compare April 13, 2021 15:38

olonho requested a review from frol April 13, 2021 15:42

olonho and others added 3 commits April 13, 2021 19:57

Fix tests.

9bca1f0

Merge branch 'master' into wasmer1_errors

5afaca4

Merge branch 'master' into wasmer1_errors

a7b3a6d

bowenwang1996 approved these changes Apr 13, 2021

View reviewed changes

olonho added 2 commits April 14, 2021 08:30

Fix EVM errors.

378b864

Merge branch 'master' into wasmer1_errors

27541bf

olonho requested a review from EgorKulikov as a code owner April 14, 2021 05:31

Fix formatting.

4d4033a

frol approved these changes Apr 14, 2021

View reviewed changes

olonho merged commit a6bf293 into master Apr 14, 2021

olonho deleted the wasmer1_errors branch April 14, 2021 10:10

matklad added a commit to matklad/nearcore that referenced this pull request May 6, 2021

fix: revert part of near#4181 to classify FailedWithNoError as determ…

68e8e07

…inistic

chefsale pushed a commit that referenced this pull request May 6, 2021

fix: revert part of #4181 to classify FailedWithNoError as determinis…

daf13dd

…tic (#4278) * fix: revert part of #4181 to classify FailedWithNoError as deterministic * bump version

matklad mentioned this pull request May 13, 2021

fix: backport wasmer 0.17 FailWithNoError fix to master #4298

Merged

		FunctionCallError::CompilationError(_)
		\| FunctionCallError::LinkError { msg: _ }

		/// An error occurred during a `FunctionCall` Action, parameter is debug message.
		FunctionCallError(String),

	WasmUnknownError(String),
	WasmUnknownError { debug_message: String },

feat: rework contract runtime error processing #4181

feat: rework contract runtime error processing #4181

Conversation

olonho commented Mar 31, 2021

Test plan

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

ailisp Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailisp commented Apr 1, 2021 • edited Loading

evgenykuzyakov commented Apr 5, 2021

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

matklad Apr 7, 2021 • edited Loading

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

matklad Apr 6, 2021 • edited Loading

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 Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

olonho commented Apr 14, 2021

ailisp Apr 1, 2021 •

edited

Loading

ailisp commented Apr 1, 2021 •

edited

Loading

matklad Apr 7, 2021 •

edited

Loading

matklad Apr 6, 2021 •

edited

Loading

olonho commented Apr 6, 2021 •

edited

Loading