From 9eb433344956633d2dadacbc91fdec8ede1e8428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 08:33:26 +0100 Subject: [PATCH 01/12] fix(ante): block gas check --- app/ante/eth.go | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 60a14a8c54..4a899c0e18 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -230,18 +230,25 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula } } - // 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") + gasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "block gas pool check") + + // return error if the tx gas is greater than the block limit (max gas) + if gasPool.IsOutOfGas() { + return ctx, sdkerrors.Wrapf( + sdkerrors.ErrOutOfGas, + "tx gas (%d) exceeds block gas limit (%d)", + ctx.GasMeter().GasConsumed(), + blockGasLimit, + ) + } } // Set ctx.GasMeter with a limit of GasWanted (gasLimit) @@ -291,6 +298,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, @@ -311,22 +334,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) From fa111d92203fbd75078fe769dce1958cb96ae123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 08:52:28 +0100 Subject: [PATCH 02/12] refactor --- app/ante/eth.go | 74 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 4a899c0e18..e518109341 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -164,7 +164,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,6 +175,12 @@ func NewEthGasConsumeDecorator( // - 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) (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()) @@ -224,7 +230,13 @@ 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 } @@ -548,31 +560,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) + + 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 := ctx.MinGasPrices().AmountOf(evmDenom).Mul(gasLimit) + + if fee.LT(requiredFee) { + return ctx, sdkerrors.Wrapf( + sdkerrors.ErrInsufficientFee, + "insufficient fees; got: %s required: %s", + fee, requiredFee, + ) } } From 763f4819a75b8a06985b67439298905fa43ace6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 09:12:31 +0100 Subject: [PATCH 03/12] rename --- app/ante/eth.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index e518109341..5365d47da0 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -249,11 +249,11 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula // 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 - gasPool := sdk.NewGasMeter(blockGasLimit) - gasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "block gas pool check") + blockGasPool := sdk.NewGasMeter(blockGasLimit) + blockGasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "block gas pool check") // return error if the tx gas is greater than the block limit (max gas) - if gasPool.IsOutOfGas() { + if blockGasPool.IsOutOfGas() { return ctx, sdkerrors.Wrapf( sdkerrors.ErrOutOfGas, "tx gas (%d) exceeds block gas limit (%d)", From 73a34cfd6a75506a7d01b86d32fee5dacf3848ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 15:24:35 +0100 Subject: [PATCH 04/12] use gas wanted --- app/ante/eth.go | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 5365d47da0..9d2141b93b 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -246,21 +246,14 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula 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 - blockGasPool := sdk.NewGasMeter(blockGasLimit) - blockGasPool.ConsumeGas(ctx.GasMeter().GasConsumedToLimit(), "block gas pool check") - - // return error if the tx gas is greater than the block limit (max gas) - if blockGasPool.IsOutOfGas() { - return ctx, sdkerrors.Wrapf( - sdkerrors.ErrOutOfGas, - "tx gas (%d) exceeds block gas limit (%d)", - ctx.GasMeter().GasConsumed(), - blockGasLimit, - ) - } + // return error if the tx gas is greater than the block limit (max gas) + 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) From 5716842092d73a0ba9e92f950e65d25d78ea8ccf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 16:58:12 +0100 Subject: [PATCH 05/12] remove consume gas logic on ante handler --- app/ante/eth.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 9d2141b93b..2d6bb25d57 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -257,11 +257,9 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula } // 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") - - newCtx := ctx.WithPriority(minPriority) + 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) From 3b4939eef6e8f4e7fd042ae9b452fc389a45d235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:40:55 +0100 Subject: [PATCH 06/12] comment --- app/ante/eth.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 2d6bb25d57..cf9be36e59 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -247,6 +247,10 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula blockGasLimit := ethermint.BlockGasLimit(ctx) // 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, @@ -256,7 +260,12 @@ func (egcd EthGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula ) } - // 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). + // The gas consumed will be then reset to the gas used by the state transition + // in the EVM. + + // 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) From a8d4988980f7ba50e8d9ef53c45dd58f01d15fcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Mon, 14 Nov 2022 15:51:40 +0100 Subject: [PATCH 07/12] c++ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e4c29217a..211e203f3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,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 From acf50f4c831a8ea6373b32a207b9ab1c671e0d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Mon, 14 Nov 2022 15:53:21 +0100 Subject: [PATCH 08/12] move min gas price --- app/ante/eth.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index cf9be36e59..90089b8f60 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -578,6 +578,7 @@ func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulat } evmDenom := mfd.evmKeeper.GetEVMDenom(ctx) + minGasPrice := ctx.MinGasPrices().AmountOf(evmDenom) for _, msg := range tx.GetMsgs() { ethMsg, ok := msg.(*evmtypes.MsgEthereumTx) @@ -587,7 +588,7 @@ func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulat fee := sdk.NewDecFromBigInt(ethMsg.GetFee()) gasLimit := sdk.NewDecFromBigInt(new(big.Int).SetUint64(ethMsg.GetGas())) - requiredFee := ctx.MinGasPrices().AmountOf(evmDenom).Mul(gasLimit) + requiredFee := minGasPrice.Mul(gasLimit) if fee.LT(requiredFee) { return ctx, sdkerrors.Wrapf( From aeedad14a48318edeb35bc6bb25cec5f0337e9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Mon, 14 Nov 2022 15:54:27 +0100 Subject: [PATCH 09/12] comment --- app/ante/eth.go | 1 + 1 file changed, 1 insertion(+) diff --git a/app/ante/eth.go b/app/ante/eth.go index 90089b8f60..35e97e05cf 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -174,6 +174,7 @@ 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 From 9ef992046ef67f375acc1c59857bea7324bbc0cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Tue, 15 Nov 2022 13:24:04 +0100 Subject: [PATCH 10/12] Update app/ante/eth.go Co-authored-by: Vladislav Varadinov --- app/ante/eth.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 35e97e05cf..4e904c1e66 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -569,14 +569,13 @@ func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulat 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) } + chainCfg := mfd.evmKeeper.GetChainConfig(ctx) + ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID()) evmDenom := mfd.evmKeeper.GetEVMDenom(ctx) minGasPrice := ctx.MinGasPrices().AmountOf(evmDenom) From f9aec75dd0da9bb0a55263661db0ee0b1391504b Mon Sep 17 00:00:00 2001 From: Freddy Caceres Date: Tue, 15 Nov 2022 15:43:04 -0500 Subject: [PATCH 11/12] fix build --- app/ante/eth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/ante/eth.go b/app/ante/eth.go index 4e904c1e66..d94c24def4 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -568,14 +568,14 @@ func (mfd EthMempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulat 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) } - chainCfg := mfd.evmKeeper.GetChainConfig(ctx) - ethCfg := chainCfg.EthereumConfig(mfd.evmKeeper.ChainID()) evmDenom := mfd.evmKeeper.GetEVMDenom(ctx) minGasPrice := ctx.MinGasPrices().AmountOf(evmDenom) From d8c7e39afd63c73493e0cb33631d2173fe4d5d2a Mon Sep 17 00:00:00 2001 From: Freddy Caceres Date: Tue, 15 Nov 2022 18:49:20 -0500 Subject: [PATCH 12/12] fix integration test script --- scripts/integration-test-all.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/integration-test-all.sh b/scripts/integration-test-all.sh index 31450eb2bb..15d1e17af4 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"