Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document internal RuntimeError possible values #1338

Closed
MarkMcCaskey opened this issue Mar 26, 2020 · 5 comments
Closed

Document internal RuntimeError possible values #1338

MarkMcCaskey opened this issue Mar 26, 2020 · 5 comments
Labels
📚 documentation Do you like to read? 🏚 stale Inactive issues or PR

Comments

@MarkMcCaskey
Copy link
Contributor

Brought to our attention in #1328:

RuntimeError should document that it can be returned by Wasmer's internal code and the a list of types that can be returned (so users can downcast them) should also be listed clearly in the docs.

We may want to additionally document this outside of the crate documentation

@maxzaver
Copy link

maxzaver commented Apr 8, 2020

To add more context. We would like to have exhaustive non-overlapping categories of errors into which wasmer_runtime::error::RuntimeError can be downcasted to:

  1. Errors returned by the host functions. If host function has return type Result<T, E> and during the contract execution it returns E we would like to be able to downcast RuntimeError to E. (Currently this is possible);
  2. Panics that happen inside the host functions. We would like to know when host function causes a panic;
  3. Panics caused by Wasm program, currently wasmer_runtime::ExceptionCode is a non-exhaustive subset of these panics.
  4. Non-deterministic error caused things not related to the program itself, e.g. running out of memory

(3) should be deterministic, meaning the same program executed on different platforms should return exactly the same error, assuming host functions have the same side effects.

Currently, there is no way to differentiate (2), (3), and (4). Also ExceptionCode is not exhaustive. Since we are executing Wasmer in the blockchain setting it poses an issue. Currently when we encounter error that is of type (2)+(3)+(4) that cannot be downcasted to ExceptionCode we need to:

  • Either terminate the entire service, since it might be (2) or (4) which might signal some resource exhaustion (continuing operating blockchain node in this condition usually leads to severe consequences, such as financial losses). Unfortunately, since it can be caused by a reproducible Wasm panic (3) an external user that submits a Wasm program to the blockchain can craft a code that will cause all blockchain nodes to terminate;
  • Continue operating. Unfortunately, if this errors turns out to be of type (2) or (4) it will be non-deterministic and non-deterministic execution is financially penalized in blockchains.

@MarkMcCaskey
Copy link
Contributor Author

@nearmax That's a great breakdown -- thanks! I agree completely that an error type upgrade is in order here to fix this situation.

4 might be tricky. I'll have to look into it more, but as far as I can remember there is no way to handle OOM in general on stable Rust (notably allocation by default in Rust doesn't return an error type, it just panics). There may be a feature on nightly that will allow this to work though -- presumably people writing embedded Rust eventually need such a thing. If so, we can add features that only work on nightly to do this.

In addition to this, or perhaps just being more explicit about an implementation detail, we should update all wasmer-internal errors to return a standardized type rather than ad-hoc strings.

@lexfrl
Copy link

lexfrl commented Apr 9, 2020

4 might be tricky. I'll have to look into it more, but as far as I can remember there is no way to handle OOM in general on stable Rust

It's absolutely ok to panic if there is a hardware problem. We want to make sure that a hardware problem will not false-positively interpreted as a valid behavior.

In short: we need to have a clear discriminator for errors which are defined by the spec (Wasm traps) and internal errors (like here

pub enum CompileError {
). Another source of non-determinism are panics which happen in host functions. We need to eliminate all sources non-determinism in our use-case.

@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the 🏚 stale Inactive issues or PR label Jul 14, 2021
@Hywan
Copy link
Contributor

Hywan commented Jul 15, 2021

There is now a separation between RuntimeError, CompileError and Trap.

I believe it's solving this issue. I'm closing it. Feel free to reopen if you believe it's not enough!

@Hywan Hywan closed this as completed Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🏚 stale Inactive issues or PR
Projects
None yet
Development

No branches or pull requests

4 participants