diff --git a/CHANGELOG.md b/CHANGELOG.md index a8819d154f7..ae7ce9a177f 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 00000000000..4849a23173d --- /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 b92576e4f3f..d501e1b427c 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 69b4f10d269..197a433a72e 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 c1a982c85ef..c2b60736e4c 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 f819a1a33d8..4ba3e0cb7c8 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 d0ca4c4186b..017d0e68fa5 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)