Generalized upgrades rb nits0 (#512)

* Minor improvements

* restore readOnly

* more updates

* Add back readOnly to allow list tests

---------

Co-authored-by: Aaron Buchwald <aaron.buchwald56@gmail.com>

core/state_processor_test.go

@@ -44,9 +44,9 @@ import (
 )
 
 var (
-	chainConfig = params.TestChainConfig
-	signer      = types.LatestSigner(chainConfig)
-	testKey, _  = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
+	config     = params.TestChainConfig
+	signer     = types.LatestSigner(config)
+	testKey, _ = crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
 )
 
 func makeTx(nonce uint64, to common.Address, amount *big.Int, gasLimit uint64, gasPrice *big.Int, data []byte) *types.Transaction {
@@ -71,12 +71,12 @@ func mkDynamicTx(nonce uint64, to common.Address, gasLimit uint64, gasTipCap, ga
 // blockchain imports bad blocks, meaning blocks which have valid headers but
 // contain invalid transactions
 func TestStateProcessorErrors(t *testing.T) {
-	chainConfig.FeeConfig.MinBaseFee = params.TestMaxBaseFee
+	config.FeeConfig.MinBaseFee = params.TestMaxBaseFee
 	{ // Tests against a 'recent' chain definition
 		var (
 			db    = rawdb.NewMemoryDatabase()
 			gspec = &Genesis{
-				Config: chainConfig,
+				Config: config,
 				Alloc: GenesisAlloc{
 					common.HexToAddress("0x71562b71999873DB5b286dF957af199Ec94617F7"): GenesisAccount{
 						Balance: big.NewInt(2000000000000000000), // 2 ether
@@ -254,7 +254,7 @@ func TestStateProcessorErrors(t *testing.T) {
 		var (
 			db    = rawdb.NewMemoryDatabase()
 			gspec = &Genesis{
-				Config: chainConfig,
+				Config: config,
 				Alloc: GenesisAlloc{
 					common.HexToAddress("0x71562b71999873DB5b286dF957af199Ec94617F7"): GenesisAccount{
 						Balance: big.NewInt(1000000000000000000), // 1 ether

core/state_transition.go

@@ -252,7 +252,7 @@ func (st *StateTransition) preCheck() error {
 		if st.evm.ChainConfig().IsPrecompileEnabled(txallowlist.ContractAddress, st.evm.Context.Time) {
 			txAllowListRole := txallowlist.GetTxAllowListStatus(st.state, st.msg.From())
 			if !txAllowListRole.IsEnabled() {
-				return fmt.Errorf("%w: %s", txallowlist.ErrSenderAddressNotAllowListed, st.msg.From())
+				return fmt.Errorf("%w: %s", vmerrs.ErrSenderAddressNotAllowListed, st.msg.From())
 			}
 		}
 	}

core/tx_pool.go

@@ -44,6 +44,7 @@ import (
 	"github.com/ava-labs/subnet-evm/params"
 	"github.com/ava-labs/subnet-evm/precompile/contracts/feemanager"
 	"github.com/ava-labs/subnet-evm/precompile/contracts/txallowlist"
+	"github.com/ava-labs/subnet-evm/vmerrs"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/prque"
@@ -698,7 +699,7 @@ func (pool *TxPool) checkTxState(from common.Address, tx *types.Transaction) err
 	if pool.chainconfig.IsPrecompileEnabled(txallowlist.ContractAddress, headTimestamp) {
 		txAllowListRole := txallowlist.GetTxAllowListStatus(pool.currentState, from)
 		if !txAllowListRole.IsEnabled() {
-			return fmt.Errorf("%w: %s", txallowlist.ErrSenderAddressNotAllowListed, from)
+			return fmt.Errorf("%w: %s", vmerrs.ErrSenderAddressNotAllowListed, from)
 		}
 	}
 	return nil

params/chain_config_precompiles.go

@@ -26,8 +26,7 @@ func (ccp *ChainConfigPrecompiles) UnmarshalJSON(data []byte) error {
 		key := module.ConfigKey
 		if value, ok := raw[key]; ok {
 			conf := module.NewConfig()
-			err := json.Unmarshal(value, conf)
-			if err != nil {
+			if err := json.Unmarshal(value, conf); err != nil {
 				return err
 			}
 			(*ccp)[key] = conf

params/config.go

@@ -172,8 +172,7 @@ func (c *ChainConfig) UnmarshalJSON(data []byte) error {
 	// Alias ChainConfig to avoid recursion
 	type _ChainConfig ChainConfig
 	tmp := _ChainConfig{}
-	err := json.Unmarshal(data, &tmp)
-	if err != nil {
+	if err := json.Unmarshal(data, &tmp); err != nil {
 		return err
 	}
 
@@ -194,14 +193,13 @@ func (c ChainConfig) MarshalJSON() ([]byte, error) {
 		return nil, err
 	}
 
-	// Marshal PrecompileUpgrade
+	// To include PrecompileUpgrades, we unmarshal the json representing c
+	// then directly add the corresponding keys to the json.
 	raw := make(map[string]json.RawMessage)
-	err = json.Unmarshal(tmp, &raw)
-	if err != nil {
+	if err := json.Unmarshal(tmp, &raw); err != nil {
 		return nil, err
 	}
 
-	// Marshal Precompiles and inline them into the JSON
 	for key, value := range c.GenesisPrecompiles {
 		conf, err := json.Marshal(value)
 		if err != nil {

params/config_test.go

@@ -154,7 +154,8 @@ func TestConfigUnmarshalJSON(t *testing.T) {
 			AllowFeeRecipients: true,
 		})
 
-	testContractNativeMinterConfig := nativeminter.NewConfig(big.NewInt(0),
+	testContractNativeMinterConfig := nativeminter.NewConfig(
+		big.NewInt(0),
 		[]common.Address{common.HexToAddress("0x8db97C7cEcE249c2b98bDC0226Cc4C2A57BF52FC")},
 		nil,
 		nil,
@@ -193,9 +194,9 @@ func TestConfigUnmarshalJSON(t *testing.T) {
 	require.Equal(rewardManagerConfig.Key(), rewardmanager.ConfigKey)
 	require.True(rewardManagerConfig.Equal(testRewardManagerConfig))
 
-	contractNativeMinterConfig := c.GenesisPrecompiles[nativeminter.ConfigKey]
-	require.Equal(contractNativeMinterConfig.Key(), nativeminter.ConfigKey)
-	require.True(contractNativeMinterConfig.Equal(testContractNativeMinterConfig))
+	nativeMinterConfig := c.GenesisPrecompiles[nativeminter.ConfigKey]
+	require.Equal(nativeMinterConfig.Key(), nativeminter.ConfigKey)
+	require.True(nativeMinterConfig.Equal(testContractNativeMinterConfig))
 
 	// Marshal and unmarshal again and check that the result is the same
 	marshaled, err := json.Marshal(c)

params/precompile_config_test.go

@@ -180,19 +180,19 @@ func TestVerifyPrecompiles(t *testing.T) {
 	admins := []common.Address{{1}}
 	tests := []struct {
 		name          string
-		upgrade       ChainConfigPrecompiles
+		precompiles   ChainConfigPrecompiles
 		expectedError string
 	}{
 		{
 			name: "invalid allow list config in tx allowlist",
-			upgrade: ChainConfigPrecompiles{
+			precompiles: ChainConfigPrecompiles{
 				txallowlist.ConfigKey: txallowlist.NewConfig(big.NewInt(3), admins, admins),
 			},
 			expectedError: "cannot set address",
 		},
 		{
 			name: "invalid initial fee manager config",
-			upgrade: ChainConfigPrecompiles{
+			precompiles: ChainConfigPrecompiles{
 				feemanager.ConfigKey: feemanager.NewConfig(big.NewInt(3), admins, nil,
 					&commontype.FeeConfig{
 						GasLimit: big.NewInt(-1),
@@ -206,7 +206,7 @@ func TestVerifyPrecompiles(t *testing.T) {
 			require := require.New(t)
 			baseConfig := *SubnetEVMDefaultChainConfig
 			config := &baseConfig
-			config.GenesisPrecompiles = tt.upgrade
+			config.GenesisPrecompiles = tt.precompiles
 
 			err := config.Verify()
 			if tt.expectedError == "" {
@@ -301,10 +301,10 @@ func TestPrecompileUpgradeUnmarshalJSON(t *testing.T) {
 		})
 	require.True(rewardManagerConf.Equal(testRewardManagerConfig))
 
-	contractNativeMinterConf := upgradeConfig.PrecompileUpgrades[1]
-	require.Equal(contractNativeMinterConf.Key(), nativeminter.ConfigKey)
-	testContractNativeMinterConfig := nativeminter.NewConfig(big.NewInt(1671543172), nil, nil, nil)
-	require.True(contractNativeMinterConf.Equal(testContractNativeMinterConfig))
+	nativeMinterConfig := upgradeConfig.PrecompileUpgrades[1]
+	require.Equal(nativeMinterConfig.Key(), nativeminter.ConfigKey)
+	expectedNativeMinterConfig := nativeminter.NewConfig(big.NewInt(1671543172), nil, nil, nil)
+	require.True(nativeMinterConfig.Equal(expectedNativeMinterConfig))
 
 	// Marshal and unmarshal again and check that the result is the same
 	upgradeBytes2, err := json.Marshal(upgradeConfig)

params/precompile_upgrade.go

@@ -21,14 +21,15 @@ var errNoKey = errors.New("PrecompileUpgrade cannot be empty")
 // PrecompileUpgrade is a helper struct embedded in UpgradeConfig.
 // It is used to unmarshal the json into the correct precompile config type
 // based on the key. Keys are defined in each precompile module, and registered in
-// params/precompile_modules.go.
+// precompile/registry/registry.go.
 type PrecompileUpgrade struct {
 	config.Config
 }
 
 // UnmarshalJSON unmarshals the json into the correct precompile config type
 // based on the key. Keys are defined in each precompile module, and registered in
 // params/precompile_modules.go.
+// precompile/registry/registry.go.
 // Ex: {"feeManagerConfig": {...}} where "feeManagerConfig" is the key
 func (u *PrecompileUpgrade) UnmarshalJSON(data []byte) error {
 	raw := make(map[string]json.RawMessage)
@@ -47,8 +48,7 @@ func (u *PrecompileUpgrade) UnmarshalJSON(data []byte) error {
 			return fmt.Errorf("unknown precompile config: %s", key)
 		}
 		config := module.NewConfig()
-		err := json.Unmarshal(value, config)
-		if err != nil {
+		if err := json.Unmarshal(value, config); err != nil {
 			return err
 		}
 		u.Config = config
@@ -163,13 +163,12 @@ func (c *ChainConfig) GetActivePrecompileConfig(address common.Address, blockTim
 // GetActivatingPrecompileConfigs returns all upgrades configured to activate during the state transition from a block with timestamp [from]
 // to a block with timestamp [to].
 func (c *ChainConfig) GetActivatingPrecompileConfigs(address common.Address, from *big.Int, to *big.Int, upgrades []PrecompileUpgrade) []config.Config {
-	configs := make([]config.Config, 0)
-
 	// Get key from address.
 	module, ok := modules.GetPrecompileModuleByAddress(address)
 	if !ok {
-		return configs
+		return nil
 	}
+	configs := make([]config.Config, 0)
 	key := module.ConfigKey
 	// First check the embedded [upgrade] for precompiles configured
 	// in the genesis chain config.
@@ -191,11 +190,12 @@ func (c *ChainConfig) GetActivatingPrecompileConfigs(address common.Address, fro
 }
 
 // CheckPrecompilesCompatible checks if [precompileUpgrades] are compatible with [c] at [headTimestamp].
-// Returns a ConfigCompatError if upgrades already forked at [headTimestamp] are missing from
-// [precompileUpgrades]. Upgrades not already forked may be modified or absent from [precompileUpgrades].
+// Returns a ConfigCompatError if upgrades already activated at [headTimestamp] are missing from
+// [precompileUpgrades]. Upgrades not already activated may be modified or absent from [precompileUpgrades].
 // Returns nil if [precompileUpgrades] is compatible with [c].
 // Assumes given timestamp is the last accepted block timestamp.
-// This ensures that as long as the node has not accepted a block with a different rule set it will allow a new upgrade to be applied as long as it activates after the last accepted block.
+// This ensures that as long as the node has not accepted a block with a different rule set it will allow a
+// new upgrade to be applied as long as it activates after the last accepted block.
 func (c *ChainConfig) CheckPrecompilesCompatible(precompileUpgrades []PrecompileUpgrade, lastTimestamp *big.Int) *ConfigCompatError {
 	for _, module := range modules.RegisteredModules() {
 		if err := c.checkPrecompileCompatible(module.Address, precompileUpgrades, lastTimestamp); err != nil {
@@ -206,15 +206,16 @@ func (c *ChainConfig) CheckPrecompilesCompatible(precompileUpgrades []Precompile
 	return nil
 }
 
-// checkPrecompileCompatible verifies that the precompile specified by [address] is compatible between [c] and [precompileUpgrades] at [headTimestamp].
-// Returns an error if upgrades already forked at [headTimestamp] are missing from [precompileUpgrades].
+// checkPrecompileCompatible verifies that the precompile specified by [address] is compatible between [c]
+// and [precompileUpgrades] at [headTimestamp].
+// Returns an error if upgrades already activated at [headTimestamp] are missing from [precompileUpgrades].
 // Upgrades that have already gone into effect cannot be modified or absent from [precompileUpgrades].
 func (c *ChainConfig) checkPrecompileCompatible(address common.Address, precompileUpgrades []PrecompileUpgrade, lastTimestamp *big.Int) *ConfigCompatError {
-	// all active upgrades must match
+	// All active upgrades (from nil to [lastTimestamp]) must match.
 	activeUpgrades := c.GetActivatingPrecompileConfigs(address, nil, lastTimestamp, c.PrecompileUpgrades)
 	newUpgrades := c.GetActivatingPrecompileConfigs(address, nil, lastTimestamp, precompileUpgrades)
 
-	// first, check existing upgrades are there
+	// Check activated upgrades are still present.
 	for i, upgrade := range activeUpgrades {
 		if len(newUpgrades) <= i {
 			// missing upgrade
@@ -224,7 +225,7 @@ func (c *ChainConfig) checkPrecompileCompatible(address common.Address, precompi
 				nil,
 			)
 		}
-		// All upgrades that have forked must be identical.
+		// All upgrades that have activated must be identical.
 		if !upgrade.Equal(newUpgrades[i]) {
 			return newCompatError(
 				fmt.Sprintf("PrecompileUpgrade[%d]", i),

plugin/evm/vm_test.go

@@ -2237,7 +2237,7 @@ func TestTxAllowListSuccessfulTx(t *testing.T) {
 	}
 
 	errs = vm.txPool.AddRemotesSync([]*types.Transaction{signedTx1})
-	if err := errs[0]; !errors.Is(err, txallowlist.ErrSenderAddressNotAllowListed) {
+	if err := errs[0]; !errors.Is(err, vmerrs.ErrSenderAddressNotAllowListed) {
 		t.Fatalf("expected ErrSenderAddressNotAllowListed, got: %s", err)
 	}
 
@@ -2334,7 +2334,7 @@ func TestTxAllowListDisablePrecompile(t *testing.T) {
 	}
 
 	errs = vm.txPool.AddRemotesSync([]*types.Transaction{signedTx1})
-	if err := errs[0]; !errors.Is(err, txallowlist.ErrSenderAddressNotAllowListed) {
+	if err := errs[0]; !errors.Is(err, vmerrs.ErrSenderAddressNotAllowListed) {
 		t.Fatalf("expected ErrSenderAddressNotAllowListed, got: %s", err)
 	}
 

plugin/evm/vm_upgrade_bytes_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/ava-labs/subnet-evm/metrics"
 	"github.com/ava-labs/subnet-evm/params"
 	"github.com/ava-labs/subnet-evm/precompile/contracts/txallowlist"
+	"github.com/ava-labs/subnet-evm/vmerrs"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -57,7 +58,7 @@ func TestVMUpgradeBytesPrecompile(t *testing.T) {
 		t.Fatal(err)
 	}
 	errs = vm.txPool.AddRemotesSync([]*types.Transaction{signedTx1})
-	if err := errs[0]; !errors.Is(err, txallowlist.ErrSenderAddressNotAllowListed) {
+	if err := errs[0]; !errors.Is(err, vmerrs.ErrSenderAddressNotAllowListed) {
 		t.Fatalf("expected ErrSenderAddressNotAllowListed, got: %s", err)
 	}
 
@@ -108,7 +109,7 @@ func TestVMUpgradeBytesPrecompile(t *testing.T) {
 
 	// Submit a rejected transaction, should throw an error
 	errs = vm.txPool.AddRemotesSync([]*types.Transaction{signedTx1})
-	if err := errs[0]; !errors.Is(err, txallowlist.ErrSenderAddressNotAllowListed) {
+	if err := errs[0]; !errors.Is(err, vmerrs.ErrSenderAddressNotAllowListed) {
 		t.Fatalf("expected ErrSenderAddressNotAllowListed, got: %s", err)
 	}
 

precompile/allowlist/allowlist.go

@@ -6,21 +6,18 @@ package allowlist
 import (
 	"errors"
 	"fmt"
-	"math/big"
 
 	"github.com/ava-labs/subnet-evm/precompile/contract"
 	"github.com/ava-labs/subnet-evm/vmerrs"
 	"github.com/ethereum/go-ethereum/common"
 )
 
-// AllowList is a precompile that allows the admin to set the permissions of
-// addresses. AllowList itself is not a precompile, but rather a helper
-// function that is used by other precompiles to determine if an address has
-// permissions.
-// The permissions are:
-// 1. No role assigned - this is equivalent to common.Hash{} and deletes the key from the DB when set
-// 2. Enabled are allowed to take certain actions
-// 3. Admin - allowed to modify both the admin and enabled list as well as take certain actions
+// AllowList is an abstraction that allows other precompiles to manage
+// which addresses can call the precompile by maintaining an allowlist
+// in the storage trie. Each account may have one of the following roles:
+// 1. NoRole - this is equivalent to common.Hash{} and deletes the key from the DB when set
+// 2. EnabledRole - allowed to call the precompile
+// 3. Admin - allowed to modify both the allowlist and call the precompile
 
 const (
 	SetAdminFuncKey      = "setAdmin"
@@ -32,10 +29,12 @@ const (
 	ReadAllowListGasCost   = contract.ReadGasCostPerSlot
 )
 
+const allowListInputLen = common.HashLength
+
 var (
-	NoRole      Role = Role(common.BigToHash(big.NewInt(0))) // No role assigned - this is equivalent to common.Hash{} and deletes the key from the DB when set
-	EnabledRole Role = Role(common.BigToHash(big.NewInt(1))) // Deployers are allowed to create new contracts
-	AdminRole   Role = Role(common.BigToHash(big.NewInt(2))) // Admin - allowed to modify both the admin and deployer list as well as deploy contracts
+	NoRole      = Role(common.BigToHash(common.Big0)) // NoRole - this is equivalent to common.Hash{} and deletes the key from the DB when set
+	EnabledRole = Role(common.BigToHash(common.Big1)) // EnabledRole - allowed to call the precompile
+	AdminRole   = Role(common.BigToHash(common.Big2)) // Admin - allowed to modify both the allowlist and call the precompile
 
 	// AllowList function signatures
 	setAdminSignature      = contract.CalculateFunctionSelector("setAdmin(address)")
@@ -44,8 +43,6 @@ var (
 	readAllowListSignature = contract.CalculateFunctionSelector("readAllowList(address)")
 	// Error returned when an invalid write is attempted
 	ErrCannotModifyAllowList = errors.New("non-admin cannot modify allow list")
-
-	allowListInputLen = common.HashLength
 )
 
 // GetAllowListStatus returns the allow list role of [address] for the precompile
@@ -152,7 +149,7 @@ func createReadAllowList(precompileAddr common.Address) contract.RunStatefulPrec
 	}
 }
 
-// createAllowListPrecompile returns a StatefulPrecompiledContract with R/W control of an allow list at [precompileAddr]
+// CreateAllowListPrecompile returns a StatefulPrecompiledContract with R/W control of an allow list at [precompileAddr]
 func CreateAllowListPrecompile(precompileAddr common.Address) contract.StatefulPrecompiledContract {
 	// Construct the contract with no fallback function.
 	allowListFuncs := CreateAllowListFunctions(precompileAddr)