From 77d9e299235fac98c07f211bb808de2be53978ab Mon Sep 17 00:00:00 2001 From: Loredana Cirstea Date: Sun, 6 Mar 2022 16:09:28 +0200 Subject: [PATCH] fix: GasMeter reset in AnteHandler `EthGasConsumeDecorator` (#964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix GasMeter reset in AnteHandler - EthGasConsumeDecorator Conforming to the spec: https://github.com/cosmos/cosmos-sdk/blob/e7066c4271ff3d33dc426dc6313c82a1201ae3c6/docs/basics/gas-fees.md > Set newCtx.GasMeter to 0, with a limit of GasWanted. > This step is extremely important, as it not only makes sure the transaction cannot consume infinite gas, > but also that ctx.GasMeter is reset in-between each DeliverTx > (ctx is set to newCtx after anteHandler is run, and the anteHandler is run each time DeliverTx is called). * Compute gasWanted in ante handler based on msg gas * Tests - check gas meter limit after EthGasConsumeDecorator ante handler runs * Update CHANGELOG.md * EthGasConsumeDecorator ante handler resets the gas meter only for CheckTx * Reset the gas meter in Keeper.EthereumTx to an infinite gas meter * Fix TestOutOfGasWhenDeployContract error check * Move gas meter reset to the innermost EthAnteHandle * add NewInfiniteGasMeterWithLimit for setting the user provided gas limit Fixes block's consumed gas calculation in the block creation phase. * Fix lint * Fix lint Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> --- CHANGELOG.md | 7 +-- app/ante/eth.go | 8 +++ app/ante/eth_test.go | 18 +++++-- types/gasmeter.go | 91 ++++++++++++++++++++++++++++++++ x/evm/keeper/state_transition.go | 2 +- 5 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 types/gasmeter.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 19e51ec4..f071ece4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,9 +40,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* (rpc) [\#529](https://github.com/tharsis/ethermint/pull/975) Fix unexpected `nil` values for `reward`, returned by `EffectiveGasTipValue(blockBaseFee)` in the `eth_feeHistory` RPC method. -* (evm) [\#529](https://github.com/tharsis/ethermint/issues/529) Add support return value on trace tx response. -* (rpc) [#970] (https://github.com/tharsis/ethermint/pull/970) Fix unexpected nil reward values on `eth_feeHistory` response +* (ante) [tharsis#964](https://github.com/tharsis/ethermint/pull/964) add NewInfiniteGasMeterWithLimit for storing the user provided gas limit. Fixes block's consumed gas calculation in the block creation phase. +* (rpc) [tharsis#975](https://github.com/tharsis/ethermint/pull/975) Fix unexpected `nil` values for `reward`, returned by `EffectiveGasTipValue(blockBaseFee)` in the `eth_feeHistory` RPC method. +* (rpc) [tharsis#970] (https://github.com/tharsis/ethermint/pull/970) Fix unexpected nil reward values on `eth_feeHistory` response +* (evm) [tharsis#529](https://github.com/tharsis/ethermint/issues/529) support return value on trace tx response. ## Improvements diff --git a/app/ante/eth.go b/app/ante/eth.go index 4255fb98..0908377c 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -159,6 +159,7 @@ func NewEthGasConsumeDecorator( // - transaction's gas limit is lower than the intrinsic gas // - user doesn't have enough balance to deduct the transaction fees (gas_limit * gas_price) // - transaction or block gas meter runs out of gas +// - sets the gas meter limit func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { params := egcd.evmKeeper.GetParams(ctx) @@ -169,6 +170,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula istanbul := ethCfg.IsIstanbul(blockHeight) london := ethCfg.IsLondon(blockHeight) evmDenom := params.EvmDenom + gasWanted := uint64(0) var events sdk.Events @@ -182,6 +184,7 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula if err != nil { return ctx, sdkerrors.Wrap(err, "failed to unpack tx data") } + gasWanted += txData.GetGas() fees, err := egcd.evmKeeper.DeductTxCostsFromUserBalance( ctx, @@ -213,6 +216,11 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula gasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "gas pool check") } + // Set ctx.GasMeter with a limit of GasWanted (gasLimit) + gasConsumed := ctx.GasMeter().GasConsumed() + ctx = ctx.WithGasMeter(ethermint.NewInfiniteGasMeterWithLimit(gasWanted)) + ctx.GasMeter().ConsumeGas(gasConsumed, "copy gas consumed") + // we know that we have enough gas on the pool to cover the intrinsic gas return next(ctx, tx, simulate) } diff --git a/app/ante/eth_test.go b/app/ante/eth_test.go index 80f5455b..3490ba69 100644 --- a/app/ante/eth_test.go +++ b/app/ante/eth_test.go @@ -205,10 +205,12 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { addr := tests.GenerateAddress() - tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil) + txGasLimit := uint64(1000) + tx := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), txGasLimit, big.NewInt(1), nil, nil, nil, nil) tx.From = addr.Hex() - tx2 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000000, big.NewInt(1), nil, nil, nil, ðtypes.AccessList{{Address: addr, StorageKeys: nil}}) + tx2GasLimit := uint64(1000000) + tx2 := evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), tx2GasLimit, big.NewInt(1), nil, nil, nil, ðtypes.AccessList{{Address: addr, StorageKeys: nil}}) tx2.From = addr.Hex() var vmdb *statedb.StateDB @@ -216,32 +218,37 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { testCases := []struct { name string tx sdk.Tx + gasLimit uint64 malleate func() expPass bool expPanic bool }{ - {"invalid transaction type", &invalidTx{}, func() {}, false, false}, + {"invalid transaction type", &invalidTx{}, 0, func() {}, false, false}, { "sender not found", evmtypes.NewTxContract(suite.app.EvmKeeper.ChainID(), 1, big.NewInt(10), 1000, big.NewInt(1), nil, nil, nil, nil), + 0, func() {}, false, false, }, { "gas limit too low", tx, + 0, func() {}, false, false, }, { "not enough balance for fees", tx2, + 0, func() {}, false, false, }, { "not enough tx gas", tx2, + 0, func() { vmdb.AddBalance(addr, big.NewInt(1000000)) }, @@ -250,6 +257,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { { "not enough block gas", tx2, + 0, func() { vmdb.AddBalance(addr, big.NewInt(1000000)) @@ -260,6 +268,7 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { { "success", tx2, + tx2GasLimit, func() { vmdb.AddBalance(addr, big.NewInt(1000000)) @@ -282,12 +291,13 @@ func (suite AnteTestSuite) TestEthGasConsumeDecorator() { return } - _, err := dec.AnteHandle(suite.ctx.WithIsCheckTx(true), tc.tx, false, nextFn) + ctx, err := dec.AnteHandle(suite.ctx.WithIsCheckTx(true).WithGasMeter(sdk.NewInfiniteGasMeter()), tc.tx, false, nextFn) if tc.expPass { suite.Require().NoError(err) } else { suite.Require().Error(err) } + suite.Require().Equal(tc.gasLimit, ctx.GasMeter().Limit()) }) } } diff --git a/types/gasmeter.go b/types/gasmeter.go new file mode 100644 index 00000000..2c6d2d18 --- /dev/null +++ b/types/gasmeter.go @@ -0,0 +1,91 @@ +package types + +import ( + fmt "fmt" + math "math" + + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// ErrorNegativeGasConsumed defines an error thrown when the amount of gas refunded results in a +// negative gas consumed amount. +// Copied from cosmos-sdk +type ErrorNegativeGasConsumed struct { + Descriptor string +} + +// ErrorGasOverflow defines an error thrown when an action results gas consumption +// unsigned integer overflow. +type ErrorGasOverflow struct { + Descriptor string +} + +type infiniteGasMeterWithLimit struct { + consumed sdk.Gas + limit sdk.Gas +} + +// NewInfiniteGasMeterWithLimit returns a reference to a new infiniteGasMeter. +func NewInfiniteGasMeterWithLimit(limit sdk.Gas) sdk.GasMeter { + return &infiniteGasMeterWithLimit{ + consumed: 0, + limit: limit, + } +} + +func (g *infiniteGasMeterWithLimit) GasConsumed() sdk.Gas { + return g.consumed +} + +func (g *infiniteGasMeterWithLimit) GasConsumedToLimit() sdk.Gas { + return g.consumed +} + +func (g *infiniteGasMeterWithLimit) Limit() sdk.Gas { + return g.limit +} + +// addUint64Overflow performs the addition operation on two uint64 integers and +// returns a boolean on whether or not the result overflows. +func addUint64Overflow(a, b uint64) (uint64, bool) { + if math.MaxUint64-a < b { + return 0, true + } + + return a + b, false +} + +func (g *infiniteGasMeterWithLimit) ConsumeGas(amount sdk.Gas, descriptor string) { + var overflow bool + // TODO: Should we set the consumed field after overflow checking? + g.consumed, overflow = addUint64Overflow(g.consumed, amount) + if overflow { + panic(ErrorGasOverflow{descriptor}) + } +} + +// RefundGas will deduct the given amount from the gas consumed. If the amount is greater than the +// gas consumed, the function will panic. +// +// Use case: This functionality enables refunding gas to the trasaction or block gas pools so that +// EVM-compatible chains can fully support the go-ethereum StateDb interface. +// See https://github.com/cosmos/cosmos-sdk/pull/9403 for reference. +func (g *infiniteGasMeterWithLimit) RefundGas(amount sdk.Gas, descriptor string) { + if g.consumed < amount { + panic(ErrorNegativeGasConsumed{Descriptor: descriptor}) + } + + g.consumed -= amount +} + +func (g *infiniteGasMeterWithLimit) IsPastLimit() bool { + return false +} + +func (g *infiniteGasMeterWithLimit) IsOutOfGas() bool { + return false +} + +func (g *infiniteGasMeterWithLimit) String() string { + return fmt.Sprintf("InfiniteGasMeter:\n consumed: %d", g.consumed) +} diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index 39014b01..fb9a082e 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -369,7 +369,7 @@ func (k *Keeper) ApplyMessageWithConfig(ctx sdk.Context, msg core.Message, trace } leftoverGas := msg.Gas() - intrinsicGas - // access list preparaion is moved from ante handler to here, because it's needed when `ApplyMessage` is called + // access list preparation is moved from ante handler to here, because it's needed when `ApplyMessage` is called // under contexts where ante handlers are not run, for example `eth_call` and `eth_estimateGas`. if rules := cfg.ChainConfig.Rules(big.NewInt(ctx.BlockHeight()), cfg.ChainConfig.MergeForkBlock != nil); rules.IsBerlin { stateDB.PrepareAccessList(msg.From(), msg.To(), vm.ActivePrecompiles(rules), msg.AccessList())