From 00db469fc05a183ab0af70dcfa128f26d9acd0d7 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Fri, 10 Aug 2018 13:14:43 -0500 Subject: [PATCH] (squash this) begin addressing PR comments --- crypto/multisig/multisignature.go | 3 +- crypto/multisig/threshold_multisig.go | 36 ++++++---- crypto/multisig/threshold_multisig_test.go | 77 +++++++++++++++------- 3 files changed, 79 insertions(+), 37 deletions(-) diff --git a/crypto/multisig/multisignature.go b/crypto/multisig/multisignature.go index bd2398b83..374b5e1b4 100644 --- a/crypto/multisig/multisignature.go +++ b/crypto/multisig/multisignature.go @@ -31,6 +31,7 @@ func getIndex(pk crypto.PubKey, keys []crypto.PubKey) int { } // AddSignature adds a signature to the multisig, at the corresponding index. +// If the signature already exists, replace it. func (mSig *Multisignature) AddSignature(sig []byte, index int) { newSigIndex := mSig.BitArray.NumOfTrueBitsBefore(index) // Signature already exists, just replace the value there @@ -64,5 +65,5 @@ func (mSig *Multisignature) AddSignatureFromPubkey(sig []byte, pubkey crypto.Pub // Marshal the multisignature with amino func (mSig *Multisignature) Marshal() []byte { - return cdc.MustMarshalBinary(mSig) + return cdc.MustMarshalBinaryBare(mSig) } diff --git a/crypto/multisig/threshold_multisig.go b/crypto/multisig/threshold_multisig.go index adc5fd129..934f7d798 100644 --- a/crypto/multisig/threshold_multisig.go +++ b/crypto/multisig/threshold_multisig.go @@ -14,7 +14,11 @@ type ThresholdMultiSignaturePubKey struct { var _ crypto.PubKey = &ThresholdMultiSignaturePubKey{} // NewThresholdMultiSignaturePubKey returns a new ThresholdMultiSignaturePubKey. +// Panics if len(pubkeys) < k or 0 >= k. func NewThresholdMultiSignaturePubKey(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") } @@ -29,12 +33,21 @@ func NewThresholdMultiSignaturePubKey(k int, pubkeys []crypto.PubKey) crypto.Pub // a concern. func (pk *ThresholdMultiSignaturePubKey) VerifyBytes(msg []byte, marshalledSig []byte) bool { var sig *Multisignature - err := cdc.UnmarshalBinary(marshalledSig, &sig) + err := cdc.UnmarshalBinaryBare(marshalledSig, &sig) if err != nil { return false } size := sig.BitArray.Size() - if len(sig.Sigs) < int(pk.K) || len(pk.Pubkeys) != size || sig.BitArray.NumOfTrueBitsBefore(size) < int(pk.K) { + // ensure bit array is the correct size + if len(pk.Pubkeys) != size { + return false + } + // ensure size of signature list + if len(sig.Sigs) < int(pk.K) || len(sig.Sigs) > size { + return false + } + // ensure at least k signatures are set + if sig.BitArray.NumOfTrueBitsBefore(size) < int(pk.K) { return false } // index in the list of signatures which we are concerned with. @@ -63,16 +76,17 @@ func (pk *ThresholdMultiSignaturePubKey) Address() crypto.Address { // 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 { - if otherKey, ok := other.(*ThresholdMultiSignaturePubKey); ok { - if pk.K != otherKey.K || len(pk.Pubkeys) != len(otherKey.Pubkeys) { + otherKey, sameType := other.(*ThresholdMultiSignaturePubKey) + if !sameType { + return false + } + 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]) { return false } - for i := 0; i < len(pk.Pubkeys); i++ { - if !pk.Pubkeys[i].Equals(otherKey.Pubkeys[i]) { - return false - } - } - return true } - return false + return true } diff --git a/crypto/multisig/threshold_multisig_test.go b/crypto/multisig/threshold_multisig_test.go index cbd447a2d..6036554a3 100644 --- a/crypto/multisig/threshold_multisig_test.go +++ b/crypto/multisig/threshold_multisig_test.go @@ -11,34 +11,60 @@ import ( "github.com/tendermint/tendermint/crypto/secp256k1" ) -func TestThresholdMultisig(t *testing.T) { - msg := []byte{1, 2, 3, 4} - pubkeys, sigs := generatePubKeysAndSignatures(5, msg) - multisigKey := NewThresholdMultiSignaturePubKey(2, pubkeys) - multisignature := NewMultisig(5) - require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) - multisignature.AddSignatureFromPubkey(sigs[0], pubkeys[0], pubkeys) - require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) - // Make sure adding the same signature twice doesn't increase number of signatures - multisignature.AddSignatureFromPubkey(sigs[0], pubkeys[0], pubkeys) - require.Equal(t, 1, len(multisignature.Sigs)) - - // Adding two signatures should make it pass, as k = 2 - multisignature.AddSignatureFromPubkey(sigs[3], pubkeys[3], pubkeys) - require.True(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) - - // Adding a third invalid signature should make verification fail. - multisignature.AddSignatureFromPubkey(sigs[0], pubkeys[4], pubkeys) - require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) +// This tests multisig functionality, but it expects the first k signatures to be valid +// TODO: Adapt it to give more flexibility about first k signatures being valid +func TestThresholdMultisigValidCases(t *testing.T) { + pkSet1, sigSet1 := generatePubKeysAndSignatures(5, []byte{1, 2, 3, 4}) + cases := []struct { + msg []byte + k int + pubkeys []crypto.PubKey + signingIndices []int + // signatures should be the same size as signingIndices. + signatures [][]byte + passAfterKSignatures []bool + }{ + { + msg: []byte{1, 2, 3, 4}, + k: 2, + pubkeys: pkSet1, + signingIndices: []int{0, 3, 1}, + signatures: sigSet1, + passAfterKSignatures: []bool{false}, + }, + } + for tcIndex, tc := range cases { + multisigKey := NewThresholdMultiSignaturePubKey(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) + 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) + 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) + 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) + 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) - // try adding the invalid signature one signature before - // first reset the multisig - multisignature.BitArray.SetIndex(4, false) - multisignature.Sigs = multisignature.Sigs[:2] - multisignature.AddSignatureFromPubkey(sigs[0], pubkeys[2], pubkeys) - require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) + 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) + } + } } +// TODO: Fully replace this test with table driven tests func TestThresholdMultisigDuplicateSignatures(t *testing.T) { msg := []byte{1, 2, 3, 4, 5} pubkeys, sigs := generatePubKeysAndSignatures(5, msg) @@ -51,6 +77,7 @@ func TestThresholdMultisigDuplicateSignatures(t *testing.T) { require.False(t, multisigKey.VerifyBytes(msg, multisignature.Marshal())) } +// TODO: Fully replace this test with table driven tests func TestMultiSigPubkeyEquality(t *testing.T) { msg := []byte{1, 2, 3, 4} pubkeys, _ := generatePubKeysAndSignatures(5, msg)