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 <vladislav.varadinov@gmail.com>

* fix build

* fix integration test script

Co-authored-by: Vladislav Varadinov <vladislav.varadinov@gmail.com>
Co-authored-by: Freddy Caceres <facs95@gmail.com>

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

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
+	// return error if the tx gas is greater than the block limit (max gas)
-	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
+	// NOTE: it's important here to use the gas wanted instead of the gas consumed
-		// if current gas consumed is greater than the limit, this funcion panics and the error is recovered on the Baseapp
+	// from the tx gas pool. The later only has the value so far since the
-		gasPool := sdk.NewGasMeter(blockGasLimit)
+	// EthSetupContextDecorator so it will never exceed the block gas limit.
-		gasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "gas pool check")
+	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)
+	// Set tx GasMeter with a limit of GasWanted (i.e gas limit from the Ethereum tx).
-	gasConsumed := ctx.GasMeter().GasConsumed()
+	// The gas consumed will be then reset to the gas used by the state transition
-	ctx = ctx.WithGasMeter(ethermint.NewInfiniteGasMeterWithLimit(gasWanted))
+	// in the EVM.
-	ctx.GasMeter().ConsumeGas(gasConsumed, "copy gas consumed")
 
-	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,
+// 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
+// This check only for local mempool purposes, and thus it is only run on (Re)CheckTx.
-// is only ran on check tx.
+// The logic is also skipped if the London hard fork and EIP-1559 are enabled.
-// It only do the check if london hardfork not enabled or feemarket not enabled, because in that case feemarket will take over the task.
 func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
-	if ctx.IsCheckTx() && !simulate {
+	if !ctx.IsCheckTx() || simulate {
-		chainCfg := mfd.evmKeeper.GetChainConfig(ctx)
+		return next(ctx, tx, simulate)
-		ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID())
+	}
-		baseFee := mfd.evmKeeper.GetBaseFee(ctx, ethCfg)
+	chainCfg := mfd.evmKeeper.GetChainConfig(ctx)
-		evmDenom := mfd.evmKeeper.GetEVMDenom(ctx)
+	ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID())
 
-		if baseFee == nil {
+	baseFee := mfd.evmKeeper.GetBaseFee(ctx, ethCfg)
-			for _, msg := range tx.GetMsgs() {
+	// skip check as the London hard fork and EIP-1559 are enabled
-				ethMsg, ok := msg.(*evmtypes.MsgEthereumTx)
+	if baseFee != nil {
-				if !ok {
+		return next(ctx, tx, simulate)
-					return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil))
+	}
-				}
 
-				feeAmt := ethMsg.GetFee()
+	evmDenom := mfd.evmKeeper.GetEVMDenom(ctx)
-				glDec := sdk.NewDec(int64(ethMsg.GetGas()))
+	minGasPrice := ctx.MinGasPrices().AmountOf(evmDenom)
-				requiredFee := ctx.MinGasPrices().AmountOf(evmDenom).Mul(glDec)
+
-				if sdk.NewDecFromBigInt(feeAmt).LT(requiredFee) {
+	for _, msg := range tx.GetMsgs() {
-					return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeAmt, requiredFee)
+		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,
+			)
 		}
 	}
 

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"