imp(ante): refactor AnteHandler (evmos#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 <[email protected]> * fix build * fix integration test script Co-authored-by: Vladislav Varadinov <[email protected]> Co-authored-by: Freddy Caceres <[email protected]>
mmsqe · Nov 18, 2022 · feed84b · feed84b
1 parent 37e394f
commit feed84b
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 55 deletions.
diff --git 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
@@ -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 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))
-				}
-
-				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)
-				}
-			}
+	if !ctx.IsCheckTx() || simulate {
+		return next(ctx, tx, simulate)
+	}
+	chainCfg := mfd.evmKeeper.GetChainConfig(ctx)
+	ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID())
+
+	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)
+	}
+
+	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
@@ -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"