Differentiate mandatory/optional Subnet-EVM Network Upgrades (#640)

* remove network upgrades from upgrade configs and enforce them in vm init

* fix tests

* remove unused var

* remove extra shutdown

* copy test chain config when modify

* use SimulatedTestChainConfig for simulated tests

* copy modified test chain config

* add optional upgrades

* Add tests

* fix comments

* use unmarshal in test

* unexport functions

* add commented out test

* add TODO comment

accounts/abi/bind/backends/simulated.go

@@ -107,9 +107,7 @@ type SimulatedBackend struct {
 // and uses a simulated blockchain for testing purposes.
 // A simulated backend always uses chainID 1337.
 func NewSimulatedBackendWithDatabase(database ethdb.Database, alloc core.GenesisAlloc, gasLimit uint64) *SimulatedBackend {
-	cpcfg := params.TestChainConfig
-	cpcfg.ChainID = big.NewInt(1337)
-	genesis := core.Genesis{Config: cpcfg, GasLimit: gasLimit, Alloc: alloc}
+	genesis := core.Genesis{Config: params.SimulatedTestChainConfig, GasLimit: gasLimit, Alloc: alloc}
 	genesis.MustCommit(database)
 	cacheConfig := &core.CacheConfig{}
 	blockchain, _ := core.NewBlockChain(database, cacheConfig, genesis.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
@@ -599,7 +597,6 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call interfaces.Call
 	for lo+1 < hi {
 		mid := (hi + lo) / 2
 		failed, _, err := executable(mid)
-
 		// If the error is not nil(consensus error), it means the provided message
 		// call or transaction will never be accepted no matter how much gas it is
 		// assigned. Return the error directly, don't struggle any more

accounts/abi/bind/backends/simulated_test.go

@@ -136,11 +136,11 @@ func TestNewSimulatedBackend(t *testing.T) {
 	sim := simTestBackend(testAddr)
 	defer sim.Close()
 
-	if sim.config != params.TestChainConfig {
+	if sim.config != params.SimulatedTestChainConfig {
 		t.Errorf("expected sim config to equal params.AllEthashProtocolChanges, got %v", sim.config)
 	}
 
-	if sim.blockchain.Config() != params.TestChainConfig {
+	if sim.blockchain.Config() != params.SimulatedTestChainConfig {
 		t.Errorf("expected sim blockchain config to equal params.AllEthashProtocolChanges, got %v", sim.config)
 	}
 

core/genesis_test.go

@@ -238,13 +238,13 @@ func TestStatefulPrecompilesConfigure(t *testing.T) {
 // regression test for precompile activation after header block
 func TestPrecompileActivationAfterHeaderBlock(t *testing.T) {
 	db := rawdb.NewMemoryDatabase()
-
+	copyCfg := *params.TestChainConfig
 	customg := Genesis{
-		Config: params.SubnetEVMDefaultChainConfig,
+		Config: &copyCfg,
 		Alloc: GenesisAlloc{
 			{1}: {Balance: big.NewInt(1), Storage: map[common.Hash]common.Hash{{1}: {1}}},
 		},
-		GasLimit: params.SubnetEVMDefaultChainConfig.FeeConfig.GasLimit.Uint64(),
+		GasLimit: params.TestChainConfig.FeeConfig.GasLimit.Uint64(),
 	}
 	genesis := customg.MustCommit(db)
 	bc, _ := NewBlockChain(db, DefaultCacheConfig, customg.Config, dummy.NewFullFaker(), vm.Config{}, common.Hash{})

core/state_processor_test.go

@@ -44,7 +44,8 @@ import (
 )
 
 var (
-	config     = params.TestChainConfig
+	cpcfg      = *params.TestChainConfig
+	config     = &cpcfg
 	signer     = types.LatestSigner(config)
 	testKey, _ = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
 )
@@ -312,7 +313,7 @@ func TestBadTxAllowListBlock(t *testing.T) {
 			PetersburgBlock:     big.NewInt(0),
 			IstanbulBlock:       big.NewInt(0),
 			MuirGlacierBlock:    big.NewInt(0),
-			NetworkUpgrades: params.NetworkUpgrades{
+			MandatoryNetworkUpgrades: params.MandatoryNetworkUpgrades{
 				SubnetEVMTimestamp: big.NewInt(0),
 			},
 			GenesisPrecompiles: params.Precompiles{

core/vm/runtime/runtime.go

@@ -74,7 +74,7 @@ func setDefaults(cfg *Config) {
 			PetersburgBlock:     new(big.Int),
 			IstanbulBlock:       new(big.Int),
 			MuirGlacierBlock:    new(big.Int),
-			NetworkUpgrades: params.NetworkUpgrades{
+			MandatoryNetworkUpgrades: params.MandatoryNetworkUpgrades{
 				SubnetEVMTimestamp: new(big.Int),
 			},
 		}

internal/ethapi/transaction_args_test.go

@@ -225,7 +225,7 @@ func newBackendMock() *backendMock {
 		PetersburgBlock:     big.NewInt(0),
 		IstanbulBlock:       big.NewInt(0),
 		MuirGlacierBlock:    big.NewInt(0),
-		NetworkUpgrades: params.NetworkUpgrades{
+		MandatoryNetworkUpgrades: params.MandatoryNetworkUpgrades{
 			SubnetEVMTimestamp: big.NewInt(1000),
 		},
 	}

params/config.go

@@ -70,26 +70,25 @@ var (
 )
 
 var (
-	// SubnetEVMDefaultChainConfig is the default configuration
+	// SubnetEVMDefaultConfig is the default configuration
+	// without any network upgrades.
 	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),
-		GenesisPrecompiles:  Precompiles{},
-		NetworkUpgrades: NetworkUpgrades{
-			SubnetEVMTimestamp: big.NewInt(0),
-		},
+		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),
+		MandatoryNetworkUpgrades: MainnetNetworkUpgrades, // This can be changed to correct network (local, test) via VM.
+		GenesisPrecompiles:       Precompiles{},
 	}
 
 	TestChainConfig = &ChainConfig{
@@ -107,14 +106,37 @@ var (
 		PetersburgBlock:     big.NewInt(0),
 		IstanbulBlock:       big.NewInt(0),
 		MuirGlacierBlock:    big.NewInt(0),
-		NetworkUpgrades:     NetworkUpgrades{big.NewInt(0)},
-		GenesisPrecompiles:  Precompiles{},
-		UpgradeConfig:       UpgradeConfig{},
+		MandatoryNetworkUpgrades: MandatoryNetworkUpgrades{
+			SubnetEVMTimestamp: big.NewInt(0),
+			DUpgradeTimestamp:  big.NewInt(0),
+		},
+		GenesisPrecompiles: Precompiles{},
+		UpgradeConfig:      UpgradeConfig{},
 	}
 
 	TestPreSubnetEVMConfig = &ChainConfig{
+		AvalancheContext:         AvalancheContext{snow.DefaultContextTest()},
+		ChainID:                  big.NewInt(1),
+		FeeConfig:                DefaultFeeConfig,
+		AllowFeeRecipients:       false,
+		HomesteadBlock:           big.NewInt(0),
+		EIP150Block:              big.NewInt(0),
+		EIP150Hash:               common.Hash{},
+		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),
+		MandatoryNetworkUpgrades: MandatoryNetworkUpgrades{},
+		GenesisPrecompiles:       Precompiles{},
+		UpgradeConfig:            UpgradeConfig{},
+	}
+
+	SimulatedTestChainConfig = &ChainConfig{
 		AvalancheContext:    AvalancheContext{snow.DefaultContextTest()},
-		ChainID:             big.NewInt(1),
+		ChainID:             big.NewInt(1337),
 		FeeConfig:           DefaultFeeConfig,
 		AllowFeeRecipients:  false,
 		HomesteadBlock:      big.NewInt(0),
@@ -127,20 +149,23 @@ var (
 		PetersburgBlock:     big.NewInt(0),
 		IstanbulBlock:       big.NewInt(0),
 		MuirGlacierBlock:    big.NewInt(0),
-		NetworkUpgrades:     NetworkUpgrades{},
-		GenesisPrecompiles:  Precompiles{},
-		UpgradeConfig:       UpgradeConfig{},
+		MandatoryNetworkUpgrades: MandatoryNetworkUpgrades{
+			SubnetEVMTimestamp: big.NewInt(0),
+			DUpgradeTimestamp:  big.NewInt(0),
+		},
+		GenesisPrecompiles: Precompiles{},
+		UpgradeConfig:      UpgradeConfig{},
 	}
 )
 
 // UpgradeConfig includes the following configs that may be specified in upgradeBytes:
 // - Timestamps that enable avalanche network upgrades,
 // - Enabling or disabling precompiles as network upgrades.
 type UpgradeConfig struct {
-	// Config for blocks/timestamps that enable network upgrades.
-	// Note: if NetworkUpgrades is specified in the JSON all previously activated
+	// Config for optional timestamps that enable network upgrades.
+	// Note: if OptionalUpgrades is specified in the JSON all previously activated
 	// forks must be present or upgradeBytes will be rejected.
-	NetworkUpgrades *NetworkUpgrades `json:"networkUpgrades,omitempty"`
+	OptionalNetworkUpgrades *OptionalNetworkUpgrades `json:"networkUpgrades,omitempty"`
 
 	// Config for modifying state as a network upgrade.
 	StateUpgrades []StateUpgrade `json:"stateUpgrades,omitempty"`
@@ -181,9 +206,10 @@ type ChainConfig struct {
 	IstanbulBlock       *big.Int `json:"istanbulBlock,omitempty"`       // Istanbul switch block (nil = no fork, 0 = already on istanbul)
 	MuirGlacierBlock    *big.Int `json:"muirGlacierBlock,omitempty"`    // Eip-2384 (bomb delay) switch block (nil = no fork, 0 = already activated)
 
-	NetworkUpgrades                // Config for timestamps that enable avalanche network upgrades
-	GenesisPrecompiles Precompiles `json:"-"` // Config for enabling precompiles from genesis. JSON encode/decode will be handled by the custom marshaler/unmarshaler.
-	UpgradeConfig      `json:"-"`  // Config specified in upgradeBytes (avalanche network upgrades or enable/disabling precompiles). Skip encoding/decoding directly into ChainConfig.
+	MandatoryNetworkUpgrades             // Config for timestamps that enable mandatory network upgrades. Skip encoding/decoding directly into ChainConfig.
+	OptionalNetworkUpgrades              // Config for optional timestamps that enable network upgrades
+	GenesisPrecompiles       Precompiles `json:"-"` // Config for enabling precompiles from genesis. JSON encode/decode will be handled by the custom marshaler/unmarshaler.
+	UpgradeConfig            `json:"-"`  // Config specified in upgradeBytes (avalanche network upgrades or enable/disabling precompiles). Skip encoding/decoding directly into ChainConfig.
 }
 
 // UnmarshalJSON parses the JSON-encoded data and stores the result in the
@@ -241,9 +267,13 @@ func (c *ChainConfig) String() string {
 	if err != nil {
 		feeBytes = []byte("cannot marshal FeeConfig")
 	}
-	networkUpgradesBytes, err := json.Marshal(c.NetworkUpgrades)
+	networkUpgradesBytes, err := json.Marshal(c.MandatoryNetworkUpgrades)
+	if err != nil {
+		networkUpgradesBytes = []byte("cannot marshal MandatoryNetworkUpgrades")
+	}
+	optionalNetworkUpgradeBytes, err := json.Marshal(c.OptionalNetworkUpgrades)
 	if err != nil {
-		networkUpgradesBytes = []byte("cannot marshal NetworkUpgrades")
+		optionalNetworkUpgradeBytes = []byte("cannot marshal OptionalNetworkUpgrades")
 	}
 	precompileUpgradeBytes, err := json.Marshal(c.GenesisPrecompiles)
 	if err != nil {
@@ -254,7 +284,7 @@ func (c *ChainConfig) String() string {
 		upgradeConfigBytes = []byte("cannot marshal UpgradeConfig")
 	}
 
-	return fmt.Sprintf("{ChainID: %v Homestead: %v EIP150: %v EIP155: %v EIP158: %v Byzantium: %v Constantinople: %v Petersburg: %v Istanbul: %v, Muir Glacier: %v, Subnet EVM: %v, FeeConfig: %v, AllowFeeRecipients: %v, NetworkUpgrades: %v, PrecompileUpgrade: %v, UpgradeConfig: %v, Engine: Dummy Consensus Engine}",
+	return fmt.Sprintf("{ChainID: %v Homestead: %v EIP150: %v EIP155: %v EIP158: %v Byzantium: %v Constantinople: %v Petersburg: %v Istanbul: %v, Muir Glacier: %v, Subnet EVM: %v, DUpgrade: %v, FeeConfig: %v, AllowFeeRecipients: %v, MandatoryNetworkUpgrades: %v, OptionalNetworkUprades: %v, PrecompileUpgrade: %v, UpgradeConfig: %v, Engine: Dummy Consensus Engine}",
 		c.ChainID,
 		c.HomesteadBlock,
 		c.EIP150Block,
@@ -266,9 +296,11 @@ func (c *ChainConfig) String() string {
 		c.IstanbulBlock,
 		c.MuirGlacierBlock,
 		c.SubnetEVMTimestamp,
+		c.DUpgradeTimestamp,
 		string(feeBytes),
 		c.AllowFeeRecipients,
 		string(networkUpgradesBytes),
+		string(optionalNetworkUpgradeBytes),
 		string(precompileUpgradeBytes),
 		string(upgradeConfigBytes),
 	)
@@ -323,7 +355,7 @@ func (c *ChainConfig) IsIstanbul(num *big.Int) bool {
 
 // IsSubnetEVM returns whether [blockTimestamp] is either equal to the SubnetEVM fork block timestamp or greater.
 func (c *ChainConfig) IsSubnetEVM(blockTimestamp *big.Int) bool {
-	return utils.IsForked(c.getNetworkUpgrades().SubnetEVMTimestamp, blockTimestamp)
+	return utils.IsForked(c.SubnetEVMTimestamp, blockTimestamp)
 }
 
 func (r *Rules) PredicatesExist() bool {
@@ -383,16 +415,16 @@ func (c *ChainConfig) Verify() error {
 	return nil
 }
 
+type fork struct {
+	name     string
+	block    *big.Int
+	optional bool // if true, the fork may be nil and next fork is still allowed
+}
+
 // CheckConfigForkOrder checks that we don't "skip" any forks, geth isn't pluggable enough
 // to guarantee that forks can be implemented in a different order than on official networks
 func (c *ChainConfig) CheckConfigForkOrder() error {
-	type fork struct {
-		name     string
-		block    *big.Int
-		optional bool // if true, the fork may be nil and next fork is still allowed
-	}
-	var lastFork fork
-	for _, cur := range []fork{
+	ethForks := []fork{
 		{name: "homesteadBlock", block: c.HomesteadBlock},
 		{name: "eip150Block", block: c.EIP150Block},
 		{name: "eip155Block", block: c.EIP155Block},
@@ -402,27 +434,11 @@ func (c *ChainConfig) CheckConfigForkOrder() error {
 		{name: "petersburgBlock", block: c.PetersburgBlock},
 		{name: "istanbulBlock", block: c.IstanbulBlock},
 		{name: "muirGlacierBlock", block: c.MuirGlacierBlock, optional: true},
-	} {
-		if cur.block != nil && common.Big0.Cmp(cur.block) != 0 {
-			return errNonGenesisForkByHeight
-		}
-		if lastFork.name != "" {
-			// Next one must be higher number
-			if lastFork.block == nil && cur.block != nil {
-				return fmt.Errorf("unsupported fork ordering: %v not enabled, but %v enabled at %v",
-					lastFork.name, cur.name, cur.block)
-			}
-			if lastFork.block != nil && cur.block != nil {
-				if lastFork.block.Cmp(cur.block) > 0 {
-					return fmt.Errorf("unsupported fork ordering: %v enabled at %v, but %v enabled at %v",
-						lastFork.name, lastFork.block, cur.name, cur.block)
-				}
-			}
-		}
-		// If it was optional and not set, then ignore it
-		if !cur.optional || cur.block != nil {
-			lastFork = cur
-		}
+	}
+
+	// Check that forks are enabled in order
+	if err := checkForks(ethForks, true); err != nil {
+		return err
 	}
 
 	// Note: In Avalanche, hard forks must take place via block timestamps instead
@@ -433,10 +449,24 @@ func (c *ChainConfig) CheckConfigForkOrder() error {
 	// Note: we do not add the optional stateful precompile configs in here because they are optional
 	// and independent, such that the ordering they are enabled does not impact the correctness of the
 	// chain config.
-	lastFork = fork{}
-	for _, cur := range []fork{
-		{name: "subnetEVMTimestamp", block: c.SubnetEVMTimestamp},
-	} {
+	if err := checkForks(c.mandatoryForkOrder(), false); err != nil {
+		return err
+	}
+
+	// Check optional forks are enabled in order
+	if err := checkForks(c.optionalForkOrder(), false); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func checkForks(forks []fork, heightFork bool) error {
+	lastFork := fork{}
+	for _, cur := range forks {
+		if heightFork && cur.block != nil && common.Big0.Cmp(cur.block) != 0 {
+			return errNonGenesisForkByHeight
+		}
 		if lastFork.name != "" {
 			// Next one must be higher number
 			if lastFork.block == nil && cur.block != nil {
@@ -497,15 +527,20 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, lastHeight *big.Int,
 		return newCompatError("Muir Glacier fork block", c.MuirGlacierBlock, newcfg.MuirGlacierBlock)
 	}
 
+	// Check avalanhe network upgrades
+	if err := c.CheckMandatoryCompatible(&newcfg.MandatoryNetworkUpgrades, lastTimestamp); err != nil {
+		return err
+	}
+
 	// Check subnet-evm specific activations
-	newNetworkUpgrades := newcfg.getNetworkUpgrades()
-	if c.UpgradeConfig.NetworkUpgrades != nil && newcfg.UpgradeConfig.NetworkUpgrades == nil {
-		// Note: if the current NetworkUpgrades are set via UpgradeConfig, then a new config
-		// without NetworkUpgrades will be treated as having specified an empty set of network
+	newOptionalNetworkUpgrades := newcfg.getOptionalNetworkUpgrades()
+	if c.UpgradeConfig.OptionalNetworkUpgrades != nil && newcfg.UpgradeConfig.OptionalNetworkUpgrades == nil {
+		// Note: if the current OptionalNetworkUpgrades are set via UpgradeConfig, then a new config
+		// without OptionalNetworkUpgrades will be treated as having specified an empty set of network
 		// upgrades (ie., treated as the user intends to cancel scheduled forks)
-		newNetworkUpgrades = &NetworkUpgrades{}
+		newOptionalNetworkUpgrades = &OptionalNetworkUpgrades{}
 	}
-	if err := c.getNetworkUpgrades().CheckCompatible(newNetworkUpgrades, lastTimestamp); err != nil {
+	if err := c.getOptionalNetworkUpgrades().CheckOptionalCompatible(newOptionalNetworkUpgrades, lastTimestamp); err != nil {
 		return err
 	}
 
@@ -523,13 +558,13 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, lastHeight *big.Int,
 	return nil
 }
 
-// getNetworkUpgrades returns NetworkUpgrades from upgrade config if set there,
+// getOptionalNetworkUpgrades returns OptionalNetworkUpgrades from upgrade config if set there,
 // otherwise it falls back to the genesis chain config.
-func (c *ChainConfig) getNetworkUpgrades() *NetworkUpgrades {
-	if upgradeConfigOverride := c.UpgradeConfig.NetworkUpgrades; upgradeConfigOverride != nil {
+func (c *ChainConfig) getOptionalNetworkUpgrades() *OptionalNetworkUpgrades {
+	if upgradeConfigOverride := c.UpgradeConfig.OptionalNetworkUpgrades; upgradeConfigOverride != nil {
 		return upgradeConfigOverride
 	}
-	return &c.NetworkUpgrades
+	return &c.OptionalNetworkUpgrades
 }
 
 // isForkIncompatible returns true if a fork scheduled at s1 cannot be rescheduled to