bit_array: Simplify subtraction

CHANGELOG_PENDING.md

@@ -1,6 +1,7 @@
 # Pending
 
 Special thanks to external contributors on this release:
+@goolAdapter, @bradyjoestar
 
 BREAKING CHANGES:
 
@@ -11,7 +12,7 @@ BREAKING CHANGES:
   * [rpc] \#2298 `/abci_query` takes `prove` argument instead of `trusted` and switches the default
     behaviour to `prove=false`
   * [privval] \#2459 Split `SocketPVMsg`s implementations into Request and Response, where the Response may contain a error message (returned by the remote signer)
-  
+
 * Apps
   * [abci] \#2298 ResponseQuery.Proof is now a structured merkle.Proof, not just
     arbitrary bytes
@@ -40,3 +41,5 @@ BUG FIXES:
 - [autofile] \#2428 Group.RotateFile need call Flush() before rename (@goolAdapter)
 - [node] \#2434 Make node respond to signal interrupts while sleeping for genesis time
 - [evidence] \#2515 fix db iter leak (@goolAdapter)
+- [common/bit_array] Fixed a bug in the `Or` function
+- [common/bit_array] Fixed a bug in the `Sub` function (@bradyjoestar)

libs/common/bit_array.go

@@ -119,14 +119,13 @@ func (bA *BitArray) Or(o *BitArray) *BitArray {
 	}
 	bA.mtx.Lock()
 	o.mtx.Lock()
-	defer func() {
-		bA.mtx.Unlock()
-		o.mtx.Unlock()
-	}()
 	c := bA.copyBits(MaxInt(bA.Bits, o.Bits))
-	for i := 0; i < len(c.Elems); i++ {
+	smaller := MinInt(len(bA.Elems), len(o.Elems))
+	for i := 0; i < smaller; i++ {
 		c.Elems[i] |= o.Elems[i]
 	}
+	bA.mtx.Unlock()
+	o.mtx.Unlock()
 	return c
 }
 
@@ -173,33 +172,30 @@ func (bA *BitArray) not() *BitArray {
 }
 
 // Sub subtracts the two bit-arrays bitwise, without carrying the bits.
-// This is essentially bA.And(o.Not()).
-// If bA is longer than o, o is right padded with zeroes.
+// Note that carryless subtraction of a - b is (a and not b).
+// The output is the same as bA, regardless of o's size.
+// If bA is longer than o, o is right padded with zeroes
 func (bA *BitArray) Sub(o *BitArray) *BitArray {
 	if bA == nil || o == nil {
 		// TODO: Decide if we should do 1's complement here?
 		return nil
 	}
 	bA.mtx.Lock()
 	o.mtx.Lock()
-	defer func() {
-		bA.mtx.Unlock()
-		o.mtx.Unlock()
-	}()
-	if bA.Bits > o.Bits {
-		c := bA.copy()
-		for i := 0; i < len(o.Elems)-1; i++ {
-			c.Elems[i] &= ^o.Elems[i]
-		}
-		i := len(o.Elems) - 1
-		if i >= 0 {
-			for idx := i * 64; idx < o.Bits; idx++ {
-				c.setIndex(idx, c.getIndex(idx) && !o.getIndex(idx))
-			}
-		}
-		return c
-	}
-	return bA.and(o.not()) // Note degenerate case where o == nil
+	// output is the same size as bA
+	c := bA.copyBits(bA.Bits)
+	// Only iterate to the minimum size between the two.
+	// If o is longer, those bits are ignored.
+	// If bA is longer, then skipping those iterations is equivalent
+	// to right padding with 0's
+	smaller := MinInt(len(bA.Elems), len(o.Elems))
+	for i := 0; i < smaller; i++ {
+		// &^ is and not in golang
+		c.Elems[i] &^= o.Elems[i]
+	}
+	bA.mtx.Unlock()
+	o.mtx.Unlock()
+	return c
 }
 
 // IsEmpty returns true iff all bits in the bit array are 0

libs/common/bit_array_test.go

@@ -75,87 +75,34 @@ func TestOr(t *testing.T) {
 	}
 }
 
-func TestSub1(t *testing.T) {
-
-	bA1, _ := randBitArray(31)
-	bA2, _ := randBitArray(51)
-	bA3 := bA1.Sub(bA2)
-
-	bNil := (*BitArray)(nil)
-	require.Equal(t, bNil.Sub(bA1), (*BitArray)(nil))
-	require.Equal(t, bA1.Sub(nil), (*BitArray)(nil))
-	require.Equal(t, bNil.Sub(nil), (*BitArray)(nil))
-
-	if bA3.Bits != bA1.Bits {
-		t.Error("Expected bA1 bits")
-	}
-	if len(bA3.Elems) != len(bA1.Elems) {
-		t.Error("Expected bA1 elems length")
-	}
-	for i := 0; i < bA3.Bits; i++ {
-		expected := bA1.GetIndex(i)
-		if bA2.GetIndex(i) {
-			expected = false
-		}
-		if bA3.GetIndex(i) != expected {
-			t.Error("Wrong bit from bA3", i, bA1.GetIndex(i), bA2.GetIndex(i), bA3.GetIndex(i))
-		}
-	}
-}
-
-func TestSub2(t *testing.T) {
-
-	bA1, _ := randBitArray(51)
-	bA2, _ := randBitArray(31)
-	bA3 := bA1.Sub(bA2)
-
-	bNil := (*BitArray)(nil)
-	require.Equal(t, bNil.Sub(bA1), (*BitArray)(nil))
-	require.Equal(t, bA1.Sub(nil), (*BitArray)(nil))
-	require.Equal(t, bNil.Sub(nil), (*BitArray)(nil))
-
-	if bA3.Bits != bA1.Bits {
-		t.Error("Expected bA1 bits")
-	}
-	if len(bA3.Elems) != len(bA1.Elems) {
-		t.Error("Expected bA1 elems length")
-	}
-	for i := 0; i < bA3.Bits; i++ {
-		expected := bA1.GetIndex(i)
-		if i < bA2.Bits && bA2.GetIndex(i) {
-			expected = false
-		}
-		if bA3.GetIndex(i) != expected {
-			t.Error("Wrong bit from bA3")
-		}
+func TestSub(t *testing.T) {
+	testCases := []struct {
+		initBA        string
+		subtractingBA string
+		expectedBA    string
+	}{
+		{`null`, `null`, `null`},
+		{`"x"`, `null`, `null`},
+		{`null`, `"x"`, `null`},
+		{`"x"`, `"x"`, `"_"`},
+		{`"xxxxxx"`, `"x_x_x_"`, `"_x_x_x"`},
+		{`"x_x_x_"`, `"xxxxxx"`, `"______"`},
+		{`"xxxxxx"`, `"x_x_x_xxxx"`, `"_x_x_x"`},
+		{`"x_x_x_xxxx"`, `"xxxxxx"`, `"______xxxx"`},
+		{`"xxxxxxxxxx"`, `"x_x_x_"`, `"_x_x_xxxxx"`},
+		{`"x_x_x_"`, `"xxxxxxxxxx"`, `"______"`},
 	}
-}
-
-func TestSub3(t *testing.T) {
-
-	bA1, _ := randBitArray(231)
-	bA2, _ := randBitArray(81)
-	bA3 := bA1.Sub(bA2)
+	for _, tc := range testCases {
+		var bA *BitArray
+		err := json.Unmarshal([]byte(tc.initBA), &bA)
+		require.Nil(t, err)
 
-	bNil := (*BitArray)(nil)
-	require.Equal(t, bNil.Sub(bA1), (*BitArray)(nil))
-	require.Equal(t, bA1.Sub(nil), (*BitArray)(nil))
-	require.Equal(t, bNil.Sub(nil), (*BitArray)(nil))
+		var o *BitArray
+		err = json.Unmarshal([]byte(tc.subtractingBA), &o)
+		require.Nil(t, err)
 
-	if bA3.Bits != bA1.Bits {
-		t.Error("Expected bA1 bits")
-	}
-	if len(bA3.Elems) != len(bA1.Elems) {
-		t.Error("Expected bA1 elems length")
-	}
-	for i := 0; i < bA3.Bits; i++ {
-		expected := bA1.GetIndex(i)
-		if i < bA2.Bits && bA2.GetIndex(i){
-			expected = false
-		}
-		if bA3.GetIndex(i) != expected {
-			t.Error("Wrong bit from bA3")
-		}
+		got, _ := json.Marshal(bA.Sub(o))
+		require.Equal(t, tc.expectedBA, string(got), "%s minus %s doesn't equal %s", tc.initBA, tc.subtractingBA, tc.expectedBA)
 	}
 }