Fork the Ethereum Address Manager + add deployment permissioning #657

cryptoAtwill · 2024-02-01T06:52:12Z

This one addresses "Add permissioning to the Ethereum Address Manager".

The approach is pretty simple, it creates an HAMT that tracks the whitelisted deployers. Only when the caller is in the HAMT, will the caller be allowed to create contracts. This PR does not copy paste the original EAM implementation but imports it, so that it's easier to maintain.

Follow up PR:

Update genesis to take whitelisted deployers as argument.

linear · 2024-02-01T06:52:15Z

ENG-608 Add permissioning to the Ethereum Address Manager

snissn · 2024-02-01T08:41:19Z

Hey! I was curious about this PR because I had done HAMT related work recently for a filecoin migration. The use of a HAMT makes sense to me in that these data structures should be relatively small and it's relatively common to use an associative data array with a null value as a set. I think the HAMT performance of using a null value over using a AMT which should be sufficient is a non issue here and any performance difference or storage overhead is nearly neglible. Still just wanted to share my reflection and feedback since in other scenarios it may be an important distinction to use an AMT as a set instead of an HAMT with has a value that is being ignored

snissn · 2024-02-01T08:42:00Z

Also looks like there's a panic in a test 😄

cryptoAtwill · 2024-02-01T08:47:28Z

@snissn Cool, thx for the tip. I used HAMT because there might be chances of dynamically changing the deployers, which HAMT might be better suited and more extendable.
I think the CI fails because I have yet to update the genesis deployment.

raulk

I'm glad this approach worked! Integrating into genesis should be pretty simple. You'd just place this actor instead of the upstream one at actor ID 10.

I wonder if you want to turn the state field into an enum of Unpermissioned and Permissioned(Optional<Cid>).

The default would be Unpermissioned (behaving just like the upstream).

Then you have Permissioned(None), where no one can deploy contracts.

And Permissioned(Cid) would contain the Hamt.

raulk · 2024-02-01T09:04:07Z

Apologies, my finger slipped, I only wanted to submit comments, not reject it outright.

cryptoAtwill · 2024-02-01T09:10:07Z

@raulk Yep, I tried that in the first place, but seems like Serialize_tuple does not support enum. I could implement it manually, but then I figured I can actually deploy the default EAM actor if the genesis does not require permission. Unless we want to stick with more complex permission mode, I think it should be good for now to handle this logic in genesis?

raulk · 2024-02-01T09:14:28Z

Interesting. I recall seeing enum fields in state or in input/response messages in builtin-actors. Could you scan the codebase there? Maybe start with the miner actor as it's the most complex one.

raulk · 2024-02-01T09:14:47Z

@fridrik01 might have an answer.

fridrik01

lgtm, but we should make sure that tests are passing before merging (its complaining that the new eam actor is not in the blockstore as it has not been loaded at genesis). Fine with addressin that in followup PR as you mention

fridrik01 · 2024-02-01T09:23:39Z

contracts/binding/src/lib.rs

@@ -2,25 +2,31 @@
 #[macro_use]


Was this change intended? We should either update the build.rs file to format this generated file, or make sure that our tools/ide do not format this file, otherwise it keeps changing between PRs even though it hasn't effectively changed

I pushed a proposal PR which keeps this file formatted when creating this file in build.rs.

somehow it's auto-formatted and I didnt aware it's pushed, I see if I can revert it.

fridrik01 · 2024-02-01T09:31:41Z

Cargo.toml

@@ -183,6 +185,7 @@ fvm_ipld_amt = "0.6.2"
 # and this copy-paste is clunky, so at least for those that have it, we should use it.
 # Keep the version here in sync with the Makefile!
 fil_actors_evm_shared = { git = "https://github.com/filecoin-project/builtin-actors", tag = "v12.0.0" }
+fil_actor_eam = { git = "https://github.com/filecoin-project/builtin-actors", tag = "v12.0.0" }


aakoshh · 2024-02-01T14:46:34Z

fendermint/actors/eam/src/lib.rs

+    actor_dispatch_unrestricted! {
+        Constructor => constructor,
+        Create => create,
+        Create2 => create2,
+        CreateExternal => create_external,
+    }


If any and all methods need to be dispatched, isn't there a way not to have to enumerate them at all?

We can create macro rules on top of this. I think this is probably the convention used and the actor is quite simple. I guess it's ok?

I mean, there must be some general method that would simply forward, like handle_message(method_number, params) ?

yeah, there is, but actor_dispatch_unrestricted actually contains some checking logic as well.

The ActorCode trait has this invoke_method method that you can trivially implement for this actor, and then you don't need to implement all the individual methods, because they all check the same thing. Something like this:

fn invoke_method<RT>( rt: &RT, method: fvm_shared::MethodNum, args: Option<fvm_ipld_encoding::ipld_block::IpldBlock>, ) -> Result<Option<fvm_ipld_encoding::ipld_block::IpldBlock>, ActorError> where RT: Runtime, RT::Blockstore: Clone, { Self::ensure_deployer_allowed(rt)?; EamActor::invoke_method(rt, method, args) }

I don't see any documentation on actor_dispatch_unrestricted so can't say what's the difference between that and actor_dispatch, but the only check I see it does is the method number. That will be performed by EamActor::invoke_method as well.

I agree with @aakoshh. Performing the permissioning at the invoke_method entrypoint should be enough (we should leave the Constructor) out of this extra layer of permissioning, since it can only be invoked by the system actor and it already performs that check.

https://github.com/filecoin-project/builtin-actors/blob/master/actors/eam/src/lib.rs#L237

This way we can get rid of every method here.

Updated accordingly

aakoshh · 2024-02-01T14:47:55Z

fendermint/actors/eam/src/state.rs

+
+impl State {
+    pub fn new<BS: Blockstore>(store: &BS) -> Result<State, ActorError> {
+        let empty_deployers = DeployerMap::empty(store, DEFAULT_HAMT_CONFIG, "empty").flush()?;


What is this "empty" thing?

It's an identifier for the map, it's not stored in the storage, but just in memory. I took this from the actor development convention. I think this is a identifier for the map or sth along that line.

aakoshh

Sorry, approved by accident.

…g-608

fendermint/actors/eam/src/state.rs

raulk · 2024-02-08T11:04:30Z

fendermint/actors/eam/src/lib.rs

+    actor_dispatch_unrestricted! {
+        Constructor => constructor,
+        Create => create,
+        Create2 => create2,
+        CreateExternal => create_external,
+    }


I agree with @aakoshh. Performing the permissioning at the invoke_method entrypoint should be enough (we should leave the Constructor) out of this extra layer of permissioning, since it can only be invoked by the system actor and it already performs that check.

https://github.com/filecoin-project/builtin-actors/blob/master/actors/eam/src/lib.rs#L237

raulk · 2024-02-08T11:05:01Z

fendermint/actors/eam/src/lib.rs

+    actor_dispatch_unrestricted! {
+        Constructor => constructor,
+        Create => create,
+        Create2 => create2,
+        CreateExternal => create_external,
+    }


This way we can get rid of every method here.

raulk · 2024-02-08T11:06:57Z

fendermint/actors/eam/src/state.rs

+const NO_RESTRICTION_MODE: u8 = 0;
+const WHITELIST_MODE: u8 = 1;
+
+impl Serialize for PermissionMode {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        match self {
+            PermissionMode::NoRestriction => {
+                let inner: (_, Vec<u8>) = (NO_RESTRICTION_MODE, vec![]);
+                Serialize::serialize(&inner, serde_tuple::Serializer(serializer))
+            }
+            PermissionMode::Whitelist(cid) => {
+                let inner = (WHITELIST_MODE, cid.to_bytes());
+                Serialize::serialize(&inner, serde_tuple::Serializer(serializer))
+            }
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for PermissionMode {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let (mode, bytes): (u8, Vec<u8>) =
+            Deserialize::deserialize(serde_tuple::Deserializer(deserializer))?;
+        Ok(match mode {
+            NO_RESTRICTION_MODE => PermissionMode::NoRestriction,
+            WHITELIST_MODE => PermissionMode::Whitelist(
+                Cid::from_bytes(&bytes).map_err(|_| Error::custom("invalid cid"))?,
+            ),
+            _ => return Err(Error::custom("invalid permission mode")),
+        })
+    }
+}


You should not be needing the custom serde here! According to @vmx here, libipld_serde_cbor supports externally tagged enums (not clear to me how it lays them out though), as well as Option types. We might need to add the #[repr] tag so serde knows which type to use for the discriminator. Perhaps that's why it didn't work for you originally?

raulk · 2024-02-08T11:21:04Z

fendermint/vm/genesis/src/lib.rs

@@ -40,6 +40,9 @@ pub struct Genesis {
    /// highest possible fidelity when we are deriving a genesis file in IPC,
    /// where the parent subnet tracks collateral.
    pub validators: Vec<Validator<Collateral>>,
+    /// List of contract addresses that are allowed to deploy contracts in the subnet. If
+    /// empty, indicates there is no restriction on who can deploy contracts
+    pub contract_deployers: Vec<SignerAddr>,


Let's roll back the genesis changes here, and instead have users who want to use this feature fork Fendermint and customize genesis within init. Rationale: there will be plenty of customizations possible down the line, and it won't scale to expose them all through genesis. In the future, these will be Wasm modules.

But for now, how are they going to config this?

that means we put the addresses in the source code? But I think this one should be in genesis as we should not allow the deployers to change regardless how source code changes. If deployers need to change, we should also go through txn submission.

Cargo.toml

adding ipc eam actor

44edd68

cryptoAtwill added 2 commits February 1, 2024 15:05

fmt code

8699df1

fmt code

71d162a

cryptoAtwill requested review from aakoshh, fridrik01 and raulk and removed request for aakoshh February 1, 2024 07:06

raulk requested changes Feb 1, 2024

View reviewed changes

fridrik01 reviewed Feb 1, 2024

View reviewed changes

aakoshh reviewed Feb 1, 2024

View reviewed changes

raulk changed the title ~~Adding ipc eam actor~~ Add permissioning to the Ethereum Address Manager Feb 1, 2024

aakoshh approved these changes Feb 1, 2024

View reviewed changes

integrate with fendermint

ae025e2

cryptoAtwill marked this pull request as draft February 2, 2024 15:39

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

8ee6196

…g-608

raulk requested changes Feb 8, 2024

View reviewed changes

cryptoAtwill added 4 commits February 8, 2024 23:00

fix code review

b1d5fd6

remove deployment of IPC EAM

951a29a

remove contract deployer

bdfb841

add comment

2c6076b

cryptoAtwill marked this pull request as ready for review February 8, 2024 15:48

cryptoAtwill requested a review from raulk February 8, 2024 15:48

cryptoAtwill and others added 3 commits February 9, 2024 00:18

fix main linter

a4e226f

fmt

b45a1b3

Merge branch 'main' into eng-608

80135dd

raulk approved these changes Feb 9, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

cryptoAtwill and others added 2 commits February 9, 2024 20:26

remove unused crate

04962ec

Merge branch 'main' into eng-608

59554c4

raulk approved these changes Feb 12, 2024

View reviewed changes

raulk changed the title ~~Add permissioning to the Ethereum Address Manager~~ Fork the Ethereum Address Manager Feb 12, 2024

raulk changed the title ~~Fork the Ethereum Address Manager~~ Fork the Ethereum Address Manager + add deployment permissioning Feb 12, 2024

raulk merged commit 5beaab2 into main Feb 12, 2024
19 checks passed

raulk deleted the eng-608 branch February 12, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork the Ethereum Address Manager + add deployment permissioning #657

Fork the Ethereum Address Manager + add deployment permissioning #657

cryptoAtwill commented Feb 1, 2024

linear bot commented Feb 1, 2024

snissn commented Feb 1, 2024

snissn commented Feb 1, 2024

cryptoAtwill commented Feb 1, 2024

raulk left a comment •

edited

Loading

raulk commented Feb 1, 2024

cryptoAtwill commented Feb 1, 2024

raulk commented Feb 1, 2024

raulk commented Feb 1, 2024

fridrik01 left a comment

fridrik01 Feb 1, 2024

fridrik01 Feb 1, 2024

cryptoAtwill Feb 1, 2024

fridrik01 Feb 1, 2024

aakoshh Feb 1, 2024

cryptoAtwill Feb 2, 2024

aakoshh Feb 2, 2024

cryptoAtwill Feb 2, 2024

aakoshh Feb 2, 2024 •

edited

Loading

aakoshh Feb 2, 2024

raulk Feb 8, 2024

raulk Feb 8, 2024

cryptoAtwill Feb 8, 2024

aakoshh Feb 1, 2024

cryptoAtwill Feb 2, 2024

aakoshh left a comment •

edited

Loading

raulk Feb 8, 2024

raulk Feb 8, 2024

raulk Feb 8, 2024

raulk Feb 8, 2024

cryptoAtwill Feb 8, 2024

cryptoAtwill Feb 8, 2024 •

edited

Loading

Fork the Ethereum Address Manager + add deployment permissioning #657

Fork the Ethereum Address Manager + add deployment permissioning #657

Conversation

cryptoAtwill commented Feb 1, 2024

linear bot commented Feb 1, 2024

snissn commented Feb 1, 2024

snissn commented Feb 1, 2024

cryptoAtwill commented Feb 1, 2024

raulk left a comment • edited Loading

Choose a reason for hiding this comment

raulk commented Feb 1, 2024

cryptoAtwill commented Feb 1, 2024

raulk commented Feb 1, 2024

raulk commented Feb 1, 2024

fridrik01 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

Choose a reason for hiding this comment

aakoshh Feb 2, 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

aakoshh left a comment • 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

cryptoAtwill Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

raulk left a comment •

edited

Loading

aakoshh Feb 2, 2024 •

edited

Loading

aakoshh left a comment •

edited

Loading

cryptoAtwill Feb 8, 2024 •

edited

Loading