Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup up Multisig naming #2255

Merged
merged 6 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions crypto/encoding/amino/amino.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package cryptoAmino

import (
amino "github.com/tendermint/go-amino"

"github.com/tendermint/tendermint/crypto"
"github.com/tendermint/tendermint/crypto/ed25519"
"github.com/tendermint/tendermint/crypto/multisig"
"github.com/tendermint/tendermint/crypto/secp256k1"
)

Expand All @@ -24,15 +26,17 @@ func RegisterAmino(cdc *amino.Codec) {
// These are all written here instead of
cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
cdc.RegisterConcrete(ed25519.PubKeyEd25519{},
"tendermint/PubKeyEd25519", nil)
ed25519.PubKeyAminoRoute, nil)
cdc.RegisterConcrete(secp256k1.PubKeySecp256k1{},
"tendermint/PubKeySecp256k1", nil)
secp256k1.PubKeyAminoRoute, nil)
cdc.RegisterConcrete(multisig.PubKeyMultisigThreshold{},
multisig.PubKeyMultisigThresholdAminoRoute, nil)

cdc.RegisterInterface((*crypto.PrivKey)(nil), nil)
cdc.RegisterConcrete(ed25519.PrivKeyEd25519{},
"tendermint/PrivKeyEd25519", nil)
ed25519.PrivKeyAminoRoute, nil)
cdc.RegisterConcrete(secp256k1.PrivKeySecp256k1{},
"tendermint/PrivKeySecp256k1", nil)
secp256k1.PrivKeyAminoRoute, nil)
}

func PrivKeyFromBytes(privKeyBytes []byte) (privKey crypto.PrivKey, err error) {
Expand Down
1 change: 1 addition & 0 deletions crypto/encoding/amino/encode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func ExamplePrintRegisteredTypes() {
//| ---- | ---- | ------ | ----- | ------ |
//| PubKeyEd25519 | tendermint/PubKeyEd25519 | 0x1624DE64 | 0x20 | |
//| PubKeySecp256k1 | tendermint/PubKeySecp256k1 | 0xEB5AE987 | 0x21 | |
//| PubKeyMultisigThreshold | tendermint/PubKeyMultisigThreshold | 0x22C1F7E2 | variable | |
//| PrivKeyEd25519 | tendermint/PrivKeyEd25519 | 0xA3288910 | 0x40 | |
//| PrivKeySecp256k1 | tendermint/PrivKeySecp256k1 | 0xE1B0F79B | 0x20 | |
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package multisig
package bitarray

import (
"bytes"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package multisig
package bitarray

import (
"encoding/json"
Expand Down
9 changes: 5 additions & 4 deletions crypto/multisig/multisignature.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,21 @@ import (
"errors"

"github.com/tendermint/tendermint/crypto"
bA "github.com/tendermint/tendermint/crypto/multisig/bitarray"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This alias does more harm than good, obscuring what is imported here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

)

// Multisignature is used to represent the signature object used in the multisigs.
// Sigs is a list of signatures, sorted by corresponding index.
type Multisignature struct {
BitArray *CompactBitArray
BitArray *bA.CompactBitArray
Sigs [][]byte
}

// NewMultisig returns a new Multisignature of size n.
func NewMultisig(n int) *Multisignature {
// Default the signature list to have a capacity of two, since we can
// expect that most multisigs will require multiple signers.
return &Multisignature{NewCompactBitArray(n), make([][]byte, 0, 2)}
return &Multisignature{bA.NewCompactBitArray(n), make([][]byte, 0, 2)}
}

// GetIndex returns the index of pk in keys. Returns -1 if not found
Expand Down Expand Up @@ -52,9 +53,9 @@ func (mSig *Multisignature) AddSignature(sig []byte, index int) {
mSig.Sigs[newSigIndex] = sig
}

// AddSignatureFromPubkey adds a signature to the multisig,
// AddSignatureFromPubKey adds a signature to the multisig,
// at the index in keys corresponding to the provided pubkey.
func (mSig *Multisignature) AddSignatureFromPubkey(sig []byte, pubkey crypto.PubKey, keys []crypto.PubKey) error {
func (mSig *Multisignature) AddSignatureFromPubKey(sig []byte, pubkey crypto.PubKey, keys []crypto.PubKey) error {
index := getIndex(pubkey, keys)
if index == -1 {
return errors.New("provided key didn't exist in pubkeys")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,24 @@ import (
"github.com/tendermint/tendermint/crypto/tmhash"
)

// ThresholdMultiSignaturePubKey implements a K of N threshold multisig.
type ThresholdMultiSignaturePubKey struct {
// PubKeyMultisigThreshold implements a K of N threshold multisig.
type PubKeyMultisigThreshold struct {
K uint `json:"threshold"`
Pubkeys []crypto.PubKey `json:"pubkeys"`
PubKeys []crypto.PubKey `json:"pubkeys"`
}

var _ crypto.PubKey = &ThresholdMultiSignaturePubKey{}
var _ crypto.PubKey = &PubKeyMultisigThreshold{}

// NewThresholdMultiSignaturePubKey returns a new ThresholdMultiSignaturePubKey.
// NewPubKeyMultisigThreshold returns a new PubKeyMultisigThreshold.
// Panics if len(pubkeys) < k or 0 >= k.
func NewThresholdMultiSignaturePubKey(k int, pubkeys []crypto.PubKey) crypto.PubKey {
func NewPubKeyMultisigThreshold(k int, pubkeys []crypto.PubKey) crypto.PubKey {
if k <= 0 {
panic("threshold k of n multisignature: k <= 0")
}
if len(pubkeys) < k {
panic("threshold k of n multisignature: len(pubkeys) < k")
}
return &ThresholdMultiSignaturePubKey{uint(k), pubkeys}
return &PubKeyMultisigThreshold{uint(k), pubkeys}
}

// VerifyBytes expects sig to be an amino encoded version of a MultiSignature.
Expand All @@ -31,15 +31,15 @@ func NewThresholdMultiSignaturePubKey(k int, pubkeys []crypto.PubKey) crypto.Pub
// 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 *ThresholdMultiSignaturePubKey) 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 {
return false
}
size := sig.BitArray.Size()
// ensure bit array is the correct size
if len(pk.Pubkeys) != size {
if len(pk.PubKeys) != size {
return false
}
// ensure size of signature list
Expand All @@ -54,7 +54,7 @@ func (pk *ThresholdMultiSignaturePubKey) VerifyBytes(msg []byte, marshalledSig [
sigIndex := 0
for i := 0; i < size; i++ {
if sig.BitArray.GetIndex(i) {
if !pk.Pubkeys[i].VerifyBytes(msg, sig.Sigs[sigIndex]) {
if !pk.PubKeys[i].VerifyBytes(msg, sig.Sigs[sigIndex]) {
return false
}
sigIndex++
Expand All @@ -63,28 +63,28 @@ func (pk *ThresholdMultiSignaturePubKey) VerifyBytes(msg []byte, marshalledSig [
return true
}

// Bytes returns the amino encoded version of the ThresholdMultiSignaturePubKey
func (pk *ThresholdMultiSignaturePubKey) Bytes() []byte {
// Bytes returns the amino encoded version of the PubKeyMultisigThreshold
func (pk *PubKeyMultisigThreshold) Bytes() []byte {
return cdc.MustMarshalBinaryBare(pk)
}

// Address returns tmhash(ThresholdMultiSignaturePubKey.Bytes())
func (pk *ThresholdMultiSignaturePubKey) Address() crypto.Address {
// Address returns tmhash(PubKeyMultisigThreshold.Bytes())
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 *ThresholdMultiSignaturePubKey) Equals(other crypto.PubKey) bool {
otherKey, sameType := other.(*ThresholdMultiSignaturePubKey)
func (pk *PubKeyMultisigThreshold) Equals(other crypto.PubKey) bool {
otherKey, sameType := other.(*PubKeyMultisigThreshold)
if !sameType {
return false
}
if pk.K != otherKey.K || len(pk.Pubkeys) != len(otherKey.Pubkeys) {
if pk.K != otherKey.K || len(pk.PubKeys) != len(otherKey.PubKeys) {
return false
}
for i := 0; i < len(pk.Pubkeys); i++ {
if !pk.Pubkeys[i].Equals(otherKey.Pubkeys[i]) {
for i := 0; i < len(pk.PubKeys); i++ {
if !pk.PubKeys[i].Equals(otherKey.PubKeys[i]) {
return false
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,30 +34,30 @@ func TestThresholdMultisigValidCases(t *testing.T) {
},
}
for tcIndex, tc := range cases {
multisigKey := NewThresholdMultiSignaturePubKey(tc.k, tc.pubkeys)
multisigKey := NewPubKeyMultisigThreshold(tc.k, tc.pubkeys)
multisignature := NewMultisig(len(tc.pubkeys))
for i := 0; i < tc.k-1; i++ {
signingIndex := tc.signingIndices[i]
multisignature.AddSignatureFromPubkey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
multisignature.AddSignatureFromPubKey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
require.False(t, multisigKey.VerifyBytes(tc.msg, multisignature.Marshal()),
"multisig passed when i < k, tc %d, i %d", tcIndex, i)
multisignature.AddSignatureFromPubkey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
multisignature.AddSignatureFromPubKey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
require.Equal(t, i+1, len(multisignature.Sigs),
"adding a signature for the same pubkey twice increased signature count by 2, tc %d", tcIndex)
}
require.False(t, multisigKey.VerifyBytes(tc.msg, multisignature.Marshal()),
"multisig passed with k - 1 sigs, tc %d", tcIndex)
multisignature.AddSignatureFromPubkey(tc.signatures[tc.signingIndices[tc.k]], tc.pubkeys[tc.signingIndices[tc.k]], tc.pubkeys)
multisignature.AddSignatureFromPubKey(tc.signatures[tc.signingIndices[tc.k]], tc.pubkeys[tc.signingIndices[tc.k]], tc.pubkeys)
require.True(t, multisigKey.VerifyBytes(tc.msg, multisignature.Marshal()),
"multisig failed after k good signatures, tc %d", tcIndex)
for i := tc.k + 1; i < len(tc.signingIndices); i++ {
signingIndex := tc.signingIndices[i]
multisignature.AddSignatureFromPubkey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
multisignature.AddSignatureFromPubKey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
require.Equal(t, tc.passAfterKSignatures[i-tc.k-1],
multisigKey.VerifyBytes(tc.msg, multisignature.Marshal()),
"multisig didn't verify as expected after k sigs, tc %d, i %d", tcIndex, i)

multisignature.AddSignatureFromPubkey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
multisignature.AddSignatureFromPubKey(tc.signatures[signingIndex], tc.pubkeys[signingIndex], tc.pubkeys)
require.Equal(t, i+1, len(multisignature.Sigs),
"adding a signature for the same pubkey twice increased signature count by 2, tc %d", tcIndex)
}
Expand All @@ -68,21 +68,21 @@ func TestThresholdMultisigValidCases(t *testing.T) {
func TestThresholdMultisigDuplicateSignatures(t *testing.T) {
msg := []byte{1, 2, 3, 4, 5}
pubkeys, sigs := generatePubKeysAndSignatures(5, msg)
multisigKey := NewThresholdMultiSignaturePubKey(2, pubkeys)
multisigKey := NewPubKeyMultisigThreshold(2, pubkeys)
multisignature := NewMultisig(5)
require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal()))
multisignature.AddSignatureFromPubkey(sigs[0], pubkeys[0], pubkeys)
multisignature.AddSignatureFromPubKey(sigs[0], pubkeys[0], pubkeys)
// Add second signature manually
multisignature.Sigs = append(multisignature.Sigs, sigs[0])
require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal()))
}

// TODO: Fully replace this test with table driven tests
func TestMultiSigPubkeyEquality(t *testing.T) {
func TestMultiSigPubKeyEquality(t *testing.T) {
msg := []byte{1, 2, 3, 4}
pubkeys, _ := generatePubKeysAndSignatures(5, msg)
multisigKey := NewThresholdMultiSignaturePubKey(2, pubkeys)
var unmarshalledMultisig *ThresholdMultiSignaturePubKey
multisigKey := NewPubKeyMultisigThreshold(2, pubkeys)
var unmarshalledMultisig *PubKeyMultisigThreshold
cdc.MustUnmarshalBinaryBare(multisigKey.Bytes(), &unmarshalledMultisig)
require.True(t, multisigKey.Equals(unmarshalledMultisig))

Expand All @@ -91,7 +91,7 @@ func TestMultiSigPubkeyEquality(t *testing.T) {
copy(pubkeysCpy, pubkeys)
pubkeysCpy[4] = pubkeys[3]
pubkeysCpy[3] = pubkeys[4]
multisigKey2 := NewThresholdMultiSignaturePubKey(2, pubkeysCpy)
multisigKey2 := NewPubKeyMultisigThreshold(2, pubkeysCpy)
require.False(t, multisigKey.Equals(multisigKey2))
}

Expand Down
6 changes: 3 additions & 3 deletions crypto/multisig/wire.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ import (
// TODO: Figure out API for others to either add their own pubkey types, or
// to make verify / marshal accept a cdc.
const (
ThresholdPubkeyAminoRoute = "tendermint/PubkeyMultisigThreshold"
PubKeyMultisigThresholdAminoRoute = "tendermint/PubKeyMultisigThreshold"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we omit the multisig from the varname since it's in the package? Also should we omit the pubkey from the var name since there's no privkey

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm .... I was hoping to reach parity between the var name and the Amino route. We can't remove Multisig from the amino route because there's no pkg information there, it needs to be the full tendermint/PubKeyMultisigThreshold.

We could consider removing Multisig from the var name, but I think we should keep PubKey for consistency/clarity

)

var cdc = amino.NewCodec()

func init() {
cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
cdc.RegisterConcrete(ThresholdMultiSignaturePubKey{},
ThresholdPubkeyAminoRoute, nil)
cdc.RegisterConcrete(PubKeyMultisigThreshold{},
PubKeyMultisigThresholdAminoRoute, nil)
cdc.RegisterConcrete(ed25519.PubKeyEd25519{},
ed25519.PubKeyAminoRoute, nil)
cdc.RegisterConcrete(secp256k1.PubKeySecp256k1{},
Expand Down