Add contract creation allow lists to EVM domains #3350

teor2345 · 2025-01-15T03:07:28Z

This PR adds a pallet which filters ethereum contract creation using an allow list. The allow list can be updated by the domain sudo account.

By default, all accounts can create contracts, to maintain compatibility with existing EVM domains. But the chainspec can be configured with an initial allow list of accounts. For our "private" EVM, that will be the domain sudo.

Close #3344.

I couldn't work out how to do a dynamic check for the domain sudo inside the pallet or runtime. I'm not sure if that's needed, because we can add the sudo account in the chainspec, or they can add themselves to the list using the (sudo) call provided by the pallet.

Part of #3353.

Code contributor checklist:

I have read, understood and followed contributing guide

vedhavyas

Make sense overall. Left questions on penalizing malicious operator including these txns in bundle

domains/runtime/evm/src/lib.rs

domains/pallets/evm_nonce_tracker/src/lib.rs

vedhavyas · 2025-01-17T08:16:03Z

domains/runtime/evm/src/lib.rs

+pub struct CheckContractCreation;
+
+impl SignedExtension for CheckContractCreation {
+    const IDENTIFIER: &'static str = "CheckContractCreation";


Can we move this Extension to the pallet itself?
Also ideal if we change the name of pallet to something more alighed with new changes
maybe just pallet-evm-tracker ?

Can we move this Extension to the pallet itself?

This also makes sense to me, as we can use this extension in the test evm runtime too, and we can write integration test for it.

Also ideal if we change the name of pallet to something more alighed with new changes
maybe just pallet-evm-tracker ?

It is okay to change the crate name but notice we can't change the pallet name in the runtime, because the pallet name is used to construct the storage key changing it make break compatibility with the existing evm domain on Taurus

Can we move this Extension to the pallet itself?

Not easily. The extension matches on the Runtime::Call enum, which is composed via a macro. Similar code isn't located in pallets, it's located in crates/subspace-runtime/src/signed_extensions.rs and crates/subspace-runtime/src/object_mapping.rs.

We could move the extension to the pallet, but we'd need to keep the code that identifies a contract creation transaction next to the code that constructs the domain runtime. And add a new ContainsContractCreation trait so the extension can call that runtime-specific code.

Also ideal if we change the name of pallet to something more alighed with new changes
maybe just pallet-evm-tracker ?

Sure, I'll do this last.
Edit: done in PR.

Not easily. The extension matches on the Runtime::Call enum, which is composed via a macro. Similar code isn't located in pallets, it's located in crates/subspace-runtime/src/signed_extensions.rs and crates/subspace-runtime/src/object_mapping.rs.

We could move the extension to the pallet, but we'd need to keep the code that identifies a contract creation transaction next to the code that constructs the domain runtime. And add a new ContainsContractCreation trait so the extension can call that runtime-specific code.

Ok, I worked out how to do this in a way that's easier to maintain. At the moment, we use complicated matching or trait impls, which need to be kept in sync between the production and test runtime code.

But if we use this design pattern, the copied impls are small and straightforward:

in the runtime, have a trait that converts into the pallet call type, for example MaybeIntoUtilityCall

in the pallet, call that trait on the RuntimeCall type to get the pallet Call type

the pallet can then depend directly on the other pallets whose structures it needs to use

This will let me move these things into pallet-evm-tracker:

is_create_contract_allowed()

is_create_unsigned_contract_allowed()

is_create_contract()

impl SignedExtension for CheckContractCreation

impl fp_self_contained::SelfContainedCall for RuntimeCall implements checks from multiple pallets, so I'm not sure where to move it. Somewhere that depends on pallet-evm-tracker, pallet-block-fees, and domain-check-weight, I guess.

domains/runtime/evm/src/lib.rs

crates/pallet-domains/src/domain_registry.rs

vedhavyas · 2025-01-17T08:29:41Z

crates/pallet-domains/src/tests.rs

@@ -633,6 +634,7 @@ fn test_bundle_format_verification() {
            bundle_slot_probability: (1, 1),
            operator_allow_list: OperatorAllowList::Anyone,
            initial_balances: Default::default(),
+            initial_evm_contract_creation_allow_list: None,
        };


maybe add another test with some address included ?

I'd like to do this after the API is stable, so I don't have to rework the tests multiple times.

Test matrix:

Instantiate domain with allow list, using all allow list combinations below

Instantiate domain without allow list (existing tests)

Update allow list using call, with all allow list combinations below

Migration v2 to v3 storage format (Taurus only)

Test contract allow/deny for all combinations of:

Anyone vs Empty vs 1 Account vs Multiple Accounts (First vs not First)

Signer vs Not Signer

Contract vs Non-Contract vs Deeply nested (contract vs non-contract)

Ethereum (self-contained) vs EVM (signed extension) calls

legacy vs vs eip1559 vs eip2930

domains/runtime/evm/src/lib.rs

domains/pallets/evm_nonce_tracker/src/lib.rs

domains/runtime/evm/src/lib.rs

crates/pallet-domains/src/runtime_registry.rs

crates/pallet-domains/src/domain_registry.rs

NingLin-P · 2025-01-17T14:16:20Z

domains/runtime/evm/src/lib.rs

+pub struct CheckContractCreation;
+
+impl SignedExtension for CheckContractCreation {
+    const IDENTIFIER: &'static str = "CheckContractCreation";


Can we move this Extension to the pallet itself?

This also makes sense to me, as we can use this extension in the test evm runtime too, and we can write integration test for it.

Also ideal if we change the name of pallet to something more alighed with new changes
maybe just pallet-evm-tracker ?

It is okay to change the crate name but notice we can't change the pallet name in the runtime, because the pallet name is used to construct the storage key changing it make break compatibility with the existing evm domain on Taurus

vedhavyas · 2025-01-20T08:12:24Z

crates/pallet-domains/src/lib.rs

@@ -161,7 +162,7 @@ pub type BlockTreeNodeFor<T> = crate::block_tree::BlockTreeNode<
 >;

 /// The current storage version.
-const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);


Is this due to Taurus being on storage version 2 already ?

Yes, in this branch: https://github.com/autonomys/subspace/tree/taurus-runtime-upgrade

Here's the exact change:
https://github.com/autonomys/subspace/pull/3342/files#diff-9f80450addc12cd3b681275c6acb663003edc27c2d8eeeb1f26147df9f8e5bbeR164

subspace/crates/pallet-domains/src/lib.rs

Line 164 in a19ba3d

const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);

vedhavyas · 2025-01-20T08:14:43Z

crates/subspace-runtime/src/lib.rs

@@ -136,7 +136,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
    spec_name: Cow::Borrowed("subspace"),
    impl_name: Cow::Borrowed("subspace"),
    authoring_version: 0,
-    spec_version: 2,


lets not bump this yet.

I do a see a new problem with maintaing a new branch for taurus @dariolina
Apart from the spec_versin, there is also sometime storge versions for each pallet that involves migrations. This will problematic when we have a seperate branch for Taurus.

I still think having a single source of truth with same spec_version would be most non-confusing way to go

In the core protocol sync, we decided to:

keep the versions the same on Taurus and mainnet, skipping mainnet version numbers if needed

if a migration is Taurus-specific, only apply it to that branch

That means we'll need 2-3 commits for this specific PR to sync the versions:

Taurus version bump (different because Taurus was already bumped by the last migration, without bumping mainnet)

Mainnet version bump

Taurus migration

And then going forward we'll need 2 commits:

Version bump for Taurus and Mainnet

Migration for Taurus and maybe Mainnet

teor2345

I've finished about half the tests, see the checklist in the comments

teor2345 · 2025-01-20T08:23:07Z

crates/pallet-domains/src/lib.rs

@@ -161,7 +162,7 @@ pub type BlockTreeNodeFor<T> = crate::block_tree::BlockTreeNode<
 >;

 /// The current storage version.
-const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);


Yes, in this branch: https://github.com/autonomys/subspace/tree/taurus-runtime-upgrade

Here's the exact change:
https://github.com/autonomys/subspace/pull/3342/files#diff-9f80450addc12cd3b681275c6acb663003edc27c2d8eeeb1f26147df9f8e5bbeR164

subspace/crates/pallet-domains/src/lib.rs

Line 164 in a19ba3d

const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);

teor2345 · 2025-01-20T09:20:44Z

crates/subspace-runtime/src/lib.rs

@@ -136,7 +136,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
    spec_name: Cow::Borrowed("subspace"),
    impl_name: Cow::Borrowed("subspace"),
    authoring_version: 0,
-    spec_version: 2,


In the core protocol sync, we decided to:

keep the versions the same on Taurus and mainnet, skipping mainnet version numbers if needed

if a migration is Taurus-specific, only apply it to that branch

That means we'll need 2-3 commits for this specific PR to sync the versions:

Taurus version bump (different because Taurus was already bumped by the last migration, without bumping mainnet)

Mainnet version bump

Taurus migration

And then going forward we'll need 2 commits:

Version bump for Taurus and Mainnet

Migration for Taurus and maybe Mainnet

teor2345 · 2025-01-21T00:53:10Z

crates/pallet-domains/src/tests.rs

@@ -633,6 +634,7 @@ fn test_bundle_format_verification() {
            bundle_slot_probability: (1, 1),
            operator_allow_list: OperatorAllowList::Anyone,
            initial_balances: Default::default(),
+            initial_evm_contract_creation_allow_list: None,
        };


Test matrix:

Instantiate domain with allow list, using all allow list combinations below

Instantiate domain without allow list (existing tests)

Update allow list using call, with all allow list combinations below

Migration v2 to v3 storage format (Taurus only)

Test contract allow/deny for all combinations of:

Anyone vs Empty vs 1 Account vs Multiple Accounts (First vs not First)

Signer vs Not Signer

Contract vs Non-Contract vs Deeply nested (contract vs non-contract)

Ethereum (self-contained) vs EVM (signed extension) calls

legacy vs vs eip1559 vs eip2930

teor2345 added enhancement New feature or request execution Subspace execution labels Jan 15, 2025

teor2345 self-assigned this Jan 15, 2025

teor2345 requested review from NingLin-P, vedhavyas and nazar-pc as code owners January 15, 2025 03:07

This comment was marked as resolved.

Sign in to view

teor2345 force-pushed the private-evm branch from 377d0da to 6a47075 Compare January 17, 2025 02:28

This comment was marked as resolved.

Sign in to view

teor2345 requested a review from NingLin-P January 17, 2025 02:32

teor2345 force-pushed the private-evm branch from 6a47075 to eabc2ac Compare January 17, 2025 05:17

This comment was marked as resolved.

Sign in to view

teor2345 mentioned this pull request Jan 17, 2025

Unify copies of GenesisDomainParams across different crates #3352

Open

This comment was marked as resolved.

Sign in to view

vedhavyas reviewed Jan 17, 2025

View reviewed changes

NingLin-P reviewed Jan 17, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

teor2345 force-pushed the private-evm branch from eabc2ac to 7497b5c Compare January 20, 2025 07:18

vedhavyas reviewed Jan 20, 2025

View reviewed changes

teor2345 added 7 commits January 21, 2025 12:51

Fix comments and formatting

929dc7f

Use consistent syntax for Call::transact

4065746

Add a contract creation filter to pallet-evm-nonce-tracker

fc682dd

Add a contract creation filter to the EVM domain

a34ff8c

Move some ethereum types to domain primitives

3223b4a

Allow contract creation to be set per domain instance

a1a44ec

Add contract creation to domain params

b3446c6

teor2345 added 6 commits January 21, 2025 12:51

Limit recursion when checking for balance transfers or contract creation

2546a80

Fix balance overflow and invalid messenger channel error code clash

8dd2b33

Reject unsigned contract calls unless anyone is allowed

c85d2b2

Rename pallet_evm_nonce_tracker to pallet-evm-tracker

8fb7c77

Mainnet: Bump storage version, and sync spec version with Taurus

4c91c21

Taurus: Add storage migration for new domain registry fields

6f92663

teor2345 force-pushed the private-evm branch from 7497b5c to b29c6f3 Compare January 21, 2025 02:51

teor2345 commented Jan 21, 2025

View reviewed changes

Add tests for contract allow lists

4a7f2bf

teor2345 force-pushed the private-evm branch from b29c6f3 to 4a7f2bf Compare January 21, 2025 03:03

This comment was marked as resolved.

Sign in to view

teor2345 added 3 commits January 22, 2025 16:17

Move test_ethereum module for reuse in contract tests

56d0e56

Add function to generate pallet-evm transaction calls

94c9f9a

Add a pallet-utility Call iterator

c5c1880

teor2345 force-pushed the private-evm branch from a94e6b3 to c5c1880 Compare January 23, 2025 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contract creation allow lists to EVM domains #3350

Add contract creation allow lists to EVM domains #3350

teor2345 commented Jan 15, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

vedhavyas left a comment

vedhavyas Jan 17, 2025

NingLin-P Jan 17, 2025

teor2345 Jan 20, 2025 •

edited

Loading

teor2345 Jan 23, 2025

vedhavyas Jan 17, 2025

teor2345 Jan 20, 2025

teor2345 Jan 21, 2025 •

edited

Loading

NingLin-P Jan 17, 2025

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

vedhavyas Jan 20, 2025

teor2345 Jan 20, 2025

vedhavyas Jan 20, 2025

teor2345 Jan 20, 2025

teor2345 left a comment

teor2345 Jan 20, 2025

teor2345 Jan 20, 2025

teor2345 Jan 21, 2025 •

edited

Loading

This comment was marked as resolved.

Add contract creation allow lists to EVM domains #3350

Are you sure you want to change the base?

Add contract creation allow lists to EVM domains #3350

Conversation

teor2345 commented Jan 15, 2025 • edited Loading

Code contributor checklist:

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

vedhavyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 Jan 20, 2025 • 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

teor2345 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

teor2345 commented Jan 15, 2025 •

edited

Loading

teor2345 Jan 20, 2025 •

edited

Loading

teor2345 Jan 21, 2025 •

edited

Loading

teor2345 Jan 21, 2025 •

edited

Loading