(squash this) begin addressing PR comments

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)
 }

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
 }

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)