From 764cfe33aa55acb46316b3e8fdfa72c7bd49a6e4 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 10 Jan 2019 22:47:20 +0000 Subject: [PATCH] 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 --- CHANGELOG_PENDING.md | 1 + crypto/ed25519/ed25519.go | 8 +++---- crypto/encoding/amino/amino.go | 28 ++++++++++++------------ crypto/encoding/amino/encode_test.go | 10 ++++----- crypto/multisig/threshold_pubkey.go | 12 +++++----- crypto/multisig/threshold_pubkey_test.go | 16 ++++++++++++++ crypto/multisig/wire.go | 4 ++-- crypto/secp256k1/secp256k1.go | 8 +++---- types/params.go | 4 ++-- types/protobuf.go | 6 ++--- 10 files changed, 57 insertions(+), 40 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index aaf80a4f2..2cedb02fc 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/crypto/ed25519/ed25519.go b/crypto/ed25519/ed25519.go index 0c659e73f..bc60838d5 100644 --- a/crypto/ed25519/ed25519.go +++ b/crypto/ed25519/ed25519.go @@ -18,8 +18,8 @@ import ( var _ crypto.PrivKey = PrivKeyEd25519{} const ( - PrivKeyAminoRoute = "tendermint/PrivKeyEd25519" - PubKeyAminoRoute = "tendermint/PubKeyEd25519" + PrivKeyAminoName = "tendermint/PrivKeyEd25519" + 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. diff --git a/crypto/encoding/amino/amino.go b/crypto/encoding/amino/amino.go index d66ecd9b1..f7be3a203 100644 --- a/crypto/encoding/amino/amino.go +++ b/crypto/encoding/amino/amino.go @@ -12,11 +12,11 @@ import ( var cdc = amino.NewCodec() -// routeTable is used to map public key concrete types back -// to their amino routes. This should eventually be handled +// nameTable is used to map public key concrete types back +// to their registered amino names. This should eventually be handled // by amino. Example usage: -// routeTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoRoute -var routeTable = make(map[reflect.Type]string, 3) +// nameTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoName +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 - routeTable[reflect.TypeOf(secp256k1.PubKeySecp256k1{})] = secp256k1.PubKeyAminoRoute - routeTable[reflect.TypeOf(&multisig.PubKeyMultisigThreshold{})] = multisig.PubKeyMultisigThresholdAminoRoute + nameTable[reflect.TypeOf(ed25519.PubKeyEd25519{})] = ed25519.PubKeyAminoName + nameTable[reflect.TypeOf(secp256k1.PubKeySecp256k1{})] = secp256k1.PubKeyAminoName + 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) { - route, found := routeTable[reflect.TypeOf(key)] +func PubkeyAminoName(cdc *amino.Codec, key crypto.PubKey) (string, bool) { + 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) { diff --git a/crypto/encoding/amino/encode_test.go b/crypto/encoding/amino/encode_test.go index 056dbec44..c8cb24136 100644 --- a/crypto/encoding/amino/encode_test.go +++ b/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}, - {secp256k1.PubKeySecp256k1{}, secp256k1.PubKeyAminoRoute, true}, - {&multisig.PubKeyMultisigThreshold{}, multisig.PubKeyMultisigThresholdAminoRoute, true}, + {ed25519.PubKeyEd25519{}, ed25519.PubKeyAminoName, true}, + {secp256k1.PubKeySecp256k1{}, secp256k1.PubKeyAminoName, 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) diff --git a/crypto/multisig/threshold_pubkey.go b/crypto/multisig/threshold_pubkey.go index ca8d42303..41abc1e50 100644 --- a/crypto/multisig/threshold_pubkey.go +++ b/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 diff --git a/crypto/multisig/threshold_pubkey_test.go b/crypto/multisig/threshold_pubkey_test.go index bfc874ebe..d442e649b 100644 --- a/crypto/multisig/threshold_pubkey_test.go +++ b/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) diff --git a/crypto/multisig/wire.go b/crypto/multisig/wire.go index 68b84fbfb..71e0db144 100644 --- a/crypto/multisig/wire.go +++ b/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) } diff --git a/crypto/secp256k1/secp256k1.go b/crypto/secp256k1/secp256k1.go index 7fc46d634..d3528fdd6 100644 --- a/crypto/secp256k1/secp256k1.go +++ b/crypto/secp256k1/secp256k1.go @@ -16,8 +16,8 @@ import ( //------------------------------------- const ( - PrivKeyAminoRoute = "tendermint/PrivKeySecp256k1" - PubKeyAminoRoute = "tendermint/PubKeySecp256k1" + PrivKeyAminoName = "tendermint/PrivKeySecp256k1" + 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) } //------------------------------------- diff --git a/types/params.go b/types/params.go index ec8a8f576..91079e76b 100644 --- a/types/params.go +++ b/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) } diff --git a/types/protobuf.go b/types/protobuf.go index 0f0d25de3..eed73b568 100644 --- a/types/protobuf.go +++ b/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{ - ABCIPubKeyTypeEd25519: ed25519.PubKeyAminoRoute, - ABCIPubKeyTypeSecp256k1: secp256k1.PubKeyAminoRoute, +var ABCIPubKeyTypesToAminoNames = map[string]string{ + ABCIPubKeyTypeEd25519: ed25519.PubKeyAminoName, + ABCIPubKeyTypeSecp256k1: secp256k1.PubKeyAminoName, } //-------------------------------------------------------