Skip to content

Commit

Permalink
Slasher: Fix surrounded votes false negative (#13612)
Browse files Browse the repository at this point in the history 
* `LastEpochWrittenForValidators`: Use golang if.

* `SaveLastEpochsWrittenForValidators`: Refactor.

* `SaveLastEpochsWrittenForValidators`: Fix when `epochByValIndex` > `batchSize`.

Before this commit, `TestStore_LastEpochWrittenForValidators` works if `validatorsCount <= 10000`
and stops working if `validatorsCount > 10000`.

* Slasher: Detect surrounded votes in multiple batches.

Fixes #13591.
  • Loading branch information
@nalepae
nalepae authored Feb 13, 2024
1 parent f2f10e7 commit 3d13c69
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 59 deletions.
26 changes: 13 additions & 13 deletions beacon-chain/db/slasherkv/slasher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
}
Expand All @@ -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
}
}
Expand Down
23 changes: 16 additions & 7 deletions beacon-chain/db/slasherkv/slasher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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)
}
}
Expand Down
32 changes: 20 additions & 12 deletions beacon-chain/slasher/detect_attestations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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
Expand Down
51 changes: 24 additions & 27 deletions beacon-chain/slasher/detect_attestations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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},
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit 3d13c69

Please sign in to comment.