feat: fvm-wasm-instrument stack limiter and gas #494

magik6k · 2022-04-14T11:42:59Z

No description provided.

Stebalien · 2022-04-19T10:17:58Z

This looks like the correct approach, but do we need to do any pre-validation first?

magik6k · 2022-04-20T12:05:50Z

Depends on filecoin-project/fvm-wasm-instrument#3

fvm/src/gas/mod.rs

fvm/src/syscalls/bind.rs

fvm/src/gas/mod.rs

Stebalien

partial review

fvm/src/call_manager/default.rs

fvm/src/gas/mod.rs

fvm/src/gas/price_list.rs

fvm/src/machine/engine.rs

fvm/src/syscalls/bind.rs

fvm/src/call_manager/default.rs

fvm/src/gas/mod.rs

fvm/src/gas/price_list.rs

fvm/src/kernel/mod.rs

fvm/src/machine/engine.rs

magik6k · 2022-04-28T17:59:15Z

Related ffi PR: filecoin-project/filecoin-ffi#273

Stebalien

nits, but the logic LGTM.

fvm/src/call_manager/default.rs

fvm/src/gas/mod.rs

fvm/src/gas/price_list.rs

fvm/src/lib.rs

Stebalien · 2022-05-04T02:28:25Z

fvm/src/machine/engine.rs

@@ -55,23 +104,20 @@ pub fn default_wasmtime_config() -> wasmtime::Config {
    // > not enabled by default.
    c.cranelift_nan_canonicalization(true);

-    // c.cranelift_opt_level(Speed); ?
+    // Set to something much higher than the instrumented limiter.
+    c.max_wasm_stack(64 << 20).unwrap();


TODO: make sure this doesn't force us to allocate 64MiB per container.

Is this just a backstop? Why do we need it? I guess because it can't be disabled entirely?

Yeah, the default is 512k

As far as I can tell wasm execution uses the native stack - when this is set to a high value the out_of_stack test will fail with:

thread 'out_of_stack' has overflowed its stack fatal runtime error: stack overflow

Given that, I don't think we're allocating here

fvm/src/machine/engine.rs

Stebalien · 2022-05-04T04:04:15Z

fvm/src/syscalls/bind.rs

+                        let out = syscall(ctx $(, $t)*).into();
+
+                        let result = match out {


I think we can re-combine these lines now.

(We'll split them again if we merge #465.)

Stebalien · 2022-05-04T04:04:54Z

fvm/src/syscalls/bind.rs

                        let (mut memory, mut data) = memory_and_data(&mut caller)?;
                        let ctx = Context{kernel: &mut data.kernel, memory: &mut memory};
-                        let result = match syscall(ctx $(, $t)*).into()? {
-                            Ok(_) => {
+                        let out = syscall(ctx $(, $t)*).into();
+
+                        let result = match out {
+                            Ok(Ok(_)) => {
                                log::trace!("syscall {}::{}: ok", module, name);
                                data.last_error = None;
-                                0
+                                Ok(0)
                            },
-                            Err(err) => {
+                            Ok(Err(err)) => {
                                let code = err.1;
                                log::trace!("syscall {}::{}: fail ({})", module, name, code as u32);
                                data.last_error = Some(backtrace::Cause::new(module, name, err));
-                                code as u32
+                                Ok(code as u32)
                            },
+                            Err(e) => Err(e.into()),
                        };


I would consider wrapping all of this in a closure (and immediately calling it) to make trapping errors a bit easier.

Stebalien · 2022-05-04T04:06:13Z

testing/integration/src/builtin.rs

    (NetworkVersion::V14, actors_v6::BUNDLE_CAR),
    (NetworkVersion::V15, actors_v7::BUNDLE_CAR),
+    (NetworkVersion::V16, actors_v7::BUNDLE_CAR), // todo bad hack


I guess this is because we still don't have a release for v8.

Yeah, the integration test runner needs a bundle to load, even if we don't use any actor code. Ideally we could run integration tests without actor bundles - that would also make those tests run much faster.

fvm/src/gas/mod.rs

Co-authored-by: raulk <[email protected]> Co-authored-by: Steven Allen <[email protected]>

magik6k force-pushed the feat/wasm-instrumentation branch 3 times, most recently from 3cab1fa to 0e5d64c Compare April 20, 2022 12:05

magik6k changed the title ~~WIP feat: fvm-wasm-instrument stack limiter~~ WIP feat: fvm-wasm-instrument stack limiter and gas Apr 21, 2022

magik6k force-pushed the feat/wasm-instrumentation branch 3 times, most recently from 28d64ed to 7ca7a63 Compare April 24, 2022 14:03

magik6k commented Apr 25, 2022

View reviewed changes

fvm/src/gas/mod.rs Outdated Show resolved Hide resolved

magik6k commented Apr 25, 2022

View reviewed changes

fvm/src/syscalls/bind.rs Outdated Show resolved Hide resolved

magik6k force-pushed the feat/wasm-instrumentation branch 4 times, most recently from 2528ed8 to 7774b8e Compare April 25, 2022 12:08

magik6k marked this pull request as ready for review April 25, 2022 12:12

magik6k changed the title ~~WIP feat: fvm-wasm-instrument stack limiter and gas~~ feat: fvm-wasm-instrument stack limiter and gas Apr 25, 2022

Stebalien reviewed Apr 25, 2022

View reviewed changes

fvm/src/gas/mod.rs Outdated Show resolved Hide resolved

magik6k force-pushed the feat/wasm-instrumentation branch from 597dde3 to 8dca7b7 Compare April 25, 2022 15:06

Stebalien reviewed Apr 25, 2022

View reviewed changes

magik6k force-pushed the feat/wasm-instrumentation branch from 244b69b to 905edf3 Compare April 27, 2022 14:54

Stebalien reviewed Apr 28, 2022

View reviewed changes

magik6k force-pushed the feat/wasm-instrumentation branch 2 times, most recently from c5b660b to 5c108f3 Compare April 28, 2022 11:25

magik6k mentioned this pull request Apr 28, 2022

fvm: Engine per network filecoin-project/filecoin-ffi#273

Merged

magik6k force-pushed the feat/wasm-instrumentation branch 2 times, most recently from b28b727 to 1e06f7b Compare April 29, 2022 15:03

Stebalien approved these changes May 4, 2022

View reviewed changes

raulk reviewed May 4, 2022

View reviewed changes

fvm/src/gas/mod.rs Outdated Show resolved Hide resolved

magik6k and others added 25 commits May 4, 2022 15:38

Cleanup frgas logic

79b7a3a

More reasonable initial exec cost

0183944

Validate raw wasm before instrumenting

689f8a1

handle negative nubers in frgas_to_gas correctly

ef868f2

miligas for wasm execution

d926f97

convert gastracker to miligas

7fe3153

mili means 1/1000

e4282ee

basic exec gas test

9d2e98c

Add basic integration test for out-of-gas

fc651b3

mili->milli

d1a7ecd

Address review

8880415

Fine clippy

95f4de3

stack limit to network config

ced762b

gas integration test

4f4cd7e

fix: avoid unwrap by using a dummy gas global

75376c6

Avoid one gas return path

5841c2a

More review addressing

06dda99

Engine per network

d776857

MultiEngine

72ce6df

Fix conformance

b7f6b88

Make clippy happy

43127db

Integration test out-of-stack instrumentation

c2b5e38

Apply suggestions from code review

0adb846

Co-authored-by: raulk <[email protected]> Co-authored-by: Steven Allen <[email protected]>

Review addressing

0c69efc

Fix comment

ea08252

magik6k force-pushed the feat/wasm-instrumentation branch from 95d1f37 to ea08252 Compare May 4, 2022 19:38

Stebalien enabled auto-merge (squash) May 4, 2022 19:54

Stebalien merged commit f4a7473 into master May 4, 2022

Stebalien deleted the feat/wasm-instrumentation branch May 4, 2022 19:58

raulk mentioned this pull request May 5, 2022

nv16 development checklist #531

Closed

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fvm-wasm-instrument stack limiter and gas #494

feat: fvm-wasm-instrument stack limiter and gas #494

magik6k commented Apr 14, 2022

Stebalien commented Apr 19, 2022

magik6k commented Apr 20, 2022

Stebalien left a comment

magik6k commented Apr 28, 2022

Stebalien left a comment

Stebalien May 4, 2022

raulk May 4, 2022

magik6k May 4, 2022

magik6k May 4, 2022

Stebalien May 4, 2022

raulk May 4, 2022

Stebalien May 4, 2022

Stebalien May 4, 2022

raulk May 4, 2022

magik6k May 4, 2022

		let out = syscall(ctx $(, $t)*).into();

		let result = match out {

feat: fvm-wasm-instrument stack limiter and gas #494

feat: fvm-wasm-instrument stack limiter and gas #494

Conversation

magik6k commented Apr 14, 2022

Stebalien commented Apr 19, 2022

magik6k commented Apr 20, 2022

Stebalien left a comment

Choose a reason for hiding this comment

magik6k commented Apr 28, 2022

Stebalien left a 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