Improves UTs coverage in shared/aggregation (#8593)

* improves shared/aggregation test suite

* improves coverage in shared/aggregation/attestations

shared/aggregation/aggregation.go

@@ -3,7 +3,6 @@ package aggregation
 
 import (
 	"errors"
-	"fmt"
 
 	"github.com/prysmaticlabs/go-bitfield"
 	"github.com/sirupsen/logrus"
@@ -29,8 +28,3 @@ type Aggregation struct {
 	// Keys is a list of candidate keys in the best available solution.
 	Keys []int
 }
-
-// String provides string representation of a solution.
-func (ag *Aggregation) String() string {
-	return fmt.Sprintf("{coverage: %#b, keys: %v}", ag.Coverage, ag.Keys)
-}

shared/aggregation/attestations/attestations_test.go

@@ -248,6 +248,70 @@ func TestAggregateAttestations_Aggregate(t *testing.T) {
 			runner()
 		})
 	}
+
+	t.Run("invalid strategy", func(t *testing.T) {
+		resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
+			AttestationAggregationStrategy: "foobar",
+		})
+		defer resetCfg()
+		_, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{}))
+		assert.ErrorContains(t, "\"foobar\": invalid aggregation strategy", err)
+	})
+
+	t.Run("broken attestation bitset", func(t *testing.T) {
+		wantErr := "bitlist cannot be nil or empty: invalid max_cover problem"
+		t.Run(string(MaxCoverAggregation), func(t *testing.T) {
+			resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
+				AttestationAggregationStrategy: string(MaxCoverAggregation),
+			})
+			defer resetCfg()
+			_, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{
+				{0b00000011, 0b0},
+				{0b00000111, 0b100},
+				{0b00000100, 0b1},
+			}))
+			assert.ErrorContains(t, wantErr, err)
+		})
+		t.Run(string(OptMaxCoverAggregation), func(t *testing.T) {
+			resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
+				AttestationAggregationStrategy: string(OptMaxCoverAggregation),
+			})
+			defer resetCfg()
+			_, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{
+				{0b00000011, 0b0},
+				{0b00000111, 0b100},
+				{0b00000100, 0b1},
+			}))
+			assert.ErrorContains(t, wantErr, err)
+		})
+	})
+
+	t.Run("candidate swapping when aggregating", func(t *testing.T) {
+		// The first item cannot be aggregated, and should be pushed down the list,
+		// by two swaps with aggregated items (aggregation is done in-place, so the very same
+		// underlying array is used for storing both aggregated and non-aggregated items).
+		resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{
+			AttestationAggregationStrategy: string(OptMaxCoverAggregation),
+		})
+		defer resetCfg()
+		got, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{
+			{0b10000000, 0b1},
+			{0b11000101, 0b1},
+			{0b00011000, 0b1},
+			{0b01010100, 0b1},
+			{0b10001010, 0b1},
+		}))
+		want := []bitfield.Bitlist{
+			{0b11011101, 0b1},
+			{0b11011110, 0b1},
+			{0b10000000, 0b1},
+		}
+		assert.NoError(t, err)
+		assert.Equal(t, len(want), len(got))
+		for i, w := range want {
+			assert.DeepEqual(t, w.Bytes(), got[i].AggregationBits.Bytes())
+		}
+	})
 }
 
 func TestAggregateAttestations_PerformanceComparison(t *testing.T) {

shared/aggregation/maxcover.go

@@ -1,7 +1,6 @@
 package aggregation
 
 import (
-	"fmt"
 	"sort"
 
 	"github.com/pkg/errors"
@@ -79,11 +78,6 @@ func (mc *MaxCoverProblem) Cover(k int, allowOverlaps bool) (*Aggregation, error
 				break
 			}
 			if !candidate.processed {
-				if !allowOverlaps && solution.Coverage.Overlaps(*candidate.bits) {
-					// Overlapping candidates violate non-intersection invariant.
-					candidate.processed = true
-					continue
-				}
 				solution.Coverage = solution.Coverage.Or(*candidate.bits)
 				solution.Keys = append(solution.Keys, candidate.key)
 				remainingBits = remainingBits.And(candidate.bits.Not())
@@ -240,8 +234,3 @@ func union(candidates []*bitfield.Bitlist64) *bitfield.Bitlist64 {
 	}
 	return ret
 }
-
-// String provides string representation of a candidate.
-func (c *MaxCoverCandidate) String() string {
-	return fmt.Sprintf("{%v, %#b, s%d, %t}", c.key, c.bits, c.score, c.processed)
-}

shared/aggregation/maxcover_test.go

@@ -475,6 +475,32 @@ func TestMaxCover_MaxCoverProblem_Cover(t *testing.T) {
 				Keys:     []int{0, 3, 1, 2, 6},
 			},
 		},
+		{
+			name: "empty bitlists",
+			args: args{k: 5, allowOverlaps: true, candidates: MaxCoverCandidates{
+				{1, &bitfield.Bitlist{0b0}, 0, false},
+				{0, &bitfield.Bitlist{0b00000000, 0b00000000, 0b00000000, 0b1}, 0, false},
+				{2, &bitfield.Bitlist{0b00000000, 0b00000000, 0b00000000, 0b1}, 0, false},
+			}},
+			wantedErr: "empty bitlists: invalid max_cover problem",
+		},
+		{
+			name: "overlapping solution dropped",
+			args: args{k: 5, allowOverlaps: false, candidates: MaxCoverCandidates{
+				{0, &bitfield.Bitlist{0b11111111, 0b11000111, 0b11111111, 0b1}, 0, false},
+				// All remaining bitlists will overlap, so will be dropped.
+				{1, &bitfield.Bitlist{0b11111111, 0b00001100, 0b11111111, 0b1}, 0, false},
+				{2, &bitfield.Bitlist{0b00000000, 0b01110000, 0b01110000, 0b1}, 0, false},
+				{3, &bitfield.Bitlist{0b00000111, 0b10000001, 0b10000000, 0b1}, 0, false},
+				{4, &bitfield.Bitlist{0b00000000, 0b00000110, 0b00000110, 0b1}, 0, false},
+				{5, &bitfield.Bitlist{0b00000000, 0b00000001, 0b01100010, 0b1}, 0, false},
+				{6, &bitfield.Bitlist{0b00001000, 0b00001000, 0b10000010, 0b1}, 0, false},
+			}},
+			want: &Aggregation{
+				Coverage: bitfield.Bitlist{0xff, 0b11000111, 0xff, 0b1},
+				Keys:     []int{0},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -625,6 +651,15 @@ func TestMaxCover_MaxCover(t *testing.T) {
 				Keys:     []int{0, 1, 2, 3, 6},
 			},
 		},
+		{
+			name: "empty bitlists",
+			args: args{k: 5, allowOverlaps: true, candidates: []*bitfield.Bitlist64{
+				bitfield.NewBitlist64From([]uint64{}),
+				bitfield.NewBitlist64From([]uint64{0b00000000, 0b00001110, 0b00001110}),
+				bitfield.NewBitlist64From([]uint64{0b00000000, 0b01110000, 0b01110000}),
+			}},
+			wantedErr: "empty bitlists: invalid max_cover problem",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {