From 571c5316df659dec3920f7d2a3d0c3cd22f913ce Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 7 Apr 2020 13:41:27 -0400 Subject: [PATCH 1/4] Fix IncrementSequenceDecorator --- CHANGELOG.md | 1 + x/auth/ante/sigverify.go | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f1fc9c7bbd7..6ae199eb41e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) [\#5905](https://github.com/cosmos/cosmos-sdk/pull/5905) Fix `IncrementSequenceDecorator` to use is `IsReCheckTx` instead of `IsCheckTx` to allow account sequence incrementing. ### State Machine Breaking diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 895be5373c61..9ff493a29e81 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -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) } From 081c28c28700a9dc55aef6c48ff6b3020a4b3c94 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 7 Apr 2020 13:43:00 -0400 Subject: [PATCH 2/4] Update PR # --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ae199eb41e6..ce320d48f65d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,7 +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) [\#5905](https://github.com/cosmos/cosmos-sdk/pull/5905) Fix `IncrementSequenceDecorator` to use is `IsReCheckTx` instead of `IsCheckTx` to allow account sequence incrementing. +* (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 From 1244644bdd713e1da4be4fe8bb03336fb1668e76 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 7 Apr 2020 13:46:40 -0400 Subject: [PATCH 3/4] Update godoc --- x/auth/ante/sigverify.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 9ff493a29e81..f5443103758b 100644 --- a/x/auth/ante/sigverify.go +++ b/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 } From 0d924fb600ce2d8acf8523cfa9c2a0c5fd4f6d21 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Tue, 7 Apr 2020 14:08:41 -0400 Subject: [PATCH 4/4] Add TestIncrementSequenceDecorator test --- x/auth/ante/sigverify.go | 1 + x/auth/ante/sigverify_test.go | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index f5443103758b..c40712bb0838 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -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) } diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index 35aaa8c9f806..54f74b3eac02 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -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()) + } +}