filecoin-project / fvm-workbench

Tooling for developing actors on the FVM
Other
6 stars 0 forks source link

Generic actor Tree implementation #30

Closed alexytsu closed 8 months ago

alexytsu commented 9 months ago

The builtin actor's check_invariant's relies on loading a Tree which is a builtin-actors-specific type.

The workbench cannot easily provide a Cid that is easily loaded as a fil_builtin_actors_state::check::Tree. In fact the current VM::state_root implementation in the workbench is incorrect as it has different semantics to the same method in the builtin-actor's test VM.

alexytsu commented 9 months ago

Instead of VM::state_root returning a Cid and constructing the ActorMap from within shared code, each implementation should create a generic ActorMap.

anorth commented 8 months ago

The real FVM has a state tree impl which is also different to the builtin-actors, but is perhaps more canonical. An alternative might be to align all the sites on that one tree implementation, implemented in the built-in actors repo. Depending on point of view, I think one could say that the tree belonged there all along, or that it has no business being defined there. I'm not advocating for this, just pointing it out as an option.

@stebalien I'm interested in your thoughts here too.

Stebalien commented 8 months ago

In general, I think using a trait here makes sense (to help decouple tings). Although, if we're not careful, we'll end up with coherence issues.


In terms of moving the state tree into the builtin actors, that doesn't make sense (at the moment):

I'd like to eventually move the entire state tree into the system actor, but that's a much larger refactor.

One possibility is to expose some basic state-tree types in fvm_shared. Specifically:

I'd also be open to introducing an entirely new filecoin_state_tree crate somewhere, either in the FVM repo or an entirely new repo.


Also note: most of the complexity in the state-tree logic in the FVM comes from gas accounting, caching, and reverts. None of those will be relevant outside the FVM itself. The only fields relevant outside of the FVM are:

https://github.com/filecoin-project/ref-fvm/blob/e9044cb8eca4ca928bede06689c4408b7837cc6f/fvm/src/state_tree.rs#L29-L32

alexytsu commented 8 months ago

Closed by https://github.com/filecoin-project/builtin-actors/pull/1459