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

Better stack limits #554

Merged
merged 5 commits into from
May 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions fvm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ byteorder = "1.4.3"
anymap = "0.12.1"
blake2b_simd = "1.0.0"
fvm-wasm-instrument = { version = "0.2.0", features = ["bulk"] }
yastl = "0.1.2"

[dependencies.wasmtime]
version = "0.35.2"
Expand Down
41 changes: 36 additions & 5 deletions fvm/src/executor/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use fvm_shared::error::{ErrorNumber, ExitCode};
use fvm_shared::message::Message;
use fvm_shared::receipt::Receipt;
use fvm_shared::ActorID;
use lazy_static::lazy_static;
use num_traits::Zero;

use super::{ApplyFailure, ApplyKind, ApplyRet, Executor};
Expand Down Expand Up @@ -38,9 +39,24 @@ impl<K: Kernel> DerefMut for DefaultExecutor<K> {
}
}

lazy_static! {
static ref EXEC_POOL: yastl::Pool = yastl::Pool::with_config(
8,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an arbitrary number; Just for chain-sync we probably don't need more than 1/2, but some users may want this to be higher e.g. to get internal sends in a bunch of historic tipsets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using NUM CPUs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some people have a lot of CPUs. We're already reserving half a gig for stacks here. We'll actually likely want to shrink this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e., 128 cores = 8GiB of memory just sitting there.

yastl::ThreadConfig::new()
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
.prefix("fvm-executor")
// fvm needs more than the deafault available stack (2MiB):
// - Max 2048 wasm stack elements, which is 16KiB of 64bit entries
// - Roughly 20KiB overhead per actor call
// - max 1024 nested calls, which means that in the worst case we need ~36MiB of stack
// We also want some more space just to be conservative, so 64MiB seems like a reasonable choice
.stack_size(64 << 20),
);
}

impl<K> Executor for DefaultExecutor<K>
where
K: Kernel,
<K::CallManager as CallManager>::Machine: Send,
{
type Kernel = K;

Expand All @@ -50,6 +66,26 @@ where
msg: Message,
apply_kind: ApplyKind,
raw_length: usize,
) -> anyhow::Result<ApplyRet> {
let mut ret = Err(anyhow!("failed to execute"));

EXEC_POOL.scoped(|scope| {
scope.execute(|| ret = self.execute_message_inner(msg, apply_kind, raw_length));
});

ret
}
}

impl<K> DefaultExecutor<K>
where
K: Kernel,
{
fn execute_message_inner(
&mut self,
msg: Message,
apply_kind: ApplyKind,
raw_length: usize,
) -> anyhow::Result<ApplyRet> {
// Validate if the message was correct, charge for it, and extract some preliminary data.
let (sender_id, gas_cost, inclusion_cost) =
Expand Down Expand Up @@ -190,12 +226,7 @@ where
}),
}
}
}

impl<K> DefaultExecutor<K>
where
K: Kernel,
{
/// Create a new [`DefaultExecutor`] for executing messages on the [`Machine`].
pub fn new(m: <K::CallManager as CallManager>::Machine) -> Self {
Self(Some(m))
Expand Down
4 changes: 2 additions & 2 deletions fvm/src/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ impl NetworkConfig {
pub fn new(network_version: NetworkVersion) -> Self {
NetworkConfig {
network_version,
max_call_depth: 4096,
max_wasm_stack: 64 * 1024,
max_call_depth: 1024,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be technically correct, we should set 1024 on nv16 and above, only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, @magik6k pointed out that we'll hit the rust stack limit at 4096. The only real solution would be to increase the thread size.

But, in practice, it's unclear if one can actually make a message that can get past 1024 calls on mainnet. By "unclear", I mean "totally not close to possible". @magik6k only got to 1025 by making a custom actor that did nothing but recursively send. On mainnet, one would have to make a recursive call in a multisig actor, where each recursive call would be significantly more expensive (running out of block gas before anything else).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tradeoffs:

  • The current way, it's unlikely that something goes wrong, and the worst-case is a fork (for anyone running with the FVM_EXPERIMENTAL flag).
  • By increasing to 4096 (without changing the stack size), it's also unlikely that something will go wrong, but we'll crash (hard) if we do go past 1024.

max_wasm_stack: 2048,
actor_debugging: false,
builtin_actors_override: None,
price_list: price_list_by_network_version(network_version),
Expand Down
1 change: 1 addition & 0 deletions testing/integration/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ features = ["cranelift", "pooling-allocator", "parallel-compilation"]
wabt = "0.10.0"
serde = { version = "1.0", features = ["derive"] }
fil_hello_world_actor = { path = 'tests/fil-hello-world-actor', version = '0.1' }
fil_stack_overflow_actor = { path = 'tests/fil-stack-overflow-actor', version = '0.1' }

[features]
default = ["fvm/testing", "fvm_shared/testing"]
13 changes: 13 additions & 0 deletions testing/integration/tests/fil-stack-overflow-actor/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[package]
name = "fil_stack_overflow_actor"
version = "0.1.0"
edition = "2021"

[dependencies]
fvm_sdk = { version = "0.6.1", path = "../../../../sdk" }
fvm_shared = { version = "0.6.1", path = "../../../../shared" }


[build-dependencies]
wasm-builder = "3.0.1"
wasmtime = "0.33.0"
12 changes: 12 additions & 0 deletions testing/integration/tests/fil-stack-overflow-actor/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
fn main() {
use wasm_builder::WasmBuilder;
WasmBuilder::new()
.with_current_project()
.import_memory()
.append_to_rust_flags("-Ctarget-feature=+crt-static")
.append_to_rust_flags("-Cpanic=abort")
.append_to_rust_flags("-Coverflow-checks=true")
.append_to_rust_flags("-Clto=true")
.append_to_rust_flags("-Copt-level=z")
.build()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
nightly
64 changes: 64 additions & 0 deletions testing/integration/tests/fil-stack-overflow-actor/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
use fvm_sdk as sdk;
use fvm_shared::address::Address;
use fvm_shared::error::ExitCode;

#[no_mangle]
pub fn invoke(_: u32) -> u32 {
let m = sdk::message::method_number();
// If we start with method 1, we'll be over recursive send limit, starting
// with method 2 should be fine
if m > 1026 {
sdk::vm::abort(0x42, None);
}

if m == 1 {
// if method 0, we want to run out of stack
recurse(m, 1000)
} else {
// 5 stack elems per level (wasm-instrument charges for highest use in the
// function) + some overhead mean that with the 2048 element wasm limit we
// can do 396 recursive calls while still being able do do a send at that
// depth
recurse(m, 396)
}
}

// we need two recurse functions; just one gets optimized into wasm loop

#[inline(never)]
pub fn recurse(m: u64, n: u64) -> u32 {
if n > 0 {
call_extern();

return recurse2(m, n - 1);
}
do_send(m)
}

#[inline(never)]
pub fn recurse2(m: u64, n: u64) -> u32 {
if n > 0 {
call_extern();

return recurse(m, n - 1);
}
do_send(m)
}

// external call to prevent the compiler from doing smart things
#[inline(never)]
pub fn call_extern() {
let _ = sdk::message::method_number();
}

#[inline(never)]
pub fn do_send(m: u64) -> u32 {
let r = sdk::send::send(&Address::new_id(10000), m + 1, Vec::new().into(), 0.into());
match r {
Ok(rec) => match rec.exit_code {
ExitCode::OK => 0,
e => sdk::vm::abort(e.value() | 0x80000000, None),
},
Err(e) => sdk::vm::abort((e as u32) | 0xc0000000, None),
}
}
77 changes: 75 additions & 2 deletions testing/integration/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ use std::env;
use cid::multihash::Multihash;
use cid::Cid;
use fvm::executor::{ApplyKind, Executor};
use fvm_integration_tests::tester::{Account, Tester};
use fvm_integration_tests::tester::{Account, IntegrationExecutor, Tester};
use fvm_ipld_blockstore::{Blockstore, MemoryBlockstore};
use fvm_ipld_encoding::tuple::*;
use fvm_ipld_encoding::DAG_CBOR;
use fvm_shared::address::Address;
use fvm_shared::bigint::BigInt;
use fvm_shared::error::ExitCode;
use fvm_shared::error::{ErrorNumber, ExitCode};
use fvm_shared::message::Message;
use fvm_shared::state::StateTreeVersion;
use fvm_shared::version::NetworkVersion;
Expand All @@ -28,6 +28,9 @@ pub struct State {
const WASM_COMPILED_PATH: &str =
"../../target/debug/wbuild/fil_hello_world_actor/fil_hello_world_actor.compact.wasm";

const WASM_COMPILED_PATH_OVERFLOW: &str =
"../../target/debug/wbuild/fil_stack_overflow_actor/fil_stack_overflow_actor.compact.wasm";

#[test]
fn hello_world() {
// Instantiate tester
Expand Down Expand Up @@ -80,6 +83,76 @@ fn hello_world() {
assert_eq!(res.msg_receipt.exit_code.value(), 16)
}

#[test]
fn native_stack_overflow() {
// Instantiate tester
let mut tester = Tester::new(
NetworkVersion::V16,
StateTreeVersion::V4,
MemoryBlockstore::default(),
)
.unwrap();

let sender: [Account; 1] = tester.create_accounts().unwrap();

// Get wasm bin
let wasm_path = env::current_dir()
.unwrap()
.join(WASM_COMPILED_PATH_OVERFLOW)
.canonicalize()
.unwrap();
let wasm_bin = std::fs::read(wasm_path).expect("Unable to read file");

// Set actor state
let actor_state = State::default();
let state_cid = tester.set_state(&actor_state).unwrap();

// Set actor
let actor_address = Address::new_id(10000);

tester
.set_actor_from_bin(&wasm_bin, state_cid, actor_address, BigInt::zero())
.unwrap();

// Instantiate machine
tester.instantiate_machine().unwrap();

let exec_test = |exec: &mut IntegrationExecutor<MemoryBlockstore>, method| {
// Send message
let message = Message {
from: sender[0].1,
to: actor_address,
gas_limit: 10_000_000_000,
method_num: method,
sequence: method - 1,
..Message::default()
};

let res = exec
.execute_message(message, ApplyKind::Explicit, 100)
.unwrap();

res.msg_receipt.exit_code.value()
};

let mut executor = tester.executor.unwrap();

// on method 0 the test actor should run out of stack
assert_eq!(
exec_test(&mut executor, 1),
ExitCode::SYS_ILLEGAL_INSTRUCTION.value()
);

// on method 1 the test actor should run out of recursive call limit
assert_eq!(
exec_test(&mut executor, 2),
0xc0000000 + (ErrorNumber::LimitExceeded as u32)
);

// on method 2 the test actor should finish successfully
assert_eq!(exec_test(&mut executor, 3), 0x80000042);
}

#[test]
fn out_of_gas() {
const WAT: &str = r#"
Expand Down