Update seen for attestation pool (prysmaticlabs#4669)

* Use `contain` instead of `overlap`
* Tests
* Update seen
* Merge branch 'master' into use-contain
* Use OR to track all of the bits we have seen so far
* Added one more test case
* gofmt
* Conflict
* Picked up Preston's changes
* Merge branch 'use-contain' of git+ssh://github.com/prysmaticlabs/prysm into use-contain

beacon-chain/operations/attestations/prepare_forkchoice.go

@@ -101,17 +101,23 @@ func (s *Service) seen(att *ethpb.Attestation) (bool, error) {
 	if err != nil {
 		return false, err
 	}
+	incomingBits := att.AggregationBits
 	savedBits, ok := s.forkChoiceProcessedRoots.Get(string(attRoot[:]))
 	if ok {
 		savedBitlist, ok := savedBits.(bitfield.Bitlist)
 		if !ok {
 			return false, errors.New("not a bit field")
 		}
-		if savedBitlist.Len() == att.AggregationBits.Len() && savedBitlist.Overlaps(att.AggregationBits) {
-			return true, nil
+		if savedBitlist.Len() == incomingBits.Len() {
+			// Returns true if the node has seen all the bits in the new bit field of the incoming attestation.
+			if savedBitlist.Contains(incomingBits) {
+				return true, nil
+			}
+			// Update the bit fields by Or'ing them with the new ones.
+			incomingBits = incomingBits.Or(savedBitlist)
 		}
 	}
 
-	s.forkChoiceProcessedRoots.Set(string(attRoot[:]), att.AggregationBits, 1 /*cost*/)
+	s.forkChoiceProcessedRoots.Set(string(attRoot[:]), incomingBits, 1 /*cost*/)
 	return false, nil
 }

beacon-chain/operations/attestations/prepare_forkchoice_test.go

@@ -220,7 +220,7 @@ func TestSeenAttestations_PresentInCache(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	att1 := &ethpb.Attestation{Data: &ethpb.AttestationData{}, Signature: []byte{'A'}, AggregationBits: bitfield.Bitlist{0x03}}
+	att1 := &ethpb.Attestation{Data: &ethpb.AttestationData{}, Signature: []byte{'A'}, AggregationBits: bitfield.Bitlist{0x13} /* 0b00010011 */}
 	got, err := s.seen(att1)
 	if err != nil {
 		t.Fatal(err)
@@ -231,12 +231,86 @@ func TestSeenAttestations_PresentInCache(t *testing.T) {
 
 	time.Sleep(100 * time.Millisecond)
 
-	att2 := &ethpb.Attestation{Data: &ethpb.AttestationData{}, Signature: []byte{'A'}, AggregationBits: bitfield.Bitlist{0x03}}
+	att2 := &ethpb.Attestation{Data: &ethpb.AttestationData{}, Signature: []byte{'A'}, AggregationBits: bitfield.Bitlist{0x17} /* 0b00010111 */}
 	got, err = s.seen(att2)
 	if err != nil {
 		t.Fatal(err)
 	}
+	if got {
+		t.Error("Wanted false, got true")
+	}
+
+	time.Sleep(100 * time.Millisecond)
+
+	att3 := &ethpb.Attestation{Data: &ethpb.AttestationData{}, Signature: []byte{'A'}, AggregationBits: bitfield.Bitlist{0x17} /* 0b00010111 */}
+	got, err = s.seen(att3)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if !got {
 		t.Error("Wanted true, got false")
 	}
 }
+
+func TestService_seen(t *testing.T) {
+	// Attestation are checked in order of this list.
+	tests := []struct {
+		att  *ethpb.Attestation
+		want bool
+	}{
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b11011},
+				Data:            &ethpb.AttestationData{Slot: 1},
+			},
+			want: false,
+		},
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b11011},
+				Data:            &ethpb.AttestationData{Slot: 1},
+			},
+			want: true, // Exact same attestation should return true
+		},
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b10101},
+				Data:            &ethpb.AttestationData{Slot: 1},
+			},
+			want: false, // Haven't seen the bit at index 2 yet.
+		},
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b11111},
+				Data:            &ethpb.AttestationData{Slot: 1},
+			},
+			want: true, // We've full committee at this point.
+		},
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b11111},
+				Data:            &ethpb.AttestationData{Slot: 2},
+			},
+			want: false, // Different root is different bitlist.
+		},
+		{
+			att: &ethpb.Attestation{
+				AggregationBits: bitfield.Bitlist{0b11111001},
+				Data:            &ethpb.AttestationData{Slot: 1},
+			},
+			want: false, // Sanity test that an attestation of different lengths does not panic.
+		},
+	}
+
+	s, err := NewService(context.Background(), &Config{Pool: NewPool()})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for i, tt := range tests {
+		if got, _ := s.seen(tt.att); got != tt.want {
+			t.Errorf("Test %d failed. Got=%v want=%v", i, got, tt.want)
+		}
+		time.Sleep(10) // Sleep briefly for cache to routine to buffer.
+	}
+}