Fix IncrementSequenceDecorator (#5950)

* Fix IncrementSequenceDecorator

* Update PR #

* Update godoc

* Add TestIncrementSequenceDecorator test

CHANGELOG.md

@@ -37,6 +37,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+## [v0.38.4] - TBD
+
+### Bug Fixes
+
+* (x/auth) [\#5950](https://github.com/cosmos/cosmos-sdk/pull/5950) Fix `IncrementSequenceDecorator` to use is `IsReCheckTx` instead of `IsCheckTx` to allow account sequence incrementing.
+
 ## [v0.38.3] - 2020-04-09
 
 * (tendermint) Bump Tendermint version to [v0.33.3](https://github.com/tendermint/tendermint/releases/tag/v0.33.3).

x/auth/ante/sigverify.go

@@ -218,13 +218,13 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 
 // IncrementSequenceDecorator handles incrementing sequences of all signers.
 // Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note,
-// there is no need to execute IncrementSequenceDecorator on CheckTx or RecheckTX
-// since it is merely updating the nonce. As a result, this has the side effect
-// that subsequent and sequential txs orginating from the same account cannot be
-// handled correctly in a reliable way. To send sequential txs orginating from the
-// same account, it is recommended to instead use multiple messages in a tx.
+// there is no need to execute IncrementSequenceDecorator on RecheckTX since
+// CheckTx would already bump the sequence number.
 //
-// CONTRACT: The tx must implement the SigVerifiableTx interface.
+// 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.
 type IncrementSequenceDecorator struct {
  ak keeper.AccountKeeper
 }
@@ -236,8 +236,8 @@ func NewIncrementSequenceDecorator(ak keeper.AccountKeeper) IncrementSequenceDec
 }
 
 func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
- // no need to increment sequence on CheckTx or RecheckTx
- if ctx.IsCheckTx() && !simulate {
+ // no need to increment sequence on RecheckTx
+ if ctx.IsReCheckTx() && !simulate {
  return next(ctx, tx, simulate)
  }
 
@@ -252,6 +252,7 @@ func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim
  if err := acc.SetSequence(acc.GetSequence() + 1); err != nil {
  panic(err)
  }
+
  isd.ak.SetAccount(ctx, acc)
  }
 

x/auth/ante/sigverify_test.go

@@ -211,3 +211,40 @@ func runSigDecorators(t *testing.T, params types.Params, multisig bool, privs ..
 
  return after - before, err
 }
+
+func TestIncrementSequenceDecorator(t *testing.T) {
+ app, ctx := createTestApp(true)
+
+ priv, _, addr := types.KeyTestPubAddr()
+ acc := app.AccountKeeper.NewAccountWithAddress(ctx, addr)
+ require.NoError(t, acc.SetAccountNumber(uint64(50)))
+ app.AccountKeeper.SetAccount(ctx, acc)
+
+ msgs := []sdk.Msg{types.NewTestMsg(addr)}
+ privKeys := []crypto.PrivKey{priv}
+ accNums := []uint64{app.AccountKeeper.GetAccount(ctx, addr).GetAccountNumber()}
+ accSeqs := []uint64{app.AccountKeeper.GetAccount(ctx, addr).GetSequence()}
+ fee := types.NewTestStdFee()
+ tx := types.NewTestTx(ctx, msgs, privKeys, accNums, accSeqs, fee)
+
+ isd := ante.NewIncrementSequenceDecorator(app.AccountKeeper)
+ antehandler := sdk.ChainAnteDecorators(isd)
+
+ testCases := []struct {
+ ctx sdk.Context
+ simulate bool
+ expectedSeq uint64
+ }{
+ {ctx.WithIsReCheckTx(true), false, 0},
+ {ctx.WithIsCheckTx(true).WithIsReCheckTx(false), false, 1},
+ {ctx.WithIsReCheckTx(true), false, 1},
+ {ctx.WithIsReCheckTx(true), false, 1},
+ {ctx.WithIsReCheckTx(true), true, 2},
+ }
+
+ for i, tc := range testCases {
+ _, err := antehandler(tc.ctx, tx, tc.simulate)
+ require.NoError(t, err, "unexpected error; tc #%d, %v", i, tc)
+ require.Equal(t, tc.expectedSeq, app.AccountKeeper.GetAccount(ctx, addr).GetSequence())
+ }
+}