From fe3f4fd535d72e50e2628f44487d3159efb4d4ec Mon Sep 17 00:00:00 2001 From: mmsqe Date: Thu, 15 Aug 2024 13:15:59 +0800 Subject: [PATCH] Problem: unnecessary GetAccount in ante handlers (#513) * Problem: unnecessary GetAccount in ante handlers reuse account after verify account balance to avoid get again when check sender sequence * rename * less MakeSigner * cast string * cleanup * fix test * fix lint * cleanup * cleanup test * rm prefix * fix * fix test, account created automatically * rename * fix comment * Apply suggestions from code review Signed-off-by: yihuang * cleanup --------- Signed-off-by: yihuang Co-authored-by: huangyi --- CHANGELOG.md | 1 + app/ante/authz.go | 2 +- app/ante/eth.go | 46 ++++++++++++++++++++++++++--------- app/ante/eth_test.go | 13 ++++++---- app/ante/handler_options.go | 10 +++++--- tests/rpc/utils.go | 2 +- x/evm/keeper/keeper.go | 12 +-------- x/evm/keeper/utils.go | 2 +- x/evm/statedb/state_object.go | 14 +++++++++++ 9 files changed, 68 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 951ca23da0..7980fc8963 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (app) [#451](https://github.com/crypto-org-chain/ethermint/pull/451) Disable block gas meter, it's not compatible with parallel tx execution. It's safe to do as long as we checks total gas-wanted against block gas limit in process proposal, which we do in default handler. * (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. ### Bug Fixes diff --git a/app/ante/authz.go b/app/ante/authz.go index 561e8ea11d..5c71dc2cef 100644 --- a/app/ante/authz.go +++ b/app/ante/authz.go @@ -53,7 +53,7 @@ func (ald AuthzLimiterDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate } if err := ald.checkDisabledMsgs(tx.GetMsgs(), false, 0); err != nil { - return ctx, errorsmod.Wrapf(errortypes.ErrUnauthorized, err.Error()) + return ctx, errorsmod.Wrap(errortypes.ErrUnauthorized, err.Error()) } return next(ctx, tx, simulate) } diff --git a/app/ante/eth.go b/app/ante/eth.go index 8d9cd28250..67a95113e0 100644 --- a/app/ante/eth.go +++ b/app/ante/eth.go @@ -28,21 +28,46 @@ import ( ethermint "github.com/evmos/ethermint/types" "github.com/evmos/ethermint/x/evm/keeper" + "github.com/evmos/ethermint/x/evm/statedb" evmtypes "github.com/evmos/ethermint/x/evm/types" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/params" ) +type AccountGetter func(sdk.AccAddress) sdk.AccountI + +// 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. +func NewCachedAccountGetter(ctx sdk.Context, ak evmtypes.AccountKeeper) AccountGetter { + accounts := make(map[string]sdk.AccountI, 1) + return func(addr sdk.AccAddress) sdk.AccountI { + acc := accounts[string(addr)] + if acc == nil { + acc = ak.GetAccount(ctx, addr) + if acc == nil { + // we create a new account in memory if it doesn't exist, + // which is only set to store when updated. + acc = ak.NewAccountWithAddress(ctx, addr) + } + accounts[string(addr)] = acc + } + return acc + } +} + // VerifyEthAccount validates checks that the sender balance is greater than the total transaction cost. -// The account will be set to store if it doesn't exis, i.e cannot be found on store. +// 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. // This AnteHandler decorator will fail if: // - any of the msgs is not a MsgEthereumTx // - from address is empty // - account balance is lower than the transaction cost func VerifyEthAccount( ctx sdk.Context, tx sdk.Tx, - evmKeeper EVMKeeper, ak evmtypes.AccountKeeper, evmDenom string, + evmKeeper EVMKeeper, evmDenom string, + accountGetter AccountGetter, ) error { if !ctx.IsCheckTx() { return nil @@ -63,13 +88,9 @@ func VerifyEthAccount( } // check whether the sender address is EOA - fromAddr := common.BytesToAddress(from) - acct := evmKeeper.GetAccount(ctx, fromAddr) - - if acct == nil { - acc := ak.NewAccountWithAddress(ctx, from) - ak.SetAccount(ctx, acc) - } else if acct.IsContract() { + acct := statedb.NewAccountFromSdkAccount(accountGetter(from)) + if acct.IsContract() { + fromAddr := common.BytesToAddress(from) return errorsmod.Wrapf(errortypes.ErrInvalidType, "the sender is not EOA: address %s, codeHash <%s>", fromAddr, acct.CodeHash) } @@ -249,7 +270,7 @@ func canTransfer(ctx sdk.Context, evmKeeper EVMKeeper, denom string, from common // contract creation, the nonce will be incremented during the transaction execution and not within // this AnteHandler decorator. func CheckAndSetEthSenderNonce( - ctx sdk.Context, tx sdk.Tx, ak evmtypes.AccountKeeper, unsafeUnOrderedTx bool, + ctx sdk.Context, tx sdk.Tx, ak evmtypes.AccountKeeper, unsafeUnOrderedTx bool, accountGetter AccountGetter, ) error { for _, msg := range tx.GetMsgs() { msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) @@ -260,11 +281,12 @@ func CheckAndSetEthSenderNonce( tx := msgEthTx.AsTransaction() // increase sequence of sender - acc := ak.GetAccount(ctx, msgEthTx.GetFrom()) + from := msgEthTx.GetFrom() + acc := accountGetter(from) if acc == nil { return errorsmod.Wrapf( errortypes.ErrUnknownAddress, - "account %s is nil", common.BytesToAddress(msgEthTx.GetFrom().Bytes()), + "account %s is nil", common.BytesToAddress(from.Bytes()), ) } nonce := acc.GetSequence() diff --git a/app/ante/eth_test.go b/app/ante/eth_test.go index 670e6ff974..c44863db27 100644 --- a/app/ante/eth_test.go +++ b/app/ante/eth_test.go @@ -92,7 +92,8 @@ func (suite *AnteTestSuite) TestNewEthAccountVerificationDecorator() { tc.malleate() suite.Require().NoError(vmdb.Commit()) - err := ante.VerifyEthAccount(suite.ctx.WithIsCheckTx(tc.checkTx), tc.tx, suite.app.EvmKeeper, suite.app.AccountKeeper, evmtypes.DefaultEVMDenom) + accountGetter := ante.NewCachedAccountGetter(suite.ctx, suite.app.AccountKeeper) + err := ante.VerifyEthAccount(suite.ctx.WithIsCheckTx(tc.checkTx), tc.tx, suite.app.EvmKeeper, evmtypes.DefaultEVMDenom, accountGetter) if tc.expPass { suite.Require().NoError(err) @@ -147,7 +148,8 @@ func (suite *AnteTestSuite) TestEthNonceVerificationDecorator() { for _, tc := range testCases { suite.Run(tc.name, func() { tc.malleate() - err := ante.CheckAndSetEthSenderNonce(suite.ctx.WithIsReCheckTx(tc.reCheckTx), tc.tx, suite.app.AccountKeeper, false) + accountGetter := ante.NewCachedAccountGetter(suite.ctx, suite.app.AccountKeeper) + err := ante.CheckAndSetEthSenderNonce(suite.ctx.WithIsReCheckTx(tc.reCheckTx), tc.tx, suite.app.AccountKeeper, false, accountGetter) if tc.expPass { suite.Require().NoError(err) @@ -514,7 +516,7 @@ func (suite *AnteTestSuite) TestEthIncrementSenderSequenceDecorator() { "account not set to store", tx, func() {}, - false, false, + true, false, }, { "success - create contract", @@ -536,15 +538,16 @@ func (suite *AnteTestSuite) TestEthIncrementSenderSequenceDecorator() { for _, tc := range testCases { suite.Run(tc.name, func() { tc.malleate() + accountGetter := ante.NewCachedAccountGetter(suite.ctx, suite.app.AccountKeeper) if tc.expPanic { suite.Require().Panics(func() { - _ = ante.CheckAndSetEthSenderNonce(suite.ctx, tc.tx, suite.app.AccountKeeper, false) + _ = ante.CheckAndSetEthSenderNonce(suite.ctx, tc.tx, suite.app.AccountKeeper, false, accountGetter) }) return } - err := ante.CheckAndSetEthSenderNonce(suite.ctx, tc.tx, suite.app.AccountKeeper, false) + err := ante.CheckAndSetEthSenderNonce(suite.ctx, tc.tx, suite.app.AccountKeeper, false, accountGetter) if tc.expPass { suite.Require().NoError(err) diff --git a/app/ante/handler_options.go b/app/ante/handler_options.go index a6396cd8a0..ccf446a88a 100644 --- a/app/ante/handler_options.go +++ b/app/ante/handler_options.go @@ -87,7 +87,6 @@ func newEthAnteHandler(options HandlerOptions) sdk.AnteHandler { feemarketParams := &blockCfg.FeeMarketParams baseFee := blockCfg.BaseFee rules := blockCfg.Rules - ethSigner := ethtypes.MakeSigner(blockCfg.ChainConfig, blockCfg.BlockNumber) // all transactions must implement FeeTx _, ok := tx.(sdk.FeeTx) @@ -118,6 +117,7 @@ func newEthAnteHandler(options HandlerOptions) sdk.AnteHandler { err = v.(error) } } else { + ethSigner := ethtypes.MakeSigner(blockCfg.ChainConfig, blockCfg.BlockNumber) err = VerifyEthSig(tx, ethSigner) ctx.SetIncarnationCache(EthSigVerificationResultCacheKey, err) } @@ -125,7 +125,11 @@ func newEthAnteHandler(options HandlerOptions) sdk.AnteHandler { return ctx, err } - if err := VerifyEthAccount(ctx, tx, options.EvmKeeper, options.AccountKeeper, evmDenom); err != nil { + // 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) + + if err := VerifyEthAccount(ctx, tx, options.EvmKeeper, evmDenom, accountGetter); err != nil { return ctx, err } @@ -141,7 +145,7 @@ func newEthAnteHandler(options HandlerOptions) sdk.AnteHandler { return ctx, err } - if err := CheckAndSetEthSenderNonce(ctx, tx, options.AccountKeeper, options.UnsafeUnorderedTx); err != nil { + if err := CheckAndSetEthSenderNonce(ctx, tx, options.AccountKeeper, options.UnsafeUnorderedTx, accountGetter); err != nil { return ctx, err } diff --git a/tests/rpc/utils.go b/tests/rpc/utils.go index 77539af10d..f1c5055c79 100644 --- a/tests/rpc/utils.go +++ b/tests/rpc/utils.go @@ -149,7 +149,7 @@ func CallWithError(method string, params interface{}) (*Response, error) { } if rpcRes.Error != nil { - return nil, fmt.Errorf(rpcRes.Error.Message) + return nil, errors.New(rpcRes.Error.Message) } return rpcRes, nil diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 99d12e085e..39997b685b 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -216,17 +216,7 @@ func (k *Keeper) GetAccount(ctx sdk.Context, addr common.Address) *statedb.Accou if acct == nil { return nil } - - codeHash := types.EmptyCodeHash - ethAcct, ok := acct.(ethermint.EthAccountI) - if ok { - codeHash = ethAcct.GetCodeHash().Bytes() - } - - return &statedb.Account{ - Nonce: acct.GetSequence(), - CodeHash: codeHash, - } + return statedb.NewAccountFromSdkAccount(acct) } // GetAccountOrEmpty returns empty account if not exist, returns error if it's not `EthAccount` diff --git a/x/evm/keeper/utils.go b/x/evm/keeper/utils.go index d2b1257692..1da011225c 100644 --- a/x/evm/keeper/utils.go +++ b/x/evm/keeper/utils.go @@ -169,7 +169,7 @@ func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc sdk.AccountI, } if ctx.BlockHeight() > 0 { if err := bankKeeper.SendCoinsFromAccountToModuleVirtual(ctx, acc.GetAddress(), authtypes.FeeCollectorName, fees); err != nil { - return errorsmod.Wrapf(errortypes.ErrInsufficientFunds, err.Error()) + return errorsmod.Wrap(errortypes.ErrInsufficientFunds, err.Error()) } } return nil diff --git a/x/evm/statedb/state_object.go b/x/evm/statedb/state_object.go index a9cffe6525..30b60b6554 100644 --- a/x/evm/statedb/state_object.go +++ b/x/evm/statedb/state_object.go @@ -19,8 +19,10 @@ import ( "bytes" "sort" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" + ethermint "github.com/evmos/ethermint/types" ) var emptyCodeHash = crypto.Keccak256(nil) @@ -39,6 +41,18 @@ func NewEmptyAccount() *Account { } } +// NewAccountFromSdkAccount extracts the nonce and code hash from the provided SDK account. +func NewAccountFromSdkAccount(acct sdk.AccountI) *Account { + acc := NewEmptyAccount() + acc.Nonce = acct.GetSequence() + + if ethAcct, ok := acct.(ethermint.EthAccountI); ok { + acc.CodeHash = ethAcct.GetCodeHash().Bytes() + } + + return acc +} + // IsContract returns if the account contains contract code. func (acct Account) IsContract() bool { return !bytes.Equal(acct.CodeHash, emptyCodeHash)