Fix sequence number handling for LegacyAmino > SignatureV2

x/auth/ante/ante_test.go

@@ -4,6 +4,8 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/cosmos/cosmos-sdk/client"
+	"github.com/cosmos/cosmos-sdk/codec"
 	"strings"
 	"testing"
 
@@ -19,6 +21,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 )
 
 // Test that simulate transaction accurately estimates gas cost
@@ -433,6 +436,139 @@ func (suite *AnteTestSuite) TestAnteHandlerSequences() {
 	}
 }
 
+// This test is exactly like the one above, but we set the codec explicitly to
+// Amino.
+// Once https://github.com/cosmos/cosmos-sdk/issues/6190 is in, we can remove
+// this, since it'll be handled by the test matrix.
+// In the meantime, we want to make double-sure amino compatibility works.
+// ref: https://github.com/cosmos/cosmos-sdk/issues/7229
+func (suite *AnteTestSuite) TestAnteHandlerSequences_ExplicitAmino() {
+	suite.app, suite.ctx = createTestApp(true)
+	suite.ctx = suite.ctx.WithBlockHeight(1)
+
+	// Set up TxConfig.
+	aminoCdc := codec.NewLegacyAmino()
+	// We're using TestMsg amino encoding in some tests, so register it here.
+	txConfig := authtypes.StdTxConfig{Cdc: aminoCdc}
+
+	suite.clientCtx = client.Context{}.
+		WithTxConfig(txConfig)
+
+	suite.anteHandler = ante.NewAnteHandler(suite.app.AccountKeeper, suite.app.BankKeeper, ante.DefaultSigVerificationGasConsumer, txConfig.SignModeHandler())
+
+	suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
+
+	// make block height non-zero to ensure account numbers part of signBytes
+	suite.ctx = suite.ctx.WithBlockHeight(1)
+
+	// Same data for every test cases
+	accounts := suite.CreateTestAccounts(3)
+	feeAmount := testdata.NewTestFeeAmount()
+	gasLimit := testdata.NewTestGasLimit()
+
+	// Variable data per test case
+	var (
+		accNums []uint64
+		msgs    []sdk.Msg
+		privs   []crypto.PrivKey
+		accSeqs []uint64
+	)
+
+	testCases := []TestCase{
+		{
+			"good tx from one signer",
+			func() {
+				msg := testdata.NewTestMsg(accounts[0].acc.GetAddress())
+				msgs = []sdk.Msg{msg}
+
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[0].priv}, []uint64{0}, []uint64{0}
+			},
+			false,
+			true,
+			nil,
+		},
+		{
+			"test sending it again fails (replay protection)",
+			func() {
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[0].priv}, []uint64{0}, []uint64{0}
+			},
+			false,
+			false,
+			sdkerrors.ErrUnauthorized,
+		},
+		{
+			"fix sequence, should pass",
+			func() {
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[0].priv}, []uint64{0}, []uint64{1}
+			},
+			false,
+			true,
+			nil,
+		},
+		{
+			"new tx with another signer and correct sequences",
+			func() {
+				msg1 := testdata.NewTestMsg(accounts[0].acc.GetAddress(), accounts[1].acc.GetAddress())
+				msg2 := testdata.NewTestMsg(accounts[2].acc.GetAddress(), accounts[0].acc.GetAddress())
+				msgs = []sdk.Msg{msg1, msg2}
+
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[0].priv, accounts[1].priv, accounts[2].priv}, []uint64{0, 1, 2}, []uint64{2, 0, 0}
+			},
+			false,
+			true,
+			nil,
+		},
+		{
+			"replay fails",
+			func() {},
+			false,
+			false,
+			sdkerrors.ErrUnauthorized,
+		},
+		{
+			"tx from just second signer with incorrect sequence fails",
+			func() {
+				msg := testdata.NewTestMsg(accounts[1].acc.GetAddress())
+				msgs = []sdk.Msg{msg}
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[1].priv}, []uint64{1}, []uint64{0}
+			},
+			false,
+			false,
+			sdkerrors.ErrUnauthorized,
+		},
+		{
+			"fix the sequence and it passes",
+			func() {
+				accSeqs = []uint64{1}
+			},
+			false,
+			true,
+			nil,
+		},
+		{
+			"fix the sequence and it passes",
+			func() {
+				msg := testdata.NewTestMsg(accounts[0].acc.GetAddress(), accounts[1].acc.GetAddress())
+				msgs = []sdk.Msg{msg}
+
+				privs, accNums, accSeqs = []crypto.PrivKey{accounts[0].priv, accounts[1].priv}, []uint64{0, 1}, []uint64{3, 2}
+			},
+			false,
+			true,
+			nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.desc), func() {
+			suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
+			tc.malleate()
+
+			suite.RunTestCase(privs, msgs, feeAmount, gasLimit, accNums, accSeqs, suite.ctx.ChainID(), tc)
+		})
+	}
+}
+
 // Test logic around fee deduction.
 func (suite *AnteTestSuite) TestAnteHandlerFees() {
 	suite.SetupTest(true) // setup

x/auth/ante/sigverify.go

@@ -170,6 +170,22 @@ func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler authsigning.S
 	}
 }
 
+func ContainsSignModeDirect(sigData signing.SignatureData) bool {
+	switch v := sigData.(type) {
+	case *signing.SingleSignatureData:
+		return v.SignMode == signing.SignMode_SIGN_MODE_DIRECT
+	case *signing.MultiSignatureData:
+		for _, s := range v.Signatures {
+			if ContainsSignModeDirect(s) {
+				return true
+			}
+		}
+		return false
+	default:
+		panic("Type Mismatch")
+	}
+}
+
 func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
 	// no need to verify signatures on recheck tx
 	if ctx.IsReCheckTx() {
@@ -211,6 +227,7 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 		// the SignatureV2 struct (it's only in the SignDoc). In this case, we
 		// cannot check sequence directly, and must do it via signature
 		// verification.
+		// if ContainsSignModeDirect(sig.Data) {
 		if !sig.SkipSequenceCheck {
 			if sig.Sequence != acc.GetSequence() {
 				return ctx, sdkerrors.Wrapf(

x/auth/client/rest/rest_test.go

@@ -2,6 +2,10 @@ package rest_test
 
 import (
 	"fmt"
+	"github.com/cosmos/cosmos-sdk/client/tx"
+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
+	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	"testing"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -103,6 +107,47 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
 	s.Require().NotEmpty(txRes.TxHash)
 }
 
+func (s *IntegrationTestSuite) TestMultipleSignedBroadcastTxRequests() {
+
+	// Set up TxConfig.
+	aminoCdc := codec.NewLegacyAmino()
+	// We're using TestMsg amino encoding in some tests, so register it here.
+	txConfig := authtypes.StdTxConfig{Cdc: aminoCdc}
+	txBuilder := txConfig.NewTxBuilder()
+
+	val0 := s.network.Validators[0]
+	val1 := s.network.Validators[0]
+	msg := types.MsgSend{FromAddress: val0.Address, ToAddress: val1.Address, Amount: sdk.Coins{sdk.NewInt64Coin("foo", 100)}}
+
+	feeAmount := sdk.Coins{sdk.NewInt64Coin("stake", 10)}
+	gasLimit := testdata.NewTestGasLimit()
+	txBuilder.SetMsgs(&msg)
+	txBuilder.SetFeeAmount(feeAmount)
+	txBuilder.SetGasLimit(gasLimit)
+
+	txFactory := tx.Factory{}
+	txFactory = txFactory.
+		WithChainID(val0.ClientCtx.ChainID).
+		WithKeybase(val0.ClientCtx.Keyring).
+		WithTxConfig(val0.ClientCtx.TxConfig).
+		WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
+
+	err := tx.Sign(txFactory, val0.Moniker, txBuilder)
+	s.Require().NoError(err)
+
+	stdTx := txBuilder.GetTx().(authtypes.StdTx)
+
+	// we just test with async mode because this tx will fail - all we care about is that it got encoded and broadcast correctly
+	res, err := s.broadcastReq(stdTx, "sync")
+	s.Require().NoError(err)
+	var txRes sdk.TxResponse
+	// NOTE: this uses amino explicitly, don't migrate it!
+	s.Require().NoError(s.cfg.LegacyAmino.UnmarshalJSON(res, &txRes))
+	// we just check for a non-empty TxHash here, the actual hash will depend on the underlying tx configuration
+	s.Require().Equal(txRes, sdk.TxResponse{})
+
+}
+
 func (s *IntegrationTestSuite) broadcastReq(stdTx authtypes.StdTx, mode string) ([]byte, error) {
 	val := s.network.Validators[0]