(squash this) address PR comments + fix bug in equality check

crypto/multisig/compact_bit_array.go

@@ -69,12 +69,12 @@ func (bA *CompactBitArray) SetIndex(i int, v bool) bool {
 	return true
 }
 
-// trueIndex returns the location of the given index, among the
+// NumOfTrueBitsBefore returns the location of the given index, among the
 // values in the bit array that are set to true.
-// e.g. if bA = _XX_X_X, trueIndex(4) = 2, since
+// e.g. if bA = _XX_X_X, NumOfTrueBitsBefore(4) = 2, since
 // the value at index 4 of the bit array is the third
 // value that is true. (And it is 0-indexed)
-func (bA *CompactBitArray) trueIndex(index int) int {
+func (bA *CompactBitArray) NumOfTrueBitsBefore(index int) int {
 	numTrueValues := 0
 	for i := 0; i < index; i++ {
 		if bA.GetIndex(i) {

crypto/multisig/compact_bit_array_test.go

@@ -150,7 +150,7 @@ func TestCompactMarshalUnmarshal(t *testing.T) {
 	}
 }
 
-func TestCompactBitArrayTrueIndex(t *testing.T) {
+func TestCompactBitArrayNumOfTrueBitsBefore(t *testing.T) {
 	testCases := []struct {
 		marshalledBA   string
 		bAIndex        []int
@@ -170,7 +170,7 @@ func TestCompactBitArrayTrueIndex(t *testing.T) {
 			require.NoError(t, err)
 
 			for i := 0; i < len(tc.bAIndex); i++ {
-				require.Equal(t, tc.trueValueIndex[i], bA.trueIndex(tc.bAIndex[i]), "tc %d, i %d", tcIndex, i)
+				require.Equal(t, tc.trueValueIndex[i], bA.NumOfTrueBitsBefore(tc.bAIndex[i]), "tc %d, i %d", tcIndex, i)
 			}
 		})
 	}

crypto/multisig/multisignature.go

@@ -1,6 +1,10 @@
 package multisig
 
-import "github.com/tendermint/tendermint/crypto"
+import (
+	"errors"
+
+	"github.com/tendermint/tendermint/crypto"
+)
 
 // Multisignature is used to represent the signature object used in the multisigs.
 // Sigs is a list of signatures, sorted by corresponding index.
@@ -17,7 +21,7 @@ func NewMultisig(n int) *Multisignature {
 }
 
 // GetIndex returns the index of pk in keys. Returns -1 if not found
-func GetIndex(pk crypto.PubKey, keys []crypto.PubKey) int {
+func getIndex(pk crypto.PubKey, keys []crypto.PubKey) int {
 	for i := 0; i < len(keys); i++ {
 		if pk.Equals(keys[i]) {
 			return i
@@ -28,30 +32,34 @@ func GetIndex(pk crypto.PubKey, keys []crypto.PubKey) int {
 
 // AddSignature adds a signature to the multisig, at the corresponding index.
 func (mSig *Multisignature) AddSignature(sig []byte, index int) {
-	i := mSig.BitArray.trueIndex(index)
+	newSigIndex := mSig.BitArray.NumOfTrueBitsBefore(index)
 	// Signature already exists, just replace the value there
 	if mSig.BitArray.GetIndex(index) {
-		mSig.Sigs[i] = sig
+		mSig.Sigs[newSigIndex] = sig
 		return
 	}
 	mSig.BitArray.SetIndex(index, true)
 	// Optimization if the index is the greatest index
-	if i > len(mSig.Sigs) {
+	if newSigIndex == len(mSig.Sigs) {
 		mSig.Sigs = append(mSig.Sigs, sig)
 		return
 	}
 	// Expand slice by one with a dummy element, move all elements after i
 	// over by one, then place the new signature in that gap.
 	mSig.Sigs = append(mSig.Sigs, make([]byte, 0))
-	copy(mSig.Sigs[i+1:], mSig.Sigs[i:])
-	mSig.Sigs[i] = sig
+	copy(mSig.Sigs[newSigIndex+1:], mSig.Sigs[newSigIndex:])
+	mSig.Sigs[newSigIndex] = sig
 }
 
 // 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) {
-	index := GetIndex(pubkey, keys)
+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")
+	}
 	mSig.AddSignature(sig, index)
+	return nil
 }
 
 // Marshal the multisignature with amino

crypto/multisig/threshold_multisig.go

@@ -5,7 +5,7 @@ import (
 	"github.com/tendermint/tendermint/crypto/tmhash"
 )
 
-// ThresholdMultiSignaturePubKey implements a K of N threshold multisig
+// ThresholdMultiSignaturePubKey implements a K of N threshold multisig.
 type ThresholdMultiSignaturePubKey struct {
 	K       uint            `json:"threshold"`
 	Pubkeys []crypto.PubKey `json:"pubkeys"`
@@ -64,10 +64,10 @@ func (pk *ThresholdMultiSignaturePubKey) Address() crypto.Address {
 // 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 {
+		if pk.K != otherKey.K || len(pk.Pubkeys) != len(otherKey.Pubkeys) {
 			return false
 		}
-		for i := uint(0); i < pk.K; i++ {
+		for i := 0; i < len(pk.Pubkeys); i++ {
 			if !pk.Pubkeys[i].Equals(otherKey.Pubkeys[i]) {
 				return false
 			}

crypto/multisig/threshold_multisig_test.go

@@ -45,7 +45,7 @@ func TestMultiSigPubkeyEquality(t *testing.T) {
 	multisigKey := NewThresholdMultiSignaturePubKey(2, pubkeys)
 	var unmarshalledMultisig *ThresholdMultiSignaturePubKey
 	cdc.MustUnmarshalBinary(multisigKey.Bytes(), &unmarshalledMultisig)
-	require.Equal(t, multisigKey, unmarshalledMultisig)
+	require.True(t, multisigKey.Equals(unmarshalledMultisig))
 
 	// Ensure that reordering pubkeys is treated as a different pubkey
 	pubkeysCpy := make([]crypto.PubKey, 5)
@@ -53,7 +53,7 @@ func TestMultiSigPubkeyEquality(t *testing.T) {
 	pubkeysCpy[4] = pubkeys[3]
 	pubkeysCpy[3] = pubkeys[4]
 	multisigKey2 := NewThresholdMultiSignaturePubKey(2, pubkeysCpy)
-	require.NotEqual(t, multisigKey, multisigKey2)
+	require.False(t, multisigKey.Equals(multisigKey2))
 }
 
 func generatePubKeysAndSignatures(n int, msg []byte) (pubkeys []crypto.PubKey, signatures [][]byte) {