-
Notifications
You must be signed in to change notification settings - Fork 627
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: stabilize wasmer2 protocol feature #4934
Conversation
Timeline:
Quality control: QA here is owned by the contract runtime team. We believe that the new implementation is significantly more reliable than the current one: it no longer uses unix-signals to handle traps, and it is strictly more correct with respect to the wasm specification. To check that we:
|
291bb7d
to
dda08dd
Compare
Hm, I don't understand the test failure, we are hitting this assert: Here's the test output: $ RUST_BACKTRACE=1 cargo test --package integration-tests --test client --all-features -- process_blocks::test_wasmer2_upgrade --exact --nocapture
Finished test [unoptimized + debuginfo] target(s) in 0.14s
Running tests/client/main.rs (target/debug/deps/client-08c58cedc18e3688)
running 1 test
thread 'process_blocks::test_wasmer2_upgrade' panicked at 'assertion failed: false', core/primitives/src/types.rs:567:21
stack backtrace:
0: rust_begin_unwind
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panicking.rs:515:5
1: core::panicking::panic_fmt
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/panicking.rs:92:14
2: core::panicking::panic
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/panicking.rs:50:5
3: near_primitives::types::validator_stake::ValidatorStake::into_v1
at /home/matklad/projects/nearcore/core/primitives/src/types.rs:567:21
4: near_chain::chain::Chain::compute_bp_hash::{{closure}}
at /home/matklad/projects/nearcore/chain/chain/src/chain.rs:400:70
5: core::iter::adapters::map::map_fold::{{closure}}
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/adapters/map.rs:82:28
6: core::iter::traits::iterator::Iterator::fold
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:2174:21
7: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/adapters/map.rs:122:9
8: core::iter::traits::iterator::Iterator::for_each
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:737:9
9: <alloc::vec::Vec<T,A> as alloc::vec::spec_extend::SpecExtend<T,I>>::spec_extend
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/spec_extend.rs:40:17
10: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/spec_from_iter_nested.rs:56:9
11: alloc::vec::source_iter_marker::<impl alloc::vec::spec_from_iter::SpecFromIter<T,I> for alloc::vec::Vec<T>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/source_iter_marker.rs:41:20
12: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/mod.rs:2453:9
13: core::iter::traits::iterator::Iterator::collect
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:1749:9
14: near_chain::chain::Chain::compute_bp_hash
at /home/matklad/projects/nearcore/chain/chain/src/chain.rs:400:40
15: near_chain::chain::Chain::new
at /home/matklad/projects/nearcore/chain/chain/src/chain.rs:281:13
16: near_client::client::Client::new
at /home/matklad/projects/nearcore/chain/client/src/client.rs:115:21
17: near_client::test_utils::setup_client_with_runtime
at /home/matklad/projects/nearcore/chain/client/src/test_utils.rs:1061:22
18: near_client::test_utils::TestEnvBuilder::build::{{closure}}
at /home/matklad/projects/nearcore/chain/client/src/test_utils.rs:1226:25
19: core::iter::adapters::map::map_fold::{{closure}}
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/adapters/map.rs:82:28
20: core::iter::traits::iterator::Iterator::fold
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:2174:21
21: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/adapters/map.rs:122:9
22: core::iter::traits::iterator::Iterator::for_each
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:737:9
23: <alloc::vec::Vec<T,A> as alloc::vec::spec_extend::SpecExtend<T,I>>::spec_extend
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/spec_extend.rs:40:17
24: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/spec_from_iter_nested.rs:56:9
25: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/spec_from_iter.rs:33:9
26: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/alloc/src/vec/mod.rs:2453:9
27: core::iter::traits::iterator::Iterator::collect
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/iter/traits/iterator.rs:1749:9
28: near_client::test_utils::TestEnvBuilder::build
at /home/matklad/projects/nearcore/chain/client/src/test_utils.rs:1221:17
29: client::process_blocks::test_wasmer2_upgrade
at ./tests/client/process_blocks.rs:2588:23
30: client::process_blocks::test_wasmer2_upgrade::{{closure}}
at ./tests/client/process_blocks.rs:2573:1
31: core::ops::function::FnOnce::call_once
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ops/function.rs:227:5
32: core::ops::function::FnOnce::call_once
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
test process_blocks::test_wasmer2_upgrade ... FAILED
failures:
failures:
process_blocks::test_wasmer2_upgrade
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 92 filtered out; finished in 0.07s
error: test failed, to rerun pass '-p integration-tests --test client' @birchmd do you have an idea what's going on here (the assert was instroduced in #4193) ? |
I've noticed that we already disable some tests here with v3 blocks, so I did the same, although I don't really like this. |
I think I've spotted a bug while looking at the code: #4938. Though, that fix just makes the assertion trigger later. |
049a3d8
to
36c7102
Compare
Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>
@@ -29,7 +29,7 @@ impl VMKind { | |||
return VMKind::Wasmer2; | |||
} | |||
|
|||
if checked_feature!("protocol_feature_wasmer2", Wasmer2, protocol_version) { | |||
if checked_feature!("stable", Wasmer2, protocol_version) { |
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.
Not sure if I 100% intention here. In which case we shall actually select Wasmer0 now? For older protocol versions?
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.
Yeah, wasmer0 will be selected if the protocol version at the runtime for a specific block is old. Covered by this assertion in tests:
# Feature to stabilize This PR stabilizes new, lower regular_op_cost. It is lowered from `3_856_371` to `2_207_874`. regular_op_cost is roughly the cost of primitive wasm operation, and is a major cost factor for computation-intensive contracts. The underlying change which allowed us to lower the cost is the switch from `wasmer0` to `wasmer2` implementation, which improved real wasmer perfomance across the board # Testing and QA Cost measurement was reproduced by several people using both old and new estimator. The cost measurement itself is rather simple (probably the simplest of all our costs), so the relative risk here is low. Additionally, the reduction "makes sense" in terms of various ad-hoc comparisons of wall-clock times I did when comparing wasm runtimes. Note that we are stabilizing this feature on a very short notice. While this in general is risky, I think it's ok for this particular case, as it is just cost reduction, and there's strong evidence that new wasmer is indeed faster. In a sense, *most* risk here is actually carried by #4934 , but that was on nightly for a significantly longed time. # Checklist - [ ] No nightly test regression - [ ] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
No description provided.