fix: don't revert gas refund logic when transaction reverted (#751)

* fix gas consumption when reverted

Apply suggestions from code review

changelog

* comments

* Update x/evm/keeper/state_transition.go

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

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
 

x/evm/handler_test.go

@@ -525,3 +525,85 @@ func (suite *EvmTestSuite) TestErrorWhenDeployContract() {
 
  // TODO: snapshot checking
 }
+
+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))
+ 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)
+}
+
+// TestGasRefundWhenReverted check that when transaction reverted, gas refund should still work.
+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.deployERC20Contract()
+
+ // 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
+}

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,14 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
  }
  }
 
+ // change to original context
+ k.WithContext(ctx)
+
+ // refund gas according to Ethereum gas accounting rules.
+ 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())
+ }
+
  if len(logs) > 0 {
  res.Logs = types.NewLogsFromEth(logs)
  // Update transient block bloom filter