evm: unit tests for gas refund (#686)

* add TestGetEthIntrinsicGas

* more test cases in TestGetEthIntrinsicGas

* add GasToRefund tests

* add TestRefundGas tests

* remove duplicate leftoverGas check

* made resetGasMeterAndConsumeGas public for testing

* TestResetGasMeterAndConsumeGas tmp

* add mintFeeCollector flag for gas refund tests

* add comment and check

* add TestResetGasMeterAndConsumeGas

* Update x/evm/keeper/state_transition_test.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

x/evm/keeper/keeper_test.go

@@ -14,9 +14,12 @@ import (
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
+	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+	tmjson "github.com/tendermint/tendermint/libs/json"
 	feemarkettypes "github.com/tharsis/ethermint/x/feemarket/types"
 
 	"github.com/tharsis/ethermint/app"
@@ -31,6 +34,7 @@ import (
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/crypto"
+	"github.com/ethereum/go-ethereum/params"
 
 	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto/tmhash"
@@ -58,6 +62,7 @@ type KeeperTestSuite struct {
 	signer   keyring.Signer
 
 	dynamicTxFee     bool
+	mintFeeCollector bool
 }
 
 /// DoSetupTest setup test environment, it uses`require.TestingT` to support both `testing.T` and `testing.B`.
@@ -86,6 +91,37 @@ func (suite *KeeperTestSuite) DoSetupTest(t require.TestingT) {
 		suite.app = app.Setup(checkTx, nil)
 	}
 
+	if suite.mintFeeCollector {
+		// mint some coin to fee collector
+		coins := sdk.NewCoins(sdk.NewCoin(types.DefaultEVMDenom, sdk.NewInt(int64(params.TxGas)-1)))
+		genesisState := app.ModuleBasics.DefaultGenesis(suite.app.AppCodec())
+		balances := []banktypes.Balance{
+			{
+				Address: suite.app.AccountKeeper.GetModuleAddress(authtypes.FeeCollectorName).String(),
+				Coins:   coins,
+			},
+		}
+		// update total supply
+		bankGenesis := banktypes.NewGenesisState(banktypes.DefaultGenesisState().Params, balances, sdk.NewCoins(sdk.NewCoin(types.DefaultEVMDenom, sdk.NewInt((int64(params.TxGas)-1)))), []banktypes.Metadata{})
+		bz := suite.app.AppCodec().MustMarshalJSON(bankGenesis)
+		require.NotNil(t, bz)
+		genesisState[banktypes.ModuleName] = suite.app.AppCodec().MustMarshalJSON(bankGenesis)
+
+		// we marshal the genesisState of all module to a byte array
+		stateBytes, err := tmjson.MarshalIndent(genesisState, "", " ")
+		require.NoError(t, err)
+
+		//Initialize the chain
+		suite.app.InitChain(
+			abci.RequestInitChain{
+				ChainId:         "ethermint_9000-1",
+				Validators:      []abci.ValidatorUpdate{},
+				ConsensusParams: simapp.DefaultConsensusParams,
+				AppStateBytes:   stateBytes,
+			},
+		)
+	}
+
 	suite.ctx = suite.app.BaseApp.NewContext(checkTx, tmproto.Header{
 		Height:          1,
 		ChainID:         "ethermint_9000-1",

x/evm/keeper/state_transition.go

@@ -244,7 +244,7 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
 	k.IncreaseTxIndexTransient()
 
 	// update the gas used after refund
-	k.resetGasMeterAndConsumeGas(res.GasUsed)
+	k.ResetGasMeterAndConsumeGas(res.GasUsed)
 	return res, nil
 }
 
@@ -398,6 +398,7 @@ func (k *Keeper) GetEthIntrinsicGas(msg core.Message, cfg *params.ChainConfig, i
 
 // GasToRefund calculates the amount of gas the state machine should refund to the sender. It is
 // capped by the refund quotient value.
+// Note: do not pass 0 to refundQuotient
 func (k *Keeper) GasToRefund(gasConsumed, refundQuotient uint64) uint64 {
 	// Apply refund counter
 	refund := gasConsumed / refundQuotient
@@ -412,6 +413,7 @@ func (k *Keeper) GasToRefund(gasConsumed, refundQuotient uint64) uint64 {
 // consumed in the transaction. Additionally, the function sets the total gas consumed to the value
 // returned by the EVM execution, thus ignoring the previous intrinsic gas consumed during in the
 // AnteHandler.
+// NOTE: DO NOT pass 0 to refundQuotient
 func (k *Keeper) RefundGas(msg core.Message, leftoverGas, refundQuotient uint64) (uint64, error) {
 	// safety check: leftover gas after execution should never exceed the gas limit defined on the message
 	if leftoverGas > msg.Gas() {
@@ -427,14 +429,6 @@ func (k *Keeper) RefundGas(msg core.Message, leftoverGas, refundQuotient uint64)
 	refund := k.GasToRefund(gasConsumed, refundQuotient)
 	leftoverGas += refund
 
-	// safety check: leftover gas after refund should never exceed the gas limit defined on the message
-	if leftoverGas > msg.Gas() {
-		return leftoverGas, stacktrace.Propagate(
-			sdkerrors.Wrapf(types.ErrInconsistentGas, "leftover gas cannot be greater than gas limit (%d > %d)", leftoverGas, msg.Gas()),
-			"failed to update gas consumed after refund of %d gas", refund,
-		)
-	}
-
 	// Return EVM tokens for remaining gas, exchanged at the original rate.
 	remaining := new(big.Int).Mul(new(big.Int).SetUint64(leftoverGas), msg.GasPrice())
 
@@ -461,9 +455,9 @@ func (k *Keeper) RefundGas(msg core.Message, leftoverGas, refundQuotient uint64)
 	return leftoverGas, nil
 }
 
-// resetGasMeterAndConsumeGas reset first the gas meter consumed value to zero and set it back to the new value
+// ResetGasMeterAndConsumeGas reset first the gas meter consumed value to zero and set it back to the new value
 // 'gasUsed'
-func (k *Keeper) resetGasMeterAndConsumeGas(gasUsed uint64) {
+func (k *Keeper) ResetGasMeterAndConsumeGas(gasUsed uint64) {
 	// reset the gas count
 	ctx := k.Ctx()
 	ctx.GasMeter().RefundGas(ctx.GasMeter().GasConsumed(), "reset the gas count")

x/evm/keeper/state_transition_benchmark_test.go

@@ -83,6 +83,8 @@ func newNativeMessage(
 	krSigner keyring.Signer,
 	ethSigner ethtypes.Signer,
 	txType byte,
+	data []byte,
+	accessList ethtypes.AccessList,
 ) (core.Message, error) {
 	msgSigner := ethtypes.MakeSigner(cfg, big.NewInt(blockHeight))
 
@@ -94,12 +96,29 @@ func newNativeMessage(
 	switch txType {
 	case ethtypes.LegacyTxType:
 		templateLegacyTx.Nonce = nonce
+		if data != nil {
+			templateLegacyTx.Data = data
+		}
 		ethTx = ethtypes.NewTx(templateLegacyTx)
 	case ethtypes.AccessListTxType:
 		templateAccessListTx.Nonce = nonce
+		if data != nil {
+			templateAccessListTx.Data = data
+		} else {
+			templateAccessListTx.Data = []byte{}
+		}
+
+		templateAccessListTx.AccessList = accessList
 		ethTx = ethtypes.NewTx(templateAccessListTx)
 	case ethtypes.DynamicFeeTxType:
 		templateDynamicFeeTx.Nonce = nonce
+
+		if data != nil {
+			templateAccessListTx.Data = data
+		} else {
+			templateAccessListTx.Data = []byte{}
+		}
+		templateAccessListTx.AccessList = accessList
 		ethTx = ethtypes.NewTx(templateDynamicFeeTx)
 		baseFee = big.NewInt(3)
 	default:
@@ -224,6 +243,8 @@ func BenchmarkApplyNativeMessage(b *testing.B) {
 			suite.signer,
 			signer,
 			ethtypes.AccessListTxType,
+			nil,
+			nil,
 		)
 		require.NoError(b, err)
 
@@ -257,6 +278,8 @@ func BenchmarkApplyNativeMessageWithLegacyTx(b *testing.B) {
 			suite.signer,
 			signer,
 			ethtypes.LegacyTxType,
+			nil,
+			nil,
 		)
 		require.NoError(b, err)
 
@@ -290,6 +313,8 @@ func BenchmarkApplyNativeMessageWithDynamicFeeTx(b *testing.B) {
 			suite.signer,
 			signer,
 			ethtypes.DynamicFeeTxType,
+			nil,
+			nil,
 		)
 		require.NoError(b, err)
 

x/evm/keeper/state_transition_test.go

@@ -2,17 +2,18 @@ package keeper_test
 
 import (
 	"fmt"
-
+	"math"
-	"github.com/ethereum/go-ethereum/common"
+	"math/big"
-
-	"github.com/tendermint/tendermint/crypto/tmhash"
-	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
-	tmtypes "github.com/tendermint/tendermint/types"
 
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-
+	"github.com/ethereum/go-ethereum/common"
+	ethtypes "github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/params"
+	"github.com/tendermint/tendermint/crypto/tmhash"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	tmtypes "github.com/tendermint/tendermint/types"
 	"github.com/tharsis/ethermint/tests"
 )
 
@@ -173,3 +174,324 @@ func (suite *KeeperTestSuite) TestGetCoinbaseAddress() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestGetEthIntrinsicGas() {
+	testCases := []struct {
+		name               string
+		data               []byte
+		accessList         ethtypes.AccessList
+		height             int64
+		isContractCreation bool
+		noError            bool
+		expGas             uint64
+	}{
+		{
+			"no data, no accesslist, not contract creation, not homestead, not istanbul",
+			nil,
+			nil,
+			1,
+			false,
+			true,
+			params.TxGas,
+		},
+		{
+			"with one zero data, no accesslist, not contract creation, not homestead, not istanbul",
+			[]byte{0},
+			nil,
+			1,
+			false,
+			true,
+			params.TxGas + params.TxDataZeroGas*1,
+		},
+		{
+			"with one non zero data, no accesslist, not contract creation, not homestead, not istanbul",
+			[]byte{1},
+			nil,
+			1,
+			true,
+			true,
+			params.TxGas + params.TxDataNonZeroGasFrontier*1,
+		},
+		// we are not able to test the ErrGasUintOverflow due to RAM limitation
+		// {
+		// 	"with big data size overflow",
+		// 	make([]byte, 271300000000000000),
+		// 	nil,
+		// 	1,
+		// 	false,
+		// 	false,
+		// 	0,
+		// },
+		{
+			"no data, one accesslist, not contract creation, not homestead, not istanbul",
+			nil,
+			[]ethtypes.AccessTuple{
+				{},
+			},
+			1,
+			false,
+			true,
+			params.TxGas + params.TxAccessListAddressGas,
+		},
+		{
+			"no data, one accesslist with one storageKey, not contract creation, not homestead, not istanbul",
+			nil,
+			[]ethtypes.AccessTuple{
+				{StorageKeys: make([]common.Hash, 1)},
+			},
+			1,
+			false,
+			true,
+			params.TxGas + params.TxAccessListAddressGas + params.TxAccessListStorageKeyGas*1,
+		},
+		{
+			"no data, no accesslist, is contract creation, is homestead, not istanbul",
+			nil,
+			nil,
+			2,
+			true,
+			true,
+			params.TxGasContractCreation,
+		},
+		{
+			"with one zero data, no accesslist, not contract creation, is homestead, is istanbul",
+			[]byte{1},
+			nil,
+			3,
+			false,
+			true,
+			params.TxGas + params.TxDataNonZeroGasEIP2028*1,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
+			suite.SetupTest() // reset
+
+			params := suite.app.EvmKeeper.GetParams(suite.ctx)
+			ethCfg := params.ChainConfig.EthereumConfig(suite.app.EvmKeeper.ChainID())
+			ethCfg.HomesteadBlock = big.NewInt(2)
+			ethCfg.IstanbulBlock = big.NewInt(3)
+			signer := ethtypes.LatestSignerForChainID(suite.app.EvmKeeper.ChainID())
+
+			suite.ctx = suite.ctx.WithBlockHeight(tc.height)
+			suite.app.EvmKeeper.WithContext(suite.ctx)
+
+			m, err := newNativeMessage(
+				suite.app.EvmKeeper.GetNonce(suite.address),
+				suite.ctx.BlockHeight(),
+				suite.address,
+				ethCfg,
+				suite.signer,
+				signer,
+				ethtypes.AccessListTxType,
+				tc.data,
+				tc.accessList,
+			)
+			suite.Require().NoError(err)
+
+			gas, err := suite.app.EvmKeeper.GetEthIntrinsicGas(m, ethCfg, tc.isContractCreation)
+			if tc.noError {
+				suite.Require().NoError(err)
+			} else {
+				suite.Require().Error(err)
+			}
+
+			suite.Require().Equal(tc.expGas, gas)
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestGasToRefund() {
+	testCases := []struct {
+		name           string
+		gasconsumed    uint64
+		refundQuotient uint64
+		expGasRefund   uint64
+		expPanic       bool
+	}{
+		{
+			"gas refund 5",
+			5,
+			1,
+			5,
+			false,
+		},
+		{
+			"gas refund 10",
+			10,
+			1,
+			10,
+			false,
+		},
+		{
+			"gas refund availableRefund",
+			11,
+			1,
+			10,
+			false,
+		},
+		{
+			"gas refund quotient 0",
+			11,
+			0,
+			0,
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
+			suite.mintFeeCollector = true
+			suite.SetupTest() // reset
+			suite.app.EvmKeeper.AddRefund(10)
+
+			if tc.expPanic {
+				panicF := func() {
+					suite.app.EvmKeeper.GasToRefund(tc.gasconsumed, tc.refundQuotient)
+				}
+				suite.Require().Panics(panicF)
+			} else {
+				gr := suite.app.EvmKeeper.GasToRefund(tc.gasconsumed, tc.refundQuotient)
+				suite.Require().Equal(tc.expGasRefund, gr)
+			}
+		})
+	}
+	suite.mintFeeCollector = false
+}
+
+func (suite *KeeperTestSuite) TestRefundGas() {
+	testCases := []struct {
+		name           string
+		leftoverGas    uint64
+		refundQuotient uint64
+		noError        bool
+		expGasRefund   uint64
+	}{
+		{
+			"leftoverGas more than tx gas limit",
+			params.TxGas + 1,
+			params.RefundQuotient,
+			false,
+			params.TxGas + 1,
+		},
+		{
+			"leftoverGas equal to tx gas limit, insufficient fee collector account",
+			params.TxGas,
+			params.RefundQuotient,
+			false,
+			params.TxGas,
+		},
+		{
+			"leftoverGas less than to tx gas limit",
+			params.TxGas - 1,
+			params.RefundQuotient,
+			true,
+			params.TxGas - 1,
+		},
+		{
+			"no leftoverGas, refund half used gas ",
+			0,
+			params.RefundQuotient,
+			true,
+			params.TxGas / params.RefundQuotient,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
+			suite.mintFeeCollector = true
+			suite.SetupTest() // reset
+
+			keeperParams := suite.app.EvmKeeper.GetParams(suite.ctx)
+			ethCfg := keeperParams.ChainConfig.EthereumConfig(suite.app.EvmKeeper.ChainID())
+			signer := ethtypes.LatestSignerForChainID(suite.app.EvmKeeper.ChainID())
+
+			m, err := newNativeMessage(
+				suite.app.EvmKeeper.GetNonce(suite.address),
+				suite.ctx.BlockHeight(),
+				suite.address,
+				ethCfg,
+				suite.signer,
+				signer,
+				ethtypes.AccessListTxType,
+				nil,
+				nil,
+			)
+			suite.Require().NoError(err)
+
+			suite.app.EvmKeeper.AddRefund(params.TxGas)
+
+			gr, err := suite.app.EvmKeeper.RefundGas(m, tc.leftoverGas, tc.refundQuotient)
+			if tc.noError {
+				suite.Require().NoError(err)
+			} else {
+				suite.Require().Error(err)
+			}
+
+			suite.Require().Equal(tc.expGasRefund, gr)
+		})
+	}
+	suite.mintFeeCollector = false
+}
+
+func (suite *KeeperTestSuite) TestResetGasMeterAndConsumeGas() {
+	testCases := []struct {
+		name        string
+		gasConsumed uint64
+		gasUsed     uint64
+		expPanic    bool
+	}{
+		{
+			"gas consumed 5, used 5",
+			5,
+			5,
+			false,
+		},
+		{
+			"gas consumed 5, used 10",
+			5,
+			10,
+			false,
+		},
+		{
+			"gas consumed 10, used 10",
+			10,
+			10,
+			false,
+		},
+		{
+			"gas consumed 11, used 10, NegativeGasConsumed panic",
+			11,
+			10,
+			true,
+		},
+		{
+			"gas consumed 1, used 10, overflow panic",
+			1,
+			math.MaxUint64,
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
+			suite.SetupTest() // reset
+
+			panicF := func() {
+				gm := sdk.NewGasMeter(10)
+				gm.ConsumeGas(tc.gasConsumed, "")
+				suite.ctx = suite.ctx.WithGasMeter(gm)
+				suite.app.EvmKeeper.WithContext(suite.ctx)
+
+				suite.app.EvmKeeper.ResetGasMeterAndConsumeGas(tc.gasUsed)
+			}
+
+			if tc.expPanic {
+				suite.Require().Panics(panicF)
+			} else {
+				suite.Require().NotPanics(panicF)
+			}
+		})
+	}
+}