FIP-0054: Introduce the EVM actor's logic #1186

arajasek · 2023-02-08T23:39:37Z

Co-authored-by: Steven Allen [email protected]
Co-authored-by: vyzo [email protected]
Co-authored-by: Melanie Riise [email protected]
Co-authored-by: Raúl Kripalani [email protected]

Extracted from next.

actors/evm/Cargo.toml

anorth · 2023-02-09T01:36:03Z

actors/evm/precompile-testdata/LICENSE

@@ -0,0 +1,165 @@
+                   GNU LESSER GENERAL PUBLIC LICENSE


This isn't PL's standard license, so can we get a README in this directory explaining why LGPL and providing the attribution that's presumably missing? Are we importing this with a JSON file or something?

(summon @filecoin-project/fvm-core-devs

@anorth Added a note to the top-level README (extracted from the next branch):

1b6a2e3

I still don't understand why we're doing this. Given that you didn't add an attribution to someone else, is this our own decision to use this license? Why is that?

I'll add the attribution. These are coming from geth and are technically licensed under the LGPL. I doubt that was intentional and I see these same files copied into other EVM implementations, but I have no way of knowing that for sure.

Ok if we're importing it, but then we're bound to add the attribution, and then things will make sense.

I've added a short readme to this directory to explain the situation.

anorth · 2023-02-09T01:58:14Z

runtime/src/runtime/mod.rs

@@ -63,12 +65,23 @@ pub trait Runtime: Primitives + Verifier + RuntimePolicy {
    /// The genesis block has epoch zero.
    fn curr_epoch(&self) -> ChainEpoch;

+    /// The ID for the EVM-based chain, as defined in https://github.com/ethereum-lists/chains.
+    fn chain_id(&self) -> ChainID;


Grumble grumble here the FVM is privileging Ethereum with this information, rather than having a Filecoin ChainID and an EVM actor or some state somewhere that holds an Ethereum-relevant version, in the same way any externally-developed client VM would have to. This is very subtly mentioned in FIP-0054 as "New environmental data is required by the FVM".

I strongly agree with the grumble here, and would like to see this changed. The current flow is that the client (eg. Lotus) knows what EVM chain ID is operational for the blockchain being processed, creates an FVM with this value ("the" chain ID), which then supplies this value through the Runtime to the FEVM's CHAINID opcode.

I think it would be in favour of your proposal to add a Filecoin ChainID, and a mapping. The biggest issue is that it requires some "process" work (starting a FIP etc.)

Simpler solutions for now could be:

Move this value to the Runtime's policy, configure it by actor bundle, and call it evm_chain_id (still privileged, but contained to the Runtime without spilling into FVM / clients)

Keep as is, but rename to evm_chain_id

This is an Ethereum-specific chain but Ethereum chain IDs have been standardised in Chain Agnostic standards. This should probably read caip_eip155_chain_id and it would be less of an offender that way. But the bottom line is that these chain IDs are actually standardised by some body that does have reasonable adoption.

Discussed with @Stebalien in standup, he provided some background context. The intention is for these IDs to serve as Filecoin ChainIDs (that are conveniently in the Ethereum namespace). Since we don't have any ChainIDs this works pretty well, and honestly is probably what most other chains do anyway.

I agree with you that this has not been sufficiently publicized, though -- a footnote in an unrelated FIP is likely not sufficient for this. @jennijuju do you have suggestions here? I don't know if this needs its own FIP, but should be included in a table of networks and chains somewhere.

(...we can even eventually include these IDs in messages as replay protection one day...)

Is the comment still accurate? Should it be something like "The ID for this chain. Note that Filecoin chain IDs are usually in the Ethereum namespace (i.e. scope "xyz") for [reasons]".

If the comment as it stands is accurate, I still think this should move into some state somewhere (EAM?) rather than be a syscall available to future native actors.

runtime/src/runtime/mod.rs

runtime/src/test_utils.rs

anorth · 2023-02-09T02:06:29Z

state/src/check.rs

@@ -222,6 +222,7 @@ pub fn check_state_invariants<'a, BS: Blockstore + Debug>(
            }
            Some(Type::Placeholder) => {}
            Some(Type::EVM) => {}
+            Some(Type::EAM) => {}


This is quite sad, is there nothing to check here? Please file an issue for implementing these checks.

anorth · 2023-02-09T02:07:34Z

test_vm/src/lib.rs

+        // EthAccount abstractions turns Placeholders into EthAccounts
+        if a.code == *PLACEHOLDER_ACTOR_CODE_ID {
+            a.code = *ETHACCOUNT_ACTOR_CODE_ID;
+        }


Grumble VM magic voodoo privileging the EVM

You're right -- this is an FVM detail that we don't actually need in the test VM here. It slightly restricts our ability to write tests around this, but we can manipulate the state directly / those should be unit tests anyway.

(Dropping it)

Thanks. I infer that there's similar code in the FVM, so my grumble transitively applies there. Thanks for dropping it, but if this is how the FVM behaves then this is probably going to end up back here again, until the FVM itself doesn't have ethereum-specifics inside it.

This is only really necessary for message validation. We do have similar magic on the FVM, but plan to drop it the second we get real account abstraction.

anorth

LGTM after adding the attribution that explains the introduction of LGPL.

anorth · 2023-02-10T00:40:44Z

runtime/src/runtime/mod.rs

@@ -63,12 +65,23 @@ pub trait Runtime: Primitives + Verifier + RuntimePolicy {
    /// The genesis block has epoch zero.
    fn curr_epoch(&self) -> ChainEpoch;

+    /// The ID for the EVM-based chain, as defined in https://github.com/ethereum-lists/chains.
+    fn chain_id(&self) -> ChainID;


Is the comment still accurate? Should it be something like "The ID for this chain. Note that Filecoin chain IDs are usually in the Ethereum namespace (i.e. scope "xyz") for [reasons]".

If the comment as it stands is accurate, I still think this should move into some state somewhere (EAM?) rather than be a syscall available to future native actors.

anorth · 2023-02-10T00:46:34Z

actors/evm/precompile-testdata/LICENSE

@@ -0,0 +1,165 @@
+                   GNU LESSER GENERAL PUBLIC LICENSE


Ok if we're importing it, but then we're bound to add the attribution, and then things will make sense.

Co-authored-by: Steven Allen <[email protected]> Co-authored-by: vyzo <[email protected]> Co-authored-by: Melanie Riise <[email protected]> Co-authored-by: Raúl Kripalani <[email protected]>

That way, we: 1. Don't depend on a fork of an unmaintained library. 2. Use a much simpler blake2f implementation.

arajasek force-pushed the asr/evm-is-here branch from 0b90954 to cc8ec2b Compare February 8, 2023 23:44

anorth reviewed Feb 9, 2023

View reviewed changes

anorth approved these changes Feb 10, 2023

View reviewed changes

arajasek force-pushed the asr/evm-is-here branch from be9c8f2 to bd6fe24 Compare February 10, 2023 17:26

arajasek and others added 7 commits February 10, 2023 12:27

FIP-0054: Introduce the EVM actor's logic

1a8b0a0

Co-authored-by: Steven Allen <[email protected]> Co-authored-by: vyzo <[email protected]> Co-authored-by: Melanie Riise <[email protected]> Co-authored-by: Raúl Kripalani <[email protected]>

Address review

8388ed4

Refactor imports, move EVM method types to types

3cf26fd

README: Add note about EVM precompile license

6ff1e59

EVM: use blake2f from parity's frontier (#1191)

31d4e74

That way, we: 1. Don't depend on a fork of an unmaintained library. 2. Use a much simpler blake2f implementation.

chore: document precompile test data

9d53bc2

Runtime: fix chain_id comment

30b4103

arajasek force-pushed the asr/evm-is-here branch from bd6fe24 to 30b4103 Compare February 10, 2023 17:27

arajasek merged commit 4f5449d into master Feb 10, 2023

arajasek deleted the asr/evm-is-here branch February 10, 2023 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIP-0054: Introduce the EVM actor's logic #1186

FIP-0054: Introduce the EVM actor's logic #1186

arajasek commented Feb 8, 2023

anorth Feb 9, 2023

jennijuju Feb 9, 2023

arajasek Feb 9, 2023

anorth Feb 10, 2023

Stebalien Feb 10, 2023

anorth Feb 10, 2023

Stebalien Feb 10, 2023

anorth Feb 9, 2023

arajasek Feb 9, 2023

raulk Feb 9, 2023

arajasek Feb 9, 2023

anorth Feb 10, 2023

anorth Feb 9, 2023

arajasek Feb 9, 2023

anorth Feb 9, 2023

arajasek Feb 9, 2023

arajasek Feb 9, 2023

anorth Feb 10, 2023

Stebalien Feb 10, 2023

anorth left a comment

anorth Feb 10, 2023

anorth Feb 10, 2023

FIP-0054: Introduce the EVM actor's logic #1186

FIP-0054: Introduce the EVM actor's logic #1186

Conversation

arajasek commented Feb 8, 2023

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

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment