Fix multisig LegacyAminoPubKey Amino marshaling

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
 

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
+}

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() {

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)
+}

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
 		}

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 {

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)