fix: Signature only flag bug on tx sign command 7632 (#8106)

* fix: Signature only flag bug on tx sign command 7632

* Update client/context.go

Co-authored-by: Cory <[email protected]>

* Update client/context.go

Co-authored-by: Cory <[email protected]>

* use named return value and closure (#8111)

This is to correctly handle deferred Close()
calls on writable files.

* set the right 'append' logic for signing transactions

* cleanup

* update tx.Sign interface by adding overwrite option

* Update Changelog

* sign command cleanup

* implementation and changelog update

* fix SignTx and tx.Sign calls

* fix: sign didn't write to a file

* update flags description

* Add tx.Sign tests

* fix grpc/server_test.go

* Update client/tx/tx.go

Co-authored-by: Cory <[email protected]>

* changelog update

* Add test to verify matching signatures

* cli_test: add integration tests for sign CMD

* add output-file flag test

* add flagAmino test

* Update x/auth/client/cli/tx_sign.go

Co-authored-by: Alessio Treglia <[email protected]>

* Update x/auth/client/cli/tx_sign.go

* update amino serialization test

* TestSign: adding unit test for signing with different modes

* Add test with Multi Signers into Robert's TxSign PR (#8142)

* Add test with Multi Signers

* remove true false

* Use SIGN_MODE_DIRECT

* Fix litn

* Use correct pubkeys

* Correct accNum and seq

* Use amino

* cleanups

* client.Sign: raise error when signing tx with multiple signers in Direct

+ added more unit tests

* add more tests

* Update client/tx/tx_test.go

Co-authored-by: Cory <[email protected]>

* fix TestGetBroadcastCommand_WithoutOfflineFlag

* Any.UnsafeSetCachedValue

* fix note packed messages in tx builder

* reorder unit tests

* Changelog update

* cleaning / linting

* cli_tes: copy validator object instead of modifying it's shared codec

* x/auth cli_test: remove custom codec creation in tests

* Update CHANGELOG.md

* updates to CHANGELOG.md

* remove unused method

* add new instance of transaction builder for TestSign

Co-authored-by: Cory <[email protected]>
Co-authored-by: SaReN <[email protected]>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Amaury <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: 6 people

CHANGELOG.md

@@ -55,15 +55,24 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (x/upgrade) [\#7979](https://github.com/cosmos/cosmos-sdk/pull/7979) keeper pubkey storage serialization migration from bech32 to protobuf.
 
+
+### Features
+* (codec/types) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) Adding `NewAnyWithCustomTypeURL` to correctly marshal Messages in TxBuilder.
+
 ### Bug Fixes
 
 * (crypto) [\#7966](https://github.com/cosmos/cosmos-sdk/issues/7966) `Bip44Params` `String()` function now correctly returns the absolute HD path by adding the `m/` prefix.
+* (x/auth/client/cli) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) fixing regression bugs in transaction signing.
+
 
 ### API Breaking
 
 * [\#8080](https://github.com/cosmos/cosmos-sdk/pull/8080) Updated the `codec.Marshaler` interface
   * Moved `MarshalAny` and `UnmarshalAny` helper functions to `codec.Marshaler` and renamed to `MarshalInterface` and `UnmarshalInterface` respectively. These functions must take interface as a parameter (not a concrete type nor `Any` object). Underneath they use `Any` wrapping for correct protobuf serialization.
 * (client) [\#8107](https://github.com/cosmos/cosmos-sdk/pull/8107) Renamed `PrintOutput` and `PrintOutputLegacy` methods of the `context.Client` object to `PrintProto` and `PrintObjectLegacy`.
+* (x/auth/tx) [\#8106](https://github.com/cosmos/cosmos-sdk/pull/8106) change related to missing append functionality in client transaction signing
+  + added `overwriteSig` argument to `x/auth/client.SignTx` and `client/tx.Sign` functions.
+  + removed `x/auth/tx.go:wrapper.GetSignatures`. The `wrapper` provides `TxBuilder` functionality, and it's a private structure. That function was not used at all and it's not exposed through the `TxBuilder` interface.
 
 
 ## [v0.40.0-rc3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.0-rc3) - 2020-11-06

client/context.go

@@ -205,14 +205,20 @@ func (ctx Context) WithInterfaceRegistry(interfaceRegistry codectypes.InterfaceR
 	return ctx
 }
 
-// PrintString prints the raw string to ctx.Output or os.Stdout
+// PrintString prints the raw string to ctx.Output if it's defined, otherwise to os.Stdout
 func (ctx Context) PrintString(str string) error {
+	return ctx.PrintBytes([]byte(str))
+}
+
+// PrintBytes prints the raw bytes to ctx.Output if it's defined, otherwise to os.Stdout.
+// NOTE: for printing a complex state object, you should use ctx.PrintOutput
+func (ctx Context) PrintBytes(o []byte) error {
 	writer := ctx.Output
 	if writer == nil {
 		writer = os.Stdout
 	}
 
-	_, err := writer.Write([]byte(str))
+	_, err := writer.Write(o)
 	return err
 }
 

client/tx/tx.go

@@ -117,7 +117,7 @@ func BroadcastTx(clientCtx client.Context, txf Factory, msgs ...sdk.Msg) error {
 		}
 	}
 
-	err = Sign(txf, clientCtx.GetFromName(), tx)
+	err = Sign(txf, clientCtx.GetFromName(), tx, true)
 	if err != nil {
 		return err
 	}
@@ -375,10 +375,21 @@ func SignWithPrivKey(
 	return sigV2, nil
 }
 
-// Sign signs a given tx with the provided name and passphrase. The bytes signed
-// over are canconical. The resulting signature will be set on the transaction.
+func checkMultipleSigners(mode signing.SignMode, tx authsigning.Tx) error {
+	if mode == signing.SignMode_SIGN_MODE_DIRECT &&
+		len(tx.GetSigners()) > 1 {
+		return sdkerrors.Wrap(sdkerrors.ErrNotSupported, "Signing in DIRECT mode is only supported for transactions with one signer only")
+	}
+	return nil
+}
+
+// Sign signs a given tx with a named key. The bytes signed over are canconical.
+// The resulting signature will be added to the transaction builder overwriting the previous
+// ones if overwrite=true (otherwise, the signature will be appended).
+// Signing a transaction with mutltiple signers in the DIRECT mode is not supprted and will
+// return an error.
 // An error is returned upon failure.
-func Sign(txf Factory, name string, txBuilder client.TxBuilder) error {
+func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig bool) error {
 	if txf.keybase == nil {
 		return errors.New("keybase must be set prior to signing a transaction")
 	}
@@ -388,12 +399,14 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error {
 		// use the SignModeHandler's default mode if unspecified
 		signMode = txf.txConfig.SignModeHandler().DefaultMode()
 	}
+	if err := checkMultipleSigners(signMode, txBuilder.GetTx()); err != nil {
+		return err
+	}
 
 	key, err := txf.keybase.Key(name)
 	if err != nil {
 		return err
 	}
-
 	pubKey := key.GetPubKey()
 	signerData := authsigning.SignerData{
 		ChainID:       txf.chainID,
@@ -418,18 +431,25 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error {
 		Data:     &sigData,
 		Sequence: txf.Sequence(),
 	}
+	var prevSignatures []signing.SignatureV2
+	if !overwriteSig {
+		prevSignatures, err = txBuilder.GetTx().GetSignaturesV2()
+		if err != nil {
+			return err
+		}
+	}
 	if err := txBuilder.SetSignatures(sig); err != nil {
 		return err
 	}
 
 	// Generate the bytes to be signed.
-	signBytes, err := txf.txConfig.SignModeHandler().GetSignBytes(signMode, signerData, txBuilder.GetTx())
+	bytesToSign, err := txf.txConfig.SignModeHandler().GetSignBytes(signMode, signerData, txBuilder.GetTx())
 	if err != nil {
 		return err
 	}
 
 	// Sign those bytes
-	sigBytes, _, err := txf.keybase.Sign(name, signBytes)
+	sigBytes, _, err := txf.keybase.Sign(name, bytesToSign)
 	if err != nil {
 		return err
 	}
@@ -445,8 +465,11 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder) error {
 		Sequence: txf.Sequence(),
 	}
 
-	// And here the tx is populated with the signature
-	return txBuilder.SetSignatures(sig)
+	if overwriteSig {
+		return txBuilder.SetSignatures(sig)
+	}
+	prevSignatures = append(prevSignatures, sig)
+	return txBuilder.SetSignatures(prevSignatures...)
 }
 
 // GasEstimateResponse defines a response definition for tx gas estimation.

client/tx/tx_test.go

@@ -10,9 +10,11 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/tx"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	txtypes "github.com/cosmos/cosmos-sdk/types/tx"
+	signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing"
 	"github.com/cosmos/cosmos-sdk/x/auth/signing"
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 )
@@ -121,49 +123,110 @@ func TestBuildUnsignedTx(t *testing.T) {
 }
 
 func TestSign(t *testing.T) {
+	requireT := require.New(t)
 	path := hd.CreateHDPath(118, 0, 0).String()
 	kr, err := keyring.New(t.Name(), "test", t.TempDir(), nil)
-	require.NoError(t, err)
-
-	var from = "test_sign"
+	requireT.NoError(err)
 
-	_, seed, err := kr.NewMnemonic(from, keyring.English, path, hd.Secp256k1)
-	require.NoError(t, err)
-	require.NoError(t, kr.Delete(from))
+	var from1 = "test_key1"
+	var from2 = "test_key2"
 
-	info, err := kr.NewAccount(from, seed, "", path, hd.Secp256k1)
-	require.NoError(t, err)
-
-	txf := tx.Factory{}.
-		WithTxConfig(NewTestTxConfig()).
-		WithAccountNumber(50).
-		WithSequence(23).
-		WithFees("50stake").
-		WithMemo("memo").
-		WithChainID("test-chain")
+	// create a new key using a mnemonic generator and test if we can reuse seed to recreate that account
+	_, seed, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1)
+	requireT.NoError(err)
+	requireT.NoError(kr.Delete(from1))
+	info1, _, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1)
+	requireT.NoError(err)
 
-	msg := banktypes.NewMsgSend(info.GetAddress(), sdk.AccAddress("to"), nil)
-	txn, err := tx.BuildUnsignedTx(txf, msg)
-	require.NoError(t, err)
+	info2, err := kr.NewAccount(from2, seed, "", path, hd.Secp256k1)
+	requireT.NoError(err)
 
-	t.Log("should failed if txf without keyring")
-	err = tx.Sign(txf, from, txn)
-	require.Error(t, err)
+	pubKey1 := info1.GetPubKey()
+	pubKey2 := info2.GetPubKey()
+	requireT.NotEqual(pubKey1.Bytes(), pubKey2.Bytes())
+	t.Log("Pub keys:", pubKey1, pubKey2)
 
-	txf = tx.Factory{}.
-		WithKeybase(kr).
+	txfNoKeybase := tx.Factory{}.
 		WithTxConfig(NewTestTxConfig()).
 		WithAccountNumber(50).
 		WithSequence(23).
 		WithFees("50stake").
 		WithMemo("memo").
 		WithChainID("test-chain")
+	txfDirect := txfNoKeybase.
+		WithKeybase(kr).
+		WithSignMode(signingtypes.SignMode_SIGN_MODE_DIRECT)
+	txfAmino := txfDirect.
+		WithSignMode(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
+	msg1 := banktypes.NewMsgSend(info1.GetAddress(), sdk.AccAddress("to"), nil)
+	msg2 := banktypes.NewMsgSend(info2.GetAddress(), sdk.AccAddress("to"), nil)
+	txb, err := tx.BuildUnsignedTx(txfNoKeybase, msg1, msg2)
+	requireT.NoError(err)
+	txb2, err := tx.BuildUnsignedTx(txfNoKeybase, msg1, msg2)
+	requireT.NoError(err)
+	txbSimple, err := tx.BuildUnsignedTx(txfNoKeybase, msg2)
+	requireT.NoError(err)
 
-	t.Log("should succeed if txf with keyring")
-	err = tx.Sign(txf, from, txn)
-	require.NoError(t, err)
+	testCases := []struct {
+		name         string
+		txf          tx.Factory
+		txb          client.TxBuilder
+		from         string
+		overwrite    bool
+		expectedPKs  []cryptotypes.PubKey
+		matchingSigs []int // if not nil, check matching signature against old ones.
+	}{
+		{"should fail if txf without keyring",
+			txfNoKeybase, txb, from1, true, nil, nil},
+		{"should fail for non existing key",
+			txfAmino, txb, "unknown", true, nil, nil},
+		{"amino: should succeed with keyring",
+			txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
+		{"direct: should succeed with keyring",
+			txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
+
+		/**** test double sign Amino mode ****/
+		{"amino: should sign multi-signers tx",
+			txfAmino, txb, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
+		{"amino: should append a second signature and not overwrite",
+			txfAmino, txb, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}},
+		{"amino: should overwrite a signature",
+			txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}},
+
+		/**** test double sign Direct mode
+		  signing transaction with more than 2 signers should fail in DIRECT mode ****/
+		{"direct: should fail to append a signature with different mode",
+			txfDirect, txb, from1, false, []cryptotypes.PubKey{}, nil},
+		{"direct: should fail to sign multi-signers tx",
+			txfDirect, txb2, from1, false, []cryptotypes.PubKey{}, nil},
+		{"direct: should fail to overwrite multi-signers tx",
+			txfDirect, txb2, from1, true, []cryptotypes.PubKey{}, nil},
+	}
+	var prevSigs []signingtypes.SignatureV2
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err = tx.Sign(tc.txf, tc.from, tc.txb, tc.overwrite)
+			if len(tc.expectedPKs) == 0 {
+				requireT.Error(err)
+			} else {
+				requireT.NoError(err)
+				sigs := testSigners(requireT, tc.txb.GetTx(), tc.expectedPKs...)
+				if tc.matchingSigs != nil {
+					requireT.Equal(prevSigs[tc.matchingSigs[0]], sigs[tc.matchingSigs[1]])
+				}
+				prevSigs = sigs
+			}
+		})
+	}
+}
 
-	t.Log("should fail for non existing key")
-	err = tx.Sign(txf, "non_existing_key", txn)
-	require.Error(t, err)
+func testSigners(require *require.Assertions, tr signing.Tx, pks ...cryptotypes.PubKey) []signingtypes.SignatureV2 {
+	sigs, err := tr.GetSignaturesV2()
+	require.Len(sigs, len(pks))
+	require.NoError(err)
+	require.Len(sigs, len(pks))
+	for i := range pks {
+		require.True(sigs[i].PubKey.Equals(pks[i]), "Signature is signed with a wrong pubkey. Got: %s, expected: %s", sigs[i].PubKey, pks[i])
+	}
+	return sigs
 }

codec/types/any.go

@@ -69,6 +69,19 @@ func NewAnyWithValue(value proto.Message) (*Any, error) {
 	return any, nil
 }
 
+// NewAnyWithCustomTypeURL same as NewAnyWithValue, but sets a custom type url, instead
+// using the one from proto.Message.
+// NOTE: This functions should be only used for types with additional logic bundled
+// into the protobuf Any serialization. For simple marshaling you should use NewAnyWithValue.
+func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) {
+	bz, err := proto.Marshal(v)
+	return &Any{
+		TypeUrl:     typeURL,
+		Value:       bz,
+		cachedValue: v,
+	}, err
+}
+
 // Pack packs the value x in the Any or returns an error. This also caches
 // the packed value so that it can be retrieved from GetCachedValue without
 // unmarshaling

server/grpc/server_test.go

@@ -154,7 +154,7 @@ func (s *IntegrationTestSuite) TestGRPCServer_BroadcastTx() {
 		WithSignMode(signing.SignMode_SIGN_MODE_DIRECT)
 
 	// Sign Tx.
-	err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, false)
+	err := authclient.SignTx(txFactory, val0.ClientCtx, val0.Moniker, txBuilder, false, true)
 	s.Require().NoError(err)
 
 	txBytes, err := val0.ClientCtx.TxConfig.TxEncoder()(txBuilder.GetTx())

simapp/simd/cmd/testnet.go

@@ -230,7 +230,7 @@ func InitTestnet(
 			WithKeybase(kb).
 			WithTxConfig(clientCtx.TxConfig)
 
-		if err := tx.Sign(txFactory, nodeDirName, txBuilder); err != nil {
+		if err := tx.Sign(txFactory, nodeDirName, txBuilder, true); err != nil {
 			return err
 		}
 

testutil/network/network.go

@@ -323,7 +323,7 @@ func New(t *testing.T, cfg Config) *Network {
 			WithKeybase(kb).
 			WithTxConfig(cfg.TxConfig)
 
-		err = tx.Sign(txFactory, nodeDirName, txBuilder)
+		err = tx.Sign(txFactory, nodeDirName, txBuilder, true)
 		require.NoError(t, err)
 
 		txBz, err := cfg.TxConfig.TxJSONEncoder()(txBuilder.GetTx())

types/errors/errors.go

@@ -131,6 +131,10 @@ var (
 	// the same resource and one of them fails.
 	ErrConflict = Register(RootCodespace, 36, "conflict")
 
+	// ErrNotSupported is returned when we call a branch of a code which is currently not
+	// supported.
+	ErrNotSupported = Register(RootCodespace, 37, "feature not supported")
+
 	// ErrPanic is only set when we recover from a panic, so we know to
 	// redact potentially sensitive system info
 	ErrPanic = Register(UndefinedCodespace, 111222, "panic")

types/tx/types.go

@@ -28,6 +28,10 @@ func (t *Tx) GetMsgs() []sdk.Msg {
 	for i, any := range anys {
 		var msg sdk.Msg
 		if isServiceMsg(any.TypeUrl) {
+			req := any.GetCachedValue()
+			if req == nil {
+				panic("Any cached value is nil. Transaction messages must be correctly packed Any values.")
+			}
 			msg = sdk.ServiceMsg{
 				MethodName: any.TypeUrl,
 				Request:    any.GetCachedValue().(sdk.MsgRequest),

x/auth/client/cli/broadcast.go

@@ -26,6 +26,10 @@ $ <appd> tx broadcast ./mytxn.json
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx := client.GetClientContextFromCmd(cmd)
+			clientCtx, err := client.ReadTxCommandFlags(clientCtx, cmd.Flags())
+			if err != nil {
+				return err
+			}
 
 			if offline, _ := cmd.Flags().GetBool(flags.FlagOffline); offline {
 				return errors.New("cannot broadcast tx during offline mode")