From 48849fe49d914aa3b274f100bd26038bbc2e8a22 Mon Sep 17 00:00:00 2001 From: mmsqe Date: Fri, 16 Aug 2024 09:34:01 +0800 Subject: [PATCH 1/2] Problem: unnecessary GetBalance in ante handlers reuse balance after verify account balance to avoid get again when check can transfer --- CHANGELOG.md | 1 + app/ante/eth.go | 49 +++++++++++++++++++++++-------------- app/ante/handler_options.go | 5 ++-- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7980fc8963..b76a786af4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (ante) [#493](https://github.com/crypto-org-chain/ethermint/pull/493) Align gasWanted for process proposal mode, [#500](https://github.com/crypto-org-chain/ethermint/pull/500) Check for overflow before adding gasLimit to gasWanted. * (ante) [#506](https://github.com/crypto-org-chain/ethermint/pull/506) Disable MsgCreatePermanentLockedAccount and MsgCreatePeriodicVestingAccount messages. * (ante) [#513](https://github.com/crypto-org-chain/ethermint/pull/513) Avoid unnecessary GetAccount and MakeSigner in ante handlers. +* (ante) [#514](https://github.com/crypto-org-chain/ethermint/pull/514) Avoid unnecessary GetBalance in ante handlers. ### Bug Fixes diff --git a/app/ante/eth.go b/app/ante/eth.go index 67a95113e0..8b4500be04 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -37,6 +37,8 @@ import ( type AccountGetter func(sdk.AccAddress) sdk.AccountI +type BalanceGetter func(sdk.AccAddress, string) *big.Int + // NewCachedAccountGetter cache the account objects during the ante handler execution, // it's safe because there's no store branching in the ante handlers, // it also creates new account in memory if it doesn't exist in the store. @@ -57,6 +59,19 @@ func NewCachedAccountGetter(ctx sdk.Context, ak evmtypes.AccountKeeper) AccountG } } +// NewCachedBalanceGetter cache the balance objects during the ante handler execution. +func NewCachedBalanceGetter(ctx sdk.Context, evmKeeper EVMKeeper) BalanceGetter { + balances := make(map[string]*big.Int, 1) + return func(addr sdk.AccAddress, denom string) *big.Int { + bal, ok := balances[string(addr)] + if !ok { + bal = evmKeeper.GetBalance(ctx, addr, denom) + balances[string(addr)] = bal + } + return bal + } +} + // VerifyEthAccount validates checks that the sender balance is greater than the total transaction cost. // The account will be created in memory if it doesn't exist, i.e cannot be found on store, which will eventually set to // store when increasing nonce. @@ -66,8 +81,9 @@ func NewCachedAccountGetter(ctx sdk.Context, ak evmtypes.AccountKeeper) AccountG // - account balance is lower than the transaction cost func VerifyEthAccount( ctx sdk.Context, tx sdk.Tx, - evmKeeper EVMKeeper, evmDenom string, + evmDenom string, accountGetter AccountGetter, + balanceGetter BalanceGetter, ) error { if !ctx.IsCheckTx() { return nil @@ -95,7 +111,7 @@ func VerifyEthAccount( "the sender is not EOA: address %s, codeHash <%s>", fromAddr, acct.CodeHash) } - balance := evmKeeper.GetBalance(ctx, from, evmDenom) + balance := balanceGetter(from, evmDenom) if err := keeper.CheckSenderBalance(sdkmath.NewIntFromBigIntMut(balance), ethTx); err != nil { return errorsmod.Wrap(err, "failed to check sender balance") } @@ -212,11 +228,11 @@ func CheckEthGasConsume( // CheckEthCanTransfer creates an EVM from the message and calls the BlockContext CanTransfer function to // see if the address can execute the transaction. func CheckEthCanTransfer( - ctx sdk.Context, tx sdk.Tx, + tx sdk.Tx, baseFee *big.Int, rules params.Rules, - evmKeeper EVMKeeper, evmParams *evmtypes.Params, + balanceGetter BalanceGetter, ) error { for _, msg := range tx.GetMsgs() { msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) @@ -244,28 +260,25 @@ func CheckEthCanTransfer( if value == nil || value.Sign() == -1 { return fmt.Errorf("value (%s) must be positive", value) } - from := common.BytesToAddress(msgEthTx.From) // check that caller has enough balance to cover asset transfer for **topmost** call // NOTE: here the gas consumed is from the context with the infinite gas meter - if value.Sign() > 0 && !canTransfer(ctx, evmKeeper, evmParams.EvmDenom, from, value) { - return errorsmod.Wrapf( - errortypes.ErrInsufficientFunds, - "failed to transfer %s from address %s using the EVM block context transfer function", - value, - from, - ) + if value.Sign() > 0 { + balance := balanceGetter(msgEthTx.GetFrom(), evmParams.EvmDenom) + if balance.Cmp(value) < 0 { + from := common.BytesToAddress(msgEthTx.From) + return errorsmod.Wrapf( + errortypes.ErrInsufficientFunds, + "failed to transfer %s from address %s using the EVM block context transfer function", + value, + from, + ) + } } } return nil } -// canTransfer adapted the core.CanTransfer from go-ethereum -func canTransfer(ctx sdk.Context, evmKeeper EVMKeeper, denom string, from common.Address, amount *big.Int) bool { - balance := evmKeeper.GetBalance(ctx, sdk.AccAddress(from.Bytes()), denom) - return balance.Cmp(amount) >= 0 -} - // CheckAndSetEthSenderNonce handles incrementing the sequence of the signer (i.e sender). If the transaction is a // contract creation, the nonce will be incremented during the transaction execution and not within // this AnteHandler decorator. diff --git a/app/ante/handler_options.go b/app/ante/handler_options.go index ccf446a88a..0e22a0cbbc 100644 --- a/app/ante/handler_options.go +++ b/app/ante/handler_options.go @@ -128,12 +128,13 @@ func newEthAnteHandler(options HandlerOptions) sdk.AnteHandler { // AccountGetter cache the account objects during the ante handler execution, // it's safe because there's no store branching in the ante handlers. accountGetter := NewCachedAccountGetter(ctx, options.AccountKeeper) + balanceGetter := NewCachedBalanceGetter(ctx, options.EvmKeeper) - if err := VerifyEthAccount(ctx, tx, options.EvmKeeper, evmDenom, accountGetter); err != nil { + if err := VerifyEthAccount(ctx, tx, evmDenom, accountGetter, balanceGetter); err != nil { return ctx, err } - if err := CheckEthCanTransfer(ctx, tx, baseFee, rules, options.EvmKeeper, evmParams); err != nil { + if err := CheckEthCanTransfer(tx, baseFee, rules, evmParams, balanceGetter); err != nil { return ctx, err } From 8f89bb1aa7af39e55bcc1b3b53af93bb7a6a63e3 Mon Sep 17 00:00:00 2001 From: mmsqe Date: Fri, 16 Aug 2024 09:35:29 +0800 Subject: [PATCH 2/2] fix test --- app/ante/eth_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/ante/eth_test.go b/app/ante/eth_test.go index c44863db27..725398ce13 100644 --- a/app/ante/eth_test.go +++ b/app/ante/eth_test.go @@ -93,7 +93,8 @@ func (suite *AnteTestSuite) TestNewEthAccountVerificationDecorator() { suite.Require().NoError(vmdb.Commit()) accountGetter := ante.NewCachedAccountGetter(suite.ctx, suite.app.AccountKeeper) - err := ante.VerifyEthAccount(suite.ctx.WithIsCheckTx(tc.checkTx), tc.tx, suite.app.EvmKeeper, evmtypes.DefaultEVMDenom, accountGetter) + balanceGetter := ante.NewCachedBalanceGetter(suite.ctx, suite.app.EvmKeeper) + err := ante.VerifyEthAccount(suite.ctx.WithIsCheckTx(tc.checkTx), tc.tx, evmtypes.DefaultEVMDenom, accountGetter, balanceGetter) if tc.expPass { suite.Require().NoError(err) @@ -460,9 +461,9 @@ func (suite *AnteTestSuite) TestCanTransferDecorator() { tc.malleate() suite.Require().NoError(vmdb.Commit()) + balanceGetter := ante.NewCachedBalanceGetter(suite.ctx, suite.app.EvmKeeper) err := ante.CheckEthCanTransfer( - suite.ctx.WithIsCheckTx(true), tc.tx, - baseFee, rules, suite.app.EvmKeeper, &evmParams, + tc.tx, baseFee, rules, &evmParams, balanceGetter, ) if tc.expPass {