-
Notifications
You must be signed in to change notification settings - Fork 687
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
feat(runtime): upgrade to Wasmer 0.17 and nightly 2020-05-15 #2668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This potentially changes the protocol. @olonho if you are certain it doesn't change anything on the protocol level, i.e, any two contracts compiled using the two different wasmer version will have the exact same output, then no need to do anything. Otherwise please update protocol version.
Yes, actually it changes protocol since this is not implemented yet.. It's defined here https://github.com/nearprotocol/nearcore/blob/0456008492e57612e44b994aa75519a03606874c/core/chain-configs/src/lib.rs#L10 |
@bowenwang1996 @frol for me |
And before that
|
|
Well, the root cause of the explosion is:
It seems that the machine run out of file descriptors. The questions here are:
Postmortem questions: |
Same problem happens on another machine, so doesn't look like a fluke. |
This changes the protocol, because it changes the error semantics. |
RuntimeError::Trap { msg: _ } => VMError::FunctionCallError(WasmUnknownError), | ||
RuntimeError::Error { data } => { | ||
RuntimeError::InvokeError(invoke_error) => match invoke_error { | ||
InvokeError::FailedWithNoError => VMError::FunctionCallError(WasmUnknownError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be super helpful if we could comment on each of these arms explaining in what cases they occur so it is clear from reading the code that we exhaustively handle all possible deterministic and non-deterministic cases correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to document it per my understanding, but contract/docs on the Wasmer side would be cool.
@olonho it is a known issue that we use a lot of file descriptors because tests are run in parallel. Try to do something like |
Indeed, raising |
@olonho Is it waiting for more comments to the error branches or anything else is needed? |
In preparation for publishing nearcore crates, I discovered some unnecessary dependencies. [cargo-udeps](https://github.com/est31/cargo-udeps) helped to indentify all the unused dependencies: ``` $ cargo +nightly udeps --workspace --all-targets --all-features --bins --tests --benches --examples ``` P.S. I had to use a newer rustc (a74d1862d 2020-05-14 worked for me) UPD: I would like to integrate `cargo udeps` into CI, but that will require Rustc version bump (potentially happening in #2668) /cc @ailisp
Will add comments and raise protocol version, and we shall be all set. |
runtime/near-vm-runner/Cargo.toml
Outdated
wasmer-runtime = { version = "0.17", features = ["default-backend-singlepass"], default-features = false } | ||
wasmer-runtime-core = { version = "0.17" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin the version to exact number: =0.17.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olonho you need to add an empty migration script to update genesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
runtime/near-vm-runner/src/errors.rs
Outdated
// we weren't expecting or that we do not handle. | ||
// As of 0.17.0, thrown only from Cranelift BE. | ||
InvokeError::UnknownTrapCode { trap_code: _, srcloc: _ } => { | ||
panic!("Impossible UnknownTrapCode error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be helpful to add trap_code
and srcloc
to the panic message, just in case we mess up and have cranelift or llvm running. Same for other panics above.
RuntimeError::Trap { msg: _ } => VMError::FunctionCallError(WasmUnknownError), | ||
RuntimeError::Error { data } => { | ||
RuntimeError::InvokeError(invoke_error) => match invoke_error { | ||
// Indicates an exceptional circumstance such as a bug in Wasmer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanations are super useful! Thank you so much!
d03209d
to
af1b4d9
Compare
93bca69
to
2c2879f
Compare
Used https://crates.io/crates/rust-latest to find latest suitable nightly. Fixes #2055 Test plan --------- cargo test --all
Used https://crates.io/crates/rust-latest to find latest suitable nightly.
Fixes #2055
Test plan
cargo test --all