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

extract actors to filecoin-project/canonical-actors #351

Merged
merged 20 commits into from
Mar 2, 2022

Conversation

raulk
Copy link
Member

@raulk raulk commented Feb 28, 2022

This is the ref-fvm side of filecoin-project/builtin-actors#4. To be paired with the raulk/initial branch on https://github.com/filecoin-project/builtin-actors.

  • Removes actors code from this repo.
  • Renames {forest=>ipld}_bitfield.
  • Introduces the actor::is_builtin_actor syscall.
  • Introduces the actor::get_code_cid_for_type syscall.
  • Makes the Machine load Wasm bytecode from the blockstore.
  • Makes testing/conformance import builtin-actors v6.
  • Adds the fvm_shared::actor::builtin module with types to support the loading of builtin actor code and the resolution of CIDs.

@raulk raulk marked this pull request as draft February 28, 2022 00:22
@raulk raulk marked this pull request as ready for review February 28, 2022 19:42
@raulk raulk requested a review from Stebalien February 28, 2022 19:42
Comment on lines +651 to +657
lazy_static! {
pub static ref DUMMY_ACCOUNT_ACTOR_CODE_ID: Cid =
Cid::new_v1(IPLD_RAW, Identity.digest(b"fil/test/dummyaccount"));
pub static ref DUMMY_INIT_ACTOR_CODE_ID: Cid =
Cid::new_v1(IPLD_RAW, Identity.digest(b"fil/test/dummyinit"));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These didn't need to be real CodeCIDs.

Comment on lines +24 to +37
#[repr(i32)]
pub enum Type {
System = 1,
Init = 2,
Cron = 3,
Account = 4,
Power = 5,
Miner = 6,
Market = 7,
PaymentChannel = 8,
Multisig = 9,
Reward = 10,
VerifiedRegistry = 11,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Explicitly enumerating these since they're part of the syscall ABI.

Comment on lines 96 to 117
// Load the built-in actors manifest.
let builtin_actors: Manifest = blockstore
.get_cbor(&builtin_actors)
.context("failed to load built-in actor index")?
.ok_or_else(|| {
anyhow!(
"blockstore doesn't contain builtin actors index with CID {}",
&builtin_actors
)
})?;

// Preload any uncached modules.
// This interface works for now because we know all actor CIDs
// ahead of time, but with user-supplied code, we won't have that
// guarantee.
engine.preload_uncached(&blockstore, builtin_actors.keys())?;

// Create a new state tree from the supplied root.
let state_tree = {
let bstore = BufferedBlockstore::new(blockstore);
StateTree::new_from_root(bstore, &context.initial_state_root)?
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant block.

for (k, bytecode) in actors {
self.load_bytecode(k, bytecode.expect("precompiled actor not found"))
.expect("failed to compile built-in actor");
/// Instantiates and caches the Wasm modules for the bytecodes addressed by
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant block.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

nits but otherwise lgtm

fvm/src/call_manager/default.rs Outdated Show resolved Hide resolved
Comment on lines 119 to 125
let is_account = self
.call_manager
.machine()
.builtin_actors()
.get(&act.code)
.cloned()
.map(actor::builtin::is_account_actor)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we'll want a method on the machine to help with this (or maybe on the manifest?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely not in the Machine as that would pollute the trait. Proper place would be in the Manifest, but right now it's a type alias and adding a method there would involve turning it into a newtype. Not sure about how the IPLD serde would interact with that, so given that our interest here is removing a 2-ocurrence duplication and that the net LoC gain of adding the newtype with deref, etc. would be higher, I'm inclined to just leave it as it is.

fvm/src/kernel/default.rs Outdated Show resolved Hide resolved
let state_tree = StateTree::new_from_root(bstore, &context.initial_state_root)?;
// Load the built-in actors manifest.
let builtin_actors: Manifest = blockstore
.get_cbor(&builtin_actors)
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: this isn't going to work the splitstore. We'll have to link to actors from the chain state.

Copy link
Member Author

@raulk raulk Mar 2, 2022

Choose a reason for hiding this comment

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

The idea we discussed was linking to these CIDs from the init actor state. But, we can't do this before the migration because that would alter the init actor state and break consensus. So the solution I gravitate towards is:

  1. Make the node (re-)import the CAR on the migration epoch.
  2. Then link the CodeCIDs in the init actor state.

In practical terms, we could do away with (2), because every actor will end up being linked from the state tree anyway:

  1. Singleton actors are permanent.
  2. Non-singleton actors cannot be destroyed, and there's many of each in the network already.

fvm/src/machine/engine.rs Outdated Show resolved Hide resolved
fvm/src/machine/engine.rs Show resolved Hide resolved
fvm/src/machine/engine.rs Outdated Show resolved Hide resolved
fvm/src/machine/engine.rs Show resolved Hide resolved
shared/src/actor/builtin.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants