From 948b39c6c7e19bfdec27e2083af226a5d765de78 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 16 Nov 2021 15:18:52 +0800 Subject: [PATCH 1/3] fix gas consumption when reverted Apply suggestions from code review changelog --- CHANGELOG.md | 1 + x/evm/handler_test.go | 81 ++++++++++++++++++++++++++++++++ x/evm/keeper/state_transition.go | 15 +++--- 3 files changed, 91 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bd6a7641e..cae282fc04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (evm, ante) [tharsis#620](https://github.com/tharsis/ethermint/pull/620) Add fee market field to EVM `Keeper` and `AnteHandler`. * (all) [tharsis#231](https://github.com/tharsis/ethermint/pull/231) Bump go-ethereum version to [`v1.10.9`](https://github.com/ethereum/go-ethereum/releases/tag/v1.10.9) * (ante) [tharsis#703](https://github.com/tharsis/ethermint/pull/703) Fix some fields in transaction are not authenticated by signature. +* (evm) [tharsis#751](https://github.com/tharsis/ethermint/pull/751) don't revert gas refund logic when transaction reverted ### Features diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index 5982077037..011a66b57f 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -525,3 +525,84 @@ func (suite *EvmTestSuite) TestErrorWhenDeployContract() { // TODO: snapshot checking } + +func (suite *EvmTestSuite) deployTestRevertContract() common.Address { + k := suite.app.EvmKeeper + nonce := k.GetNonce(suite.from) + ctorArgs, err := types.ERC20Contract.ABI.Pack("", suite.from, big.NewInt(0)) + suite.Require().NoError(err) + msg := ethtypes.NewMessage( + suite.from, + nil, + nonce, + big.NewInt(0), + 2000000, + big.NewInt(1), + nil, + nil, + append(types.ERC20Contract.Bin, ctorArgs...), + nil, + true, + ) + rsp, err := k.ApplyMessage(msg, nil, true) + suite.Require().NoError(err) + suite.Require().False(rsp.Failed()) + return crypto.CreateAddress(suite.from, nonce) +} + +func (suite *EvmTestSuite) TestGasRefundWhenReverted() { + suite.SetupTest() + k := suite.app.EvmKeeper + + // the bug only reproduce when there are hooks + k.SetHooks(&DummyHook{}) + + // add some fund to pay gas fee + k.AddBalance(suite.from, big.NewInt(10000000000)) + + contract := suite.deployTestRevertContract() + + // the call will fail because no balance + data, err := types.ERC20Contract.ABI.Pack("transfer", common.BigToAddress(big.NewInt(1)), big.NewInt(10)) + suite.Require().NoError(err) + + tx := types.NewTx( + suite.chainID, + k.GetNonce(suite.from), + &contract, + big.NewInt(0), + 41000, + big.NewInt(1), + nil, + nil, + data, + nil, + ) + tx.From = suite.from.String() + err = tx.Sign(suite.ethSigner, suite.signer) + suite.Require().NoError(err) + + before := k.GetBalance(suite.from) + + txData, err := types.UnpackTxData(tx.Data) + suite.Require().NoError(err) + _, err = k.DeductTxCostsFromUserBalance(suite.ctx, *tx, txData, "aphoton", true, true, true) + suite.Require().NoError(err) + + res, err := k.EthereumTx(sdk.WrapSDKContext(suite.ctx), tx) + suite.Require().NoError(err) + suite.Require().True(res.Failed()) + + after := k.GetBalance(suite.from) + + suite.Require().Equal(uint64(21861), res.GasUsed) + // check gas refund works + suite.Require().Equal(big.NewInt(21861), new(big.Int).Sub(before, after)) +} + +// DummyHook implements EvmHooks interface +type DummyHook struct{} + +func (dh *DummyHook) PostTxProcessing(ctx sdk.Context, txHash common.Hash, logs []*ethtypes.Log) error { + return nil +} diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index cd05a1d9d4..1c603782c6 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -218,12 +218,6 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT return nil, stacktrace.Propagate(err, "failed to apply ethereum core message") } - // refund gas prior to handling the vm error in order to match the Ethereum gas consumption instead of the default SDK one. - err = k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom) - if err != nil { - return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From()) - } - res.Hash = txHash.Hex() logs := k.GetTxLogsTransient(txHash) @@ -241,6 +235,15 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT } } + // change to original context + k.WithContext(ctx) + + // refund gas according to Ethereum gas accounting rules. + err = k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom) + if err != nil { + return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From()) + } + if len(logs) > 0 { res.Logs = types.NewLogsFromEth(logs) // Update transient block bloom filter From ed45559cdc95acabaf55fde04cd3a6f4b1ee9e12 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 16 Nov 2021 21:04:52 +0800 Subject: [PATCH 2/3] comments --- x/evm/handler_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index 011a66b57f..a7d6264bb9 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -526,7 +526,7 @@ func (suite *EvmTestSuite) TestErrorWhenDeployContract() { // TODO: snapshot checking } -func (suite *EvmTestSuite) deployTestRevertContract() common.Address { +func (suite *EvmTestSuite) deployERC20Contract() common.Address { k := suite.app.EvmKeeper nonce := k.GetNonce(suite.from) ctorArgs, err := types.ERC20Contract.ABI.Pack("", suite.from, big.NewInt(0)) @@ -550,6 +550,7 @@ func (suite *EvmTestSuite) deployTestRevertContract() common.Address { return crypto.CreateAddress(suite.from, nonce) } +// TestGasRefundWhenReverted check that when transaction reverted, gas refund should still work. func (suite *EvmTestSuite) TestGasRefundWhenReverted() { suite.SetupTest() k := suite.app.EvmKeeper @@ -560,7 +561,7 @@ func (suite *EvmTestSuite) TestGasRefundWhenReverted() { // add some fund to pay gas fee k.AddBalance(suite.from, big.NewInt(10000000000)) - contract := suite.deployTestRevertContract() + contract := suite.deployERC20Contract() // the call will fail because no balance data, err := types.ERC20Contract.ABI.Pack("transfer", common.BigToAddress(big.NewInt(1)), big.NewInt(10)) From ff7373ad729ee62b0b4d7d1430d9ff6428843e98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Tue, 16 Nov 2021 15:25:39 +0100 Subject: [PATCH 3/3] Update x/evm/keeper/state_transition.go --- x/evm/keeper/state_transition.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index 1c603782c6..1b3b0d20c1 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -239,8 +239,7 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT k.WithContext(ctx) // refund gas according to Ethereum gas accounting rules. - err = k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom) - if err != nil { + if err := k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom); err != nil { return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From()) }