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

feat: stabilize wasmer2 protocol feature #4934

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Oct 5, 2021

No description provided.

@matklad
Copy link
Contributor Author

matklad commented Oct 5, 2021

Timeline:

  • This commit stabilizes wasmer2 on master
  • It'll get to testnet on Oct 18
  • It'll be on the test net for two weeks before we roll-out the release to the beta net

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:

  • fuzzed wasmer2 in isolation (to find crashes or other bad behaviors, no)
  • fuzzed wasmer2 against wasmer0 (to find if they behave identically. This uncovered a couple of minor wasm spec violations in wasmer0)
  • used the history replay tool to confirm that wasmer0 and wasmer2 behave identically on the mainnet history. This helped to uncover a bug in history replay tool, but didn't show specific discrepancies otherwise. Note: there's still some doubt as to whether the tool caught the entire history or a subset of it: https://near.zulipchat.com/#narrow/stream/295306-nearinc-contract-runtime/topic/playback.
  • carefully looked at the source code for wasmer2 to see if anything is odd.

@matklad
Copy link
Contributor Author

matklad commented Oct 5, 2021

cc @bowenwang1996 @olonho @ailisp @nearmax

@matklad matklad mentioned this pull request Oct 5, 2021
1 task
@matklad matklad force-pushed the m/stabilize-wasmer2 branch 3 times, most recently from 291bb7d to dda08dd Compare October 6, 2021 09:12
@matklad
Copy link
Contributor Author

matklad commented Oct 6, 2021

Hm, I don't understand the test failure, we are hitting this assert:

https://github.com/matklad/nearcore/blob/dda08dd417d29de9c749862bd4890055783301a0/core/primitives/src/types.rs#L565-L567

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) ?

@matklad
Copy link
Contributor Author

matklad commented Oct 6, 2021

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.

@matklad
Copy link
Contributor Author

matklad commented Oct 6, 2021

I think I've spotted a bug while looking at the code: #4938. Though, that fix just makes the assertion trigger later.

core/primitives/src/version.rs Outdated Show resolved Hide resolved
matklad and others added 2 commits October 6, 2021 14:15
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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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:

https://github.com/near/nearcore/pull/4934/files#diff-643527edf40e44364083d5b79dd004280537c45bc22e7627f503cdeae4a4976fL2661

@near-bulldozer near-bulldozer bot merged commit a1334ee into near:master Oct 7, 2021
near-bulldozer bot pushed a commit that referenced this pull request Oct 12, 2021
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants