Don't use pointer receivers for PubKeyMultisigThreshold (#3100)

* Don't use pointer receivers for PubKeyMultisigThreshold

* test that showcases panic when PubKeyMultisigThreshold are used in sdk:

 - deserialization will fail in `readInfo` which tries to read a
 `crypto.PubKey` into a `localInfo` (called by
  cosmos-sdk/client/keys.GetKeyInfo)

* Update changelog

* Rename routeTable to nameTable, multisig key is no longer a pointer

* sed -i 's/PubKeyAminoRoute/PubKeyAminoName/g' `grep -lrw PubKeyAminoRoute .`

upon Jae's request

* AminoRoutes -> AminoNames

* sed -e 's/PrivKeyAminoRoute/PrivKeyAminoName/g'

* Update crypto/encoding/amino/amino.go

Co-Authored-By: alessio <quadrispro@ubuntu.com>

CHANGELOG_PENDING.md

@@ -32,3 +32,4 @@ Special thanks to external contributors on this release:
 
 ### BUG FIXES:
 - [types] \#2926 do not panic if retrieving the private validator's public key fails
+- [crypto/encoding] \#3101 Fix `PubKeyMultisigThreshold` unmarshalling into `crypto.PubKey` interface

crypto/ed25519/ed25519.go

@@ -18,8 +18,8 @@ import (
 var _ crypto.PrivKey = PrivKeyEd25519{}
 
 const (
-	PrivKeyAminoRoute = "tendermint/PrivKeyEd25519"
+	PrivKeyAminoName = "tendermint/PrivKeyEd25519"
-	PubKeyAminoRoute  = "tendermint/PubKeyEd25519"
+	PubKeyAminoName  = "tendermint/PubKeyEd25519"
 	// Size of an Edwards25519 signature. Namely the size of a compressed
 	// Edwards25519 point, and a field element. Both of which are 32 bytes.
 	SignatureSize = 64
@@ -30,11 +30,11 @@ var cdc = amino.NewCodec()
 func init() {
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
 	cdc.RegisterConcrete(PubKeyEd25519{},
-		PubKeyAminoRoute, nil)
+		PubKeyAminoName, nil)
 
 	cdc.RegisterInterface((*crypto.PrivKey)(nil), nil)
 	cdc.RegisterConcrete(PrivKeyEd25519{},
-		PrivKeyAminoRoute, nil)
+		PrivKeyAminoName, nil)
 }
 
 // PrivKeyEd25519 implements crypto.PrivKey.

crypto/encoding/amino/amino.go

@@ -12,11 +12,11 @@ import (
 
 var cdc = amino.NewCodec()
 
-// routeTable is used to map public key concrete types back
+// nameTable is used to map public key concrete types back
-// to their amino routes. This should eventually be handled
+// to their registered amino names. This should eventually be handled
 // by amino. Example usage:
-// routeTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoRoute
+// nameTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoName
-var routeTable = make(map[reflect.Type]string, 3)
+var nameTable = make(map[reflect.Type]string, 3)
 
 func init() {
 	// NOTE: It's important that there be no conflicts here,
@@ -29,16 +29,16 @@ func init() {
 
 	// TODO: Have amino provide a way to go from concrete struct to route directly.
 	// Its currently a private API
-	routeTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoRoute
+	nameTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoName
-	routeTable[reflect.TypeOf(secp256k1.PubKeySecp256k1{})] = secp256k1.PubKeyAminoRoute
+	nameTable[reflect.TypeOf(secp256k1.PubKeySecp256k1{})] = secp256k1.PubKeyAminoName
-	routeTable[reflect.TypeOf(&multisig.PubKeyMultisigThreshold{})] = multisig.PubKeyMultisigThresholdAminoRoute
+	nameTable[reflect.TypeOf(multisig.PubKeyMultisigThreshold{})] = multisig.PubKeyMultisigThresholdAminoRoute
 }
 
-// PubkeyAminoRoute returns the amino route of a pubkey
+// PubkeyAminoName returns the amino route of a pubkey
 // cdc is currently passed in, as eventually this will not be using
 // a package level codec.
-func PubkeyAminoRoute(cdc *amino.Codec, key crypto.PubKey) (string, bool) {
+func PubkeyAminoName(cdc *amino.Codec, key crypto.PubKey) (string, bool) {
-	route, found := routeTable[reflect.TypeOf(key)]
+	route, found := nameTable[reflect.TypeOf(key)]
 	return route, found
 }
 
@@ -47,17 +47,17 @@ func RegisterAmino(cdc *amino.Codec) {
 	// These are all written here instead of
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
 	cdc.RegisterConcrete(ed25519.PubKeyEd25519{},
-		ed25519.PubKeyAminoRoute, nil)
+		ed25519.PubKeyAminoName, nil)
 	cdc.RegisterConcrete(secp256k1.PubKeySecp256k1{},
-		secp256k1.PubKeyAminoRoute, nil)
+		secp256k1.PubKeyAminoName, nil)
 	cdc.RegisterConcrete(multisig.PubKeyMultisigThreshold{},
 		multisig.PubKeyMultisigThresholdAminoRoute, nil)
 
 	cdc.RegisterInterface((*crypto.PrivKey)(nil), nil)
 	cdc.RegisterConcrete(ed25519.PrivKeyEd25519{},
-		ed25519.PrivKeyAminoRoute, nil)
+		ed25519.PrivKeyAminoName, nil)
 	cdc.RegisterConcrete(secp256k1.PrivKeySecp256k1{},
-		secp256k1.PrivKeyAminoRoute, nil)
+		secp256k1.PrivKeyAminoName, nil)
 }
 
 func PrivKeyFromBytes(privKeyBytes []byte) (privKey crypto.PrivKey, err error) {

crypto/encoding/amino/encode_test.go

@@ -128,18 +128,18 @@ func TestPubKeyInvalidDataProperReturnsEmpty(t *testing.T) {
 	require.Nil(t, pk)
 }
 
-func TestPubkeyAminoRoute(t *testing.T) {
+func TestPubkeyAminoName(t *testing.T) {
 	tests := []struct {
 		key   crypto.PubKey
 		want  string
 		found bool
 	}{
-		{ed25519.PubKeyEd25519{}, ed25519.PubKeyAminoRoute, true},
+		{ed25519.PubKeyEd25519{}, ed25519.PubKeyAminoName, true},
-		{secp256k1.PubKeySecp256k1{}, secp256k1.PubKeyAminoRoute, true},
+		{secp256k1.PubKeySecp256k1{}, secp256k1.PubKeyAminoName, true},
-		{&multisig.PubKeyMultisigThreshold{}, multisig.PubKeyMultisigThresholdAminoRoute, true},
+		{multisig.PubKeyMultisigThreshold{}, multisig.PubKeyMultisigThresholdAminoRoute, true},
 	}
 	for i, tc := range tests {
-		got, found := PubkeyAminoRoute(cdc, tc.key)
+		got, found := PubkeyAminoName(cdc, tc.key)
 		require.Equal(t, tc.found, found, "not equal on tc %d", i)
 		if tc.found {
 			require.Equal(t, tc.want, got, "not equal on tc %d", i)

crypto/multisig/threshold_pubkey.go

@@ -11,7 +11,7 @@ type PubKeyMultisigThreshold struct {
 	PubKeys []crypto.PubKey `json:"pubkeys"`
 }
 
-var _ crypto.PubKey = &PubKeyMultisigThreshold{}
+var _ crypto.PubKey = PubKeyMultisigThreshold{}
 
 // NewPubKeyMultisigThreshold returns a new PubKeyMultisigThreshold.
 // Panics if len(pubkeys) < k or 0 >= k.
@@ -22,7 +22,7 @@ func NewPubKeyMultisigThreshold(k int, pubkeys []crypto.PubKey) crypto.PubKey {
 	if len(pubkeys) < k {
 		panic("threshold k of n multisignature: len(pubkeys) < k")
 	}
-	return &PubKeyMultisigThreshold{uint(k), pubkeys}
+	return PubKeyMultisigThreshold{uint(k), pubkeys}
 }
 
 // VerifyBytes expects sig to be an amino encoded version of a MultiSignature.
@@ -31,7 +31,7 @@ func NewPubKeyMultisigThreshold(k int, pubkeys []crypto.PubKey) crypto.PubKey {
 // and all signatures are valid. (Not just k of the signatures)
 // The multisig uses a bitarray, so multiple signatures for the same key is not
 // a concern.
-func (pk *PubKeyMultisigThreshold) VerifyBytes(msg []byte, marshalledSig []byte) bool {
+func (pk PubKeyMultisigThreshold) VerifyBytes(msg []byte, marshalledSig []byte) bool {
 	var sig *Multisignature
 	err := cdc.UnmarshalBinaryBare(marshalledSig, &sig)
 	if err != nil {
@@ -64,18 +64,18 @@ func (pk *PubKeyMultisigThreshold) VerifyBytes(msg []byte, marshalledSig []byte)
 }
 
 // Bytes returns the amino encoded version of the PubKeyMultisigThreshold
-func (pk *PubKeyMultisigThreshold) Bytes() []byte {
+func (pk PubKeyMultisigThreshold) Bytes() []byte {
 	return cdc.MustMarshalBinaryBare(pk)
 }
 
 // Address returns tmhash(PubKeyMultisigThreshold.Bytes())
-func (pk *PubKeyMultisigThreshold) Address() crypto.Address {
+func (pk PubKeyMultisigThreshold) Address() crypto.Address {
 	return crypto.Address(tmhash.Sum(pk.Bytes()))
 }
 
 // Equals returns true iff pk and other both have the same number of keys, and
 // all constituent keys are the same, and in the same order.
-func (pk *PubKeyMultisigThreshold) Equals(other crypto.PubKey) bool {
+func (pk PubKeyMultisigThreshold) Equals(other crypto.PubKey) bool {
 	otherKey, sameType := other.(*PubKeyMultisigThreshold)
 	if !sameType {
 		return false

crypto/multisig/threshold_pubkey_test.go

@@ -95,6 +95,22 @@ func TestMultiSigPubKeyEquality(t *testing.T) {
 	require.False(t, multisigKey.Equals(multisigKey2))
 }
 
+func TestPubKeyMultisigThresholdAminoToIface(t *testing.T) {
+	msg := []byte{1, 2, 3, 4}
+	pubkeys, _ := generatePubKeysAndSignatures(5, msg)
+	multisigKey := NewPubKeyMultisigThreshold(2, pubkeys)
+
+	ab, err := cdc.MarshalBinaryLengthPrefixed(multisigKey)
+	require.NoError(t, err)
+	// like other crypto.Pubkey implementations (e.g. ed25519.PubKeyEd25519),
+	// PubKeyMultisigThreshold should be deserializable into a crypto.PubKey:
+	var pubKey crypto.PubKey
+	err = cdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey)
+	require.NoError(t, err)
+
+	require.Equal(t, multisigKey, pubKey)
+}
+
 func generatePubKeysAndSignatures(n int, msg []byte) (pubkeys []crypto.PubKey, signatures [][]byte) {
 	pubkeys = make([]crypto.PubKey, n)
 	signatures = make([][]byte, n)

crypto/multisig/wire.go

@@ -20,7 +20,7 @@ func init() {
 	cdc.RegisterConcrete(PubKeyMultisigThreshold{},
 		PubKeyMultisigThresholdAminoRoute, nil)
 	cdc.RegisterConcrete(ed25519.PubKeyEd25519{},
-		ed25519.PubKeyAminoRoute, nil)
+		ed25519.PubKeyAminoName, nil)
 	cdc.RegisterConcrete(secp256k1.PubKeySecp256k1{},
-		secp256k1.PubKeyAminoRoute, nil)
+		secp256k1.PubKeyAminoName, nil)
 }

crypto/secp256k1/secp256k1.go

@@ -16,8 +16,8 @@ import (
 
 //-------------------------------------
 const (
-	PrivKeyAminoRoute = "tendermint/PrivKeySecp256k1"
+	PrivKeyAminoName = "tendermint/PrivKeySecp256k1"
-	PubKeyAminoRoute  = "tendermint/PubKeySecp256k1"
+	PubKeyAminoName  = "tendermint/PubKeySecp256k1"
 )
 
 var cdc = amino.NewCodec()
@@ -25,11 +25,11 @@ var cdc = amino.NewCodec()
 func init() {
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
 	cdc.RegisterConcrete(PubKeySecp256k1{},
-		PubKeyAminoRoute, nil)
+		PubKeyAminoName, nil)
 
 	cdc.RegisterInterface((*crypto.PrivKey)(nil), nil)
 	cdc.RegisterConcrete(PrivKeySecp256k1{},
-		PrivKeyAminoRoute, nil)
+		PrivKeyAminoName, nil)
 }
 
 //-------------------------------------

types/params.go

@@ -34,7 +34,7 @@ type EvidenceParams struct {
 }
 
 // ValidatorParams restrict the public key types validators can use.
-// NOTE: uses ABCI pubkey naming, not Amino routes.
+// NOTE: uses ABCI pubkey naming, not Amino names.
 type ValidatorParams struct {
 	PubKeyTypes []string `json:"pub_key_types"`
 }
@@ -107,7 +107,7 @@ func (params *ConsensusParams) Validate() error {
 	// Check if keyType is a known ABCIPubKeyType
 	for i := 0; i < len(params.Validator.PubKeyTypes); i++ {
 		keyType := params.Validator.PubKeyTypes[i]
-		if _, ok := ABCIPubKeyTypesToAminoRoutes[keyType]; !ok {
+		if _, ok := ABCIPubKeyTypesToAminoNames[keyType]; !ok {
 			return cmn.NewError("params.Validator.PubKeyTypes[%d], %s, is an unknown pubkey type",
 				i, keyType)
 		}

types/protobuf.go

@@ -25,9 +25,9 @@ const (
 )
 
 // TODO: Make non-global by allowing for registration of more pubkey types
-var ABCIPubKeyTypesToAminoRoutes = map[string]string{
+var ABCIPubKeyTypesToAminoNames = map[string]string{
-	ABCIPubKeyTypeEd25519:   ed25519.PubKeyAminoRoute,
+	ABCIPubKeyTypeEd25519:   ed25519.PubKeyAminoName,
-	ABCIPubKeyTypeSecp256k1: secp256k1.PubKeyAminoRoute,
+	ABCIPubKeyTypeSecp256k1: secp256k1.PubKeyAminoName,
 }
 
 //-------------------------------------------------------