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 <[email protected]> * cleanup --------- Signed-off-by: yihuang <[email protected]> Co-authored-by: huangyi <[email protected]>
crypto-org-chain · Aug 15, 2024 · fe3f4fd · fe3f4fd
1 parent 7ef8af1
commit fe3f4fd
Show file tree

Hide file tree

Showing 9 changed files with 68 additions and 34 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,14 +117,19 @@ 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)
 		}
 		if err != nil {
 			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
@@ -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
@@ -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
@@ -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
@@ -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)