From feed84b09cdc46820faddbababf3f69ce881789a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Fri, 18 Nov 2022 22:41:16 +0100 Subject: [PATCH] imp(ante): refactor `AnteHandler` (#1455) * fix(ante): block gas check * refactor * rename * use gas wanted * remove consume gas logic on ante handler * comment * c++ * move min gas price * comment * Update app/ante/eth.go Co-authored-by: Vladislav Varadinov * fix build * fix integration test script Co-authored-by: Vladislav Varadinov Co-authored-by: Freddy Caceres --- CHANGELOG.md | 1 + app/ante/eth.go | 136 +++++++++++++++++++------------- scripts/integration-test-all.sh | 3 + 3 files changed, 87 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e31180f..38d6a1c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (ante) [#1455](https://github.com/evmos/ethermint/pull/1455) Refactor `AnteHandler` logic * (evm) [#1444](https://github.com/evmos/ethermint/pull/1444) Improve performance of `eth_estimateGas` * (ante) [\#1388](https://github.com/evmos/ethermint/pull/1388) Optimize AnteHandler gas consumption * (lint) [#1298](https://github.com/evmos/ethermint/pull/1298) 150 character line length limit, `gofumpt`, and linting diff --git a/app/ante/eth.go b/app/ante/eth.go index 0c59e234..9f495596 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -165,7 +165,7 @@ func NewEthGasConsumeDecorator( // (during CheckTx only) and that the sender has enough balance to pay for the gas cost. // // Intrinsic gas for a transaction is the amount of gas that the transaction uses before the -// transaction is executed. The gas is a constant value plus any cost inccured by additional bytes +// transaction is executed. The gas is a constant value plus any cost incurred by additional bytes // of data supplied with the transaction. // // This AnteHandler decorator will fail if: @@ -175,7 +175,14 @@ func NewEthGasConsumeDecorator( // - 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 +// - gas limit is greater than the block gas meter limit func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // gas consumption limit already checked during CheckTx so there's no need to + // verify it again during ReCheckTx + if ctx.IsReCheckTx() { + return next(ctx, tx, simulate) + } + chainCfg := egcd.evmKeeper.GetChainConfig(ctx) ethCfg := chainCfg.EthereumConfig(egcd.evmKeeper.ChainID()) @@ -225,32 +232,45 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula return ctx, sdkerrors.Wrapf(err, "failed to deduct transaction costs from user balance") } - events = append(events, sdk.NewEvent(sdk.EventTypeTx, sdk.NewAttribute(sdk.AttributeKeyFee, fees.String()))) + events = append(events, + sdk.NewEvent( + sdk.EventTypeTx, + sdk.NewAttribute(sdk.AttributeKeyFee, fees.String()), + ), + ) + if priority < minPriority { minPriority = priority } } - // TODO: change to typed events ctx.EventManager().EmitEvents(events) - // TODO: deprecate after https://github.com/cosmos/cosmos-sdk/issues/9514 is fixed on SDK blockGasLimit := ethermint.BlockGasLimit(ctx) - // NOTE: safety check - if blockGasLimit > 0 { - // generate a copy of the gas pool (i.e block gas meter) to see if we've run out of gas for this block - // if current gas consumed is greater than the limit, this funcion panics and the error is recovered on the Baseapp - gasPool := sdk.NewGasMeter(blockGasLimit) - gasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "gas pool check") + // return error if the tx gas is greater than the block limit (max gas) + + // NOTE: it's important here to use the gas wanted instead of the gas consumed + // from the tx gas pool. The later only has the value so far since the + // EthSetupContextDecorator so it will never exceed the block gas limit. + if gasWanted > blockGasLimit { + return ctx, sdkerrors.Wrapf( + sdkerrors.ErrOutOfGas, + "tx gas (%d) exceeds block gas limit (%d)", + gasWanted, + blockGasLimit, + ) } - // 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") + // Set tx GasMeter with a limit of GasWanted (i.e gas limit from the Ethereum tx). + // The gas consumed will be then reset to the gas used by the state transition + // in the EVM. - newCtx := ctx.WithPriority(minPriority) + // FIXME: use a custom gas configuration that doesn't add any additional gas and only + // takes into account the gas consumed at the end of the EVM transaction. + newCtx := ctx. + WithGasMeter(ethermint.NewInfiniteGasMeterWithLimit(gasWanted)). + WithPriority(minPriority) // we know that we have enough gas on the pool to cover the intrinsic gas return next(newCtx, tx, simulate) @@ -292,6 +312,22 @@ func (ctd CanTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate ) } + if evmtypes.IsLondon(ethCfg, ctx.BlockHeight()) { + if baseFee == nil { + return ctx, sdkerrors.Wrap( + evmtypes.ErrInvalidBaseFee, + "base fee is supported but evm block context value is nil", + ) + } + if coreMsg.GasFeeCap().Cmp(baseFee) < 0 { + return ctx, sdkerrors.Wrapf( + sdkerrors.ErrInsufficientFee, + "max fee per gas less than block base fee (%s < %s)", + coreMsg.GasFeeCap(), baseFee, + ) + } + } + // NOTE: pass in an empty coinbase address and nil tracer as we don't need them for the check below cfg := &evmtypes.EVMConfig{ ChainConfig: ethCfg, @@ -312,22 +348,6 @@ func (ctd CanTransferDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate coreMsg.From(), ) } - - if evmtypes.IsLondon(ethCfg, ctx.BlockHeight()) { - if baseFee == nil { - return ctx, sdkerrors.Wrap( - evmtypes.ErrInvalidBaseFee, - "base fee is supported but evm block context value is nil", - ) - } - if coreMsg.GasFeeCap().Cmp(baseFee) < 0 { - return ctx, sdkerrors.Wrapf( - sdkerrors.ErrInsufficientFee, - "max fee per gas less than block base fee (%s < %s)", - coreMsg.GasFeeCap(), baseFee, - ) - } - } } return next(ctx, tx, simulate) @@ -546,31 +566,41 @@ func NewEthMempoolFeeDecorator(ek EVMKeeper) EthMempoolFeeDecorator { } } -// AnteHandle ensures that the provided fees meet a minimum threshold for the validator, -// if this is a CheckTx. This is only for local mempool purposes, and thus -// is only ran on check tx. -// It only do the check if london hardfork not enabled or feemarket not enabled, because in that case feemarket will take over the task. +// AnteHandle ensures that the provided fees meet a minimum threshold for the validator. +// This check only for local mempool purposes, and thus it is only run on (Re)CheckTx. +// The logic is also skipped if the London hard fork and EIP-1559 are enabled. func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { - if ctx.IsCheckTx() && !simulate { - chainCfg := mfd.evmKeeper.GetChainConfig(ctx) - ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID()) - baseFee := mfd.evmKeeper.GetBaseFee(ctx, ethCfg) - evmDenom := mfd.evmKeeper.GetEVMDenom(ctx) + if !ctx.IsCheckTx() || simulate { + return next(ctx, tx, simulate) + } + chainCfg := mfd.evmKeeper.GetChainConfig(ctx) + ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID()) - if baseFee == nil { - for _, msg := range tx.GetMsgs() { - ethMsg, ok := msg.(*evmtypes.MsgEthereumTx) - if !ok { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil)) - } + baseFee := mfd.evmKeeper.GetBaseFee(ctx, ethCfg) + // skip check as the London hard fork and EIP-1559 are enabled + if baseFee != nil { + return next(ctx, tx, simulate) + } - feeAmt := ethMsg.GetFee() - glDec := sdk.NewDec(int64(ethMsg.GetGas())) - requiredFee := ctx.MinGasPrices().AmountOf(evmDenom).Mul(glDec) - if sdk.NewDecFromBigInt(feeAmt).LT(requiredFee) { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeAmt, requiredFee) - } - } + evmDenom := mfd.evmKeeper.GetEVMDenom(ctx) + minGasPrice := ctx.MinGasPrices().AmountOf(evmDenom) + + for _, msg := range tx.GetMsgs() { + ethMsg, ok := msg.(*evmtypes.MsgEthereumTx) + if !ok { + return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil)) + } + + fee := sdk.NewDecFromBigInt(ethMsg.GetFee()) + gasLimit := sdk.NewDecFromBigInt(new(big.Int).SetUint64(ethMsg.GetGas())) + requiredFee := minGasPrice.Mul(gasLimit) + + if fee.LT(requiredFee) { + return ctx, sdkerrors.Wrapf( + sdkerrors.ErrInsufficientFee, + "insufficient fees; got: %s required: %s", + fee, requiredFee, + ) } } diff --git a/scripts/integration-test-all.sh b/scripts/integration-test-all.sh index e924311e..2790ba79 100755 --- a/scripts/integration-test-all.sh +++ b/scripts/integration-test-all.sh @@ -65,12 +65,15 @@ fi echo "compiling ethermint" make build + # PID array declaration arr=() init_func() { "$PWD"/build/ethermintd keys add $KEY"$i" --keyring-backend test --home "$DATA_DIR$i" --no-backup --algo "eth_secp256k1" "$PWD"/build/ethermintd init $MONIKER --chain-id $CHAINID --home "$DATA_DIR$i" + # Set gas limit in genesis + cat $DATA_DIR$i/config/genesis.json | jq '.consensus_params["block"]["max_gas"]="10000000"' > $DATA_DIR$i/config/tmp_genesis.json && mv $DATA_DIR$i/config/tmp_genesis.json $DATA_DIR$i/config/genesis.json "$PWD"/build/ethermintd add-genesis-account \ "$("$PWD"/build/ethermintd keys show "$KEY$i" --keyring-backend test -a --home "$DATA_DIR$i")" 1000000000000000000aphoton,1000000000000000000stake \ --keyring-backend test --home "$DATA_DIR$i"