Remove old PubKeyMultisigThreshold (#7284)

* WIP on protobuf keys

* Use Type() and Bytes() in sr25519 pub key Equals

* Add tests

* Add few more tests

* Update other pub/priv key types Equals

* Fix PrivKey's Sign method

* Rename variables in tests

* Fix infinite recursive calls

* Use tm ed25519 keys

* Add Sign and VerifySignature tests

* Remove ed25519 and sr25519 references

* proto linting

* Add proto crypto file

* Implement some of the new multisig proto type methods

* Add tests for MultisigThresholdPubKey

* Add tests for pubkey pb/amino conversion functions

* Move crypto types.go and register new proto pubkeys

* Add missing pointer ref

* Address review comments

* panic in MultisigThresholdPubKey VerifySignature

* Use internal crypto.PubKey in multisig

* Add tests for MultisigThresholdPubKey VerifyMultisignature

* Only keep LegacyAminoMultisigThresholdPubKey and move to proto keys to v1

* Remove conversion functions and introduce internal PubKey type

* Start removal of old PubKeyMultisigThreshold references

* Remove old secp256k1 PubKey and PrivKey

* Uncomment test case

* Fix linting issues

* More linting

* Revert tests keys values

* Add Amino overrides to proto keys

* Add pubkey test

* Fix tests

* Use threshold isntead of K

* Standardize Type

* Revert standardize types commit

* Add comment

* Simplify proto names

* Fixed merge issues

* Uncomment tests

* Remove old multisig

* Add amino marshal for multisig

* Fix lint

* Correctly register amino

* One test left!

* Remove old struct

* Fix test

* Fix test

* Unpack into tmcrypto

* Remove old threshold pubkey tests

* Fix register amino

* Fix lint

* Use sdk crypto PubKey in multisig UnpackInterfaces

* Potential fix?

* Use anil's suggestion

Co-authored-by: Aaron Craelius <aaronc@users.noreply.github.com>
Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>

client/keys/add.go

@@ -16,7 +16,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/input"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
@@ -174,7 +174,7 @@ func RunAddCmd(cmd *cobra.Command, args []string, kb keyring.Keyring, inBuf *buf
 				})
 			}
 
-			pk := multisig.NewPubKeyMultisigThreshold(multisigThreshold, pks)
+			pk := multisig.NewLegacyAminoPubKey(multisigThreshold, pks)
 			if _, err := kb.SaveMultisig(name, pk); err != nil {
 				return err
 			}

client/keys/show.go

@@ -10,8 +10,8 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/crypto/ledger"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
@@ -83,7 +83,7 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) {
 			return err
 		}
 
-		multikey := multisig.NewPubKeyMultisigThreshold(multisigThreshold, pks)
+		multikey := multisig.NewLegacyAminoPubKey(multisigThreshold, pks)
 		info = keyring.NewMultiInfo(defaultMultiSigKeyName, multikey)
 	}
 

client/keys/show_test.go

@@ -11,15 +11,18 @@ import (
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 func Test_multiSigKey_Properties(t *testing.T) {
 	tmpKey1 := secp256k1.GenPrivKeyFromSecret([]byte("mySecret"))
-	pk := multisig.NewPubKeyMultisigThreshold(1, []crypto.PubKey{tmpKey1.PubKey()})
+	pk := multisig.NewLegacyAminoPubKey(
+		1,
+		[]crypto.PubKey{tmpKey1.PubKey()},
+	)
 	tmp := keyring.NewMultiInfo("myMultisig", pk)
 
 	require.Equal(t, "myMultisig", tmp.GetName())

crypto/codec/amino.go

@@ -8,7 +8,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/codec"
 	kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 )
 
 var amino *codec.LegacyAmino
@@ -21,20 +21,19 @@ func init() {
 // RegisterCrypto registers all crypto dependency types with the provided Amino
 // codec.
 func RegisterCrypto(cdc *codec.LegacyAmino) {
+	// TODO We now register both Tendermint's PubKey and our own PubKey. In the
+	// long-term, we should move away from Tendermint's PubKey, and delete this
+	// first line.
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
+	cdc.RegisterInterface((*cryptotypes.PubKey)(nil), nil)
 	cdc.RegisterConcrete(ed25519.PubKey{},
 		ed25519.PubKeyName, nil)
 	cdc.RegisterConcrete(sr25519.PubKey{},
 		sr25519.PubKeyName, nil)
 	cdc.RegisterConcrete(&secp256k1.PubKey{},
 		secp256k1.PubKeyName, nil)
-	// TODO Follow-up in https://github.com/cosmos/cosmos-sdk/pull/7284
-	// Remove `multisig.PubKeyMultisigThreshold{}`, and register instead
-	// kmultisig.LegacyAminoPubKey{} on `PubKeyAminoRoute`.
-	cdc.RegisterConcrete(multisig.PubKeyMultisigThreshold{},
-		multisig.PubKeyAminoRoute, nil)
 	cdc.RegisterConcrete(&kmultisig.LegacyAminoPubKey{},
-		"cosmos-sdk/LegacyAminoPubKey", nil)
+		kmultisig.PubKeyAminoRoute, nil)
 
 	cdc.RegisterInterface((*crypto.PrivKey)(nil), nil)
 	cdc.RegisterConcrete(ed25519.PrivKey{},

crypto/keyring/info.go

@@ -5,8 +5,9 @@ import (
 
 	"github.com/tendermint/tendermint/crypto"
 
+	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/types"
 )
 
@@ -191,18 +192,18 @@ type multiInfo struct {
 
 // NewMultiInfo creates a new multiInfo instance
 func NewMultiInfo(name string, pub crypto.PubKey) Info {
-	multiPK := pub.(multisig.PubKeyMultisigThreshold)
+	multiPK := pub.(*multisig.LegacyAminoPubKey)
 
 	pubKeys := make([]multisigPubKeyInfo, len(multiPK.PubKeys))
-	for i, pk := range multiPK.PubKeys {
+	for i, pk := range multiPK.GetPubKeys() {
 		// TODO: Recursively check pk for total weight?
 		pubKeys[i] = multisigPubKeyInfo{pk, 1}
 	}
 
 	return &multiInfo{
 		Name:      name,
 		PubKey:    pub,
-		Threshold: multiPK.K,
+		Threshold: uint(multiPK.Threshold),
 		PubKeys:   pubKeys,
 	}
 }
@@ -237,6 +238,13 @@ func (i multiInfo) GetPath() (*hd.BIP44Params, error) {
 	return nil, fmt.Errorf("BIP44 Paths are not available for this type")
 }
 
+// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
+func (i multiInfo) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {
+	multiPK := i.PubKey.(*multisig.LegacyAminoPubKey)
+
+	return codectypes.UnpackInterfaces(multiPK, unpacker)
+}
+
 // encoding info
 func marshalInfo(i Info) []byte {
 	return CryptoCdc.MustMarshalBinaryLengthPrefixed(i)
@@ -245,5 +253,24 @@ func marshalInfo(i Info) []byte {
 // decoding info
 func unmarshalInfo(bz []byte) (info Info, err error) {
 	err = CryptoCdc.UnmarshalBinaryLengthPrefixed(bz, &info)
+	if err != nil {
+		return nil, err
+	}
+
+	// After unmarshalling into &info, if we notice that the info is a
+	// multiInfo, then we unmarshal again, explicitly in a multiInfo this time.
+	// Since multiInfo implements UnpackInterfacesMessage, this will correctly
+	// unpack the underlying anys inside the multiInfo.
+	//
+	// This is a workaround, as go cannot check that an interface (Info)
+	// implements another interface (UnpackInterfacesMessage).
+	_, ok := info.(multiInfo)
+	if ok {
+		var multi multiInfo
+		err = CryptoCdc.UnmarshalBinaryLengthPrefixed(bz, &multi)
+
+		return multi, err
+	}
+
 	return
 }

crypto/keyring/keyring_test.go

@@ -13,8 +13,8 @@ import (
 
 	"github.com/cosmos/cosmos-sdk/crypto"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	"github.com/cosmos/cosmos-sdk/testutil"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
@@ -390,10 +390,11 @@ func TestInMemoryLanguage(t *testing.T) {
 func TestInMemoryCreateMultisig(t *testing.T) {
 	kb, err := New("keybasename", "memory", "", nil)
 	require.NoError(t, err)
-	multi := multisig.PubKeyMultisigThreshold{
-		K:       1,
-		PubKeys: []tmcrypto.PubKey{secp256k1.GenPrivKey().PubKey()},
-	}
+	multi := multisig.NewLegacyAminoPubKey(
+		1, []tmcrypto.PubKey{
+			secp256k1.GenPrivKey().PubKey(),
+		},
+	)
 	_, err = kb.SaveMultisig("multi", multi)
 	require.NoError(t, err)
 }
@@ -980,7 +981,13 @@ func TestAltKeyring_SaveMultisig(t *testing.T) {
 	require.NoError(t, err)
 
 	key := "multi"
-	pub := multisig.NewPubKeyMultisigThreshold(2, []tmcrypto.PubKey{mnemonic1.GetPubKey(), mnemonic2.GetPubKey()})
+	pub := multisig.NewLegacyAminoPubKey(
+		2,
+		[]tmcrypto.PubKey{
+			&secp256k1.PubKey{Key: mnemonic1.GetPubKey().Bytes()},
+			&secp256k1.PubKey{Key: mnemonic2.GetPubKey().Bytes()},
+		},
+	)
 
 	info, err := keyring.SaveMultisig(key, pub)
 	require.Nil(t, err)

crypto/keyring/output_test.go

@@ -6,8 +6,8 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/tendermint/tendermint/crypto"
 
+	kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
@@ -16,7 +16,7 @@ func TestBech32KeysOutput(t *testing.T) {
 	bechTmpKey := sdk.MustBech32ifyPubKey(sdk.Bech32PubKeyTypeAccPub, tmpKey)
 	tmpAddr := sdk.AccAddress(tmpKey.Address().Bytes())
 
-	multisigPks := multisig.NewPubKeyMultisigThreshold(1, []crypto.PubKey{tmpKey})
+	multisigPks := kmultisig.NewLegacyAminoPubKey(1, []crypto.PubKey{tmpKey})
 	multiInfo := NewMultiInfo("multisig", multisigPks)
 	accAddr := sdk.AccAddress(multiInfo.GetPubKey().Address().Bytes())
 	bechPubKey := sdk.MustBech32ifyPubKey(sdk.Bech32PubKeyTypeAccPub, multiInfo.GetPubKey())

crypto/keys/multisig/codec.go

@@ -0,0 +1,35 @@
+package multisig
+
+import (
+	"github.com/tendermint/tendermint/crypto"
+	"github.com/tendermint/tendermint/crypto/ed25519"
+	"github.com/tendermint/tendermint/crypto/sr25519"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
+)
+
+// TODO: Figure out API for others to either add their own pubkey types, or
+// to make verify / marshal accept a AminoCdc.
+const (
+	PubKeyAminoRoute = "tendermint/PubKeyMultisigThreshold"
+)
+
+var AminoCdc = codec.NewLegacyAmino()
+
+func init() {
+	// TODO We now register both Tendermint's PubKey and our own PubKey. In the
+	// long-term, we should move away from Tendermint's PubKey, and delete this
+	// first line.
+	AminoCdc.RegisterInterface((*crypto.PubKey)(nil), nil)
+	AminoCdc.RegisterInterface((*cryptotypes.PubKey)(nil), nil)
+	AminoCdc.RegisterConcrete(ed25519.PubKey{},
+		ed25519.PubKeyName, nil)
+	AminoCdc.RegisterConcrete(sr25519.PubKey{},
+		sr25519.PubKeyName, nil)
+	AminoCdc.RegisterConcrete(&secp256k1.PubKey{},
+		secp256k1.PubKeyName, nil)
+	AminoCdc.RegisterConcrete(&LegacyAminoPubKey{},
+		PubKeyAminoRoute, nil)
+}

crypto/keys/multisig/multisig.go

@@ -3,34 +3,46 @@ package multisig
 import (
 	fmt "fmt"
 
-	"github.com/cosmos/cosmos-sdk/codec"
+	tmcrypto "github.com/tendermint/tendermint/crypto"
 
 	"github.com/cosmos/cosmos-sdk/codec/types"
 	crypto "github.com/cosmos/cosmos-sdk/crypto/types"
+	multisigtypes "github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
-	tmcrypto "github.com/tendermint/tendermint/crypto"
-
-	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
+	proto "github.com/gogo/protobuf/proto"
 )
 
-// In the refactor in https://github.com/cosmos/cosmos-sdk/pull/7284, make sure
-// to use AminoCdc here.
-var cdc = codec.NewProtoCodec(types.NewInterfaceRegistry())
+var _ multisigtypes.PubKey = &LegacyAminoPubKey{}
+var _ types.UnpackInterfacesMessage = &LegacyAminoPubKey{}
 
-var _ multisig.PubKey = &LegacyAminoPubKey{}
+// NewLegacyAminoPubKey returns a new LegacyAminoPubKey.
+// Panics if len(pubKeys) < k or 0 >= k.
+func NewLegacyAminoPubKey(k int, pubKeys []tmcrypto.PubKey) *LegacyAminoPubKey {
+	if k <= 0 {
+		panic("threshold k of n multisignature: k <= 0")
+	}
+	if len(pubKeys) < k {
+		panic("threshold k of n multisignature: len(pubKeys) < k")
+	}
+	anyPubKeys, err := packPubKeys(pubKeys)
+	if err != nil {
+		panic(err)
+	}
+	return &LegacyAminoPubKey{Threshold: uint32(k), PubKeys: anyPubKeys}
+}
 
 // Address implements crypto.PubKey Address method
-func (m *LegacyAminoPubKey) Address() crypto.Address {
+func (m *LegacyAminoPubKey) Address() tmcrypto.Address {
 	return tmcrypto.AddressHash(m.Bytes())
 }
 
 // Bytes returns the proto encoded version of the LegacyAminoPubKey
 func (m *LegacyAminoPubKey) Bytes() []byte {
-	return cdc.MustMarshalBinaryBare(m)
+	return AminoCdc.MustMarshalBinaryBare(m)
 }
 
-// VerifyMultisignature implements the multisig.PubKey VerifyMultisignature method
-func (m *LegacyAminoPubKey) VerifyMultisignature(getSignBytes multisig.GetSignBytesFunc, sig *signing.MultiSignatureData) error {
+// VerifyMultisignature implements the multisigtypes.PubKey VerifyMultisignature method
+func (m *LegacyAminoPubKey) VerifyMultisignature(getSignBytes multisigtypes.GetSignBytesFunc, sig *signing.MultiSignatureData) error {
 	bitarray := sig.BitArray
 	sigs := sig.Signatures
 	size := bitarray.Count()
@@ -62,7 +74,7 @@ func (m *LegacyAminoPubKey) VerifyMultisignature(getSignBytes multisig.GetSignBy
 					return err
 				}
 			case *signing.MultiSignatureData:
-				nestedMultisigPk, ok := pubKeys[i].(multisig.PubKey)
+				nestedMultisigPk, ok := pubKeys[i].(multisigtypes.PubKey)
 				if !ok {
 					return fmt.Errorf("unable to parse pubkey of index %d", i)
 				}
@@ -101,7 +113,7 @@ func (m *LegacyAminoPubKey) GetPubKeys() []tmcrypto.PubKey {
 // Equals returns true if m and other both have the same number of keys, and
 // all constituent keys are the same, and in the same order.
 func (m *LegacyAminoPubKey) Equals(key tmcrypto.PubKey) bool {
-	otherKey, ok := key.(multisig.PubKey)
+	otherKey, ok := key.(multisigtypes.PubKey)
 	if !ok {
 		return false
 	}
@@ -128,3 +140,28 @@ func (m *LegacyAminoPubKey) GetThreshold() uint {
 func (m *LegacyAminoPubKey) Type() string {
 	return "PubKeyMultisigThreshold"
 }
+
+// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
+func (m *LegacyAminoPubKey) UnpackInterfaces(unpacker types.AnyUnpacker) error {
+	for _, any := range m.PubKeys {
+		var pk crypto.PubKey
+		err := unpacker.UnpackAny(any, &pk)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func packPubKeys(pubKeys []tmcrypto.PubKey) ([]*types.Any, error) {
+	anyPubKeys := make([]*types.Any, len(pubKeys))
+
+	for i := 0; i < len(pubKeys); i++ {
+		any, err := types.NewAnyWithValue(pubKeys[i].(proto.Message))
+		if err != nil {
+			return nil, err
+		}
+		anyPubKeys[i] = any
+	}
+	return anyPubKeys, nil
+}