Filecoin Virtual Machine integration #8293

raulk · 2022-03-11T01:20:32Z

Related (but not blocked by) filecoin-project/filecoin-ffi#217

This PR integrates the reference FVM into Lotus. In doing so, it also integrates the new Rust-based builtin actors through the FFI.

The salient changes are:

Abstracting out the Virtual Machine in Lotus with a new Virtual Machine Interface that is implemented by the Lotus VM (suggested by @raulk to be renamed to Legacy VM) and the new FVM struct in Lotus.
Integrating the FVM and builtin actors into the FFI (see FVM integration filecoin-ffi#217). The FFI exposes new endpoints to:
- Construct FVMs
- Execute messages (both implicit and explicit)
- Flush
Refactor the circulating supply logic to supply a partially-calculated value to the FVM for v14 (and before) only. This logic can be dropped if FVM support for v14 is not desired.
Put usage of the new FVM behind the LOTUS_USE_FVM_EXPERIMENTAL flag. If enabled, Lotus will use FVMs (and through it the Rust-based actors) instead of the Lotus VM and Golang-based actors.

1. Move lock, loading, etc into GetFilVested. 2. Call it directly when creating the FVM. 3. Detach GetFilLocked from state manager. Really, this just makes it a bit easier to reason about this code.

…work-version feat: cli: set current network version from params

Updates the FVM

Fvm: impl VerifyConsensusFault

This is what the lotus VM does.

Use either Lotus VM or FVM consistently

FVM: support nv15

codecov · 2022-03-12T19:42:35Z

Codecov Report

Merging #8293 (9ea623e) into master (362c73b) will decrease coverage by 0.11%.
The diff coverage is 26.42%.

@@            Coverage Diff             @@
##           master    #8293      +/-   ##
==========================================
- Coverage   40.34%   40.23%   -0.12%     
==========================================
  Files         679      681       +2     
  Lines       73961    74139     +178     
==========================================
- Hits        29842    29832      -10     
- Misses      38897    39076     +179     
- Partials     5222     5231       +9

Impacted Files	Coverage Δ
chain/stmgr/utils.go	`21.84% <0.00%> (-0.76%)`	⬇️
chain/vm/fvm.go	`0.00% <0.00%> (ø)`
chain/vm/gas.go	`78.57% <ø> (ø)`
chain/vm/gas_v0.go	`92.94% <ø> (ø)`
.../lotus-sim/simulation/blockbuilder/blockbuilder.go	`0.00% <0.00%> (ø)`
node/builder_chain.go	`100.00% <ø> (ø)`
chain/consensus/filcns/compute_state.go	`71.59% <25.00%> (-2.53%)`	⬇️
chain/vm/vmi.go	`50.00% <50.00%> (ø)`
chain/stmgr/supply.go	`63.28% <61.90%> (-0.50%)`	⬇️
chain/stmgr/call.go	`65.94% <64.28%> (-5.57%)`	⬇️
... and 29 more

jennijuju · 2022-03-15T04:19:42Z

go.sum

 github.com/filecoin-project/specs-actors/v7 v7.0.0-20211222192039-c83bea50c402/go.mod h1:p6LIOFezA1rgRLMewbvdi3Pp6SAu+q9FtJ9CAleSjrE=
+github.com/filecoin-project/specs-actors/v7 v7.0.0-rc1.0.20220118005651-2470cb39827e/go.mod h1:TA5FwCna+Yi36POaT7SLKXsgEDvJwc0V/L6ZsO19B9M=


can we haz a tag? whats new in go actors anyways? PRU fix?

Idk why go sum wants this tbh...but go sum thinks for itself (the version in go.mod is correct)

yeah just ignore go.sum, it only records all checksums seen as part of resolving deps. you can often end up with several older versions listed in go.sum compared to the one actually being used.

jennijuju · 2022-03-15T04:20:04Z

go.sum

@@ -1943,6 +1942,7 @@ github.com/whyrusleeping/cbor-gen v0.0.0-20200826160007-0b9f6c5fb163/go.mod h1:f
 github.com/whyrusleeping/cbor-gen v0.0.0-20210118024343-169e9d70c0c2/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
 github.com/whyrusleeping/cbor-gen v0.0.0-20210219115102-f37d292932f2/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
 github.com/whyrusleeping/cbor-gen v0.0.0-20210303213153-67a261a1d291/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
+github.com/whyrusleeping/cbor-gen v0.0.0-20210713220151-be142a5ae1a8/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=


no change from master here

jennijuju · 2022-03-15T04:22:09Z

@raulk IIUC yushi implemented this but you opened pr - if so, you should review this

chain/consensus/filcns/compute_state.go

chain/stmgr/call.go

raulk · 2022-03-14T14:51:05Z

chain/stmgr/call.go

+	stTree, err := sm.StateTree(bstate)
+	if err != nil {
+		return nil, err
+	}


There seems to be an error behaviour change in here. Mind explaining?

@raulk Well, there's a new potential error here, if that's what you mean, I can add a descreptive error message. The cause of the change is just that we don't have (or want) the StateTree method on the VM interface.

raulk · 2022-03-15T10:57:35Z

chain/stmgr/call.go

+	stateCid, err = vmi.Flush(ctx)
+	if err != nil {
+		return nil, xerrors.Errorf("flushing vm: %w", err)
+	}
+
+	stTree, err := state.LoadStateTree(cbor.NewCborStore(buffStore), stateCid)
+	if err != nil {
+		return nil, xerrors.Errorf("loading state tree: %w", err)
+	}


Let's add a comment here stating why we need to flush and load the state tree.

This Flush wouldn't be necessary with the intrinsic VM, so consider adding a specialization here, since the Lotus VM is not going anywhere for the foreseeable future and this would cause a small perf regression.

Will do.

Eh, I don't think I care enough -- the minor perf hit to simulating messages seems fine. I'd rather not have more IF FVM clauses than strictly necessary.

chain/vm/fvm.go

raulk · 2022-03-15T11:29:32Z

chain/vm/fvm.go

+}
+
+func NewFVM(ctx context.Context, opts *VMOpts) (*FVM, error) {
+	circToReport := opts.FilVested


Are there no solutions other than snaking through this FilVested field? Seems a little bit ad-hoc when combined with the pre-existing CircSupplyCalc field. And the fact that it won't be even used nv15 and forward, makes me even uneasier.

An alternative solution would be to expand the signature of CircSupplyCalc to take in the network version. Since the value of that is a StateManager member, it has all it needs to return FilVested if nv < 15.

Then this logic can be entirely independent of the network version, and we'd just pass the value down and let the FVM do the right thing.

@raulk My priority here is not changing the existing logic for the Lotus VM. I would rather have a new field that the LotusVM entirely ignores -- that way if there's anything wrong with this PR, only FVM M0 testing is affected.

If it'll make you feel better, we can drop this entirely from the final released version of Lotus for the v16 upgrade and just always use the calculated CircSupply.

chain/vm/vmi.go

ZenGround0 · 2022-03-16T16:31:26Z

chain/consensus/filcns/compute_state.go

+		if os.Getenv("LOTUS_USE_FVM_EXPERIMENTAL") == "1" {
+			// This is needed so that the FVM does not have to duplicate the genesis vesting schedule, one
+			// of the components of the circ supply calc.
+			// This field is NOT needed by the LegacyVM, and also NOT needed by the FVM from v15 onwards.


What changed in v15 so that this would not be needed by FVM?

First Protocol Improvement / Bugfix mentioned here

(also adding a link in the comment)

ZenGround0 · 2022-03-16T16:35:30Z

chain/gen/genesis/miners.go

@@ -95,12 +95,13 @@ func SetupStorageMiners(ctx context.Context, cs *store.ChainStore, sys vm.Syscal
 		Syscalls:       mkFakedSigSyscalls(sys),
 		CircSupplyCalc: csc,
 		NetworkVersion: nv,
-		BaseFee:        types.NewInt(0),
+		BaseFee:        big.Zero(),
+		FilVested:      big.Zero(),


We're only passing these opts into legacy VMs from here correct? So this is just nice zero value setting for clarity and won't cause v14 devnets to read bad values for the vested fil?

Correct, yeah. Also Vested fil is equal to zero at genesis, so should be consistent.

ZenGround0 · 2022-03-16T17:03:45Z

chain/stmgr/call.go

@@ -64,6 +70,8 @@ func (sm *StateManager) Call(ctx context.Context, msg *types.Message, ts *types.
 		pheight = ts.Height() - 1
 	}

+	// Since we're simulating a future message, pretend we're applying it in the "next" tipset


I think this is a current message and we're executing it for real. "Current message is applied at parent height + 1"

Clarifying, thanks.

magik6k

Looks good, just one question.

magik6k · 2022-03-16T17:23:30Z

chain/vm/fvm.go

+
+// VerifyConsensusFault is similar to the one in syscalls.go used by the LegacyVM, except it never errors
+// Errors are logged and "no fault" is returned, which is functionally what go-actors does anyway
+func (x *FvmExtern) VerifyConsensusFault(ctx context.Context, a, b, extra []byte) (*ffi_cgo.ConsensusFault, int64) {


What happens if any of those extern calls panic? Do we crash the node, or do we behave like the current VM and just fail execution?

@magik6k Thanks a lot for raising this. This is a somewhat open question about exactly what we wanna do -- currently:

Rust panics get converted to errors at the FFI layer

Extern panics such as this one are...unclear? Maybe also get lowered to errors at the FFI laye, maybe crash the node? @Stebalien isn't sure, we can relatively easy answer this by experimenting.

Any error returned by the FFI (including such downgraded panics) cause the ApplyMessage call to fail and the node to freeze. In terms of the LegacyVM, this means all such errors are "fatal errors". I'm fairly opposed to this, but a decision will be made later.

Extern panics such as this one are...unclear? Maybe also get lowered to errors at the FFI laye, maybe crash the node? @Stebalien isn't sure, we can relatively easy answer this by experimenting.

The Rust => Go call path is brand new, so this definitely deserves a ton of attention.

raulk

LGTM, barring @magik6k's very valid concern about panic handling/translation at the FFI layer. I can't approve because GitHub thinks I'm the author of this PR.

arajasek and others added 30 commits February 4, 2022 12:46

:Hook up the FVM

2a669b9

update fvm/ffi bindings

fc74a6c

update ffi

04092f3

fvm: fix implicit messages and message inclusion gas charging

279cdd0

fvm: feed in correct "base" circulating supply

32b3618

pass only fil-vested into FVM

e8d771f

chore: cleanup fil vested calculation

e8bdf81

1. Move lock, loading, etc into GetFilVested. 2. Call it directly when creating the FVM. 3. Detach GetFilLocked from state manager. Really, this just makes it a bit easier to reason about this code.

ffi: update FFI for fvm changes

7438628

ffi: update for fixed FVM lifetime management

0d6eb7f

ffi: update fvm

7ef1513

Merge branch 'master' into feat/fvm

ee69899

Merge pull request #8120 from filecoin-project/travis/set-current-net…

4767f3d

…work-version feat: cli: set current network version from params

chore: update FFI

d835cad

Updates the FVM

Fvm: impl VerifyConsensusFault

393479e

address review feedback

5be125a

Merge pull request #8101 from filecoin-project/asr/fault

562c59b

Fvm: impl VerifyConsensusFault

Merge branch 'master' into feat/fvm

6e1d5c5

fvm: set gas costs to nil for implicit messages

aad3762

This is what the lotus VM does.

fvm: time message execution

420c5fb

chore: refactor: rename NewVM to NewLotusVM

9bb936b

feat: use either lotus vm or fvm consistently

b6682f4

fix: set FilVested when constructing VmOpts

d58babe

stmgr: call needs to flush VM before fetching nonce

2ba34ad

Merge pull request #8174 from filecoin-project/asr/consistent-vm

755ce8c

Use either Lotus VM or FVM consistently

Merge branch 'master' into feat/fvm

dd91857

Merge branch 'master' into feat/fvm

0a67b6e

FVM: support nv15

d2054e8

Merge pull request #8197 from filecoin-project/asr/fvm/nv15

6d4fb88

FVM: support nv15

Rename FVM envvar to LOTUS_USE_FVM_EXPERIMENTAL

05fa9c8

Merge branch 'master' into feat/fvm

67889b4

Fix broken go.sum

1bf40ad

arajasek marked this pull request as ready for review March 12, 2022 19:31

arajasek requested a review from a team as a code owner March 12, 2022 19:31

arajasek mentioned this pull request Mar 12, 2022

Integrate the ref-fvm into Lotus #8059

Closed

jennijuju reviewed Mar 15, 2022

View reviewed changes

jennijuju requested review from ZenGround0, magik6k, arajasek and Stebalien March 15, 2022 04:21

raulk commented Mar 15, 2022

View reviewed changes

Update FFI: fix cargo.lock

6c51adc

arajasek mentioned this pull request Mar 15, 2022

Implement FIP-0031 #8209

Closed

4 tasks

arajasek added 5 commits March 15, 2022 19:34

Rename vm.VMI to vm.Interface

673f558

Address review

e6117c4

Rename the Lotus VM to LegacyVM

9ea623e

Merge branch 'master' into feat/fvm

16128a0

CircSupply: Remove unused method

37539cc

ZenGround0 reviewed Mar 16, 2022

View reviewed changes

magik6k approved these changes Mar 16, 2022

View reviewed changes

arajasek merged commit 90d78f9 into master Mar 16, 2022

arajasek deleted the feat/fvm branch March 16, 2022 19:51

raulk commented Mar 16, 2022

View reviewed changes

arajasek restored the feat/fvm branch March 16, 2022 19:57

This was referenced Mar 16, 2022

feat: fvm: FVM integration #8332

Merged

fix: avoid racy memstores when estimating gas #8351

Merged

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

Filecoin Virtual Machine integration #8293

Filecoin Virtual Machine integration #8293

raulk commented Mar 11, 2022 •

edited by arajasek

Loading

codecov bot commented Mar 12, 2022 •

edited

Loading

jennijuju Mar 15, 2022

arajasek Mar 15, 2022

mvdan Mar 18, 2022

jennijuju Mar 15, 2022

arajasek Mar 15, 2022

jennijuju commented Mar 15, 2022

raulk Mar 14, 2022

arajasek Mar 15, 2022 •

edited

Loading

raulk Mar 15, 2022

arajasek Mar 15, 2022

raulk Mar 15, 2022

arajasek Mar 15, 2022

ZenGround0 Mar 16, 2022

arajasek Mar 16, 2022

ZenGround0 Mar 16, 2022

arajasek Mar 16, 2022

ZenGround0 Mar 16, 2022

arajasek Mar 16, 2022

magik6k left a comment

magik6k Mar 16, 2022

arajasek Mar 16, 2022

raulk Mar 16, 2022 •

edited

Loading

raulk left a comment

		github.com/filecoin-project/specs-actors/v7 v7.0.0-20211222192039-c83bea50c402/go.mod h1:p6LIOFezA1rgRLMewbvdi3Pp6SAu+q9FtJ9CAleSjrE=
		github.com/filecoin-project/specs-actors/v7 v7.0.0-rc1.0.20220118005651-2470cb39827e/go.mod h1:TA5FwCna+Yi36POaT7SLKXsgEDvJwc0V/L6ZsO19B9M=

Filecoin Virtual Machine integration #8293

Filecoin Virtual Machine integration #8293

Conversation

raulk commented Mar 11, 2022 • edited by arajasek Loading

codecov bot commented Mar 12, 2022 • edited Loading

Codecov Report

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

jennijuju commented Mar 15, 2022

Choose a reason for hiding this comment

arajasek Mar 15, 2022 • 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

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

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

raulk commented Mar 11, 2022 •

edited by arajasek

Loading

codecov bot commented Mar 12, 2022 •

edited

Loading

arajasek Mar 15, 2022 •

edited

Loading

raulk Mar 16, 2022 •

edited

Loading