Feature/evm #1252

branan · 2023-03-14T02:35:45Z

Description

EVM-compatible API/RPC enablement

Currently the ethereum chain ID is hardcoded to "999999", if anyone wants to try to poke it with web3.js

Todo for safe deployment publicly:

Finalize account conversion
Think about weights in more than a copy-and-paste way
Runtime call filter to disallow EVM pallet calls we don't want
EVM transaction filtering to disallow ETH calls we don't want
Fixing hardcoded values in service setup

Changes and Descriptions

TODO

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Manual validation of RPC calls:

ethereum pallet storage contains block info
evm pallet extrinsics to deploy and run a smart contract
eth Substrate RPCs can query block data
actual ethereum RPCs through web3.js (was holding off here until I had the node-side bits wired together)

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I rebased on the latest main branch

branan · 2023-03-14T02:43:28Z

runtime/development/src/lib.rs

@@ -1696,6 +1704,81 @@ impl pallet_liquidity_rewards::Config for Runtime {
 	type WeightInfo = ();
 }

+const WEIGHT_PER_GAS: u64 = 20_000;


This is copied directly from the Frontier template project - TODO is to determine if this is a good value (large enough to be safe without wasting block time)

It may be a good idea to check how Moonbeam does this

runtime/development/src/lib.rs

branan · 2023-03-14T02:45:41Z

runtime/development/src/lib.rs

+	}
+}
+
+impl pallet_base_fee::Config for Runtime {


All again copied from the template project

src/service.rs

mustermeiszer

I would move the evm setup into its seperate file, just like XCM stuff.
And are there some notes on what we add here and what is the reasoning?
Like what is the usage of those two:

node-side frontier tasks
node-side import queue

Regarding the service:

Do you think it is possible to create a seperate command, that will only then use an start node impl that includes all the EVM stuff?

Overall, I think it makes sense to let this only life in a feature branch. It seems to be breaking too much stuff on the service that I would not want to add to the production env. WDYTY?

src/service.rs

mikiquantum · 2023-03-21T20:58:37Z

runtime/development/src/precompiles.rs

+{
+	fn execute(&self, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult> {
+		// 1025 is chosen for compatibility with Moonbeam
+		if handle.code_address() == addr(1025) {


Can you reuse this, to avoid duplicating the addr(1025)?

Suggested change

if handle.code_address() == addr(1025) {

if self::is_precompile(handle.code_address().into()) {

It should be a const anyway ;)

I'd rather not rely on is_precompile in execute since we'll need to rewrite it again to use a match on addresses when we add more precompiles in the future.

Oh, my point is mostly about having to touch in two places (might oversee it) when updating that constant. But I do not know enough of how is the is_precompile used or will be used.

src/service.rs

mikiquantum · 2023-03-21T21:26:19Z

runtime/development/src/lib.rs

+	type WithdrawOrigin = EnsureAddressTruncated;
+}
+
+impl pallet_evm_chain_id::Config for Runtime {}


How is the chainId value set for an ongoing running chain? Manual raw storage set?

It comes from genesis. For adding EVM to a running chain we'll need to implement a migration (we'll also need that to define the "EVM Genesis Block" which will be based on the first block which contains EVM support. Lots of fun :) )

Got it 👍

mikiquantum · 2023-03-21T21:30:41Z

runtime/development/src/lib.rs

 /// Extrinsic type that has already been checked.
-pub type CheckedExtrinsic = generic::CheckedExtrinsic<AccountId, RuntimeCall, SignedExtra>;
+pub type CheckedExtrinsic =
+	fp_self_contained::CheckedExtrinsic<AccountId, RuntimeCall, SignedExtra, H160>;


Would still be possible to validate calls the non-evm way? I am wondering on changes needed on clients like GSRPC

fp_self_contained::CheckedExtrinsic is pretty much a transparent wrapper around generic::CheckedExtrinsic that just exposes the necessary trait impls for handling EVM calls in the runtime API. I looked into the code to see if it would break SCALE-encoded compatibility with the existing Extrinsic/Call structures, and AFAICT it's safe

Cool, and I assume the extrinsic payload to be signed is the same? Or a client would need to form the extrinsic to be signed differently?

mikiquantum

Nice! thanks @branan !

mikiquantum · 2023-03-22T17:00:19Z

runtime/development/src/precompiles.rs

+{
+	fn execute(&self, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult> {
+		// 1025 is chosen for compatibility with Moonbeam
+		if handle.code_address() == addr(1025) {


Oh, my point is mostly about having to touch in two places (might oversee it) when updating that constant. But I do not know enough of how is the is_precompile used or will be used.

mikiquantum · 2023-03-22T17:03:16Z

runtime/development/src/lib.rs

+	type WithdrawOrigin = EnsureAddressTruncated;
+}
+
+impl pallet_evm_chain_id::Config for Runtime {}


Got it 👍

mikiquantum · 2023-03-22T17:04:33Z

runtime/development/src/lib.rs

 /// Extrinsic type that has already been checked.
-pub type CheckedExtrinsic = generic::CheckedExtrinsic<AccountId, RuntimeCall, SignedExtra>;
+pub type CheckedExtrinsic =
+	fp_self_contained::CheckedExtrinsic<AccountId, RuntimeCall, SignedExtra, H160>;


Cool, and I assume the extrinsic payload to be signed is the same? Or a client would need to form the extrinsic to be signed differently?

branan · 2023-03-30T03:45:22Z

I'm working on rebasing this over the 0.9.37 work at the same time as shuffling the code around - there are major conflicts in the changes to services code, and I don't wan to do two refactors.

mustermeiszer

@branan do you have an ETA when we can merge this into main with a split node implementation?
What is left to do for that?

src/service.rs

branan · 2023-04-04T23:34:03Z

Node and runtime EVM code has been split into separate modules now, and I've done a few more small cleanups. I'll ticket the remaining things to "productionize" the code, but otherwise this should be good to land as soon as 0.9.37 is merged. Tomorrow, if all the release shenanigans go correctly?

For now this is targeted at the 0.9.37 branch so that the diff is readable/manageable. It will rebase cleanly onto main once the 0.9.37 work is in.

branan · 2023-04-04T23:34:38Z

Nix build failure is the same as 0.9.37 branch

branan · 2023-04-05T01:19:09Z

rustdoc and fmt failures also look like they should be from the base 0.9.37 PR? I haven't touched the files that failed here

mustermeiszer

The split looks good. Thanks @branan!

* Actually create a genesis block * use Parachain sync strategy

branan · 2023-04-10T22:56:02Z

@offerijns @NunoAlexandre @lemunozm This is ready for final review for merge into the dev environment. There's still some work to do for making it production-ready for other runtimes, but it's in a state where things that API consumers will interact with are stable.

lemunozm

A very fast review, but I feel like I need to stay a long time to make a useful review. I'll try to do this tomorrow.

Could you link any related spec documents about this?

runtime/development/src/lib.rs

src/chain_spec.rs

src/service/evm.rs

NunoAlexandre

Left many comments but no blockers but no blockers as nothing pops up as wrong. I am, however, a bit nervous to merge this amount of code without any added test coverage, which I would love to see in place, at the very least to test account version between the Substrate and the Evm sides 👀

NunoAlexandre · 2023-04-11T18:29:26Z

runtime/development/src/evm.rs

+
+pub mod precompile;
+
+pub struct FindAuthorTruncated<F>(PhantomData<F>);


Can we document this?

Yes, should be documented in a comment here and in the spec. Will do 👍

I suppose all of the minor open TODOs will be handled in a subsequent PR?

@wischli Yeah. Most of the TODOs are "make things configurable" or "understand this enough to decide for ourselves instead of copying the template". That's all part of what I've been calling the "productionization" work.

NunoAlexandre · 2023-04-11T18:30:30Z

runtime/development/src/evm.rs

+}
+
+#[derive(Encode, Decode, Default)]
+struct EthereumAccount(H160);


Since this module is called evm I'd suggest we rename this to just Account or Address:

Suggested change

struct EthereumAccount(H160);

struct Account(H160);

Name is leftover from before it was moved to it own module. Totally agree

NunoAlexandre · 2023-04-11T18:33:45Z

runtime/development/src/evm.rs

+
+pub struct ExtendedAddressMapping;
+
+impl AddressMapping<AccountId> for ExtendedAddressMapping {


This converts accounts of two different networks, it would be cool to refer to AccountId with a full path to make that more obvious. Like fn into_account_id(address: H160) -> cfg_types::AccountId {

It's the same network, just two different namespaces within the network - we are expanding an H160 ethereum account into an H256 substrate account. But both are still "centrifuge accounts".

The AddressMapping trait is part of the EVM pallet interface, but I will see what I can do to add comments or more descriptive parameter names to make it clearer what is going on.

NunoAlexandre · 2023-04-11T18:33:58Z

runtime/development/src/evm.rs

+	}
+
+	fn ideal() -> Permill {
+		Permill::from_parts(500_000)


Where do we get these from?

Totally copy and pasted from the frontier template. These constants are on the list of "things to understand before it lands in production". But I don't think it needs to be locked down to land it in dev

NunoAlexandre · 2023-04-11T18:34:17Z

runtime/development/src/evm.rs

+	}
+}
+
+pub struct BaseFeeThreshold;


It would be cool to document all of these actually

I honestly don't 100% understand the fee pallets, but I'll do my best

NunoAlexandre · 2023-04-11T18:38:58Z

runtime/development/src/lib.rs

@@ -1764,6 +1773,12 @@ construct_runtime!(
 		Migration: pallet_migration_manager::{Pallet, Call, Storage, Event<T>} = 199,
 		// admin stuff
 		Sudo: pallet_sudo::{Pallet, Call, Config<T>, Storage, Event<T>} = 200,
+
+		// EVM pallets
+		EVM: pallet_evm::{Pallet, Config, Call, Storage, Event<T>} = 160,


It seems that we are indexing the evm pallets with a lower number than pallets above that; is that intentional? Shouldn't we move them up to be sorted ASC?

I picked a random range that didn't overlap with anything else. I don't really know how we should organize pallet IDs in our runtime - it's only a u8, so the range is very limited.

@wischli do you have thoughts or feelings here?

I think that's fine but I would still move it up to match the order

Yes, we should move it up in this case. However, we should always be careful with these things as the order of the pallet in the construct_runtime macro decide the order of the on_initialize (in-order) and on_finalize (reverse of order) executions.

So for the crucial part, the indices are irrelevant. Moreover, indices should not be swapped and most ideally not be reused. The later part will be interesting to see in the future, given we only have u8 supported. But Kusama/Polkadot will hit this limitation much earlier 😅

I've been trying a bit to think about how the separation of "API stability" and "ABI stability" that I'm familiar with from the C world map to Substrate - things like indices I think are a big part of this. Let's chat about it some this week if we have time :)

NunoAlexandre · 2023-04-11T18:41:35Z

runtime/development/src/lib.rs

@@ -2014,6 +2112,160 @@ impl_runtime_apis! {
 		}
 	}

+	// Frontier APIs
+	impl fp_rpc::EthereumRuntimeRPCApi<Block> for Runtime {


Could we move this to evm.rs or rather not?

Yup, I missed this in the move commit

Hmm, on second look I'm not sure - these are part of an impl_runtime_apis macro invocation Can we have more than one of those?

NunoAlexandre · 2023-04-11T18:44:06Z

src/chain_spec.rs

@@ -1084,6 +1088,10 @@ fn development_genesis(
 		parachain_system: Default::default(),
 		treasury: Default::default(),
 		interest_accrual: Default::default(),
+		base_fee: Default::default(),
+		evm_chain_id: development_runtime::EVMChainIdConfig { chain_id: 999_999 },


Do we really want this value? Can't we set it to 2031 (Centrifuge chain id) for example?

For the development runtime I don't think it matters. For production on Altair and Centrifuge we'll use the 2088/2031 parachain IDs (both of those are still available as evm IDs)

I wonder if for testing it would be helpful to have it the same as Centrifuge to reduce the number of things changing between the development and the centrifuge runtimes but if you don't think it's something for us to be concerned with that's fine 👍

Generally test networks have different IDs in the Ethereum ecosystem. That would mean even Catalyst should have a unique ID from the production Centrifuge chain. It is a 64-bit ID, so there is lots of room in the namespace.

I think it's better that any code we write (even tests) be chain-id-agnostic, rather than breaking the convention and reusing production chain IDs for local networks

NunoAlexandre · 2023-04-11T18:44:40Z

src/rpc/evm.rs

+
+use std::{collections::BTreeMap, sync::Arc};
+
+// Frontier


Suggested change

// Frontier

Leftover from moving the code out of the shared RPC namespace. Will remove.

NunoAlexandre · 2023-04-11T18:44:48Z

src/rpc/evm.rs

+use fp_rpc::{ConvertTransaction, ConvertTransactionRuntimeApi, EthereumRuntimeRPCApi};
+use fp_storage::EthereumStorageSchema;
+use jsonrpsee::RpcModule;
+// Substrate


Suggested change

// Substrate

branan commented Mar 14, 2023

View reviewed changes

runtime/development/src/lib.rs Outdated Show resolved Hide resolved

branan commented Mar 14, 2023

View reviewed changes

runtime/development/src/lib.rs Outdated Show resolved Hide resolved

branan commented Mar 14, 2023

View reviewed changes

branan force-pushed the feature/evm branch from 69c4fdc to 95c2597 Compare March 19, 2023 20:36

branan marked this pull request as ready for review March 19, 2023 22:15

branan requested review from mikiquantum and mustermeiszer as code owners March 19, 2023 22:15

branan commented Mar 19, 2023

View reviewed changes

src/service.rs Outdated Show resolved Hide resolved

branan commented Mar 19, 2023

View reviewed changes

src/service.rs Outdated Show resolved Hide resolved

mustermeiszer reviewed Mar 21, 2023

View reviewed changes

src/service.rs Outdated Show resolved Hide resolved

src/service.rs Outdated Show resolved Hide resolved

src/service.rs Outdated Show resolved Hide resolved

mikiquantum reviewed Mar 21, 2023

View reviewed changes

mikiquantum reviewed Mar 22, 2023

View reviewed changes

branan force-pushed the feature/evm branch from 6c05133 to 91f093a Compare April 3, 2023 22:09

branan requested review from hieronx, lemunozm and cdamian as code owners April 3, 2023 22:09

mustermeiszer reviewed Apr 4, 2023

View reviewed changes

src/service.rs Outdated Show resolved Hide resolved

branan changed the base branch from main to polkadot-v0.9.37 April 4, 2023 23:12

branan mentioned this pull request Apr 5, 2023

Polkadot v0.9.37 #1241

Merged

mustermeiszer previously approved these changes Apr 6, 2023

View reviewed changes

Base automatically changed from polkadot-v0.9.37 to main April 7, 2023 13:41

Branan Riley added 4 commits April 10, 2023 14:27

Add Frontier pallets to development runtime

4f3c471

RPCs and runtime APIs

947f5f0

Use ethereum block hash mapping for EVM

799a782

Use a transparent & reversible account mapping

91acdcd

Branan Riley added 13 commits April 10, 2023 15:09

Cleanup lints

bd4bab9

Add ETH account to genesis

771c9ae

Add remaining node-side frontier pieces

691e640

Fix ethereum block data mapping

9ab97ac

* Actually create a genesis block * use Parachain sync strategy

Add Dispatch precompile

55b1448

Deduplicate sp-database crate

0f37e02

Update nix hash

276a7ad

Move EVM runtime components to a module

3cef471

Move EVM service setup to its own file

0e2a9b9

rpc::eth -> rpc::evm for consistency

7ad48ee

Add copyright headers

c9fcad2

Use const for dispatch precompile address

7e22b7a

Fix taplo

9822e24

branan dismissed mustermeiszer’s stale review via 9822e24 April 10, 2023 22:40

branan force-pushed the feature/evm branch from dcc862d to 9822e24 Compare April 10, 2023 22:40

lemunozm reviewed Apr 11, 2023

View reviewed changes

runtime/development/src/lib.rs Show resolved Hide resolved

src/chain_spec.rs Show resolved Hide resolved

src/service/evm.rs Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into feature/evm

a7b96f5

NunoAlexandre reviewed Apr 11, 2023

View reviewed changes

NunoAlexandre approved these changes Apr 12, 2023

View reviewed changes

hieronx merged commit a40e830 into main Apr 12, 2023
11 checks passed

wischli mentioned this pull request Apr 13, 2023

Leftovers from Feature/evm #1252 #1319

Open

branan mentioned this pull request Apr 23, 2023

Cleanup un-addressed feedback from initial EVM PR #1325

Closed

lemunozm deleted the feature/evm branch April 23, 2023 12:52

	if handle.code_address() == addr(1025) {
	if self::is_precompile(handle.code_address().into()) {


		pub mod precompile;

		pub struct FindAuthorTruncated<F>(PhantomData<F>);


		pub struct ExtendedAddressMapping;

		impl AddressMapping<AccountId> for ExtendedAddressMapping {

Feature/evm #1252

Feature/evm #1252

Conversation

branan commented Mar 14, 2023 • edited

Description

Changes and Descriptions

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

NunoAlexandre Mar 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer 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

branan Mar 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikiquantum 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

branan commented Mar 30, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

branan commented Apr 4, 2023

branan commented Apr 4, 2023

branan commented Apr 5, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

branan commented Apr 10, 2023

lemunozm left a comment • edited

Choose a reason for hiding this comment

NunoAlexandre 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

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

Choose a reason for hiding this comment

branan commented Mar 14, 2023 •

edited

NunoAlexandre Mar 15, 2023 •

edited

branan Mar 22, 2023 •

edited

lemunozm left a comment •

edited