Skip to content

Commit

Permalink
Problem: unnecessary GetAccount in ante handlers (#513)
Browse files Browse the repository at this point in the history
* 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 <huang@crypto.com>

* cleanup

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: huangyi <huang@crypto.com>
  • Loading branch information
mmsqe and yihuang authored Aug 15, 2024
1 parent 7ef8af1 commit fe3f4fd
Show file tree
Hide file tree
Showing 9 changed files with 68 additions and 34 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion app/ante/authz.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
46 changes: 34 additions & 12 deletions app/ante/eth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand Down
13 changes: 8 additions & 5 deletions app/ante/eth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -514,7 +516,7 @@ func (suite *AnteTestSuite) TestEthIncrementSenderSequenceDecorator() {
"account not set to store",
tx,
func() {},
false, false,
true, false,
},
{
"success - create contract",
Expand All @@ -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)
Expand Down
10 changes: 7 additions & 3 deletions app/ante/handler_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion tests/rpc/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 1 addition & 11 deletions x/evm/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion x/evm/keeper/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions x/evm/statedb/state_object.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit fe3f4fd

Please sign in to comment.