Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IncrementSequenceDecorator #5950

Merged
merged 4 commits into from
Apr 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ types (eg. keys) to the `auth` module internal amino codec.
* (rest) [\#5906](https://github.com/cosmos/cosmos-sdk/pull/5906) Fix an issue that make some REST calls panic when sending
invalid or incomplete requests.
* (x/genutil) [\#5938](https://github.com/cosmos/cosmos-sdk/pull/5938) Fix `InitializeNodeValidatorFiles` error handling.
* (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.

### State Machine Breaking

Expand Down
17 changes: 9 additions & 8 deletions x/auth/ante/sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
}

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

Expand Down
37 changes: 37 additions & 0 deletions x/auth/ante/sigverify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,3 +212,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())
}
}