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

@@ -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),
 	)
 }

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)
-}

app/posthandler/posthandler.go

@@ -16,7 +16,6 @@ func NewPostHandler(
 	ek EVMKeeper,
 ) sdk.PostHandler {
 	return sdk.ChainPostDecorators(
-		NewSequenceIncrementDecorator(ak),
 		NewGasRefundDecorator(ek),
 	)
 }

x/evm/ante/context_keys.go

@@ -5,4 +5,5 @@ type contextKey int
 
 const (
 	ContextKeyGasPrices contextKey = iota
+	ContextKeySequenceIncremented
 )

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)
+}

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))
+	}
+}