filecoin-project / builtin-actors

The Filecoin built-in actors
Other
81 stars 76 forks source link

Builtin Actor Registry #122

Closed Stebalien closed 2 years ago

Stebalien commented 2 years ago

Currently:

  1. Actor code (wasm) is passed into the FVM via a CAR file.
  2. We use "fake" CIDs to address actors.

As of nv16, this code will live on-chain as state and will be referenced by real CIDs. We could continue passing in a CAR file with these actors when we construct the machine, however:

  1. Parsing the CAR file each time is inefficient.
  2. This actor code is state, so it should live in the state-tree. Otherwise, we could end up with strange mismatches.
  3. Not all actor types will be instantiated at genesis. If we import the CAR file once when we, e.g., start a test network, something like the splitstore may garbage collect these actors before anyone constructs one of them.

Proposal: Include a builtin-actor "manifest" in either the init actor, or the system actor.

Specifically (ish):

  1. Change the "manifest" block in the actor bundle (https://github.com/filecoin-project/builtin-actors/blob/76eecbfc1828bab6e46623b5582b845a2d18404e/src/lib.rs#L23) to be "correct" IPLD.
    1. Currently, this manifest block is a mapping of CIDs to numbers identifying the different actor types.
    2. We should change this to a mapping of actor names (strings) to actors. This is slightly less efficient, but we can optimize lookups at runtime. We could also change it to be a list of some sort, but I feel that this is a case where we might as well be verbose.
  2. Link this manifest into the init actor's state, or the system actor's state. Where this should live isn't entirely cut and dry:
    1. Eventually, we'll have an actor "registry" in the init actor so the init actor knows which actors can and cannot be deployed. However, the builtin actor manifest is a bit different because we use it for permission checks, not just initialization/deployment.
    2. We have quite a few tools that read the init actor's state, and the state layout hasn't ever changed. If we put this registry in the init actor, we'd need to support reading multiple versions of this state. This isn't hard, just slightly annoying.
Stebalien commented 2 years ago

cc @ZenGround0 @vyzo @raulk @anorth

Stebalien commented 2 years ago

NOTE: this needs to get PRed to the FIP, but I wanted to get the exact design nailed down before doing that.

raulk commented 2 years ago

Thanks for starting this, @Stebalien.

Placement

To me it sounds like the right place to place the built-in actor registry is the system actor.

The Filecoin spec defines this actor as the "general system actor". That's rather unspecific, but I would imagine this is the appropriate place to hold consensus-critical static system configuration values that the network needs to agree on. The list of built-in actors falls under that category.

In fact, this creates precedent to store more consensus-critical, general, static configuration values in-band in the state tree, e.g. the gas pricing schedule, like we have discussed in the past 👍

And eventually, we could move the network_name from the init actor to the system actor, as it really doesn't belong in the former. (I suppose it was placed in the init actor for convenience, but not correctness.)

Implementation notes

Data structure

It would be extremely convenient to make the data structure inside the state tree and the bundle identical.

I do still think that a Map<Cid, SomeMetadataStruct> is the right type:

raulk commented 2 years ago

NOTE: this needs to get PRed to the FIP, but I wanted to get the exact design nailed down before doing that.

Since I'm editing FIP-0031 to prepare it for Last Call, I can own the FIP update. But let's make a decision today ;-)

raulk commented 2 years ago

Outcome from sync discussion:

anorth commented 2 years ago

This mostly sounds ok to me. A few minor concerns:

Stebalien commented 2 years ago

What's the CID in the manifest value the CID of? The WASM code, or a metadata struct, or ...?

Currently the wasm code. I've filed https://github.com/filecoin-project/ref-fvm/issues/744 for discussion.

I think it would be helpful to have a design for how the init actor is going to store code for user actors so that we can ensure this manifest doesn't lead to two different paths when instantiating builtin vs user actors later. The OP noted both that change is hard, but also we must change this anyway, so there's not much point trying to avoid it now.

I agree. We didn't address that here because we don't need to address it till M2. But having at least a design sketch would be helpful.

Consider a simple list of tuples as the representation for a small mapping. Introducing IPLD Map here would be the first usage in all of Filecoin, AFAIK, so it's not guaranteed to be friction-free.

That is a good point. I think we'll need to "get over it" eventually, but this may not be the best place to do so.

Introducing IPLD Map here would be the first usage in all of Filecoin

Pedantically, I feel that I need to remind you about the genesis block.

jennijuju commented 2 years ago

@raulk to confirm if this is all good!

raulk commented 2 years ago

Confirmed; this is implemented.