Fix set/unset bugs with fuzzer test

exp/lighthorizon/index/types/bitmap.go

@@ -2,9 +2,12 @@ package index
 
 import (
 	"bytes"
+	"fmt"
 	"io"
+	"strings"
 	"sync"
 
+	"github.com/stellar/go/support/ordered"
 	"github.com/stellar/go/xdr"
 )
 
@@ -102,20 +105,15 @@ func (i *BitmapIndex) setActive(index uint32) error {
 			// Expand the bitmap
 			if index < i.rangeFirstBit() {
 				// ...to the left
-				c := (i.rangeFirstBit() - index) / 8
-				if (i.rangeFirstBit()-index)%8 != 0 {
-					c++
-				}
-				newBytes := make([]byte, c)
+				newBytes := make([]byte, distance(index, i.rangeFirstBit()))
 				i.bitmap = append(newBytes, i.bitmap...)
-
 				b := bitShiftLeft(index)
 				i.bitmap[0] = i.bitmap[0] | b
 
 				i.firstBit = index
 			} else if index > i.rangeLastBit() {
 				// ... to the right
-				newBytes := make([]byte, (index-i.rangeLastBit())/8+1)
+				newBytes := make([]byte, distance(i.rangeLastBit(), index))
 				i.bitmap = append(i.bitmap, newBytes...)
 				b := bitShiftLeft(index)
 				loc := (index - i.rangeFirstBit()) / 8
@@ -129,6 +127,12 @@ func (i *BitmapIndex) setActive(index uint32) error {
 	return nil
 }
 
+// distance returns how many bytes occur between the two given indices. Note
+// that j >= i, otherwise the result will be negative.
+func distance(i, j uint32) int {
+	return (int(j)-1)/8 - (int(i)-1)/8
+}
+
 func (i *BitmapIndex) setInactive(index uint32) error {
 	// Is this index even active in the first place?
 	if i.firstBit == 0 || index < i.rangeFirstBit() || index > i.rangeLastBit() {
@@ -152,54 +156,52 @@ func (i *BitmapIndex) setInactive(index uint32) error {
 			return err
 		} else {
 			// Trim all (now-)empty bytes off the front.
-			i.bitmap = i.bitmap[int(nextBit/8)-int(i.firstBit/8):]
+			diff := distance(i.firstBit, nextBit)
+			i.bitmap = i.bitmap[diff:]
 			i.firstBit = nextBit
 		}
 	} else if int(loc) == len(i.bitmap)-1 {
-		if i.bitmap[loc] == 0 {
-			i.bitmap = i.bitmap[:loc-1] // trim last (now-empty) byte
+		idx := -1
 
+		if i.bitmap[loc] == 0 {
 			// find the latest non-empty byte, to set as the new "end"
-			for j := len(i.bitmap) - 1; j >= 0; j-- {
-				if i.bitmap[j] == 0 {
-					continue
-				}
-
-				offset, _ := maxBitAfter(i.bitmap[j], 0)
-				byteOffset := uint32(len(i.bitmap)-1) * 8
-				i.lastBit = i.rangeFirstBit() + byteOffset + offset
-				i.bitmap = i.bitmap[:j+1]
-				break
+			j := len(i.bitmap) - 1
+			for i.bitmap[j] == 0 {
+				j--
 			}
-		} else if i.lastBit == index {
-			// Otherwise, do we need to adjust the range? Imagine we had
-			// 0b0011_0100 and we unset the last active bit.
-			//         ^
-			// Then, we need to adjust our internal lastBit tracker to represent
-			// the ^ bit above. This means finding the first previous set bit.
 
+			i.bitmap = i.bitmap[:j+1]
+			idx = 8
+		} else if i.lastBit == index {
 			// Get the "bit number" of the last active bit (i.e. the one we just
 			// turned off) to mark the starting point for the search.
-			idx := uint32(1)
+			idx = 8
 			if index%8 != 0 {
-				idx = 8 - (index % 8)
+				idx = int(index % 8)
 			}
+		}
 
+		// Do we need to adjust the range? Imagine we had 0b0011_0100 and we
+		// unset the last active bit.
+		//         ^
+		// Then, we need to adjust our internal lastBit tracker to represent the
+		// ^ bit above. This means finding the first previous set bit.
+		if idx > -1 {
+			l := uint32(len(i.bitmap) - 1)
 			// Imagine we had 0b0011_0100 and we unset the last active bit.
 			//                     ^
 			// Then, we need to adjust our internal lastBit tracker to represent
 			// the ^ bit above. This means finding the first previous set bit.
 			j, ok := int(idx), false
 			for ; j >= 0 && !ok; j-- {
-				_, ok = maxBitAfter(i.bitmap[loc], uint32(j))
+				_, ok = maxBitAfter(i.bitmap[l], uint32(j))
 			}
 
 			// We know from the earlier conditional that *some* bit is set, so
 			// we know that j represents the index of the bit that's the new
 			// "last active" bit.
 			firstByte := i.rangeFirstBit()
-			byteOffset := 8 * uint32(len(i.bitmap)-1)
-			i.lastBit = firstByte + byteOffset + uint32(j) + 1
+			i.lastBit = firstByte + (l * 8) + uint32(j) + 1
 		}
 	}
 
@@ -343,3 +345,24 @@ func (i *BitmapIndex) Buffer() *bytes.Buffer {
 func (i *BitmapIndex) Flush() []byte {
 	return i.Buffer().Bytes()
 }
+
+// DebugCompare returns a string that compares this bitmap to another bitmap
+// byte-by-byte in binary form as two columns.
+func (i *BitmapIndex) DebugCompare(j *BitmapIndex) string {
+	output := make([]string, ordered.Max(len(i.bitmap), len(j.bitmap)))
+	for n := 0; n < len(output); n++ {
+		if n < len(i.bitmap) {
+			output[n] += fmt.Sprintf("%08b", i.bitmap[n])
+		} else {
+			output[n] += "        "
+		}
+
+		output[n] += " | "
+
+		if n < len(j.bitmap) {
+			output[n] += fmt.Sprintf("%08b", j.bitmap[n])
+		}
+	}
+
+	return strings.Join(output, "\n")
+}

exp/lighthorizon/index/types/bitmap_test.go

@@ -3,6 +3,7 @@ package index
 import (
 	"fmt"
 	"io"
+	"math/rand"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -194,6 +195,39 @@ func TestSetInactive(t *testing.T) {
 	assert.Equal(t, []byte{0b0100_0000, 0b0010_0000}, index.bitmap)
 	assert.EqualValues(t, 2, index.firstBit)
 	assert.EqualValues(t, 2+9, index.lastBit)
+
+	index.setInactive(1) // should be a no-op
+	assert.Equal(t, []byte{0b0100_0000, 0b0010_0000}, index.bitmap)
+	assert.EqualValues(t, 2, index.firstBit)
+	assert.EqualValues(t, 2+9, index.lastBit)
+}
+
+// TestFuzzerSetInactive attempt to fuzz random bits into two bitmap sets, one
+// by addition, and one by subtraction - then, it compares the outcome.
+func TestFuzzySetUnset(t *testing.T) {
+	permLen := uint32(128) // should be a multiple of 8
+	setBitsCount := permLen / 2
+
+	for n := 0; n < 10_000; n++ {
+		randBits := rand.Perm(int(permLen))
+		setBits := randBits[:setBitsCount]
+		clearBits := randBits[setBitsCount:]
+
+		// set all first, then clear the others
+		clearBitmap := &BitmapIndex{}
+		for i := uint32(1); i <= permLen; i++ {
+			clearBitmap.setActive(i)
+		}
+
+		setBitmap := &BitmapIndex{}
+		for i := range setBits {
+			setBitmap.setActive(uint32(setBits[i]) + 1)
+			clearBitmap.setInactive(uint32(clearBits[i]) + 1)
+		}
+
+		require.Equalf(t, setBitmap, clearBitmap,
+			"bitmaps aren't equal:\n%s", setBitmap.DebugCompare(clearBitmap))
+	}
 }
 
 func TestNextActive(t *testing.T) {