Fix slasher multiple proposals false negative.

If a first batch of blocks is sent with: - validator 1 - slot 4 - signing root 1 - validator 1 - slot 5 - signing root 1 Then, if a second batch of blocks is sent with: - validator 1 - slot 4 - signing root 2 Because we have two blocks proposed by the same validator (1) and for the same slot (4), but with two different signing roots (1 and 2), the validator 1 should be slashed. This is not the case before this commit. A new test case has been added as well to check this.
prysmaticlabs · Jan 29, 2024 · 39b37e6 · 39b37e6
1 parent fadb50a
commit 39b37e6
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 123 deletions.
diff --git a/beacon-chain/slasher/detect_blocks.go b/beacon-chain/slasher/detect_blocks.go
@@ -5,7 +5,6 @@ import (
 
 	"github.com/pkg/errors"
 	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"
 	"go.opencensus.io/trace"
 )
@@ -60,7 +59,7 @@ func (s *Service) detectProposerSlashings(
 	// We save the safe proposals (with respect to the database) to our database.
 	// If some proposals in incomingProposals are slashable with respect to each other,
 	// we (arbitrarily) save the last one to the database.
-	if err := s.saveSafeProposals(ctx, incomingProposals, databaseSlashings); err != nil {
+	if err := s.serviceCfg.Database.SaveBlockProposals(ctx, incomingProposals); err != nil {
 		return nil, errors.Wrap(err, "could not save safe proposals")
 	}
 
@@ -69,51 +68,6 @@ func (s *Service) detectProposerSlashings(
 	return totalSlashings, nil
 }
 
-// Check for double proposals in our database given a list of incoming block proposals.
-// For the proposals that were not slashable with respect to the database,
-// we save them to the database.
-// For the proposals that are slashable with respect to the content of proposals themselves,
-// we (arbitrarily) save the last one to the dtatabase.
-func (s *Service) saveSafeProposals(
-	ctx context.Context,
-	proposals []*slashertypes.SignedBlockHeaderWrapper,
-	proposerSlashings []*ethpb.ProposerSlashing,
-) error {
-	ctx, span := trace.StartSpan(ctx, "slasher.saveSafeProposals")
-	defer span.End()
-
-	filteredProposals := filterSafeProposals(proposals, proposerSlashings)
-	return s.serviceCfg.Database.SaveBlockProposals(ctx, filteredProposals)
-}
-
-// filterSafeProposals, given a list of proposals and a list of proposer slashings,
-// filters out proposals for which the proposer index is found in proposer slashings.
-func filterSafeProposals(
-	proposals []*slashertypes.SignedBlockHeaderWrapper,
-	proposerSlashings []*ethpb.ProposerSlashing,
-) []*slashertypes.SignedBlockHeaderWrapper {
-	// We initialize a map of proposers that are safe from slashing.
-	safeProposers := make(map[primitives.ValidatorIndex]*slashertypes.SignedBlockHeaderWrapper, len(proposals))
-
-	for _, proposal := range proposals {
-		safeProposers[proposal.SignedBeaconBlockHeader.Header.ProposerIndex] = proposal
-	}
-
-	for _, proposerSlashing := range proposerSlashings {
-		// If a proposer is found to have committed a slashable offense, we delete
-		// them from the safe proposers map.
-		delete(safeProposers, proposerSlashing.Header_1.Header.ProposerIndex)
-	}
-
-	// We save all the proposals that are determined "safe" and not-slashable to our database.
-	safeProposals := make([]*slashertypes.SignedBlockHeaderWrapper, 0, len(safeProposers))
-	for _, proposal := range safeProposers {
-		safeProposals = append(safeProposals, proposal)
-	}
-
-	return safeProposals
-}
-
 // proposalKey build a key which is a combination of the slot and the proposer index.
 // If a validator proposes several blocks for the same slot, then several (potentially slashable)
 // proposals will correspond to the same key.

diff --git a/beacon-chain/slasher/detect_blocks_test.go b/beacon-chain/slasher/detect_blocks_test.go
@@ -46,95 +46,113 @@ func Test_processQueuedBlocks_DetectsDoubleProposals(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "detects double proposals in the different batches",
+			wraps: []wrapped{
+				{
+					5,
+					[]*slashertypes.SignedBlockHeaderWrapper{
+						createProposalWrapper(t, 4, 1, []byte{1}),
+						createProposalWrapper(t, 5, 1, []byte{1}),
+					},
+				},
+				{
+					6,
+					[]*slashertypes.SignedBlockHeaderWrapper{
+						createProposalWrapper(t, 4, 1, []byte{2}),
+					},
+				},
+			},
+		},
 	}
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-		hook := logTest.NewGlobal()
+			hook := logTest.NewGlobal()
 			beaconDB := dbtest.SetupDB(t)
-		slasherDB := dbtest.SetupSlasherDB(t)
-		ctx, cancel := context.WithCancel(context.Background())
-
-		beaconState, err := util.NewBeaconState()
-		require.NoError(t, err)
-
-		// Initialize validators in the state.
-		numVals := params.BeaconConfig().MinGenesisActiveValidatorCount
-		validators := make([]*ethpb.Validator, numVals)
-		privKeys := make([]bls.SecretKey, numVals)
-		for i := range validators {
-			privKey, err := bls.RandKey()
-			require.NoError(t, err)
-			privKeys[i] = privKey
-			validators[i] = &ethpb.Validator{
-				PublicKey:             privKey.PublicKey().Marshal(),
-				WithdrawalCredentials: make([]byte, 32),
-			}
-		}
-		err = beaconState.SetValidators(validators)
-		require.NoError(t, err)
-		domain, err := signing.Domain(
-			beaconState.Fork(),
-			0,
-			params.BeaconConfig().DomainBeaconProposer,
-			beaconState.GenesisValidatorsRoot(),
-		)
-		require.NoError(t, err)
-
-		mockChain := &mock.ChainService{
-			State: beaconState,
-		}
-		s := &Service{
-			serviceCfg: &ServiceConfig{
-				Database:             slasherDB,
-				StateNotifier:        &mock.MockStateNotifier{},
-				HeadStateFetcher:     mockChain,
-				StateGen:             stategen.New(beaconDB, doublylinkedtree.New()),
-				SlashingPoolInserter: &slashingsmock.PoolMock{},
-				ClockWaiter:          startup.NewClockSynchronizer(),
-			},
-			params:    DefaultParams(),
-			blksQueue: newBlocksQueue(),
-		}
-
-		parentRoot := bytesutil.ToBytes32([]byte("parent"))
-		err = s.serviceCfg.StateGen.SaveState(ctx, parentRoot, beaconState)
-		require.NoError(t, err)
-
-		currentSlotChan := make(chan primitives.Slot)
-		s.wg.Add(1)
-		go func() {
-			s.processQueuedBlocks(ctx, currentSlotChan)
-		}()
-
-		for _, wrap := range tt.wraps {
-			// Add valid signatures to the block headers we are testing.
-			for _, proposalWrapper := range wrap.signedBlkHeaders {
-				proposalWrapper.SignedBeaconBlockHeader.Header.ParentRoot = parentRoot[:]
-				headerHtr, err := proposalWrapper.SignedBeaconBlockHeader.Header.HashTreeRoot()
-				require.NoError(t, err)
+			slasherDB := dbtest.SetupSlasherDB(t)
+			ctx, cancel := context.WithCancel(context.Background())
 
-				container := &ethpb.SigningData{
-					ObjectRoot: headerHtr[:],
-					Domain:     domain,
-				}
+			beaconState, err := util.NewBeaconState()
+			require.NoError(t, err)
 
-				signingRoot, err := container.HashTreeRoot()
+			// Initialize validators in the state.
+			numVals := params.BeaconConfig().MinGenesisActiveValidatorCount
+			validators := make([]*ethpb.Validator, numVals)
+			privKeys := make([]bls.SecretKey, numVals)
+			for i := range validators {
+				privKey, err := bls.RandKey()
 				require.NoError(t, err)
+				privKeys[i] = privKey
+				validators[i] = &ethpb.Validator{
+					PublicKey:             privKey.PublicKey().Marshal(),
+					WithdrawalCredentials: make([]byte, 32),
+				}
+			}
+			err = beaconState.SetValidators(validators)
+			require.NoError(t, err)
+			domain, err := signing.Domain(
+				beaconState.Fork(),
+				0,
+				params.BeaconConfig().DomainBeaconProposer,
+				beaconState.GenesisValidatorsRoot(),
+			)
+			require.NoError(t, err)
 
-				privKey := privKeys[proposalWrapper.SignedBeaconBlockHeader.Header.ProposerIndex]
-				proposalWrapper.SignedBeaconBlockHeader.Signature = privKey.Sign(signingRoot[:]).Marshal()
+			mockChain := &mock.ChainService{
+				State: beaconState,
+			}
+			s := &Service{
+				serviceCfg: &ServiceConfig{
+					Database:             slasherDB,
+					StateNotifier:        &mock.MockStateNotifier{},
+					HeadStateFetcher:     mockChain,
+					StateGen:             stategen.New(beaconDB, doublylinkedtree.New()),
+					SlashingPoolInserter: &slashingsmock.PoolMock{},
+					ClockWaiter:          startup.NewClockSynchronizer(),
+				},
+				params:    DefaultParams(),
+				blksQueue: newBlocksQueue(),
 			}
 
-			s.blksQueue.extend(wrap.signedBlkHeaders)
+			parentRoot := bytesutil.ToBytes32([]byte("parent"))
+			err = s.serviceCfg.StateGen.SaveState(ctx, parentRoot, beaconState)
+			require.NoError(t, err)
 
-			currentSlot := primitives.Slot(4)
-			currentSlotChan <- currentSlot
-		}
+			currentSlotChan := make(chan primitives.Slot)
+			s.wg.Add(1)
+			go func() {
+				s.processQueuedBlocks(ctx, currentSlotChan)
+			}()
+
+			for _, wrap := range tt.wraps {
+				// Add valid signatures to the block headers we are testing.
+				for _, proposalWrapper := range wrap.signedBlkHeaders {
+					proposalWrapper.SignedBeaconBlockHeader.Header.ParentRoot = parentRoot[:]
+					headerHtr, err := proposalWrapper.SignedBeaconBlockHeader.Header.HashTreeRoot()
+					require.NoError(t, err)
+
+					container := &ethpb.SigningData{
+						ObjectRoot: headerHtr[:],
+						Domain:     domain,
+					}
+
+					signingRoot, err := container.HashTreeRoot()
+					require.NoError(t, err)
+
+					privKey := privKeys[proposalWrapper.SignedBeaconBlockHeader.Header.ProposerIndex]
+					proposalWrapper.SignedBeaconBlockHeader.Signature = privKey.Sign(signingRoot[:]).Marshal()
+				}
+
+				s.blksQueue.extend(wrap.signedBlkHeaders)
+
+				currentSlot := primitives.Slot(4)
+				currentSlotChan <- currentSlot
+			}
 
-		cancel()
-		s.wg.Wait()
-		require.LogsContain(t, hook, "Proposer slashing detected")
+			cancel()
+			s.wg.Wait()
+			require.LogsContain(t, hook, "Proposer slashing detected")
 		})
 	}
 }