From 40b3b9ae1b52def527cf3a155139ed359cc197af Mon Sep 17 00:00:00 2001 From: JayT106 Date: Wed, 20 Oct 2021 15:09:06 -0400 Subject: [PATCH] evm: unit tests for gas refund (#686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 38 +- x/evm/keeper/state_transition.go | 16 +- .../keeper/state_transition_benchmark_test.go | 25 ++ x/evm/keeper/state_transition_test.go | 336 +++++++++++++++++- 4 files changed, 396 insertions(+), 19 deletions(-) diff --git a/x/evm/keeper/keeper_test.go b/x/evm/keeper/keeper_test.go index 980daa02..5f881f34 100644 --- a/x/evm/keeper/keeper_test.go +++ b/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" @@ -57,7 +61,8 @@ type KeeperTestSuite struct { appCodec codec.Codec signer keyring.Signer - dynamicTxFee bool + 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", diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index faaffa25..69aa9c4f 100644 --- a/x/evm/keeper/state_transition.go +++ b/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") diff --git a/x/evm/keeper/state_transition_benchmark_test.go b/x/evm/keeper/state_transition_benchmark_test.go index 1d2deb93..d3c1dcce 100644 --- a/x/evm/keeper/state_transition_benchmark_test.go +++ b/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) diff --git a/x/evm/keeper/state_transition_test.go b/x/evm/keeper/state_transition_test.go index 258d9dbd..dc791e36 100644 --- a/x/evm/keeper/state_transition_test.go +++ b/x/evm/keeper/state_transition_test.go @@ -2,17 +2,18 @@ package keeper_test import ( "fmt" - - "github.com/ethereum/go-ethereum/common" - - "github.com/tendermint/tendermint/crypto/tmhash" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - tmtypes "github.com/tendermint/tendermint/types" + "math" + "math/big" 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) + } + }) + } +}