Evm part 2 #1349

branan · 2023-05-17T01:20:15Z

Description

This brings the EVM support to Altair and Centrifuge runtimes, with additional restrictions compared to the Development runtime. I particular:

The production runtimes filter all non-root calls to the EVM pallet
The production runtimes disallow creating contracts via the EVM RPC calls

Changes and Descriptions

Add EVM support to Altair runtime
Add EVM support to Centrifuge runtime
Add EVM support to altair/centrifuge node services
Various minor EVM cleanups

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
I have added tests that prove my fix is effective or that my feature works

This adds EVM support to the Altair and Centrifuge runtimes, with the following caveats: * All EVM pallet functionality is filtered on the substrate side * The EVM `create` RPC is filtered in the runtimes This allows EVM RPC calls to be made to invoke precompiles or smart contracts deployed by Governance (root).

branan · 2023-05-17T01:41:31Z

There are no benchmarks in this version of Frontier 🎉. It'll be something to add in 0.9.40

thea-leake

Not sure if it's still supposed to be marked as draft as per PR comment, but looks good to me :)

runtime/altair/src/lib.rs

runtime/development/src/evm.rs

NunoAlexandre

One thing that popped up for me on this first round. Will give it a more detailed looked later 👍

NunoAlexandre

Nothing major :)

runtime/altair/src/evm.rs

NunoAlexandre · 2023-05-22T15:27:30Z

runtime/altair/src/evm.rs

+pub struct FindAuthorTruncated<F>(PhantomData<F>);
+impl<F: FindAuthor<u32>> FindAuthor<H160> for FindAuthorTruncated<F> {
+	fn find_author<'a, I>(digests: I) -> Option<H160>
+	where
+		I: 'a + IntoIterator<Item = (ConsensusEngineId, &'a [u8])>,
+	{
+		if let Some(author_index) = F::find_author(digests) {
+			let authority_id = Aura::authorities()[author_index as usize].clone();
+			return Some(H160::from_slice(&authority_id.to_raw_vec()[4..24]));
+		}
+		None
+	}
+}


This kind of runtime-agnostic trait impl could to to runtime-commons and just be referenced in the different runtimes instead of duplicated in all of them 🧹

I think it needs to be made generic over the Runtime (that Aura in there is really pallet_aura::Pallet::<Runtime>), but indeed could be made properly runtime agnostic. Worth doing for sure

Ah, my bad, I didn't spot the Aura bit 🙃 I don't know how to make stuff generic over a runtime when referencing a specific pallet from that runtime tho.

We could add a Get<Vec<Authorities>> and impl that for Aura in the actual runtime. But feel like this makes it more opaque than actually easier to understand.

The `create` runtime API is used for the `call` and `estimate_gas` RPCs, but not for transactions stored in a block. Transactions in a block are encoded as wrapped extrinsics. This wrapping is done by the `fp_rpc::ConvertTransaction` runtime API. I don't believe filtering at this conversion point is safe, since a malicious node could choose to create a transaction differently and still add it to the pool. Instead, we do the filtering on conversion from an "unchecked" to a "checked" extrinsic. For both Substrate and Ethereum transactions, this conversion typically involves just validating that transaction fees can be paid, that signatures are valid, etc. We add additional logic to catch EVM create invocations and indicate that such transactions are invalid.

Co-authored-by: Nuno Alexandre <[email protected]>

mustermeiszer

Nice and clean!! Thanks a lot @branan!

mustermeiszer · 2023-05-23T15:24:59Z

src/service/evm.rs

+	#[clap(long)]
+	pub enable_dev_signer: bool,


maybe default to false

WDYT?

bool in clap defaults to false implicitly

This brings EVM account conversion and connectors DomainAccount conversion into one single location. Moving forward, this can be expanded to support account conversions across multiple sources using xcm v3 MultiLocations. But as we are still on XCM v2 we don't have the necessary XCM primitives yet.

branan · 2023-05-24T05:23:58Z

runtime/common/src/account_conversion.rs

+impl<R> Convert<DomainAddress, AccountId> for AccountConverter<R> {
+	fn convert(domain_address: DomainAddress) -> AccountId {
+		match domain_address {
+			DomainAddress::Centrifuge(addr) => AccountId::new(addr),


@NunoAlexandre this behavior is different from what DomainAddress::into_address_truncating did. This is a direct unwrapping, whereas into_address_truncating would have done something like [<tag_bytes>,<truncated_address>]. I didn't see anything in the comments or commit messages that made that behavior seem intentional, and there were no tests that covered it.

We can change this so that DomainAddress::Centrifuge encodes into something different if we so desire

Ah, good point. For all I know, this is exactly what we want since the idea of DomainAddress::Centrifuge(address) is exactly to represent a direct account address on the Centrifuge chain.

branan · 2023-05-24T05:32:41Z

Regarding account conversion:

The MultiLocation based idea won't work until we're on XCM V3. We're still on polkadot v0.9.37 and v2, which is missing several pieces we need to represent addresses correctly. Since for now we only need EVM account conversion, I've gone ahead and implemented without going through MultiLocation.
I've also skipped the Runtime API for now - the EVM account conversion is very simple to implement in JS, and it makes more sense to introduce the runtime APIs in terms of MultiLocations when we have the updated XCM version.

We do still now have a single, central location to expand account conversion as we need it, which is the important part.

NunoAlexandre · 2023-05-24T07:01:09Z

Regarding account conversion:

The MultiLocation based idea won't work until we're on XCM V3. We're still on polkadot v0.9.37 and v2, which is missing several pieces we need to represent addresses correctly. Since for now we only need EVM account conversion, I've gone ahead and implemented without going through MultiLocation.

The update to 0.9.38 is pretty much finalised apart from a few failing tests so you can always choose to rebase this branch :) Out of curiosity, what's missing in 0.9.37/xcm v2 that would help us represent addresses correctly?

branan · 2023-05-24T13:59:04Z

Out of curiosity, what's missing in 0.9.37/xcm v2 that would help us represent addresses correctly?

Junction::GlobalConsensus and NetworkId::Ethereum are necessary to represent an EVM chain (or the EVM component of our chain). They're freshly-added in XCMv3

NunoAlexandre · 2023-05-24T14:00:03Z

Out of curiosity, what's missing in 0.9.37/xcm v2 that would help us represent addresses correctly?

Junction::GlobalConsensus and NetworkId::Ethereum are necessary to represent an EVM chain (or the EVM component of our chain). They're freshly-added in XCMv3

Makes sense 👍

branan · 2023-05-24T16:32:22Z

I've flipped the auto-merge switch, this is ready for final review 🎉

branan · 2023-05-24T18:31:09Z

I think that final thumbs from @NunoAlexandre and @mustermeiszer will be enough to satisfy the codeowners checks (though any other final review is appreciated)

mustermeiszer

Re-approve! Thanks for pushing this

Branan Riley added 7 commits May 16, 2023 14:19

Cleanup un-addressed feedback from initial EVM PR

05e8ba7

Split some EVM parts out to common

23c25d3

Add EVM node support to altair & centrifuge

93598c2

Remove non-evm service fns

95c6c71

Remove unused dynamic_fee data provider

084861b

fix lints

77d2475

branan requested review from mikiquantum, mustermeiszer, NunoAlexandre and hieronx as code owners May 17, 2023 01:20

This comment was marked as outdated.

Sign in to view

Subalfred

092f96c

thea-leake reviewed May 17, 2023

View reviewed changes

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

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

NunoAlexandre reviewed May 22, 2023

View reviewed changes

Branan Riley and others added 7 commits May 22, 2023 14:53

Update runtime/altair/src/evm.rs

fbee4e7

Co-authored-by: Nuno Alexandre <[email protected]>

Add EVM configuration to CLI

c95f77a

Move WEIGHT_PER_GAS constant to common

8438027

Merge remote-tracking branch 'origin/main' into evm-part-2

c4bbe7a

Merge remote-tracking branch 'origin/evm-part-2' into evm-part-2

6296d3a

Add EVM Chain ID migration to Centrifuge and Altair

82cc485

mustermeiszer previously approved these changes May 23, 2023

View reviewed changes

Allow clippy lint in code from Frontier template

a2ea6bb

branan dismissed mustermeiszer’s stale review via a2ea6bb May 23, 2023 18:52

branan commented May 24, 2023

View reviewed changes

Add a few more comments

2dfcb78

Add tests for account conversion

e00fdc7

branan enabled auto-merge (squash) May 24, 2023 16:32

Update endowed EVM accounts to account for new conversion

9b861ea

branan force-pushed the evm-part-2 branch from 7a2e6b6 to 9b861ea Compare May 24, 2023 16:35

taplo fmt

3e3146f

mustermeiszer approved these changes May 24, 2023

View reviewed changes

branan merged commit b829c36 into main May 24, 2023

NunoAlexandre deleted the evm-part-2 branch May 25, 2023 06:55

NunoAlexandre mentioned this pull request May 25, 2023

Polkadot v0.9.38 #1332

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evm part 2 #1349

Evm part 2 #1349

branan commented May 17, 2023 •

edited

Loading

This comment was marked as outdated.

branan commented May 17, 2023

thea-leake left a comment

NunoAlexandre left a comment

NunoAlexandre left a comment

NunoAlexandre May 22, 2023

branan May 22, 2023

NunoAlexandre May 23, 2023

mustermeiszer May 23, 2023

mustermeiszer left a comment

mustermeiszer May 23, 2023

branan May 24, 2023

branan May 24, 2023

NunoAlexandre May 24, 2023

branan commented May 24, 2023

NunoAlexandre commented May 24, 2023

branan commented May 24, 2023

NunoAlexandre commented May 24, 2023

branan commented May 24, 2023

branan commented May 24, 2023

mustermeiszer left a comment

Evm part 2 #1349

Evm part 2 #1349

Conversation

branan commented May 17, 2023 • edited Loading

Description

Changes and Descriptions

Checklist:

This comment was marked as outdated.

branan commented May 17, 2023

thea-leake left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

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

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

branan commented May 24, 2023

NunoAlexandre commented May 24, 2023

branan commented May 24, 2023

NunoAlexandre commented May 24, 2023

branan commented May 24, 2023

branan commented May 24, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

branan commented May 17, 2023 •

edited

Loading