Slasher: Fix surrounded votes false negative

beacon-chain/db/slasherkv/slasher.go

@@ -46,17 +46,18 @@ func (s *Store) LastEpochWrittenForValidators(
 		for i, encodedIndex := range encodedIndexes {
 			var epoch primitives.Epoch
 
-			epochBytes := bkt.Get(encodedIndex)
-			if epochBytes != nil {
+			if epochBytes := bkt.Get(encodedIndex); epochBytes != nil {
 				if err := epoch.UnmarshalSSZ(epochBytes); err != nil {
 					return err
 				}
 			}
 
-			attestedEpochs = append(attestedEpochs, &slashertypes.AttestedEpochForValidator{
+			attestedEpoch := &slashertypes.AttestedEpochForValidator{
 				ValidatorIndex: validatorIndexes[i],
 				Epoch:          epoch,
-			})
+			}
+
+			attestedEpochs = append(attestedEpochs, attestedEpoch)
 		}
 
 		return nil
@@ -83,18 +84,20 @@ func (s *Store) SaveLastEpochsWrittenForValidators(
 			return ctx.Err()
 		}
 
+		encodedIndex := encodeValidatorIndex(valIndex)
+
 		encodedEpoch, err := epoch.MarshalSSZ()
 		if err != nil {
 			return err
 		}
 
-		encodedIndexes = append(encodedIndexes, encodeValidatorIndex(valIndex))
+		encodedIndexes = append(encodedIndexes, encodedIndex)
 		encodedEpochs = append(encodedEpochs, encodedEpoch)
 	}
 
 	// The list of validators might be too massive for boltdb to handle in a single transaction,
 	// so instead we split it into batches and write each batch.
-	for i := 0; i < len(encodedIndexes); i += batchSize {
+	for start := 0; start < len(encodedIndexes); start += batchSize {
 		if ctx.Err() != nil {
 			return ctx.Err()
 		}
@@ -105,17 +108,14 @@ func (s *Store) SaveLastEpochsWrittenForValidators(
 			}
 
 			bkt := tx.Bucket(attestedEpochsByValidator)
+			end := min(start+batchSize, len(encodedIndexes))
 
-			minimum := i + batchSize
-			if minimum > len(encodedIndexes) {
-				minimum = len(encodedIndexes)
-			}
-
-			for j, encodedIndex := range encodedIndexes[i:minimum] {
+			for j, encodedIndex := range encodedIndexes[start:end] {
 				if ctx.Err() != nil {
 					return ctx.Err()
 				}
-				if err := bkt.Put(encodedIndex, encodedEpochs[j]); err != nil {
+
+				if err := bkt.Put(encodedIndex, encodedEpochs[j+start]); err != nil {
 					return err
 				}
 			}

beacon-chain/db/slasherkv/slasher_test.go

@@ -46,21 +46,29 @@ func TestStore_AttestationRecordForValidator_SaveRetrieve(t *testing.T) {
 func TestStore_LastEpochWrittenForValidators(t *testing.T) {
 	ctx := context.Background()
 	beaconDB := setupDB(t)
-	indices := []primitives.ValidatorIndex{1, 2, 3}
-	epoch := primitives.Epoch(5)
+
+	validatorsCount := 11000
+	indices := make([]primitives.ValidatorIndex, validatorsCount)
+	epochs := make([]primitives.Epoch, validatorsCount)
+
+	for i := 0; i < validatorsCount; i++ {
+		indices[i] = primitives.ValidatorIndex(i)
+		epochs[i] = primitives.Epoch(i)
+	}
 
 	attestedEpochs, err := beaconDB.LastEpochWrittenForValidators(ctx, indices)
 	require.NoError(t, err)
 	require.Equal(t, true, len(attestedEpochs) == len(indices))
+
 	for _, item := range attestedEpochs {
 		require.Equal(t, primitives.Epoch(0), item.Epoch)
 	}
 
-	epochsByValidator := map[primitives.ValidatorIndex]primitives.Epoch{
-		1: epoch,
-		2: epoch,
-		3: epoch,
+	epochsByValidator := make(map[primitives.ValidatorIndex]primitives.Epoch, validatorsCount)
+	for i := 0; i < validatorsCount; i++ {
+		epochsByValidator[indices[i]] = epochs[i]
 	}
+
 	err = beaconDB.SaveLastEpochsWrittenForValidators(ctx, epochsByValidator)
 	require.NoError(t, err)
 
@@ -70,9 +78,10 @@ func TestStore_LastEpochWrittenForValidators(t *testing.T) {
 
 	for i, retrievedEpoch := range retrievedEpochs {
 		want := &slashertypes.AttestedEpochForValidator{
-			Epoch:          epoch,
+			Epoch:          epochs[i],
 			ValidatorIndex: indices[i],
 		}
+
 		require.DeepEqual(t, want, retrievedEpoch)
 	}
 }

beacon-chain/slasher/detect_attestations.go

@@ -54,9 +54,7 @@ func (s *Service) checkSlashableAttestations(
 	surroundStart := time.Now()
 
 	for validatorChunkIndex, attestations := range groupedByValidatorChunkIndexAtts {
-		// The fact that we use always slashertypes.MinSpan is probably the root cause of
-		// https://github.com/prysmaticlabs/prysm/issues/13591
-		surroundSlashings, err := s.checkSurrounds(ctx, attestations, slashertypes.MinSpan, currentEpoch, validatorChunkIndex)
+		surroundSlashings, err := s.checkSurrounds(ctx, attestations, currentEpoch, validatorChunkIndex)
 		if err != nil {
 			return nil, err
 		}
@@ -107,27 +105,32 @@ func (s *Service) checkSlashableAttestations(
 func (s *Service) checkSurrounds(
 	ctx context.Context,
 	attestations []*slashertypes.IndexedAttestationWrapper,
-	chunkKind slashertypes.ChunkKind,
 	currentEpoch primitives.Epoch,
 	validatorChunkIndex uint64,
 ) (map[[fieldparams.RootLength]byte]*ethpb.AttesterSlashing, error) {
 	// Map of updated chunks by chunk index, which will be saved at the end.
-	updatedChunks := make(map[uint64]Chunker)
+	updatedMinChunks, updatedMaxChunks := map[uint64]Chunker{}, map[uint64]Chunker{}
+
 	groupedByChunkIndexAtts := s.groupByChunkIndex(attestations)
 	validatorIndexes := s.params.validatorIndexesInChunk(validatorChunkIndex)
 
 	slashings := map[[fieldparams.RootLength]byte]*ethpb.AttesterSlashing{}
 
 	// Update epoch for validators.
 	for _, validatorIndex := range validatorIndexes {
-		// This function modifies `updatedChunks` in place.
-		if err := s.epochUpdateForValidator(ctx, updatedChunks, validatorChunkIndex, chunkKind, currentEpoch, validatorIndex); err != nil {
-			return nil, errors.Wrapf(err, "could not update validator index chunks %d", validatorIndex)
+		// This function modifies `updatedMinChunks` in place.
+		if err := s.epochUpdateForValidator(ctx, updatedMinChunks, validatorChunkIndex, slashertypes.MinSpan, currentEpoch, validatorIndex); err != nil {
+			return nil, errors.Wrapf(err, "could not update validator index for min chunks %d", validatorIndex)
+		}
+
+		// This function modifies `updatedMaxChunks` in place.
+		if err := s.epochUpdateForValidator(ctx, updatedMaxChunks, validatorChunkIndex, slashertypes.MaxSpan, currentEpoch, validatorIndex); err != nil {
+			return nil, errors.Wrapf(err, "could not update validator index for max chunks %d", validatorIndex)
 		}
 	}
 
 	// Check for surrounding votes.
-	surroundingSlashings, err := s.updateSpans(ctx, updatedChunks, groupedByChunkIndexAtts, slashertypes.MinSpan, validatorChunkIndex, currentEpoch)
+	surroundingSlashings, err := s.updateSpans(ctx, updatedMinChunks, groupedByChunkIndexAtts, slashertypes.MinSpan, validatorChunkIndex, currentEpoch)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not update min attestation spans for validator chunk index %d", validatorChunkIndex)
 	}
@@ -137,7 +140,7 @@ func (s *Service) checkSurrounds(
 	}
 
 	// Check for surrounded votes.
-	surroundedSlashings, err := s.updateSpans(ctx, updatedChunks, groupedByChunkIndexAtts, slashertypes.MaxSpan, validatorChunkIndex, currentEpoch)
+	surroundedSlashings, err := s.updateSpans(ctx, updatedMaxChunks, groupedByChunkIndexAtts, slashertypes.MaxSpan, validatorChunkIndex, currentEpoch)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not update max attestation spans for validator chunk index %d", validatorChunkIndex)
 	}
@@ -146,8 +149,13 @@ func (s *Service) checkSurrounds(
 		slashings[root] = slashing
 	}
 
-	if err := s.saveUpdatedChunks(ctx, updatedChunks, chunkKind, validatorChunkIndex); err != nil {
-		return nil, err
+	// Save updated chunks into the database.
+	if err := s.saveUpdatedChunks(ctx, updatedMinChunks, slashertypes.MinSpan, validatorChunkIndex); err != nil {
+		return nil, errors.Wrap(err, "could not save chunks for min spans")
+	}
+
+	if err := s.saveUpdatedChunks(ctx, updatedMaxChunks, slashertypes.MaxSpan, validatorChunkIndex); err != nil {
+		return nil, errors.Wrap(err, "could not save chunks for max spans")
 	}
 
 	return slashings, nil

beacon-chain/slasher/detect_attestations_test.go

@@ -171,7 +171,6 @@ func Test_processAttestations(t *testing.T) {
 			name: "Detects surrounding vote (source 50, target 51), (source 0, target 1000) - single step",
 			steps: []*step{
 				{
-
 					currentEpoch: 1000,
 					attestationsInfo: []*attestationInfo{
 						{source: 50, target: 51, indices: []uint64{0}, beaconBlockRoot: nil},
@@ -190,7 +189,6 @@ func Test_processAttestations(t *testing.T) {
 			name: "Detects surrounding vote (source 50, target 51), (source 0, target 1000) - two steps",
 			steps: []*step{
 				{
-
 					currentEpoch: 1000,
 					attestationsInfo: []*attestationInfo{
 						{source: 50, target: 51, indices: []uint64{0}, beaconBlockRoot: nil},
@@ -229,31 +227,30 @@ func Test_processAttestations(t *testing.T) {
 				},
 			},
 		},
-		// Uncomment when https://github.com/prysmaticlabs/prysm/issues/13591 is fixed
-		// {
-		// 	name: "Detects surrounded vote (source 0, target 3), (source 1, target 2) - two steps",
-		// 	steps: []*step{
-		// 		{
-		// 			currentEpoch: 4,
-		// 			attestationsInfo: []*attestationInfo{
-		// 				{source: 0, target: 3, indices: []uint64{0, 1}, beaconBlockRoot: nil},
-		// 			},
-		// 			expectedSlashingsInfo: nil,
-		// 		},
-		// 		{
-		// 			currentEpoch: 4,
-		// 			attestationsInfo: []*attestationInfo{
-		// 				{source: 1, target: 2, indices: []uint64{0, 1}, beaconBlockRoot: nil},
-		// 			},
-		// 			expectedSlashingsInfo: []*slashingInfo{
-		// 				{
-		// 					attestationInfo_1: &attestationInfo{source: 0, target: 3, indices: []uint64{0, 1}, beaconBlockRoot: nil},
-		// 					attestationInfo_2: &attestationInfo{source: 1, target: 2, indices: []uint64{0, 1}, beaconBlockRoot: nil},
-		// 				},
-		// 			},
-		// 		},
-		// 	},
-		// },
+		{
+			name: "Detects surrounded vote (source 0, target 3), (source 1, target 2) - two steps",
+			steps: []*step{
+				{
+					currentEpoch: 4,
+					attestationsInfo: []*attestationInfo{
+						{source: 0, target: 3, indices: []uint64{0, 1}, beaconBlockRoot: nil},
+					},
+					expectedSlashingsInfo: nil,
+				},
+				{
+					currentEpoch: 4,
+					attestationsInfo: []*attestationInfo{
+						{source: 1, target: 2, indices: []uint64{0, 1}, beaconBlockRoot: nil},
+					},
+					expectedSlashingsInfo: []*slashingInfo{
+						{
+							attestationInfo_1: &attestationInfo{source: 1, target: 2, indices: []uint64{0, 1}, beaconBlockRoot: nil},
+							attestationInfo_2: &attestationInfo{source: 0, target: 3, indices: []uint64{0, 1}, beaconBlockRoot: nil},
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "Detects double vote, (source 1, target 2), (source 0, target 2) - single step",
 			steps: []*step{