Slasher: Refactor and add tests (#13589)

* `helpers.go`: Improve naming consistency.

* `detect_attestations.go`: Improve readability.

* `receive.go`: Add `attsQueueSize` in log message.

* `checkSlashableAttestations`: Improve logging.

`avgBatchProcessingTime` is not displayed any more if not batch is
processed.

* `loadChunks`: Use explicit `chunkKind` and `chunkIndices`.

* `getChunk`: Use specific `chunkIndex` and `chunkKind`.

* `validatorIndicesInChunk` -> `validatorIndexesInChunk`.

* `epochUpdateForValidator`: Use explicit arguments.

* `getChunk`: Change order of arguments.

* `latestEpochWrittenForValidator`: Use `ok` parameter.

So the default value is not any more considered as the absence of
value.

* `applyAttestationForValidator`: Use explicit arguments.

* `updateSpans`: Use explicit arguments.

* `saveUpdatedChunks`: Use explicit arguments.

* `checkSurrounds`: Use explicit arguments.

We see here that, previously, in `checkSlashableAttestations`,
`checkSurrounds` was called with the default value of `slashertypes`: `MinSpan`.

Now, we set it expliciterly at `MinSpan`, which may explicit a bug.

* `epochUpdateForValidator`: Set modified by the function argument first.

* `applyAttestationForValidator`: Set mutated argument `chunksByChunkIdx`first.

* `applyAttestationForValidator`: Rename variables.

* `Test_processQueuedAttestations`: Fix test.

Two tests were actually exactly the same.

* `updateSpans`: Keep happy path in the outer scope.

Even if in this case the "happy" path means slashing.

* `checkSurrounds`: Rename variable.

* `getChunk`: Avoid side effects.

It adds a few lines for callers, but it does not modify any more
arguments and it does what it says: getting a chunk.

* `CheckSlashable`: Flatten.

* `detect_attestations_test.go`: Simplify.

* `CheckSlashable`: Add error log in case of missing attestation.

* `processQueuedAttestations`: Extract a sub function.

So testing will be easier.

* `processAttesterSlashings` and `processProposerSlashings`: Improve.

* `processAttesterSlashings`: Return processed slashings.

* `createAttestationWrapper`: Rename variables.

* `signingRoot` ==> `headerRoot` or `dataRoot`.

Before this commit, there is two typse of `signing root`s floating around.
- The first one is a real signing root, aka a hash tree root computed from an object root and
a domain. This real signing root is the object ready to be signed.
- The second one is a "false" signing root, which is actually just the hash tree root of an object. This object is either the `Data` field of an attestation, or the `Header` field of a block.

Having 2 differents objects with the same name `signing root` is quite confusing.
This commit renames wrongly named `signing root` objects.

* `createAttestationWrapper` => `createAttestationWrapperEmptySig`.

So it's clear for the user that the created attestation wrapper has an empty signature.

* Implement `createAttestationWrapper`.

* `processAttestations`: Return processed attester slashings.

* Test `processAttestations` instead of `processQueuedAttestations`.

By testing `processAttestations` instead of `processQueuedAttestations`, we get rid of a lot of tests fixtures, including the 200 ms sleep.

The whole testing duration is shorter.

* `Test_processAttestations`: Allow multiple steps.

* `Test_processAttestations`: Add double steps tests.

Some new failing tests are commented with a corresponding github issue.

* `NextChunkStartEpoch`: Fix function comment.

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

* `chunks.go`: Avoid templating log messages.

* `checkSlashableAttestations`: Simplify duration computation.

---------

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

beacon-chain/db/slasherkv/slasher.go

@@ -21,9 +21,8 @@ import (
 )
 
 const (
-	// Signing root (32 bytes)
 	attestationRecordKeySize = 32 // Bytes.
-	signingRootSize          = 32 // Bytes.
+	rootSize                 = 32 // Bytes.
 )
 
 // LastEpochWrittenForValidators given a list of validator indices returns the latest
@@ -170,15 +169,15 @@ func (s *Store) CheckAttesterDoubleVotes(
 						continue
 					}
 
-					// Retrieve the attestation record corresponding to the signing root
+					// Retrieve the attestation record corresponding to the data root
 					// from the database.
 					encExistingAttRecord := attRecordsBkt.Get(attRecordsKey)
 					if encExistingAttRecord == nil {
 						continue
 					}
 
-					existingSigningRoot := bytesutil.ToBytes32(attRecordsKey[:signingRootSize])
-					if existingSigningRoot == attToProcess.SigningRoot {
+					existingDataRoot := bytesutil.ToBytes32(attRecordsKey[:rootSize])
+					if existingDataRoot == attToProcess.DataRoot {
 						continue
 					}
 
@@ -281,20 +280,20 @@ func (s *Store) SaveAttestationRecordsForValidators(
 
 	return s.db.Update(func(tx *bolt.Tx) error {
 		attRecordsBkt := tx.Bucket(attestationRecordsBucket)
-		signingRootsBkt := tx.Bucket(attestationDataRootsBucket)
+		dataRootsBkt := tx.Bucket(attestationDataRootsBucket)
 
 		for i := len(attestations) - 1; i >= 0; i-- {
 			attestation := attestations[i]
 
-			if err := attRecordsBkt.Put(attestation.SigningRoot[:], encodedRecords[i]); err != nil {
+			if err := attRecordsBkt.Put(attestation.DataRoot[:], encodedRecords[i]); err != nil {
 				return err
 			}
 
 			for _, valIdx := range attestation.IndexedAttestation.AttestingIndices {
 				encIdx := encodeValidatorIndex(primitives.ValidatorIndex(valIdx))
 
 				key := append(encodedTargetEpoch[i], encIdx...)
-				if err := signingRootsBkt.Put(key, attestation.SigningRoot[:]); err != nil {
+				if err := dataRootsBkt.Put(key, attestation.DataRoot[:]); err != nil {
 					return err
 				}
 			}
@@ -396,14 +395,14 @@ func (s *Store) CheckDoubleBlockProposals(
 
 			// If there is no existing proposal record (empty result), then there is no double proposal.
 			// We can continue to the next proposal.
-			if len(encExistingProposalWrapper) < signingRootSize {
+			if len(encExistingProposalWrapper) < rootSize {
 				continue
 			}
 
 			// Compare the proposal signing root in the DB with the incoming proposal signing root.
 			// If they differ, we have a double proposal.
-			existingSigningRoot := bytesutil.ToBytes32(encExistingProposalWrapper[:signingRootSize])
-			if existingSigningRoot != incomingProposal.SigningRoot {
+			existingRoot := bytesutil.ToBytes32(encExistingProposalWrapper[:rootSize])
+			if existingRoot != incomingProposal.HeaderRoot {
 				existingProposalWrapper, err := decodeProposalRecord(encExistingProposalWrapper)
 				if err != nil {
 					return err
@@ -610,18 +609,18 @@ func encodeAttestationRecord(att *slashertypes.IndexedAttestationWrapper) ([]byt
 	// Compress attestation.
 	compressedAtt := snappy.Encode(nil, encodedAtt)
 
-	return append(att.SigningRoot[:], compressedAtt...), nil
+	return append(att.DataRoot[:], compressedAtt...), nil
 }
 
 // Decode attestation record from bytes.
 // The input encoded attestation record consists in the signing root concatened with the compressed attestation record.
 func decodeAttestationRecord(encoded []byte) (*slashertypes.IndexedAttestationWrapper, error) {
-	if len(encoded) < signingRootSize {
-		return nil, fmt.Errorf("wrong length for encoded attestation record, want minimum %d, got %d", signingRootSize, len(encoded))
+	if len(encoded) < rootSize {
+		return nil, fmt.Errorf("wrong length for encoded attestation record, want minimum %d, got %d", rootSize, len(encoded))
 	}
 
 	// Decompress attestation.
-	decodedAttBytes, err := snappy.Decode(nil, encoded[signingRootSize:])
+	decodedAttBytes, err := snappy.Decode(nil, encoded[rootSize:])
 	if err != nil {
 		return nil, err
 	}
@@ -633,13 +632,13 @@ func decodeAttestationRecord(encoded []byte) (*slashertypes.IndexedAttestationWr
 	}
 
 	// Decode signing root.
-	signingRootBytes := encoded[:signingRootSize]
-	signingRoot := bytesutil.ToBytes32(signingRootBytes)
+	dataRootBytes := encoded[:rootSize]
+	dataRoot := bytesutil.ToBytes32(dataRootBytes)
 
 	// Return decoded attestation.
 	attestation := &slashertypes.IndexedAttestationWrapper{
 		IndexedAttestation: decodedAtt,
-		SigningRoot:        signingRoot,
+		DataRoot:           dataRoot,
 	}
 
 	return attestation, nil
@@ -654,18 +653,18 @@ func encodeProposalRecord(blkHdr *slashertypes.SignedBlockHeaderWrapper) ([]byte
 		return nil, err
 	}
 	compressedHdr := snappy.Encode(nil, encodedHdr)
-	return append(blkHdr.SigningRoot[:], compressedHdr...), nil
+	return append(blkHdr.HeaderRoot[:], compressedHdr...), nil
 }
 
 func decodeProposalRecord(encoded []byte) (*slashertypes.SignedBlockHeaderWrapper, error) {
-	if len(encoded) < signingRootSize {
+	if len(encoded) < rootSize {
 		return nil, fmt.Errorf(
-			"wrong length for encoded proposal record, want %d, got %d", signingRootSize, len(encoded),
+			"wrong length for encoded proposal record, want %d, got %d", rootSize, len(encoded),
 		)
 	}
-	signingRoot := encoded[:signingRootSize]
+	dataRoot := encoded[:rootSize]
 	decodedBlkHdr := &ethpb.SignedBeaconBlockHeader{}
-	decodedHdrBytes, err := snappy.Decode(nil, encoded[signingRootSize:])
+	decodedHdrBytes, err := snappy.Decode(nil, encoded[rootSize:])
 	if err != nil {
 		return nil, err
 	}
@@ -674,7 +673,7 @@ func decodeProposalRecord(encoded []byte) (*slashertypes.SignedBlockHeaderWrappe
 	}
 	return &slashertypes.SignedBlockHeaderWrapper{
 		SignedBeaconBlockHeader: decodedBlkHdr,
-		SigningRoot:             bytesutil.ToBytes32(signingRoot),
+		HeaderRoot:              bytesutil.ToBytes32(dataRoot),
 	}, nil
 }
 

beacon-chain/db/slasherkv/slasher_test.go

@@ -40,7 +40,7 @@ func TestStore_AttestationRecordForValidator_SaveRetrieve(t *testing.T) {
 	require.NoError(t, err)
 	assert.DeepEqual(t, target, attRecord.IndexedAttestation.Data.Target.Epoch)
 	assert.DeepEqual(t, source, attRecord.IndexedAttestation.Data.Source.Epoch)
-	assert.DeepEqual(t, sr, attRecord.SigningRoot)
+	assert.DeepEqual(t, sr, attRecord.DataRoot)
 }
 
 func TestStore_LastEpochWrittenForValidators(t *testing.T) {
@@ -224,9 +224,9 @@ func TestStore_ExistingBlockProposals(t *testing.T) {
 
 	// Second time checking same proposals but all with different signing root should
 	// return all double proposals.
-	proposals[0].SigningRoot = bytesutil.ToBytes32([]byte{2})
-	proposals[1].SigningRoot = bytesutil.ToBytes32([]byte{2})
-	proposals[2].SigningRoot = bytesutil.ToBytes32([]byte{2})
+	proposals[0].HeaderRoot = bytesutil.ToBytes32([]byte{2})
+	proposals[1].HeaderRoot = bytesutil.ToBytes32([]byte{2})
+	proposals[2].HeaderRoot = bytesutil.ToBytes32([]byte{2})
 
 	doubleProposals, err = beaconDB.CheckDoubleBlockProposals(ctx, proposals)
 	require.NoError(t, err)
@@ -515,7 +515,7 @@ func createProposalWrapper(t *testing.T, slot primitives.Slot, proposerIndex pri
 		StateRoot:     bytesutil.PadTo(signingRoot, 32),
 		BodyRoot:      params.BeaconConfig().ZeroHash[:],
 	}
-	signRoot, err := header.HashTreeRoot()
+	headerRoot, err := header.HashTreeRoot()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -524,15 +524,16 @@ func createProposalWrapper(t *testing.T, slot primitives.Slot, proposerIndex pri
 			Header:    header,
 			Signature: params.BeaconConfig().EmptySignature[:],
 		},
-		SigningRoot: signRoot,
+		HeaderRoot: headerRoot,
 	}
 }
 
-func createAttestationWrapper(source, target primitives.Epoch, indices []uint64, signingRoot []byte) *slashertypes.IndexedAttestationWrapper {
-	signRoot := bytesutil.ToBytes32(signingRoot)
-	if signingRoot == nil {
-		signRoot = params.BeaconConfig().ZeroHash
+func createAttestationWrapper(source, target primitives.Epoch, indices []uint64, dataRootBytes []byte) *slashertypes.IndexedAttestationWrapper {
+	dataRoot := bytesutil.ToBytes32(dataRootBytes)
+	if dataRootBytes == nil {
+		dataRoot = params.BeaconConfig().ZeroHash
 	}
+
 	data := &ethpb.AttestationData{
 		BeaconBlockRoot: params.BeaconConfig().ZeroHash[:],
 		Source: &ethpb.Checkpoint{
@@ -544,13 +545,14 @@ func createAttestationWrapper(source, target primitives.Epoch, indices []uint64,
 			Root:  params.BeaconConfig().ZeroHash[:],
 		},
 	}
+
 	return &slashertypes.IndexedAttestationWrapper{
 		IndexedAttestation: &ethpb.IndexedAttestation{
 			AttestingIndices: indices,
 			Data:             data,
 			Signature:        params.BeaconConfig().EmptySignature[:],
 		},
-		SigningRoot: signRoot,
+		DataRoot: dataRoot,
 	}
 }
 

beacon-chain/slasher/BUILD.bazel

@@ -30,7 +30,6 @@ go_library(
         "//beacon-chain/operations/slashings:go_default_library",
         "//beacon-chain/slasher/types:go_default_library",
         "//beacon-chain/startup:go_default_library",
-        "//beacon-chain/state:go_default_library",
         "//beacon-chain/state/stategen:go_default_library",
         "//beacon-chain/sync:go_default_library",
         "//config/fieldparams:go_default_library",
@@ -78,6 +77,7 @@ go_test(
         "//config/params:go_default_library",
         "//consensus-types/primitives:go_default_library",
         "//crypto/bls:go_default_library",
+        "//crypto/bls/common:go_default_library",
         "//encoding/bytesutil:go_default_library",
         "//proto/prysm/v1alpha1:go_default_library",
         "//testing/assert:go_default_library",

beacon-chain/slasher/chunks.go

@@ -10,16 +10,15 @@ import (
 	slashertypes "github.com/prysmaticlabs/prysm/v4/beacon-chain/slasher/types"
 	"github.com/prysmaticlabs/prysm/v4/consensus-types/primitives"
 	ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1"
+	"github.com/sirupsen/logrus"
 )
 
 // A struct encapsulating input arguments to
 // functions used for attester slashing detection and
 // loading, saving, and updating min/max span chunks.
 type chunkUpdateArgs struct {
-	kind                slashertypes.ChunkKind
-	chunkIndex          uint64
-	validatorChunkIndex uint64
-	currentEpoch        primitives.Epoch
+	chunkIndex   uint64
+	currentEpoch primitives.Epoch
 }
 
 // Chunker defines a struct which represents a slice containing a chunk for K different validator's
@@ -199,30 +198,45 @@ func (m *MinSpanChunksSlice) CheckSlashable(
 		)
 	}
 
-	if targetEpoch > minTarget {
-		existingAttRecord, err := slasherDB.AttestationRecordForValidator(
-			ctx, validatorIdx, minTarget,
-		)
-		if err != nil {
-			return nil, errors.Wrapf(
-				err, "could not get existing attestation record at target %d", minTarget,
-			)
-		}
+	if targetEpoch <= minTarget {
+		// The incoming attestation does not surround any existing ones.
+		return nil, nil
+	}
 
-		if existingAttRecord == nil {
-			return nil, nil
-		}
+	// The incoming attestation surrounds an existing one.
+	existingAttRecord, err := slasherDB.AttestationRecordForValidator(ctx, validatorIdx, minTarget)
+	if err != nil {
+		return nil, errors.Wrapf(err, "could not get existing attestation record at target %d", minTarget)
+	}
 
-		if sourceEpoch < existingAttRecord.IndexedAttestation.Data.Source.Epoch {
-			surroundingVotesTotal.Inc()
-			return &ethpb.AttesterSlashing{
-				Attestation_1: attestation.IndexedAttestation,
-				Attestation_2: existingAttRecord.IndexedAttestation,
-			}, nil
+	if existingAttRecord == nil {
+		// This case should normally not happen. If this happen, it means we previously
+		// recorded in our min/max DB an distance corresponding to an attestaiton, but WITHOUT
+		// recording the attestation itself. As a consequence, we say there is no surrounding vote,
+		// but we log an error.
+		fields := logrus.Fields{
+			"validatorIndex": validatorIdx,
+			"targetEpoch":    minTarget,
 		}
+
+		log.WithFields(fields).Error("No existing attestation record found while a surrounding vote was detected.")
+		return nil, nil
+	}
+
+	if existingAttRecord.IndexedAttestation.Data.Source.Epoch <= sourceEpoch {
+		// This case should normally not happen, since if we have targetEpoch > minTarget,
+		// then there is at least one attestation we surround.
+		// However, it can happens if we have multiple attestation with the same target
+		// but with a different source. In this case, we have both a double vote AND a surround vote.
+		// The validator will be slashed for the double vote, and the surround vote will be ignored.
+		return nil, nil
 	}
 
-	return nil, nil
+	surroundingVotesTotal.Inc()
+	return &ethpb.AttesterSlashing{
+		Attestation_1: attestation.IndexedAttestation,
+		Attestation_2: existingAttRecord.IndexedAttestation,
+	}, nil
 }
 
 // CheckSlashable takes in a validator index and an incoming attestation
@@ -252,29 +266,45 @@ func (m *MaxSpanChunksSlice) CheckSlashable(
 		)
 	}
 
-	if targetEpoch < maxTarget {
-		existingAttRecord, err := slasherDB.AttestationRecordForValidator(
-			ctx, validatorIdx, maxTarget,
-		)
-		if err != nil {
-			return nil, errors.Wrapf(
-				err, "could not get existing attestation record at target %d", maxTarget,
-			)
-		}
+	if targetEpoch >= maxTarget {
+		// The incoming attestation is not surrounded by any existing ones.
+		return nil, nil
+	}
 
-		if existingAttRecord == nil {
-			return nil, nil
-		}
+	// The incoming attestation is surrounded by an existing one.
+	existingAttRecord, err := slasherDB.AttestationRecordForValidator(ctx, validatorIdx, maxTarget)
+	if err != nil {
+		return nil, errors.Wrapf(err, "could not get existing attestation record at target %d", maxTarget)
+	}
 
-		if existingAttRecord.IndexedAttestation.Data.Source.Epoch < sourceEpoch {
-			surroundedVotesTotal.Inc()
-			return &ethpb.AttesterSlashing{
-				Attestation_1: existingAttRecord.IndexedAttestation,
-				Attestation_2: attestation.IndexedAttestation,
-			}, nil
+	if existingAttRecord == nil {
+		// This case should normally not happen. If this happen, it means we previously
+		// recorded in our min/max DB an distance corresponding to an attestaiton, but WITHOUT
+		// recording the attestation itself. As a consequence, we say there is no surrounded vote,
+		// but we log an error.
+		fields := logrus.Fields{
+			"validatorIndex": validatorIdx,
+			"targetEpoch":    maxTarget,
 		}
+
+		log.WithFields(fields).Error("No existing attestation record found while a surrounded vote was detected.")
+		return nil, nil
+	}
+
+	if existingAttRecord.IndexedAttestation.Data.Source.Epoch >= sourceEpoch {
+		// This case should normally not happen, since if we have targetEpoch < maxTarget,
+		// then there is at least one attestation that surrounds us.
+		// However, it can happens if we have multiple attestation with the same target
+		// but with a different source. In this case, we have both a double vote AND a surround vote.
+		// The validator will be slashed for the double vote, and the surround vote will be ignored.
+		return nil, nil
 	}
-	return nil, nil
+
+	surroundedVotesTotal.Inc()
+	return &ethpb.AttesterSlashing{
+		Attestation_1: existingAttRecord.IndexedAttestation,
+		Attestation_2: attestation.IndexedAttestation,
+	}, nil
 }
 
 // Update a min span chunk for a validator index starting at the current epoch, e_c, then updating
@@ -492,13 +522,13 @@ func (m *MinSpanChunksSlice) NextChunkStartEpoch(startEpoch primitives.Epoch) pr
 //	max_spans_val_i = [[-, -, -], [-, -, -], [-, -, -]]
 //
 // If C = chunkSize is 3 epochs per chunk, and we input start epoch of chunk 1 which is 3. The next start
-// epoch is the start epoch of chunk 2, which is epoch 4. This is computed as:
+// epoch is the start epoch of chunk 2, which is epoch 6. This is computed as:
 //
 //	first_epoch(chunkIndex(startEpoch)+1)
 //	first_epoch(chunkIndex(3)+1)
 //	first_epoch(1 + 1)
 //	first_epoch(2)
-//	4
+//	6
 func (m *MaxSpanChunksSlice) NextChunkStartEpoch(startEpoch primitives.Epoch) primitives.Epoch {
 	return m.params.firstEpoch(m.params.chunkIndex(startEpoch) + 1)
 }