From ed359ac459ec9ce06d4fc3341e6e209aba78f61f Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Thu, 31 Oct 2024 18:44:52 +0900 Subject: [PATCH] fix: do not use `posthandler` to handle failed tx sequence increment (#92) * fix to do not use posthandler to handle failed tx sequence increment * add test --- app/ante/ante.go | 4 +- app/ante/sigverify.go | 85 ----------------------------- app/ante/sigverify_test.go | 97 ---------------------------------- app/posthandler/posthandler.go | 1 - app/posthandler/sequence.go | 59 --------------------- x/evm/ante/context_keys.go | 1 + x/evm/ante/sequence.go | 48 +++++++++++++++++ x/evm/ante/sequence_test.go | 56 ++++++++++++++++++++ x/evm/keeper/msg_server.go | 53 +++++++++++++------ 9 files changed, 143 insertions(+), 261 deletions(-) delete mode 100644 app/ante/sigverify_test.go delete mode 100644 app/posthandler/sequence.go create mode 100644 x/evm/ante/sequence.go create mode 100644 x/evm/ante/sequence_test.go diff --git a/app/ante/ante.go b/app/ante/ante.go index 027611b..3be51e4 100644 --- a/app/ante/ante.go +++ b/app/ante/ante.go @@ -97,7 +97,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewValidateSigCountDecorator(options.AccountKeeper), ante.NewSigGasConsumeDecorator(options.AccountKeeper, sigGasConsumer), NewSigVerificationDecorator(options.AccountKeeper, options.EVMKeeper, options.SignModeHandler), - NewIncrementSequenceDecorator(options.AccountKeeper), + evmante.NewIncrementSequenceDecorator(options.AccountKeeper), ibcante.NewRedundantRelayDecorator(options.IBCkeeper), auctionante.NewAuctionDecorator(options.AuctionKeeper, options.TxEncoder, options.MevLane), } @@ -111,6 +111,6 @@ func CreateAnteHandlerForOPinit(ak ante.AccountKeeper, ek *evmkeeper.Keeper, sig ante.NewValidateSigCountDecorator(ak), ante.NewSigGasConsumeDecorator(ak, ante.DefaultSigVerificationGasConsumer), NewSigVerificationDecorator(ak, ek, signModeHandler), - NewIncrementSequenceDecorator(ak), + evmante.NewIncrementSequenceDecorator(ak), ) } diff --git a/app/ante/sigverify.go b/app/ante/sigverify.go index 0b4a464..00a083b 100644 --- a/app/ante/sigverify.go +++ b/app/ante/sigverify.go @@ -22,7 +22,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/types" "github.com/initia-labs/initia/crypto/ethsecp256k1" - evmtypes "github.com/initia-labs/minievm/x/evm/types" ) // SigVerificationDecorator verifies all signatures for a tx and return an error if any are invalid. Note, @@ -198,87 +197,3 @@ func consumeMultisignatureVerificationGas( return nil } - -// IncrementSequenceDecorator handles incrementing sequences of all signers. -// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note, -// there is need to execute IncrementSequenceDecorator on RecheckTx since -// BaseApp.Commit() will set the check state based on the latest header. -// -// NOTE: Since CheckTx and DeliverTx state are managed separately, subsequent and -// sequential txs orginating from the same account cannot be handled correctly in -// a reliable way unless sequence numbers are managed and tracked manually by a -// client. It is recommended to instead use multiple messages in a tx. -// -// NOTE: When we execute evm messages, it whould handle sequence number increment internally, -// so we need to decrease sequence number if it is used in EVM. -type IncrementSequenceDecorator struct { - ak authante.AccountKeeper -} - -func NewIncrementSequenceDecorator(ak authante.AccountKeeper) IncrementSequenceDecorator { - return IncrementSequenceDecorator{ - ak: ak, - } -} - -func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { - sigTx, ok := tx.(authsigning.SigVerifiableTx) - if !ok { - return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") - } - - // increment sequence of all signers - signers, err := sigTx.GetSigners() - if err != nil { - return sdk.Context{}, err - } - - for _, signer := range signers { - acc := isd.ak.GetAccount(ctx, signer) - if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { - panic(err) - } - - isd.ak.SetAccount(ctx, acc) - } - - // decrement sequence of all signers which is used in EVM - // when we execute evm messages, it whould handle sequence number increment. - // - // NOTE: Need to revert it if a tx failed. See ../posthandler/sequence.go - if simulate || ctx.ExecMode() == sdk.ExecModeFinalize { - signerMap := make(map[string]bool) - for _, msg := range tx.GetMsgs() { - var caller string - switch msg := msg.(type) { - case *evmtypes.MsgCreate: - caller = msg.Sender - case *evmtypes.MsgCreate2: - caller = msg.Sender - case *evmtypes.MsgCall: - caller = msg.Sender - default: - continue - } - - if _, ok := signerMap[caller]; ok { - continue - } - signerMap[caller] = true - - callerAccAddr, err := isd.ak.AddressCodec().StringToBytes(caller) - if err != nil { - return ctx, err - } - - acc := isd.ak.GetAccount(ctx, callerAccAddr) - if err := acc.SetSequence(acc.GetSequence() - 1); err != nil { - panic(err) - } - - isd.ak.SetAccount(ctx, acc) - } - } - - return next(ctx, tx, simulate) -} diff --git a/app/ante/sigverify_test.go b/app/ante/sigverify_test.go deleted file mode 100644 index b2e5cdb..0000000 --- a/app/ante/sigverify_test.go +++ /dev/null @@ -1,97 +0,0 @@ -package ante_test - -import ( - cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" - "github.com/cosmos/cosmos-sdk/testutil/testdata" - sdk "github.com/cosmos/cosmos-sdk/types" - - "github.com/initia-labs/minievm/app/ante" - evmtypes "github.com/initia-labs/minievm/x/evm/types" -) - -func (suite *AnteTestSuite) TestIncrementSequenceDecorator() { - suite.SetupTest() // setup - suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() - - priv, _, addr := testdata.KeyTestPubAddr() - acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr) - suite.NoError(acc.SetAccountNumber(uint64(50))) - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) - - msgs := []sdk.Msg{testdata.NewTestMsg(addr)} - suite.NoError(suite.txBuilder.SetMsgs(msgs...)) - privs := []cryptotypes.PrivKey{priv} - accNums := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetAccountNumber()} - accSeqs := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()} - feeAmount := testdata.NewTestFeeAmount() - gasLimit := testdata.NewTestGasLimit() - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) - - tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID()) - suite.NoError(err) - - isd := ante.NewIncrementSequenceDecorator(suite.app.AccountKeeper) - antehandler := sdk.ChainAnteDecorators(isd) - - testCases := []struct { - ctx sdk.Context - simulate bool - expectedSeq uint64 - }{ - {suite.ctx.WithIsReCheckTx(true), false, 1}, - {suite.ctx.WithIsCheckTx(true).WithIsReCheckTx(false), false, 2}, - {suite.ctx.WithIsReCheckTx(true), false, 3}, - {suite.ctx.WithIsReCheckTx(true), false, 4}, - {suite.ctx.WithIsReCheckTx(true), true, 5}, - } - - for i, tc := range testCases { - _, err := antehandler(tc.ctx, tx, tc.simulate) - suite.NoError(err, "unexpected error; tc #%d, %v", i, tc) - suite.Equal(tc.expectedSeq, suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()) - } -} - -func (suite *AnteTestSuite) TestIncrementSequenceDecorator_EVMMessages() { - suite.SetupTest() // setup - suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() - - priv, _, addr := testdata.KeyTestPubAddr() - acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr) - suite.NoError(acc.SetAccountNumber(uint64(50))) - suite.app.AccountKeeper.SetAccount(suite.ctx, acc) - - msgs := []sdk.Msg{&evmtypes.MsgCreate{Sender: addr.String()}, &evmtypes.MsgCreate2{Sender: addr.String()}, &evmtypes.MsgCall{Sender: addr.String()}} - suite.NoError(suite.txBuilder.SetMsgs(msgs...)) - privs := []cryptotypes.PrivKey{priv} - accNums := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetAccountNumber()} - accSeqs := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()} - feeAmount := testdata.NewTestFeeAmount() - gasLimit := testdata.NewTestGasLimit() - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) - - tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID()) - suite.NoError(err) - - isd := ante.NewIncrementSequenceDecorator(suite.app.AccountKeeper) - antehandler := sdk.ChainAnteDecorators(isd) - - testCases := []struct { - ctx sdk.Context - simulate bool - expectedSeq uint64 - }{ - {suite.ctx.WithExecMode(sdk.ExecModeCheck), false, 1}, - {suite.ctx.WithExecMode(sdk.ExecModeCheck), true, 1}, - {suite.ctx.WithExecMode(sdk.ExecModeFinalize), false, 1}, - {suite.ctx.WithExecMode(sdk.ExecModeCheck), false, 2}, - } - - for i, tc := range testCases { - _, err := antehandler(tc.ctx, tx, tc.simulate) - suite.NoError(err, "unexpected error; tc #%d, %v", i, tc) - suite.Equal(tc.expectedSeq, suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()) - } -} diff --git a/app/posthandler/posthandler.go b/app/posthandler/posthandler.go index bd180e0..04132f8 100644 --- a/app/posthandler/posthandler.go +++ b/app/posthandler/posthandler.go @@ -16,7 +16,6 @@ func NewPostHandler( ek EVMKeeper, ) sdk.PostHandler { return sdk.ChainPostDecorators( - NewSequenceIncrementDecorator(ak), NewGasRefundDecorator(ek), ) } diff --git a/app/posthandler/sequence.go b/app/posthandler/sequence.go deleted file mode 100644 index aa79d46..0000000 --- a/app/posthandler/sequence.go +++ /dev/null @@ -1,59 +0,0 @@ -package posthandler - -import ( - sdk "github.com/cosmos/cosmos-sdk/types" - authante "github.com/cosmos/cosmos-sdk/x/auth/ante" - evmtypes "github.com/initia-labs/minievm/x/evm/types" -) - -var _ sdk.PostDecorator = &SequenceIncrementDecorator{} - -type SequenceIncrementDecorator struct { - ak authante.AccountKeeper -} - -func NewSequenceIncrementDecorator(ak authante.AccountKeeper) sdk.PostDecorator { - return &SequenceIncrementDecorator{ - ak, - } -} - -// If a transaction fails, we need to revert the sequence decrement for EVM messages that were executed in the ante handler. -// This is necessary because the sequence increment in EVM was reverted due to the failure. -func (sid *SequenceIncrementDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simulate, success bool, next sdk.PostHandler) (newCtx sdk.Context, err error) { - if !success && ctx.ExecMode() == sdk.ExecModeFinalize { - signerMap := make(map[string]bool) - for _, msg := range tx.GetMsgs() { - var caller string - switch msg := msg.(type) { - case *evmtypes.MsgCreate: - caller = msg.Sender - case *evmtypes.MsgCreate2: - caller = msg.Sender - case *evmtypes.MsgCall: - caller = msg.Sender - default: - continue - } - - if _, ok := signerMap[caller]; ok { - continue - } - signerMap[caller] = true - - callerAccAddr, err := sid.ak.AddressCodec().StringToBytes(caller) - if err != nil { - return ctx, err - } - - acc := sid.ak.GetAccount(ctx, callerAccAddr) - if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { - panic(err) - } - - sid.ak.SetAccount(ctx, acc) - } - } - - return next(ctx, tx, simulate, success) -} diff --git a/x/evm/ante/context_keys.go b/x/evm/ante/context_keys.go index 5625f86..7fbc8c6 100644 --- a/x/evm/ante/context_keys.go +++ b/x/evm/ante/context_keys.go @@ -5,4 +5,5 @@ type contextKey int const ( ContextKeyGasPrices contextKey = iota + ContextKeySequenceIncremented ) diff --git a/x/evm/ante/sequence.go b/x/evm/ante/sequence.go new file mode 100644 index 0000000..63f5611 --- /dev/null +++ b/x/evm/ante/sequence.go @@ -0,0 +1,48 @@ +package ante + +import ( + errorsmod "cosmossdk.io/errors" + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + authante "github.com/cosmos/cosmos-sdk/x/auth/ante" + authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" +) + +// IncrementSequenceDecorator is an AnteDecorator that increments the sequence number +// of all signers in a transaction. It also sets a flag in the context to indicate +// that the sequence has been incremented in the ante handler. +type IncrementSequenceDecorator struct { + ak authante.AccountKeeper +} + +func NewIncrementSequenceDecorator(ak authante.AccountKeeper) IncrementSequenceDecorator { + return IncrementSequenceDecorator{ + ak: ak, + } +} + +func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + sigTx, ok := tx.(authsigning.SigVerifiableTx) + if !ok { + return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") + } + + // increment sequence of all signers + signers, err := sigTx.GetSigners() + if err != nil { + return sdk.Context{}, err + } + + for _, signer := range signers { + acc := isd.ak.GetAccount(ctx, signer) + if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { + panic(err) + } + + isd.ak.SetAccount(ctx, acc) + } + + // set a flag in context to indicate that sequence has been incremented in ante handler + ctx = ctx.WithValue(ContextKeySequenceIncremented, true) + return next(ctx, tx, simulate) +} diff --git a/x/evm/ante/sequence_test.go b/x/evm/ante/sequence_test.go new file mode 100644 index 0000000..79f0cdb --- /dev/null +++ b/x/evm/ante/sequence_test.go @@ -0,0 +1,56 @@ +package ante_test + +import ( + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/initia-labs/minievm/x/evm/ante" +) + +func (suite *AnteTestSuite) TestIncrementSequenceDecorator() { + suite.SetupTest() // setup + suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() + + priv, _, addr := testdata.KeyTestPubAddr() + acc := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr) + suite.NoError(acc.SetAccountNumber(uint64(50))) + suite.app.AccountKeeper.SetAccount(suite.ctx, acc) + + msgs := []sdk.Msg{testdata.NewTestMsg(addr)} + suite.NoError(suite.txBuilder.SetMsgs(msgs...)) + privs := []cryptotypes.PrivKey{priv} + accNums := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetAccountNumber()} + accSeqs := []uint64{suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()} + feeAmount := testdata.NewTestFeeAmount() + gasLimit := testdata.NewTestGasLimit() + suite.txBuilder.SetFeeAmount(feeAmount) + suite.txBuilder.SetGasLimit(gasLimit) + + tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID()) + suite.NoError(err) + + isd := ante.NewIncrementSequenceDecorator(suite.app.AccountKeeper) + antehandler := sdk.ChainAnteDecorators(isd) + + testCases := []struct { + ctx sdk.Context + simulate bool + expectedSeq uint64 + }{ + {suite.ctx.WithIsReCheckTx(true), false, 1}, + {suite.ctx.WithIsCheckTx(true).WithIsReCheckTx(false), false, 2}, + {suite.ctx.WithIsReCheckTx(true), false, 3}, + {suite.ctx.WithIsReCheckTx(true), false, 4}, + {suite.ctx.WithIsReCheckTx(true), true, 5}, + } + + for i, tc := range testCases { + ctx, err := antehandler(tc.ctx, tx, tc.simulate) + suite.NoError(err, "unexpected error; tc #%d, %v", i, tc) + suite.Equal(tc.expectedSeq, suite.app.AccountKeeper.GetAccount(suite.ctx, addr).GetSequence()) + + // the flag should be set in the context + suite.NotNil(ctx.Value(ante.ContextKeySequenceIncremented)) + } +} diff --git a/x/evm/keeper/msg_server.go b/x/evm/keeper/msg_server.go index 45aa5c7..1724f8f 100644 --- a/x/evm/keeper/msg_server.go +++ b/x/evm/keeper/msg_server.go @@ -13,6 +13,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + evmante "github.com/initia-labs/minievm/x/evm/ante" "github.com/initia-labs/minievm/x/evm/types" ) @@ -31,6 +32,12 @@ func (ms *msgServerImpl) Create(ctx context.Context, msg *types.MsgCreate) (*typ return nil, err } + // handle cosmos<>evm different sequence increment logic + ctx, err = ms.handleSequenceIncremented(ctx, sender, true) + if err != nil { + return nil, err + } + // argument validation caller, err := ms.convertToEVMAddress(ctx, sender, true) if err != nil { @@ -90,6 +97,12 @@ func (ms *msgServerImpl) Create2(ctx context.Context, msg *types.MsgCreate2) (*t return nil, err } + // handle cosmos<>evm different sequence increment logic + ctx, err = ms.handleSequenceIncremented(ctx, sender, true) + if err != nil { + return nil, err + } + // argument validation caller, err := ms.convertToEVMAddress(ctx, sender, true) if err != nil { @@ -142,19 +155,6 @@ func (ms *msgServerImpl) Create2(ctx context.Context, msg *types.MsgCreate2) (*t }, nil } -// increaseNonce increases the nonce of the given account. -func (ms *msgServerImpl) increaseNonce(ctx context.Context, caller sdk.AccAddress) error { - senderAcc := ms.accountKeeper.GetAccount(ctx, caller) - if senderAcc == nil { - senderAcc = ms.accountKeeper.NewAccountWithAddress(ctx, caller) - } - if err := senderAcc.SetSequence(senderAcc.GetSequence() + 1); err != nil { - return err - } - ms.accountKeeper.SetAccount(ctx, senderAcc) - return nil -} - // Call implements types.MsgServer. func (ms *msgServerImpl) Call(ctx context.Context, msg *types.MsgCall) (*types.MsgCallResponse, error) { sender, err := ms.ac.StringToBytes(msg.Sender) @@ -162,10 +162,9 @@ func (ms *msgServerImpl) Call(ctx context.Context, msg *types.MsgCall) (*types.M return nil, err } - // increase nonce before execution like evm does - // - // NOTE: evm only increases nonce at Call not Create, so we should do the same. - if err := ms.increaseNonce(ctx, sender); err != nil { + // handle cosmos<>evm different sequence increment logic + ctx, err = ms.handleSequenceIncremented(ctx, sender, false) + if err != nil { return nil, err } @@ -256,3 +255,23 @@ func (ms *msgServerImpl) testFeeDenom(ctx context.Context, params types.Params) return nil } + +// In the Cosmos SDK, the sequence number is incremented in the ante handler. +// In the EVM, the sequence number is incremented during the execution of create and create2 messages. +// However, for call messages, the sequence number is incremented in the ante handler like the Cosmos SDK. +// To prevent double incrementing the sequence number during EVM execution, we need to decrement it here for create messages. +func (k *msgServerImpl) handleSequenceIncremented(ctx context.Context, sender sdk.AccAddress, isCreate bool) (context.Context, error) { + sdkCtx := sdk.UnwrapSDKContext(ctx) + + // decrement sequence of the sender + if isCreate && sdkCtx.Value(evmante.ContextKeySequenceIncremented) != nil { + acc := k.accountKeeper.GetAccount(ctx, sender) + if err := acc.SetSequence(acc.GetSequence() - 1); err != nil { + return ctx, err + } + + k.accountKeeper.SetAccount(ctx, acc) + } + + return sdkCtx.WithValue(evmante.ContextKeySequenceIncremented, nil), nil +}