FVM integration #217

raulk · 2021-12-17T14:08:16Z

No description provided.

raulk · 2021-12-17T14:12:39Z

rust/src/fvm/machine.rs

+        initial_pages: 1024, //FIXME
+        max_pages: 32768,    // FIXME


@Stebalien any sense of the values we want to set here? Noting that WASM page size is 64KiB, so we may want to start with 16 pages (1MiB), and have no upper limit for M0? We probaly want to instrument the WASM runtime to record the pages we ended up allocating with every execution.

No upper limit for now. We probably also need an exception for cron.

And yeah, we need to measure it. Then 10x at least.

Note: I'd actually base the max memory on the total callstack. So we'll configure some max pages here, then set max_pages to max_pages - used_pages when instantiating a new instance.

raulk · 2021-12-17T14:13:42Z

rust/src/fvm/machine.rs

+    state_root_ptr: *const u8,
+    state_root_len: libc::size_t,
+    blockstore_id: u64,
+    externs_id: u64,


I think externs can be stateless if the blockstore is not folded into the externs, and kept as a separate entity (which seems to be where we're trending), but need to think more about this.

At the very least, we'd need to be able to tell go which blockstore it should use. But if we go that way, caching will be really hard.

I'd rather go the other way and use a single ID for both externs and the blockstore. Basically, just get rid of the cgo blockstore and replace it with a cgo externs (using the same logic).

We've now combined them. We allow passing them as separate "handles", but under the hood we're using the same object.

rust/src/fvm/machine.rs

raulk · 2021-12-17T14:30:12Z

rust/src/fvm/machine.rs

+pub unsafe extern "C" fn fil_fvm_machine_finish_message(
+    machine_id: u64,
+    // TODO: actual message
+) {
+    // catch_panic_response(|| {
+    init_log();
+
+    info!("fil_fvm_machine_flush_message: start");
+
+    let machines = FVM_MAP.lock().unwrap();
+    let machine = machines.get(&machine_id);
+    match machine {
+        Some(machine) => {
+            todo!("execute message")
+        }
+        None => {
+            todo!("invalid machine id")
+        }
+    }
+
+    info!("fil_fvm_machine_flush_message: end");
+    // })
+}


I don't think we have a "finish message" operation. What we finish is the machine, which flushes all effectively linked blocks to the blockstore, and returns the new state root CID.

@Stebalien do we expect the node to construct the receipts root, or do we want to accumulate receipts in the machine, to then return the root?

I expect the node to deal with that, honestly. Basically:

The VM handles the state-tree only.

The Node handles everything related to the blockchain.

rust/src/fvm/types.rs

cryptonemo · 2022-01-19T18:00:49Z

Currently requires filecoin-project/ref-fvm#277

We can just pass pointers around in this case.

And correctly handle out of bounds conditions.

We don't need it, but it makes matching existing APIs simpler.

avoids some build issues

rust/src/fvm/externs.rs

rust/src/fvm/machine.rs

dignifiedquire · 2022-03-23T14:52:46Z

rust/src/fvm/machine.rs

+            Box::leak(return_bytes);
+        }
+
+        // TODO: Do something with the backtrace.


is this expected to be done before being merged?

I wasn't planning on it.

dignifiedquire · 2022-03-23T14:54:11Z

rust/src/fvm/machine.rs

+
+#[no_mangle]
+pub unsafe extern "C" fn fil_fvm_machine_execute_message(
+    executor: *mut libc::c_void,


why is this a void pointer, instead of a pointer to a concrete type?

The concrete type is opaque. Ideally, we'd use rust-lang/rust#43467. But c_void is the best we can do for now.

See https://docs.rs/libc/0.2.121/libc/enum.c_void.html for how this works.

To be clear, the concrete type is opaque to go.

dignifiedquire · 2022-03-23T14:54:54Z

rust/src/fvm/machine.rs

+
+use cid::Cid;
+use ffi_toolkit::{catch_panic_response, raw_ptr, rust_str_to_c_str, FCPResponseStatus};
+use futures::executor::block_on;


is the futures executor the one being used across the fvm?

We use the async-std executor inside the conformance tests.

We don't use async at all inside the FVM itself.

We need this here just to read the car file. Which we should probably just convert to sync.

We had too many parameters.

We don't support it anyways, and this simplifies the code a bit.

We've disabled the feature so this should no longer crash if we an identity hash code.

And switch to rust edition 2021, with the version-2 feature resolver.

This switches to the new bundle method.

arajasek

Looked at this with fresh-ish eyes, and it seems good.

bls.go

cgo/errors.go

fvm.go

That way we can't get a mismatch.

cryptonemo · 2022-03-29T17:58:11Z

fvm.go

+func (f *FVM) ApplyMessage(msgBytes []byte, chainLen uint) (*ApplyRet, error) {
+	// NOTE: we need to call KeepAlive here (and below) because go doesn't guarantee that the
+	// receiver will live to the end of the function. If we don't do this, go _will_ garbage
+	// collecting the FVM, causing us to run the finalizer while we're in the middle of using


Comment typo

rust/src/fvm/machine.rs

cryptonemo · 2022-03-30T20:20:52Z

rust/src/fvm/types.rs

+    pub error_msg: *const libc::c_char,
+    pub status_code: FCPResponseStatus,
+    pub exit_code: u64,
+    pub return_ptr: *const u8,


We should probably confirm a leakcheck test to be sure -- this still strikes me as a leak.

rust/src/proofs/api.rs

cryptonemo · 2022-03-31T12:44:47Z

rust/src/fvm/types.rs

+    pub error_msg: *const libc::c_char,
+    pub status_code: FCPResponseStatus,
+    pub exit_code: u64,
+    pub return_ptr: *const u8,


Hrm, I'm trying to leak-check this and getting core dumps -- can anyone else give that a go to see if it's just me?

Something like:

$ time valgrind --leak-check=full -s target/release/deps/filcrypto-0b2e0900682a616d test_sealing_aggregation_v1_1 --ignored > LOG 2>&1 Illegal instruction (core dumped) real 0m1.282s user 0m1.055s sys 0m0.084s

cryptonemo · 2022-03-31T12:45:47Z

rust/src/fvm/types.rs

+    pub error_msg: *const libc::c_char,
+    pub status_code: FCPResponseStatus,
+    pub exit_code: u64,
+    pub return_ptr: *const u8,


Seems to run fine otherwise :-(

$ target/release/deps/filcrypto-0b2e0900682a616d test_sealing_aggregation_v1_1 --ignored running 1 test test proofs::api::tests::test_sealing_aggregation_v1_1 ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 12 filtered out; finished in 12.17s

chore: build: deps: use the latest v6 & v7 actors

raulk commented Dec 17, 2021

View reviewed changes

cryptonemo and others added 9 commits January 11, 2022 11:05

feat: get basic fvm code pulled in

f48a5c8

feat: try to instantiate an fvm machine with basic defaults

665619b

store machines and retrieve them

0a0463c

fix compile and update deps

7e6e4c8

feat: add execute_message API and some more code

497da29

feat: update dependencies

67ce452

Apply some review feedback

768ed2a

fix: rebased, but the fvm imports are broken after they were moved

97279f3

Update to the latest FVM code

cb7bc67

arajasek force-pushed the fvm branch from 7dab108 to cb7bc67 Compare January 14, 2022 18:24

feat: update to latest fvm code, using latest proofs code

061b759

cryptonemo and others added 17 commits January 19, 2022 13:29

feat: update to latest fvm

dd1f6e2

style: fix clippy warnings

6ab5769

style: rust fmt

db01ec7

add go bindings

27ff0fa

dep: update specs-actors to master

f42386e

fix compile

6bae4f3

fvm: remove the fvm registry map

deb3597

We can just pass pointers around in this case.

fvm: avoid double-freeing the return data

7b89e76

fvm: implement machine flush

65f2e4e

fvm: optimize bigint conversions

e6730c4

And correctly handle out of bounds conditions.

fvm: move externs into FFI

81bf427

fvm: fixup return types

a5ead0e

fvm: get rust -> go bindings compiling and linking

b6cfd8e

fvm: add context to externs

f4fb585

We don't need it, but it makes matching existing APIs simpler.

fvm: move fvm back to the top-level

dc4040f

avoids some build issues

fvm: match lotus randomness API

86f38e0

chore: update deps

8807f57

dignifiedquire reviewed Mar 23, 2022

View reviewed changes

rust/src/fvm/externs.rs Show resolved Hide resolved

dignifiedquire reviewed Mar 23, 2022

View reviewed changes

rust/src/fvm/machine.rs Show resolved Hide resolved

dignifiedquire reviewed Mar 23, 2022

View reviewed changes

rust/src/fvm/machine.rs Show resolved Hide resolved

dignifiedquire reviewed Mar 23, 2022

View reviewed changes

Stebalien and others added 11 commits March 24, 2022 12:00

doc(fvm): document bindgen feature

d7affa9

chore(fvm): rename out parameters to make usage clear

bd31298

feat(fvm): construct FVM with an options struct

27aa405

We had too many parameters.

chore: remove 32bit support from the FVM

20b1cd2

We don't support it anyways, and this simplifies the code a bit.

doc(fvm): document why we explicitly call KeepAlive

046b147

doc(fvm): document cgo constants/logic

5df6856

fix: remove identity hash workaround

7b28c52

We've disabled the feature so this should no longer crash if we an identity hash code.

doc(fvm): expand rust-side extern/blockstore documentation

f69feda

chore: bump to actor v7.0.7

7f93fe5

fix: completely remove the multihash identity feature

5ec79e9

And switch to rust edition 2021, with the version-2 feature resolver.

chore: update fvm & actors

dd1c85a

This switches to the new bundle method.

arajasek approved these changes Mar 29, 2022

View reviewed changes

chore: update FVM

5bb0764

dignifiedquire reviewed Mar 30, 2022

View reviewed changes

bls.go Show resolved Hide resolved

dignifiedquire reviewed Mar 30, 2022

View reviewed changes

cgo/errors.go Outdated Show resolved Hide resolved

dignifiedquire reviewed Mar 30, 2022

View reviewed changes

fvm.go Show resolved Hide resolved

fix: use exported constants in FVM/cgo errors

9522c86

That way we can't get a mismatch.

cryptonemo reviewed Mar 31, 2022

View reviewed changes

Stebalien and others added 3 commits March 31, 2022 19:19

chore(fvm): fix code comment

d418486

use the latest v6 & v7 actors

bf1b7b1

Merge pull request #256 from filecoin-project/jen/actorbump

4b939fe

chore: build: deps: use the latest v6 & v7 actors

cryptonemo approved these changes Apr 1, 2022

View reviewed changes

Stebalien merged commit 5d00bb4 into master Apr 1, 2022

eshon mentioned this pull request Apr 7, 2022

FVM Milestone 1 Bug Bounty Program filecoin-project/community#504

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FVM integration #217

FVM integration #217

raulk commented Dec 17, 2021

raulk Dec 17, 2021

Stebalien Dec 17, 2021

Stebalien Dec 17, 2021

raulk Dec 17, 2021 •

edited

Loading

Stebalien Dec 17, 2021

Stebalien Mar 24, 2022

raulk Dec 17, 2021

Stebalien Dec 17, 2021

cryptonemo commented Jan 19, 2022

dignifiedquire Mar 23, 2022

Stebalien Mar 24, 2022

dignifiedquire Mar 23, 2022

Stebalien Mar 24, 2022

Stebalien Mar 24, 2022

dignifiedquire Mar 23, 2022

Stebalien Mar 24, 2022

arajasek left a comment

cryptonemo Mar 29, 2022

cryptonemo Mar 30, 2022

cryptonemo Mar 31, 2022

cryptonemo Mar 31, 2022

FVM integration #217

FVM integration #217

Conversation

raulk commented Dec 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Dec 17, 2021 • edited Loading

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

cryptonemo commented Jan 19, 2022

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

arajasek 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

raulk Dec 17, 2021 •

edited

Loading