Wire in the customized EAM actor into genesis #676

cryptoAtwill · 2024-02-08T07:31:04Z

This one builds upon eng-608 but performs the deployment of customized EAM actor. The approach is:

Load the manifest as it is
Created a replace_built_in_actor method that reads the state of system actor, replaces the target built in actor and updates the system actor manifest.

…dify-builtin-actor

…g-608

… modify-builtin-actor

fridrik01 · 2024-02-08T10:33:08Z

fendermint/vm/interpreter/src/fvm/state/genesis.rs

+        Ok(state)
+    }
+
+    pub async fn new_state(


Since we no longer parse the bundles here, consider renaming to something like new_empty_state

fridrik01 · 2024-02-08T10:44:34Z

fendermint/vm/interpreter/src/fvm/genesis/builtin.rs

+        for (_, code_cid) in builtin.iter_mut().filter(|(name, _)| name == "eam") {
+            *code_cid = *code
+        }


do we not have a constant for the name?, also we should assert that this name was actually found in the builtin vector

We added one in https://github.com/consensus-shipyard/ipc/pull/610/files#diff-a5d767cb02a5b7648a64451054b2e648413395275d693e7ea683dd9676d01115R129 , although not for this and it's not accessible, but we could move that type to a common place.

The FVM has them here but they aren't accessible.

fridrik01 · 2024-02-08T10:56:00Z

fendermint/vm/interpreter/src/fvm/genesis/builtin.rs

+    async fn replace_eam_actor<DB>(
+        &self,
+        state: &mut FvmGenesisState<DB>,
+        store: &MemoryBlockstore,
+        builtin: &mut Vec<CodeByName>,
+    ) -> anyhow::Result<(Cid, Cid)>


We should rename this function or split it in two functions as its not clear what its doing by its name. We could then make it more general whereby the caller can send the actor name it wants to replace (and even a list in the future)

raulk

Let's consider a different approach here.

Bundles

Have each custom actor under fendermint/actors generate its own bundle. So instead of generating a monolithic custom actors bundle, have the chainmetadata build generate a one-actor bundle. Same for the EAM -- generates its own bundle.
Rename settings.builtin_actors_bundle to settings.system_actor_bundles. Make it variadic so it can accept many paths. This should take the builtin actors and the chainmetadata actor.
Add a new field settings.extra_actor_bundles, also a variadic setting, it can take many paths. This is the one where users would add the EAM actor if they want to (we don't ship it by default!).

Blockstore load

We load all bundles into the blockstore, starting with system bundles, then moving to extra bundles. We retain each Manifest.
We merge all Manifests by key, where earlier manifests are overridden by entries in later manifests. You can probably use Iterator#fold here. This is how the built-in EAM would be overridden by the custom EAM.

Actor instantiation

I would keep this as it was (in the original init method), and would instruct users to override whatever they wanted in-place. In the future, we can make this nicer by calling out to a customize_genesis_state_tree method that devs can implement to customize the base state tree, but we can follow up on that later.

cryptoAtwill · 2024-02-08T12:54:38Z

I feel it's clearer if we are very specific about which built-in actor we want to replace. For example, if we want to replace EAM actor, we just have a parameter in genesis/config like:

built_in_actors:
  default: ...
  customize:
    eam: path to eam

I feel this way is better than extra_actor_bundles, because after parsing the extra_actor_bundles, we need to guess by name if the custom actor should actually replace the default built-in actor, unless we kind of assumed if the name overlaps, we replace the default one. But this is ambiguous as the code id could be different and they just share similar names.

aakoshh · 2024-02-08T13:12:08Z

I'll have a look at the code, but once again I don't think configuring replacements can be done in YAML files, because the replacement actor can have its state instantiation logic, which requires coding.

I recommended a method like this on Slack:

state
            .replace_builtin_actor(
                fendermint_actor_eam::EAM_ACTOR_NAME,
                eam::EAM_ACTOR_ID,
                fendermint_actor_eam::State::new(genesis.whitelisted_deployers),

IMO this works because:

You say exactly which actor needs to be replaced by specifying the ActorId of the one that by now should already exist in the system actor state. Its original name shouldn't matter, nor the name of the one you replace it with.
It allows you to create custom state and splice it into the state tree.

It has to be explicit, and name should ideally be unique across the manifests if we want to fold them together.

We have to completely forget about the code ID for this purpose, it only applies to the builtin actor bundle, nothing else. We can trust that the team maintaining the builtin actor bundle can ensure that the bundle contains the contracts in the exact order that they expect, but anything custom should not have such assumptions.

aakoshh · 2024-02-08T13:22:45Z

Rename settings.builtin_actors_bundle to settings.system_actor_bundles. Make it variadic so it can accept many paths. This should take the builtin actors and the chainmetadata actor.

Add a new field settings.extra_actor_bundles, also a variadic setting, it can take many paths. This is the one where users would add the EAM actor if they want to (we don't ship it by default!).

I slightly disagree with this one. I think having the builtin actor bundle as its own setting is fine because it's special: it's maintained by an external team and it's the only one that works with the FVM Manifest type and the code ID assumtpions. For that reason I would keep its own, and handle all the custom bundles, including the chainmetadata, separately.

All the custom bundles should use string identifiers. I think it's not difficult to keep them separate by namespace prefixes, but folding them together should at least allow someone to shadow, if that's what they want to do. The genesis could look for actors by name, and complain if they are missing.

Have each custom actor under fendermint/actors generate its own bundle. So instead of generating a monolithic custom actors bundle, have the chainmetadata build generate a one-actor bundle. Same for the EAM -- generates its own bundle.

I agree that the custom actors setting should be variadic. OTOH I think that perhaps keeping our own custom actors as a multi-actor-bundle has the benefit that it's easier to add another actor without having to change the configuration. Say we kept just two fields: builtin_actor_bundle and custom_actor_bundles, with only the second being variadic; it's easier for everyone who wants to extend it to add a fendermint-extras.car and be content that if there is a newer image with a newer actor, the setting like fendermint-extras.car,fluence-randomx.car doesn't need to be updated.

But then again if we have a dedicated setting for user actors, that works fine as well.

aakoshh · 2024-02-08T13:24:02Z

fendermint/app/src/app.rs

@@ -446,11 +446,11 @@ where
            anyhow!("failed to load custom actor bundle CAR from {custom_actors_bundle:?}: {e}")
        })?;

-        let state = FvmGenesisState::new(
+        let state = FvmGenesisState::new_state(


Is this rename necessary? "state::new" says the same thing as "state::new_state" already.

because new is taken and I dont want to update this method that will break all the tests. If this approach is ok, I will replace new.

aakoshh · 2024-02-08T13:29:24Z

fendermint/vm/interpreter/src/fvm/genesis/mod.rs

+        let builtin_deployer = BuiltInActorDeployer::new(&eth_builtin_ids, &eth_libs);
+        let addr_to_id = builtin_deployer
+            .process_genesis(&mut state, &genesis)
+            .await?;


It doesn't seem like this needs to be a struct, a simple method would do just fine, wouldn't it?

aakoshh

Sorry, I would still prefer the flow I suggested on Slack.

Instead of moving a large swathe of builtin-actor creation and then modifying parts of it to handle the EAM actor, but nothing else, I would do the following:

Keep the actor creation where it is, go through and load the entire builtin bundle as it is.
Implement the replace_builtin_actor method that look in the custom bundle already parsed into the GenesisState and do the necessary change in the state tree
- replace the specified actor ID with the new definition and
- load the manifest held by the system actor
- replace the CID in the manifest and persist the new version to the store
- overwrite the state of the system actor to point at the new manifest root
Call the method once from the genesis interpreter to replace the built-in EAM with ours

This is much easier to extend to allow users to do their own replacements.

(The benefit of this flow is also that you always see the CID and the corresponding actor state going hand-in-hand, which would not be the case if first the CID was replaced, then the regular built-ins created with wrong state, then correct state inserted).

cryptoAtwill · 2024-02-08T13:56:10Z

It makes me miss Java Spring Boot sometime, we can just create annotation and annotate the class with @ReplaceBuildInActor(...). It's quite difficult to do the same in rust.

cryptoAtwill · 2024-02-08T14:05:33Z

@aakoshh It's actually a bit trickier to replace built-in actors. It needs to be done before SystemActor is created. I think the flow of this PR is correct but code arrangement can be changed. It has to be:

Load all bundles in memory
Replace the built-in actors with state initialization
Generate new cid
Init system actor with cid above
Init everything else

raulk · 2024-02-08T14:07:17Z

I defer to what @aakoshh suggests. However:

The chainmetadata actor is compulsory for Fendermint and the EVM to work, whilst the modified EAM is not, so I would suggest putting these in two different categories. The former is not a custom actor, it's a system actor.
I tried to avoid having three tiers of actors: builtin (inherited), system (currently chainmetadata, but in the future could include native versions of IPC actors, and more), custom (whatever the user wants). That's why I suggested consolidating builtin and system into system. But I see the logistical difficulties that Akosh mentions about the "code id".
I do not agree with wanting to reserve a whole category/tier for builtin-actors simply because they're externally provided (i.e. based on provenance). It's just circumstancial that we use that bundle in bulk today. In the near future I'd expect us to repack that bundle leaving out actors we don't use/wire (miner, storage power, etc.)

cryptoAtwill · 2024-02-08T14:17:40Z

Why not create a ActorBuilder struct that takes the built-in bundle and system/custom actor bundle path. then we add a few methods:

fn use_built_in_actor(name);
fn replace_built_in_actor(build_in_actor_name, custom_actor_name, fn -> State);
fn use_custom_actor(name, fn -> State);

fn build() -> (build_in_manifest, custom_manifest)

cryptoAtwill · 2024-02-09T14:06:20Z

@raulk The implementation is updated, but I cannot achieve @aakoshh 's method fully because the Manifest is completely loaded in genesis state. I have to do what’s done in the PR to get around it. If we are going to refactor it further, maybe we can store the built-in actor's name and code cid vector in genesis state instead of the Manifest.

aakoshh · 2024-02-12T11:39:27Z

I don't understand why the system actor state has to be fully created up front. The actor state is just a CID in the StateTree under the actor's ID. We should be able to load the content from the blockstore, save a modified copy and update the CID in the StateTree.

I wouldn't go for this ActorBuilder simply because the FvmGenesisState already has these methods, and the interpreter is calling it. Building actors is basically what they do.

aakoshh · 2024-02-12T11:41:10Z

fendermint/vm/interpreter/src/fvm/genesis.rs

+            // fendermint_actor_eam::PermissionModeParams::AllowList(
+            //  vec![...]
+            // )
+            fendermint_actor_eam::PermissionModeParams::Unrestricted,


Is there a ticket number to track that this has to go in the genesis file?

@aakoshh My view here is that we should be extremely selective with what goes in genesis, as it could easily lead to configuration bloat as we keep adding more custom actors, or more knobs. Since our early users will be forking Fendermint, I'd rather have them use whichever mechanism they want to set the policy here.

My opinion is what can change should go into the config, rather than be hardcoded and force people to fork just to change a value. We obviously cannot leave it like this, it has to go into some config. Alternatively we should not do the replacement here at all, just farm out the ability to do so into the application root, to minimise the surface area of where users have to look if they fork or import the code.

In general, things that affect the consensus should be in the genesis.json file, and other settings that can vary between nodes should live outside. This once clearly affects the state, so it's a thing for genesis.json.

In the Comsos SDK for example, every module can have their own section in the genesis, it's just a JSON file after all, and which keys goes in there depends on the node configuration. Here's the genesis for staking for example.

I agree that we don't want bloat. For example I didn't put in the ability to disable the chain metadata, even though it affects consensus, because I think it won't be used outside tests. However, this particular actor is useless without configuration, and everyone has to agree on who is allowed to deploy. In this case sharing the genesis is easiest if it's nicely wrapped up in a single file, rather than that file, plus some other config that people have to remember to also send to their peers.

That said, I see that the fact we grab configuration for the genesis from on-chain complicates things to some extent, ie. not sure if we want this to be a configuration of the subnet creation CLI and the contracts.

Anyway, I just wanted to point out that it has to go somewhere.

@aakoshh the original intention was to offer this actor, but not to wire it in and ship it as a default actor in genesis (which is what this PR is doing). Keeping the code inchantations commented out with some instructions is what I originally proposed here.

The rationale being that bloating genesis does not scale as we introduce additional IPC actors down the line: new FVM runtimes, new system services (tunnels, bridges, etc.), etc. We'll eventually move to a content-addressed manifest model that declares the contents of the subnet both at genesis, and at upgrade checkpoints.

But if you feel strongly about shipping with this modified actor as a default and exposing the config through genesis, I can live with that. More so because @cryptoAtwill had already done that and he dropped the change on my request.

@cryptoAtwill let's just call the shots here -- @cryptoAtwill can you re-add the genesis config for this actor and make sure that it can tackle all three cases (unrestricted, restricted with nobody allowed, restricted with specific addresses allowed).

aakoshh · 2024-02-12T11:44:58Z

fendermint/vm/interpreter/src/fvm/state/genesis.rs

+        let mut intermediate_builtin = load_builtin(builtin_actor_bundle).await?;
+
+        replace_built_in_actor(
+            fendermint_actor_eam::IPC_EAM_ACTOR_NAME,
+            &custom_actor_manifest,
+            &mut intermediate_builtin,
+        )?;
+
+        // copy builtin actors from memory to store
+        intermediate_builtin.mem_store.copy_to(&store)?;


If you can do this here, I'm confident that you can also do this in the interpreter so that it can be offered to the users as well, and as I mentioned the actor name/CID can be kept together with the definition of its state, rather than be split in two places: that the name is replaced here, and you have to follow up with the corresponding state in a completely different place, and if you don't then things go horribly wrong.

I thought about doing what you mentioned, but the issue I found quite strange was the FvmGenesisState takes the manifest_cid and manifest as part of the struct. When we deploy the contracts, there is no access to the original builtin actor bundle . I understand where you are coming from and I think that’s the better way. But I feel we can refactor the FvmGenesisState to accept bundle_bytes in place of manifest_cid and manifest that way we can have everything in one place and it would feel natural.

I might be wrong as I haven't tried it myself, but I don't think taking manifest bytes or a loaded and parsed manifest makes much of a difference to what I'm saying. The manifests are loaded in new and loaded immediately, so that methods like create_builtin_actor and create_custom_actor can refer to the content by code IDs and names.

I think you are pointing out this difference because we are still talking about a different thing. I suspect you want to keep it as bytes so you can manipulate those bytes before loading them in the store, as you do now IIRC. I am suggesting not to do that.

Instead, I suggest that we load all bundles into manifests at the start, as we do now. The builtin actor bundle goes in the ref-fvm Manifest, the custom actors go in our custom manifest that indexes by name. Then we load the regular builtin actors as we do now, no change in that. After that, we implement the code that can replace_builtin_actor method: when it's called, the System actor will already exist, and its builtin_actors CID will match the one in the FvmGenesisState::manifest_data_cid. At that point we can ignore the manifest_data_cid; we load the Vec<(String, Cid)> pointed at the by the System actors state, find the name we want to replace, store the new vector, assign its CID to the actor state, store the actor state, modify the system actor's ID in the StateTree to point at the new state, and we can also store the values inside the FvmGenesisState as well if we load a new Manifest.

This leaves some intermediate values in the blockstore but that's okay because they won't be persisted as nothing will point at them.

@aakoshh I see where you are coming from, so now manifest_data_cid is just for initial loading and subsequent updates are directly read from the system actor state. This works for me as well!

raulk · 2024-02-15T10:34:48Z

fendermint/vm/interpreter/src/fvm/genesis.rs

-                None,
-            )
-            .context("failed to create EAM actor")?;
-
        // Burnt funds actor (it's just an account).


Is there any harm in keeping this here? The replace method should be able to override this.

Yes, please keep it. If we open up replacements to users, we don't want to modify this part of the code, so we should walk that path here as well IMO.

correct, but it should work without this as well.

True, it works without it - the question is, does it work with it?

raulk · 2024-02-15T10:36:59Z

fendermint/vm/interpreter/src/fvm/genesis.rs

+            // fendermint_actor_eam::PermissionModeParams::AllowList(
+            //  vec![...]
+            // )
+            fendermint_actor_eam::PermissionModeParams::Unrestricted,


@aakoshh My view here is that we should be extremely selective with what goes in genesis, as it could easily lead to configuration bloat as we keep adding more custom actors, or more knobs. Since our early users will be forking Fendermint, I'd rather have them use whichever mechanism they want to set the policy here.

fendermint/vm/interpreter/src/fvm/state/genesis.rs

raulk · 2024-02-15T10:40:12Z

fendermint/vm/interpreter/src/fvm/state/genesis.rs

+            Stage::Tree(s) => s.get_actor(actor)?,
+            Stage::Exec(s) => s.state_tree().get_actor(actor)?,


I'm not familiar with this stuff, so defer to @aakoshh's feedback for correctness here.

It looks correct, however I think it can be implemented on top of with_state_tree already:

let actor_state_cid = self.with_state_tree( |s| s.get_actor(actor), |s| s.get_actor(actor) ).ok_or_else(|| anyhow!("actor state {actor} not found, is it deployed?"))?

Doesn't make a huge difference in this case, but it's why that method exists.

In fact it's already used like that here

@raulk this pattern is used because the FvmGenesisState starts with a StateTree but later evolves into having a full FvmExecState to actually run EVM contract deployments.

that works for me too, when I looked at the function definition, I had the impression with_state_tree is mutating the state and it requires &mut self. THis way there is no need for &mut

Yeah, I didn't want two versions, it was already verbose enough and I had access to &mut everywhere I needed this.

aakoshh · 2024-02-15T11:33:47Z

fendermint/vm/interpreter/src/fvm/state/genesis.rs

@@ -211,7 +279,7 @@ where
    }

    /// Creates an actor using code specified in the manifest.
-    fn create_actor_internal(
+    pub(crate) fn create_actor_internal(


Why does this have to be public now?

my bad, changes leaked from previous version

aakoshh

Thank you, it looks correct 👍

My comments aren't blockers, just minor things I'd consider changing for the sake of consistency.

raulk · 2024-02-15T14:02:58Z

contracts/binding/src/lib.rs

Revert fmt pls.

raulk · 2024-02-15T14:06:44Z

fendermint/app/src/cmd/genesis.rs

@@ -45,6 +45,7 @@ cmd! {
      power_scale: self.power_scale,
      validators: Vec::new(),
      accounts: Vec::new(),
+      eam_permission_mode: PermissionMode::Unrestricted,


How does the user specify the permission mode when constructing genesis?

This is the default setup, so when a new genesis is created by calling fendermint, the default mode is Unrestricted. Then users can update this field to restricted.

raulk · 2024-02-15T15:46:24Z

contracts/binding/src/lib.rs

There is still a diff here @cryptoAtwill. Can you revert all changes on this file?

raulk · 2024-02-16T02:05:44Z

contracts/binding/src/lib.rs

Revert diff please.

I think your editor is messing with you!

cryptoAtwill added 8 commits February 1, 2024 14:34

adding ipc eam actor

44edd68

fmt code

8699df1

fmt code

71d162a

integrate with fendermint

ae025e2

initial commit

61c33a1

Merge branch 'main' of protocol-github:consensus-shipyard/ipc into mo…

53155d4

…dify-builtin-actor

unit tests passing

6a90127

Merge branch 'main' of protocol-github:consensus-shipyard/ipc into en…

8ee6196

…g-608

cryptoAtwill requested review from raulk, fridrik01 and aakoshh February 8, 2024 07:31

Merge branch 'eng-608' of protocol-github:consensus-shipyard/ipc into…

6d1f95b

… modify-builtin-actor

cryptoAtwill marked this pull request as draft February 8, 2024 07:34

fridrik01 reviewed Feb 8, 2024

View reviewed changes

raulk reviewed Feb 8, 2024

View reviewed changes

aakoshh reviewed Feb 8, 2024

View reviewed changes

cryptoAtwill added 5 commits February 8, 2024 23:00

fix code review

b1d5fd6

remove deployment of IPC EAM

951a29a

remove contract deployer

bdfb841

add comment

2c6076b

fix main linter

a4e226f

cryptoAtwill requested review from fridrik01, aakoshh and raulk February 9, 2024 23:48

aakoshh reviewed Feb 12, 2024

View reviewed changes

Base automatically changed from eng-608 to main February 12, 2024 16:20

cryptoAtwill added 3 commits February 15, 2024 14:44

merge with main and update implementation

9dfa80e

remove unused code

761ac73

update comment to todo

90dfca6

cryptoAtwill requested a review from aakoshh February 15, 2024 06:49

raulk reviewed Feb 15, 2024

View reviewed changes

aakoshh reviewed Feb 15, 2024

View reviewed changes

aakoshh approved these changes Feb 15, 2024

View reviewed changes

add genesis permission mode

f9b040c

cryptoAtwill requested a review from raulk February 15, 2024 13:40

format code

e646d3f

raulk reviewed Feb 15, 2024

View reviewed changes

revert fmt binding

dc197fb

raulk reviewed Feb 15, 2024

View reviewed changes

contracts/binding/src/lib.rs Outdated

Copy link

Contributor

raulk Feb 15, 2024

Choose a reason for hiding this comment

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

There is still a diff here @cryptoAtwill. Can you revert all changes on this file?

Add genesis command to update EAM permission mode (#709)

4357c40

raulk reviewed Feb 16, 2024

View reviewed changes

Merge branch 'main' into modify-builtin-actor

a954cb5

raulk changed the title ~~Modify builtin EAM actor~~ Wire in the customized EAM actor into genesis Feb 16, 2024

cryptoAtwill added 2 commits February 16, 2024 20:01

Merge branch 'main' into modify-builtin-actor

6966d83

Revert lib.rs

58a073d

raulk merged commit 9a2c7a1 into main Feb 16, 2024
19 of 20 checks passed

raulk deleted the modify-builtin-actor branch February 16, 2024 18:26

mb1896 pushed a commit that referenced this pull request Feb 19, 2024

Wire in the customized EAM actor into genesis (#676)

5d7f80b

		Stage::Tree(s) => s.get_actor(actor)?,
		Stage::Exec(s) => s.state_tree().get_actor(actor)?,

Wire in the customized EAM actor into genesis #676

Wire in the customized EAM actor into genesis #676

Conversation

cryptoAtwill commented Feb 8, 2024 • 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

raulk left a comment

Choose a reason for hiding this comment

Bundles

Blockstore load

Actor instantiation

cryptoAtwill commented Feb 8, 2024

aakoshh commented Feb 8, 2024

aakoshh commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh left a comment • edited Loading

Choose a reason for hiding this comment

cryptoAtwill commented Feb 8, 2024

cryptoAtwill commented Feb 8, 2024

raulk commented Feb 8, 2024 • edited Loading

cryptoAtwill commented Feb 8, 2024 • edited Loading

cryptoAtwill commented Feb 9, 2024

aakoshh commented Feb 12, 2024

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 Feb 15, 2024 • 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

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

aakoshh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoAtwill commented Feb 8, 2024 •

edited

Loading

aakoshh left a comment •

edited

Loading

raulk commented Feb 8, 2024 •

edited

Loading

cryptoAtwill commented Feb 8, 2024 •

edited

Loading

raulk Feb 15, 2024 •

edited

Loading