Fix sequence number handling for LegacyAmino > SignatureV2

types/tx/signing/signature.go

@@ -35,7 +35,6 @@ type SignatureV2 struct {
 	// - false (by default) in SIGN_MODE_DIRECT
 	// - true in SIGN_MODE_LEGACY_AMINO_JSON
 	// ref: https://github.com/cosmos/cosmos-sdk/issues/7229
-	SkipSequenceCheck bool
 }
 
 // SignatureDataToProto converts a SignatureData to SignatureDescriptor_Data.

x/auth/ante/sigverify.go

@@ -170,6 +170,22 @@ func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler authsigning.S
 	}
 }
 
+func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
+	switch v := sigData.(type) {
+	case *signing.SingleSignatureData:
+		return v.SignMode == signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON
+	case *signing.MultiSignatureData:
+		for _, s := range v.Signatures {
+			if !OnlyLegacyAminoSigners(s) {
+				return false
+			}
+		}
+		return true
+	default:
+		return false
+	}
+}
+
 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,7 +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 !sig.SkipSequenceCheck {
+		if !OnlyLegacyAminoSigners(sig.Data) {
 			if sig.Sequence != acc.GetSequence() {
 				return ctx, sdkerrors.Wrapf(
 					sdkerrors.ErrWrongSequence,
@@ -237,7 +253,9 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 			err := authsigning.VerifySignature(pubKey, signerData, sig.Data, svd.signModeHandler, tx)
 			if err != nil {
 				var errMsg string
-				if sig.SkipSequenceCheck {
+				if OnlyLegacyAminoSigners(sig.Data) {
+					// If all signers are using SIGN_MODE_LEGACY_AMINO, we rely on VerifySignature to check account sequence number,
+					// and therefore communicate sequence number as a potential cause of error
 					errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)", accNum, acc.GetSequence(), chainID)
 				} else {
 					errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d) and chain-id (%s)", accNum, chainID)

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/testutil/testdata"
+	"github.com/cosmos/cosmos-sdk/types/tx/signing"
+	authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
 	"testing"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -103,6 +107,70 @@ func (s *IntegrationTestSuite) TestBroadcastTxRequest() {
 	s.Require().NotEmpty(txRes.TxHash)
 }
 
+func (s *IntegrationTestSuite) TestMultipleSyncBroadcastTxRequests() {
+
+	val0 := s.network.Validators[0]
+	txConfig := authtypes.StdTxConfig{Cdc: s.cfg.LegacyAmino}
+
+	// First test transaction from validator should have sequence=1 (non-genesis tx)
+	testCases := []struct {
+		desc     string
+		sequence uint64
+	}{
+		{
+			"First tx",
+			1,
+		},
+		{
+			"Second tx",
+			2,
+		},
+	}
+	for _, tc := range testCases {
+		s.Run(fmt.Sprintf("Case %s", tc.desc), func() {
+
+			msg := &types.MsgSend{
+				val0.Address,
+				val0.Address,
+				sdk.Coins{sdk.NewInt64Coin("foo", 100)},
+			}
+
+			// prepare txBuilder with msg
+			txBuilder := txConfig.NewTxBuilder()
+			feeAmount := sdk.Coins{sdk.NewInt64Coin(s.cfg.BondDenom, 10)}
+			gasLimit := testdata.NewTestGasLimit()
+			txBuilder.SetMsgs(msg)
+			txBuilder.SetFeeAmount(feeAmount)
+			txBuilder.SetGasLimit(gasLimit)
+
+			// setup txFactory
+			txFactory := tx.Factory{}
+			txFactory = txFactory.
+				WithChainID(val0.ClientCtx.ChainID).
+				WithKeybase(val0.ClientCtx.Keyring).
+				WithTxConfig(txConfig).
+				WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON).
+				WithSequence(tc.sequence)
+
+			// sign Tx (offline mode so we can manually set sequence number)
+			err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, true)
+			s.Require().NoError(err)
+
+			// broadcast test with sync mode, as we want to run CheckTx to verify account sequence is correct
+			stdTx := txBuilder.GetTx().(authtypes.StdTx)
+			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 check for a exitCode=0, indicating a successful broadcast
+			s.Require().Equal(uint32(0), txRes.Code)
+		})
+	}
+
+}
+
 func (s *IntegrationTestSuite) broadcastReq(stdTx authtypes.StdTx, mode string) ([]byte, error) {
 	val := s.network.Validators[0]
 

x/auth/testutil/suite.go

@@ -239,7 +239,6 @@ func (s *TxConfigTestSuite) TestTxEncodeDecode() {
 			SignMode:  signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON,
 			Signature: dummySig,
 		},
-		SkipSequenceCheck: s.TxConfig.SignModeHandler().DefaultMode() == signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON,
 	}
 
 	txBuilder := s.TxConfig.NewTxBuilder()

x/auth/types/stdtx.go

@@ -395,9 +395,8 @@ func StdSignatureToSignatureV2(cdc *codec.LegacyAmino, sig StdSignature) (signin
 	}
 
 	return signing.SignatureV2{
-		PubKey:            pk,
-		Data:              data,
-		SkipSequenceCheck: true,
+		PubKey: pk,
+		Data:   data,
 	}, nil
 }