From 5f71e93b1e0f9a7d9617f5ea88f5c18b73361a7b Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 15 Mar 2021 12:18:46 +0000 Subject: [PATCH 1/3] add --output-document to multisign-batch (#8873) Closes: #8870 Co-authored-by: SaReN --- x/auth/client/cli/tx_multisign.go | 12 +++++++++++- x/auth/client/cli/tx_sign.go | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/x/auth/client/cli/tx_multisign.go b/x/auth/client/cli/tx_multisign.go index c04d37123c2f..7fdcace968f5 100644 --- a/x/auth/client/cli/tx_multisign.go +++ b/x/auth/client/cli/tx_multisign.go @@ -217,6 +217,7 @@ The SIGN_MODE_DIRECT sign mode is not supported.' flagMultisig, "", "Address of the multisig account that the transaction signs on behalf of", ) + cmd.Flags().String(flags.FlagOutputDocument, "", "The document is written to the given file instead of STDOUT") flags.AddTxFlagsToCmd(cmd) return cmd @@ -277,6 +278,15 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error { txFactory = txFactory.WithAccountNumber(accnum).WithSequence(seq) } + // prepare output document + closeFunc, err := setOutputFile(cmd) + if err != nil { + return err + } + + defer closeFunc() + clientCtx.WithOutput(cmd.OutOrStdout()) + for i := 0; scanner.Scan(); i++ { txBldr, err := txCfg.WrapTxBuilder(scanner.Tx()) if err != nil { @@ -350,7 +360,7 @@ func makeBatchMultisignCmd() func(cmd *cobra.Command, args []string) error { txFactory = txFactory.WithSequence(sequence) } - return nil + return scanner.UnmarshalErr() } } diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index 955554512125..f814f9b99a41 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -137,7 +137,7 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error { return err } - return scanner.Err() + return scanner.UnmarshalErr() } } From d4d27e1c0c138d76ab3082ba8e380d4d26db050a Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Mon, 15 Mar 2021 13:42:10 +0100 Subject: [PATCH 2/3] Fix multisig LegacyAminoPubKey Amino marshaling (#8841) * Use v034auth RegisterCrypto * Add custom amino for LegacyAminoPubKey * Fix registercrypto * Revert old PR * revert some genutil stuff * Add comment * Add changelog * Remove binary marshalling * Fix lint * Fix lint again * Fix lint, 3rd time's a charm * ignore wrong linter warning * Fix UnmarshalAmioJSON Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Alessio Treglia Co-authored-by: Anil Kumar Kammari --- CHANGELOG.md | 1 + crypto/keys/multisig/amino.go | 88 +++++++++++++++++++++++++++ crypto/keys/multisig/codec.go | 3 + crypto/keys/multisig/multisig_test.go | 78 +++++++++++++++++++++++- x/auth/legacy/legacytx/stdtx.go | 2 +- x/auth/legacy/v040/migrate.go | 11 +--- x/genutil/legacy/v038/migrate.go | 2 + 7 files changed, 172 insertions(+), 13 deletions(-) create mode 100644 crypto/keys/multisig/amino.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a8819d154f7e..ae7ce9a177fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [\#8434](https://github.com/cosmos/cosmos-sdk/pull/8434) Fix legacy REST API `GET /bank/total` and `GET /bank/total/{denom}` in swagger * (x/slashing) [\#8427](https://github.com/cosmos/cosmos-sdk/pull/8427) Fix query signing infos command * (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value +* (crypto) [\#8841](https://github.com/cosmos/cosmos-sdk/pull/8841) Fix legacy multisig amino marshaling, allowing migrations to work between v0.39 and v0.40+. ## [v0.42.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.0) - 2021-03-08 diff --git a/crypto/keys/multisig/amino.go b/crypto/keys/multisig/amino.go new file mode 100644 index 000000000000..4849a23173d2 --- /dev/null +++ b/crypto/keys/multisig/amino.go @@ -0,0 +1,88 @@ +package multisig + +import ( + types "github.com/cosmos/cosmos-sdk/codec/types" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// tmMultisig implements a K of N threshold multisig. It is used for +// Amino JSON marshaling of LegacyAminoPubKey (see below for details). +// +// This struct is copy-pasted from: +// https://github.com/tendermint/tendermint/blob/v0.33.9/crypto/multisig/threshold_pubkey.go +// +// This struct was used in the SDK <=0.39. In 0.40 and the switch to protobuf, +// it has been converted to LegacyAminoPubKey. However, there's one difference: +// the threshold field was an `uint` before, and an `uint32` after. This caused +// amino marshaling to be breaking: amino marshals `uint32` as a JSON number, +// and `uint` as a JSON string. +// +// In this file, we're overriding LegacyAminoPubKey's default JSON Amino +// marshaling by using this struct. Please note that we are NOT overriding the +// Amino binary marshaling, as that _might_ introduce breaking changes in the +// keyring, where multisigs are amino-binary-encoded. +// +// ref: https://github.com/cosmos/cosmos-sdk/issues/8776 +type tmMultisig struct { + K uint `json:"threshold"` + PubKeys []cryptotypes.PubKey `json:"pubkeys"` +} + +// protoToTm converts a LegacyAminoPubKey into a tmMultisig. +func protoToTm(protoPk *LegacyAminoPubKey) (tmMultisig, error) { + var ok bool + pks := make([]cryptotypes.PubKey, len(protoPk.PubKeys)) + for i, pk := range protoPk.PubKeys { + pks[i], ok = pk.GetCachedValue().(cryptotypes.PubKey) + if !ok { + return tmMultisig{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (cryptotypes.PubKey)(nil), pk.GetCachedValue()) + } + } + + return tmMultisig{ + K: uint(protoPk.Threshold), + PubKeys: pks, + }, nil +} + +// tmToProto converts a tmMultisig into a LegacyAminoPubKey. +func tmToProto(tmPk tmMultisig) (*LegacyAminoPubKey, error) { + var err error + pks := make([]*types.Any, len(tmPk.PubKeys)) + for i, pk := range tmPk.PubKeys { + pks[i], err = types.NewAnyWithValue(pk) + if err != nil { + return nil, err + } + } + + return &LegacyAminoPubKey{ + Threshold: uint32(tmPk.K), + PubKeys: pks, + }, nil +} + +// MarshalAminoJSON overrides amino JSON unmarshaling. +func (m LegacyAminoPubKey) MarshalAminoJSON() (tmMultisig, error) { //nolint:golint + return protoToTm(&m) +} + +// UnmarshalAminoJSON overrides amino JSON unmarshaling. +func (m *LegacyAminoPubKey) UnmarshalAminoJSON(tmPk tmMultisig) error { + protoPk, err := tmToProto(tmPk) + if err != nil { + return err + } + + // Instead of just doing `*m = *protoPk`, we prefer to modify in-place the + // existing Anys inside `m` (instead of allocating new Anys), as so not to + // break the `.compat` fields in the existing Anys. + for i := range m.PubKeys { + m.PubKeys[i].TypeUrl = protoPk.PubKeys[i].TypeUrl + m.PubKeys[i].Value = protoPk.PubKeys[i].Value + } + m.Threshold = protoPk.Threshold + + return nil +} diff --git a/crypto/keys/multisig/codec.go b/crypto/keys/multisig/codec.go index b92576e4f3f5..d501e1b427cf 100644 --- a/crypto/keys/multisig/codec.go +++ b/crypto/keys/multisig/codec.go @@ -15,6 +15,9 @@ const ( PubKeyAminoRoute = "tendermint/PubKeyMultisigThreshold" ) +//nolint +// Deprecated: Amino is being deprecated in the SDK. But even if you need to +// use Amino for some reason, please use `codec/legacy.Cdc` instead. var AminoCdc = codec.NewLegacyAmino() func init() { diff --git a/crypto/keys/multisig/multisig_test.go b/crypto/keys/multisig/multisig_test.go index 69b4f10d2694..197a433a72eb 100644 --- a/crypto/keys/multisig/multisig_test.go +++ b/crypto/keys/multisig/multisig_test.go @@ -6,7 +6,9 @@ import ( "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/codec/legacy" "github.com/cosmos/cosmos-sdk/codec/types" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" @@ -272,12 +274,12 @@ func TestPubKeyMultisigThresholdAminoToIface(t *testing.T) { pubkeys, _ := generatePubKeysAndSignatures(5, msg) multisigKey := kmultisig.NewLegacyAminoPubKey(2, pubkeys) - ab, err := kmultisig.AminoCdc.MarshalBinaryLengthPrefixed(multisigKey) + ab, err := legacy.Cdc.MarshalBinaryLengthPrefixed(multisigKey) require.NoError(t, err) // like other cryptotypes.Pubkey implementations (e.g. ed25519.PubKey), // LegacyAminoPubKey should be deserializable into a cryptotypes.LegacyAminoPubKey: var pubKey kmultisig.LegacyAminoPubKey - err = kmultisig.AminoCdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey) + err = legacy.Cdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey) require.NoError(t, err) require.Equal(t, multisigKey.Equals(&pubKey), true) @@ -329,3 +331,75 @@ func reorderPubKey(pk *kmultisig.LegacyAminoPubKey) (other *kmultisig.LegacyAmin other = &kmultisig.LegacyAminoPubKey{Threshold: 2, PubKeys: pubkeysCpy} return } + +func TestAminoBinary(t *testing.T) { + pubKey1 := secp256k1.GenPrivKey().PubKey() + pubKey2 := secp256k1.GenPrivKey().PubKey() + multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2}) + + // Do a round-trip key->bytes->key. + bz, err := legacy.Cdc.MarshalBinaryBare(multisigKey) + require.NoError(t, err) + var newMultisigKey cryptotypes.PubKey + err = legacy.Cdc.UnmarshalBinaryBare(bz, &newMultisigKey) + require.NoError(t, err) + require.Equal(t, multisigKey.Threshold, newMultisigKey.(*kmultisig.LegacyAminoPubKey).Threshold) +} + +func TestAminoMarshalJSON(t *testing.T) { + pubKey1 := secp256k1.GenPrivKey().PubKey() + pubKey2 := secp256k1.GenPrivKey().PubKey() + multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2}) + + bz, err := legacy.Cdc.MarshalJSON(multisigKey) + require.NoError(t, err) + + // Note the quotes around `"2"`. They are present because we are overriding + // the Amino JSON marshaling of LegacyAminoPubKey (using tmMultisig). + // Without the override, there would not be any quotes. + require.Contains(t, string(bz), "\"threshold\":\"2\"") +} + +func TestAminoUnmarshalJSON(t *testing.T) { + // This is a real multisig from the Akash chain. It has been exported from + // v0.39, hence the `threshold` field as a string. + // We are testing that when unmarshaling this JSON into a LegacyAminoPubKey + // with amino, there's no error. + // ref: https://github.com/cosmos/cosmos-sdk/issues/8776 + pkJSON := `{ + "type": "tendermint/PubKeyMultisigThreshold", + "value": { + "pubkeys": [ + { + "type": "tendermint/PubKeySecp256k1", + "value": "AzYxq2VNeD10TyABwOgV36OVWDIMn8AtI4OFA0uQX2MK" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A39cdsrm00bTeQ3RVZVqjkH8MvIViO9o99c8iLiNO35h" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A/uLLCZph8MkFg2tCxqSMGwFfPHdt1kkObmmrqy9aiYD" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A4mOMhM5gPDtBAkAophjRs6uDGZm4tD4Dbok3ai4qJi8" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A90icFucrjNNz2SAdJWMApfSQcARIqt+M2x++t6w5fFs" + } + ], + "threshold": "3" + } +}` + + cdc := codec.NewLegacyAmino() + cryptocodec.RegisterCrypto(cdc) + + var pk cryptotypes.PubKey + err := cdc.UnmarshalJSON([]byte(pkJSON), &pk) + require.NoError(t, err) + require.Equal(t, uint32(3), pk.(*kmultisig.LegacyAminoPubKey).Threshold) +} diff --git a/x/auth/legacy/legacytx/stdtx.go b/x/auth/legacy/legacytx/stdtx.go index c1a982c85efe..c2b60736e4c3 100644 --- a/x/auth/legacy/legacytx/stdtx.go +++ b/x/auth/legacy/legacytx/stdtx.go @@ -287,7 +287,7 @@ func (tx StdTx) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { // Signatures contain PubKeys, which need to be unpacked. for _, s := range tx.Signatures { - err := codectypes.UnpackInterfaces(s, unpacker) + err := s.UnpackInterfaces(unpacker) if err != nil { return err } diff --git a/x/auth/legacy/v040/migrate.go b/x/auth/legacy/v040/migrate.go index f819a1a33d8b..4ba3e0cb7c81 100644 --- a/x/auth/legacy/v040/migrate.go +++ b/x/auth/legacy/v040/migrate.go @@ -2,7 +2,6 @@ package v040 import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" - multisigtypes "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" v039auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v039" v040auth "github.com/cosmos/cosmos-sdk/x/auth/types" @@ -13,15 +12,7 @@ import ( func convertBaseAccount(old *v039auth.BaseAccount) *v040auth.BaseAccount { var any *codectypes.Any - _, ok := old.PubKey.(*multisigtypes.LegacyAminoPubKey) - - // If pubkey is multisig type, then leave it as nil for now - // Ref: https://github.com/cosmos/cosmos-sdk/issues/8776#issuecomment-790552126 - // Else if the old genesis had a pubkey, we pack it inside an Any. - // Or else, we just leave it nil. - if ok { - // TODO migrate multisig public_keys - } else if old.PubKey != nil { + if old.PubKey != nil { var err error any, err = codectypes.NewAnyWithValue(old.PubKey) if err != nil { diff --git a/x/genutil/legacy/v038/migrate.go b/x/genutil/legacy/v038/migrate.go index d0ca4c4186bc..017d0e68fa53 100644 --- a/x/genutil/legacy/v038/migrate.go +++ b/x/genutil/legacy/v038/migrate.go @@ -3,6 +3,7 @@ package v038 import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" v036auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v036" v038auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v038" v036distr "github.com/cosmos/cosmos-sdk/x/distribution/legacy/v036" @@ -19,6 +20,7 @@ import ( // Migrate migrates exported state from v0.36/v0.37 to a v0.38 genesis state. func Migrate(appState types.AppMap, _ client.Context) types.AppMap { v036Codec := codec.NewLegacyAmino() + cryptocodec.RegisterCrypto(v036Codec) v036gov.RegisterLegacyAminoCodec(v036Codec) v036distr.RegisterLegacyAminoCodec(v036Codec) v036params.RegisterLegacyAminoCodec(v036Codec) From 42919c8f338535d1bf2147ae299026a8389eeae8 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 15 Mar 2021 13:50:12 +0000 Subject: [PATCH 3/3] server: wrap errors (#8879) Follow-up of #8842 --- server/test_helpers.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/test_helpers.go b/server/test_helpers.go index bb19df695d45..68d87aa08d62 100644 --- a/server/test_helpers.go +++ b/server/test_helpers.go @@ -3,6 +3,8 @@ package server import ( "fmt" "net" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) // Get a free address for a test tendermint server @@ -14,7 +16,7 @@ func FreeTCPAddr() (addr, port string, err error) { } if err := l.Close(); err != nil { - return "", "", fmt.Errorf("couldn't close the listener: %w", err) + return "", "", sdkerrors.Wrap(err, "couldn't close the listener") } portI := l.Addr().(*net.TCPAddr).Port