Add requirement that Subnet-evm upgrade is enabled at genesis #419

anusha-ctrl · 2022-12-20T22:59:38Z

Why this should be merged

We should add a requirement that the SubnetEVM upgrade is enabled so that we can deprecate support for it being disabled. It should always be enabled from genesis.

How this works

We check the blockTimestamp at genesis and see if it is greater than or equal to SubnetEVMTimestamp. SubnetEVMTimestamp should be set to 0 so the SubnetEVM upgrade is enabled at genesis.
This should error if the genesisJSON did not include subnetEVMTimestamp or subnetEVMTimestamp was set to some timestamp > 0
We add a config flag so the user can skip this subnetevm upgrade check for the time being.

Here is an example of the chain config

// SubnetEVMDefaultChainConfig is the default configuration
	SubnetEVMDefaultChainConfig = &ChainConfig{
		ChainID:            SubnetEVMChainID,
		FeeConfig:          DefaultFeeConfig,
		AllowFeeRecipients: false,

		HomesteadBlock:      big.NewInt(0),
		EIP150Block:         big.NewInt(0),
		EIP150Hash:          common.HexToHash("0x2086799aeebeae135c246c65021c82b4e15a2c451340993aacfd2751886514f0"),
		EIP155Block:         big.NewInt(0),
		EIP158Block:         big.NewInt(0),
		ByzantiumBlock:      big.NewInt(0),
		ConstantinopleBlock: big.NewInt(0),
		PetersburgBlock:     big.NewInt(0),
		IstanbulBlock:       big.NewInt(0),
		MuirGlacierBlock:    big.NewInt(0),

		NetworkUpgrades: NetworkUpgrades{
			SubnetEVMTimestamp: big.NewInt(0),
		},
	}

How this was tested

2 unit tests were added

darioush · 2022-12-20T23:27:05Z

plugin/evm/vm_test.go

+}
+
+func TestSubnetEVMUpgradeNotEnabledAtGenesis(t *testing.T) {
+	ctx, dbManager, genesisBytes, issuer := setupGenesis(t, genesisJSONSubnetEVMLateEnablement)


alternatively we can parse the json for genesisJSONSubnetEVM and modify the value in the test.

plugin/evm/vm.go

anusha-ctrl · 2022-12-21T01:00:18Z

Need to fix some existing tests. With this new guard in place, it should no longer be possible to abort an Subnet-EVM upgrade as it is a requirement

anusha-ctrl · 2022-12-21T18:52:49Z

plugin/evm/vm_upgrade_bytes_test.go

 	// Get a json specifying a Network upgrade at genesis
 	// to apply as upgradeBytes.
-	subnetEVMTimestamp := time.Unix(10, 0)
+	subnetEVMTimestamp := big.NewInt(0)


subnetEVMTimestamp must be 0 to be activated at genesis

Can we require that this is set directly in the genesis rather than in the upgrade bytes to make this simpler?

anusha-ctrl · 2022-12-21T18:54:52Z

plugin/evm/vm_upgrade_bytes_test.go


-	// abort a fork specified in genesis
-	upgradeConfig.NetworkUpgrades.SubnetEVMTimestamp = nil
+	upgradeConfig.NetworkUpgrades = nil


no longer testing abort, but now testing the fallback genesis bytes

aaronbuchwald

LGTM

ceyonur

code generally lgtm. I just wonder if we should add a flag to avago-chain config to skip this check? We can emphasize this is just for testing purposes and should not be used in producion. it can also still be used for chains that are bot upgraded to subnet-evm.

ceyonur · 2023-01-11T14:25:36Z

plugin/evm/vm.go

+	if !vm.chainConfig.IsSubnetEVM(common.Big0) {
+		return errSubnetEVMUpgradeNotEnabled
+	}
+


Shall we move this below to upgradeBytes parsing, in case this was activated by a upgrade at genesis? Could this be a possibility? @aaronbuchwald @darioush

They could include it in the upgrade bytes to activate at timestamp 0, but it SHOULD always be included in the genesis, so I think it's preferable to error here rather than after applying the upgradeBytes. I actually asked @anusha-ctrl to make this change to put it here.

just thinking someone might want to fix their genesis via this method (or by removing the checks via config flag).

aaronbuchwald · 2023-01-11T15:19:16Z

code generally lgtm. I just wonder if we should add a flag to avago-chain config to skip this check? We can emphasize this is just for testing purposes and should not be used in producion. it can also still be used for chains that are bot upgraded to subnet-evm.

Good idea, this way we do not need to create a new release if anyone runs into this unexpectedly. I confirmed in https://github.com/ava-labs/public-chain-assets/ that none of the networks here should be impacted.

@anusha-ctrl could you add a config flag that will allow a user to override this check in case anyone runs into it along with a TODO to remove once we confirm that there are no complaints after making this change?

…/subnet-evm into add-upgrade-requirement

ceyonur

Nice 👍

plugin/evm/vm_upgrade_bytes_test.go

plugin/evm/vm_test.go

aaronbuchwald

LGTM

anusha-ctrl added 6 commits December 20, 2022 14:57

Add requirement that Subnet-evm upgrade is enabled at genesis

44fb324

Add test for nil

756a872

remove err

b7afb02

modify comment

5b59ecb

add one more comment

c9addb4

test improvements

226cdd9

anusha-ctrl added this to the [email protected] milestone Dec 20, 2022

anusha-ctrl linked an issue Dec 20, 2022 that may be closed by this pull request

Add Requirement that SubnetEVM Upgrade is Enabled #264

Closed

anusha-ctrl marked this pull request as ready for review December 20, 2022 23:22

anusha-ctrl requested review from ceyonur, darioush, patrick-ogrady and aaronbuchwald as code owners December 20, 2022 23:22

fix comment

551f896

darioush approved these changes Dec 20, 2022

View reviewed changes

darioush reviewed Dec 20, 2022

View reviewed changes

plugin/evm/vm.go Outdated Show resolved Hide resolved

fix comment

23356a3

anusha-ctrl requested a review from darioush December 20, 2022 23:33

darioush reviewed Dec 20, 2022

View reviewed changes

plugin/evm/vm.go Outdated Show resolved Hide resolved

timestamp

3fd850e

anusha-ctrl requested a review from darioush December 20, 2022 23:45

fix existing test:

4289b66

anusha-ctrl marked this pull request as draft December 21, 2022 00:59

anusha-ctrl added 2 commits December 20, 2022 17:03

fix one more existing test

6c547ce

fix spacing

d28645e

anusha-ctrl commented Dec 21, 2022

View reviewed changes

clean up

9c399ce

anusha-ctrl requested review from aaronbuchwald and ceyonur January 11, 2023 00:59

Merge branch 'master' into add-upgrade-requirement

efcfd0c

aaronbuchwald assigned anusha-ctrl Jan 11, 2023

aaronbuchwald approved these changes Jan 11, 2023

View reviewed changes

ceyonur reviewed Jan 11, 2023

View reviewed changes

anusha-ctrl added 2 commits January 11, 2023 10:07

add config flag

06406b2

Merge branch 'add-upgrade-requirement' of https://github.com/ava-labs…

067db7a

…/subnet-evm into add-upgrade-requirement

ceyonur approved these changes Jan 11, 2023

View reviewed changes

darioush reviewed Jan 11, 2023

View reviewed changes

plugin/evm/vm_upgrade_bytes_test.go Outdated Show resolved Hide resolved

anusha-ctrl added 4 commits January 11, 2023 10:30

add unit test to test override

ab79415

lint

9ef4d85

remove skips

473c66d

lint

b9c3af5

anusha-ctrl requested review from darioush, aaronbuchwald and ceyonur January 11, 2023 19:27

Merge branch 'master' into add-upgrade-requirement

7a31c12

darioush reviewed Jan 11, 2023

View reviewed changes

plugin/evm/vm_test.go Outdated Show resolved Hide resolved

anusha-ctrl added 2 commits January 11, 2023 16:10

unify to one test

e650678

fix typo

09361b1

anusha-ctrl requested a review from darioush January 12, 2023 00:15

darioush approved these changes Jan 12, 2023

View reviewed changes

aaronbuchwald modified the milestones: [email protected], [email protected] Jan 13, 2023

aaronbuchwald approved these changes Jan 13, 2023

View reviewed changes

aaronbuchwald merged commit 880ec77 into master Jan 13, 2023

aaronbuchwald deleted the add-upgrade-requirement branch January 13, 2023 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add requirement that Subnet-evm upgrade is enabled at genesis #419

Add requirement that Subnet-evm upgrade is enabled at genesis #419

Uh oh!

anusha-ctrl commented Dec 20, 2022 •

edited

Loading

Uh oh!

darioush Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

anusha-ctrl commented Dec 21, 2022 •

edited

Loading

Uh oh!

anusha-ctrl Dec 21, 2022

Uh oh!

aaronbuchwald Dec 22, 2022

Uh oh!

anusha-ctrl Dec 21, 2022

Uh oh!

aaronbuchwald left a comment

Uh oh!

ceyonur left a comment

Uh oh!

ceyonur Jan 11, 2023

Uh oh!

aaronbuchwald Jan 11, 2023

Uh oh!

ceyonur Jan 11, 2023

Uh oh!

aaronbuchwald commented Jan 11, 2023

Uh oh!

ceyonur left a comment

Uh oh!

Uh oh!

Uh oh!

aaronbuchwald left a comment

Uh oh!

Uh oh!

Add requirement that Subnet-evm upgrade is enabled at genesis #419

Add requirement that Subnet-evm upgrade is enabled at genesis #419

Uh oh!

Conversation

anusha-ctrl commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Uh oh!

darioush Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anusha-ctrl commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anusha-ctrl Dec 21, 2022

Choose a reason for hiding this comment

Uh oh!

aaronbuchwald Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

anusha-ctrl Dec 21, 2022

Choose a reason for hiding this comment

Uh oh!

aaronbuchwald left a comment

Choose a reason for hiding this comment

Uh oh!

ceyonur left a comment

Choose a reason for hiding this comment

Uh oh!

ceyonur Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

aaronbuchwald Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

ceyonur Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

aaronbuchwald commented Jan 11, 2023

Uh oh!

ceyonur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aaronbuchwald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anusha-ctrl commented Dec 20, 2022 •

edited

Loading

anusha-ctrl commented Dec 21, 2022 •

edited

Loading