More selective on marking block as bad (#10681)

* Starting, looking for feedbacks

* Update error.go

* Wrap invalid blocks to rest of the processings

* More tests

* More tests

* Fix tests

* Update process_block_test.go

* Update execution_engine_test.go

* Update BUILD.bazel

* Nishant's feedback and Kasey's recommendation

* Add comments on what an invalid block is

* Update beacon-chain/blockchain/error.go

Co-authored-by: Potuz <potuz@prysmaticlabs.com>

* Update beacon-chain/blockchain/error.go

Co-authored-by: Potuz <potuz@prysmaticlabs.com>

* Rm faulty invalid conditions

Co-authored-by: Potuz <potuz@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/blockchain/error.go

@@ -17,3 +17,35 @@ var (
 	// block is not a valid optimistic candidate block
 	errNotOptimisticCandidate = errors.New("block is not suitable for optimistic sync")
 )
+
+// An invalid block is the block that fails state transition based on the core protocol rules.
+// The beacon node shall not be accepting nor building blocks that branch off from an invalid block.
+// Some examples of invalid blocks are:
+// The block violates state transition rules.
+// The block is deemed invalid according to execution layer client.
+// The block violates certain fork choice rules (before finalized slot, not finalized ancestor)
+type invalidBlock struct {
+	error
+}
+
+type invalidBlockError interface {
+	Error() string
+	InvalidBlock() bool
+}
+
+// InvalidBlock returns true for `invalidBlock`.
+func (e invalidBlock) InvalidBlock() bool {
+	return true
+}
+
+// IsInvalidBlock returns true if the error has `invalidBlock`.
+func IsInvalidBlock(e error) bool {
+	if e == nil {
+		return false
+	}
+	d, ok := e.(invalidBlockError)
+	if !ok {
+		return IsInvalidBlock(errors.Unwrap(e))
+	}
+	return d.InvalidBlock()
+}

beacon-chain/blockchain/error_test.go

@@ -0,0 +1,17 @@
+package blockchain
+
+import (
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/prysmaticlabs/prysm/testing/require"
+)
+
+func TestIsInvalidBlock(t *testing.T) {
+	require.Equal(t, false, IsInvalidBlock(ErrInvalidPayload))
+	err := invalidBlock{ErrInvalidPayload}
+	require.Equal(t, true, IsInvalidBlock(err))
+
+	newErr := errors.Wrap(err, "wrap me")
+	require.Equal(t, true, IsInvalidBlock(newErr))
+}

beacon-chain/blockchain/execution_engine.go

@@ -174,14 +174,14 @@ func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
 	body := blk.Block().Body()
 	enabled, err := blocks.IsExecutionEnabledUsingHeader(postStateHeader, body)
 	if err != nil {
-		return false, errors.Wrap(err, "could not determine if execution is enabled")
+		return false, errors.Wrap(invalidBlock{err}, "could not determine if execution is enabled")
 	}
 	if !enabled {
 		return true, nil
 	}
 	payload, err := body.ExecutionPayload()
 	if err != nil {
-		return false, errors.Wrap(err, "could not get execution payload")
+		return false, errors.Wrap(invalidBlock{err}, "could not get execution payload")
 	}
 	lastValidHash, err := s.cfg.ExecutionEngineCaller.NewPayload(ctx, payload)
 	switch err {
@@ -213,7 +213,7 @@ func (s *Service) notifyNewPayload(ctx context.Context, postStateVersion int,
 			"blockRoot":    fmt.Sprintf("%#x", root),
 			"invalidCount": len(invalidRoots),
 		}).Warn("Pruned invalid blocks")
-		return false, ErrInvalidPayload
+		return false, invalidBlock{ErrInvalidPayload}
 	default:
 		return false, errors.WithMessage(ErrUndefinedExecutionEngineError, err.Error())
 	}

beacon-chain/blockchain/execution_engine_test.go

@@ -387,12 +387,13 @@ func Test_NotifyNewPayload(t *testing.T) {
 	require.NoError(t, fcs.InsertOptimisticBlock(ctx, 1, r, [32]byte{}, params.BeaconConfig().ZeroHash, 0, 0))
 
 	tests := []struct {
-		name           string
 		postState      state.BeaconState
+		invalidBlock   bool
 		isValidPayload bool
 		blk            interfaces.SignedBeaconBlock
 		newPayloadErr  error
 		errString      string
+		name           string
 	}{
 		{
 			name:           "phase 0 post state",
@@ -424,6 +425,7 @@ func Test_NotifyNewPayload(t *testing.T) {
 			newPayloadErr:  powchain.ErrInvalidPayloadStatus,
 			errString:      ErrInvalidPayload.Error(),
 			isValidPayload: false,
+			invalidBlock:   true,
 		},
 		{
 			name:           "altair pre state, altair block",
@@ -535,9 +537,13 @@ func Test_NotifyNewPayload(t *testing.T) {
 			isValidPayload, err := service.notifyNewPayload(ctx, postVersion, postHeader, tt.blk)
 			if tt.errString != "" {
 				require.ErrorContains(t, tt.errString, err)
+				if tt.invalidBlock {
+					require.Equal(t, true, IsInvalidBlock(err))
+				}
 			} else {
 				require.NoError(t, err)
 				require.Equal(t, tt.isValidPayload, isValidPayload)
+				require.Equal(t, false, IsInvalidBlock(err))
 			}
 		})
 	}
@@ -690,7 +696,11 @@ func Test_IsOptimisticCandidateBlock(t *testing.T) {
 		require.NoError(t, service.cfg.BeaconDB.SaveBlock(ctx, wrappedParentBlock))
 
 		err = service.optimisticCandidateBlock(ctx, tt.blk)
-		require.Equal(t, tt.err, err)
+		if tt.err != nil {
+			require.Equal(t, tt.err.Error(), err.Error())
+		} else {
+			require.NoError(t, err)
+		}
 	}
 }
 

beacon-chain/blockchain/pow_block.go

@@ -64,8 +64,9 @@ func (s *Service) validateMergeBlock(ctx context.Context, b interfaces.SignedBea
 		return err
 	}
 	if !valid {
-		return fmt.Errorf("invalid TTD, configTTD: %s, currentTTD: %s, parentTTD: %s",
+		err := fmt.Errorf("invalid TTD, configTTD: %s, currentTTD: %s, parentTTD: %s",
 			params.BeaconConfig().TerminalTotalDifficulty, mergeBlockTD, mergeBlockParentTD)
+		return invalidBlock{err}
 	}
 
 	log.WithFields(logrus.Fields{

beacon-chain/blockchain/pow_block_test.go

@@ -144,7 +144,9 @@ func Test_validateMergeBlock(t *testing.T) {
 
 	cfg.TerminalTotalDifficulty = "1"
 	params.OverrideBeaconConfig(cfg)
-	require.ErrorContains(t, "invalid TTD, configTTD: 1, currentTTD: 2, parentTTD: 1", service.validateMergeBlock(ctx, b))
+	err = service.validateMergeBlock(ctx, b)
+	require.ErrorContains(t, "invalid TTD, configTTD: 1, currentTTD: 2, parentTTD: 1", err)
+	require.Equal(t, true, IsInvalidBlock(err))
 }
 
 func Test_getBlkParentHashAndTD(t *testing.T) {

beacon-chain/blockchain/process_block.go

@@ -93,7 +93,7 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
 	ctx, span := trace.StartSpan(ctx, "blockChain.onBlock")
 	defer span.End()
 	if err := helpers.BeaconBlockIsNil(signed); err != nil {
-		return err
+		return invalidBlock{err}
 	}
 	b := signed.Block()
 
@@ -108,7 +108,7 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.SignedBeaconBlo
 	}
 	postState, err := transition.ExecuteStateTransition(ctx, preState, signed)
 	if err != nil {
-		return err
+		return invalidBlock{err}
 	}
 	postStateVersion, postStateHeader, err := getStateVersionAndPayload(postState)
 	if err != nil {
@@ -328,7 +328,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
 	}
 
 	if err := helpers.BeaconBlockIsNil(blks[0]); err != nil {
-		return nil, nil, err
+		return nil, nil, invalidBlock{err}
 	}
 	b := blks[0].Block()
 
@@ -371,7 +371,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
 
 		set, preState, err = transition.ExecuteStateTransitionNoVerifyAnySig(ctx, preState, b)
 		if err != nil {
-			return nil, nil, err
+			return nil, nil, invalidBlock{err}
 		}
 		// Save potential boundary states.
 		if slots.IsEpochStart(preState.Slot()) {
@@ -392,7 +392,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []interfaces.SignedBeac
 	}
 	verify, err := sigSet.Verify()
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, invalidBlock{err}
 	}
 	if !verify {
 		return nil, nil, errors.New("batch block signature verification failed")
@@ -664,7 +664,7 @@ func (s *Service) validateMergeTransitionBlock(ctx context.Context, stateVersion
 	// Skip validation if block has an empty payload.
 	payload, err := blk.Block().Body().ExecutionPayload()
 	if err != nil {
-		return err
+		return invalidBlock{err}
 	}
 	if blocks.IsEmptyPayload(payload) {
 		return nil

beacon-chain/blockchain/process_block_helpers.go

@@ -115,7 +115,7 @@ func (s *Service) VerifyFinalizedBlkDescendant(ctx context.Context, root [32]byt
 			bytesutil.Trunc(root[:]), finalizedBlk.Slot(), bytesutil.Trunc(bFinalizedRoot),
 			bytesutil.Trunc(fRoot[:]))
 		tracing.AnnotateError(span, err)
-		return err
+		return invalidBlock{err}
 	}
 	return nil
 }
@@ -132,7 +132,8 @@ func (s *Service) verifyBlkFinalizedSlot(b interfaces.BeaconBlock) error {
 		return err
 	}
 	if finalizedSlot >= b.Slot() {
-		return fmt.Errorf("block is equal or earlier than finalized block, slot %d < slot %d", b.Slot(), finalizedSlot)
+		err = fmt.Errorf("block is equal or earlier than finalized block, slot %d < slot %d", b.Slot(), finalizedSlot)
+		return invalidBlock{err}
 	}
 	return nil
 }

beacon-chain/blockchain/process_block_test.go

@@ -1318,9 +1318,10 @@ func TestVerifyBlkDescendant(t *testing.T) {
 		finalizedRoot [32]byte
 	}
 	tests := []struct {
-		name      string
-		args      args
-		wantedErr string
+		name             string
+		args             args
+		wantedErr        string
+		invalidBlockRoot bool
 	}{
 		{
 			name: "could not get finalized block in block service cache",
@@ -1343,7 +1344,8 @@ func TestVerifyBlkDescendant(t *testing.T) {
 				finalizedRoot: r1,
 				parentRoot:    r,
 			},
-			wantedErr: "is not a descendant of the current finalized block slot",
+			wantedErr:        "is not a descendant of the current finalized block slot",
+			invalidBlockRoot: true,
 		},
 		{
 			name: "is descendant",
@@ -1360,6 +1362,9 @@ func TestVerifyBlkDescendant(t *testing.T) {
 		err = service.VerifyFinalizedBlkDescendant(ctx, tt.args.parentRoot)
 		if tt.wantedErr != "" {
 			assert.ErrorContains(t, tt.wantedErr, err)
+			if tt.invalidBlockRoot {
+				require.Equal(t, true, IsInvalidBlock(err))
+			}
 		} else if err != nil {
 			assert.NoError(t, err)
 		}
@@ -1471,6 +1476,60 @@ func TestOnBlock_CanFinalize(t *testing.T) {
 	require.Equal(t, f.Epoch, service.FinalizedCheckpt().Epoch)
 }
 
+func TestOnBlock_NilBlock(t *testing.T) {
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+	fcs := protoarray.New(0, 0, [32]byte{'a'})
+	depositCache, err := depositcache.New()
+	require.NoError(t, err)
+	opts := []Option{
+		WithDatabase(beaconDB),
+		WithStateGen(stategen.New(beaconDB)),
+		WithForkChoiceStore(fcs),
+		WithDepositCache(depositCache),
+	}
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+
+	err = service.onBlock(ctx, nil, [32]byte{})
+	require.Equal(t, true, IsInvalidBlock(err))
+}
+
+func TestOnBlock_InvalidSignature(t *testing.T) {
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+	fcs := protoarray.New(0, 0, [32]byte{'a'})
+	depositCache, err := depositcache.New()
+	require.NoError(t, err)
+	opts := []Option{
+		WithDatabase(beaconDB),
+		WithStateGen(stategen.New(beaconDB)),
+		WithForkChoiceStore(fcs),
+		WithDepositCache(depositCache),
+		WithStateNotifier(&mock.MockStateNotifier{}),
+	}
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+
+	gs, keys := util.DeterministicGenesisState(t, 32)
+	require.NoError(t, service.saveGenesisData(ctx, gs))
+	gBlk, err := service.cfg.BeaconDB.GenesisBlock(ctx)
+	require.NoError(t, err)
+	gRoot, err := gBlk.Block().HashTreeRoot()
+	require.NoError(t, err)
+	service.store.SetFinalizedCheckptAndPayloadHash(&ethpb.Checkpoint{Root: gRoot[:]}, [32]byte{})
+
+	blk, err := util.GenerateFullBlock(gs, keys, util.DefaultBlockGenConfig(), 1)
+	require.NoError(t, err)
+	blk.Signature = []byte{'a'} // Mutate the signature.
+	r, err := blk.Block.HashTreeRoot()
+	require.NoError(t, err)
+	wsb, err := wrapper.WrappedSignedBeaconBlock(blk)
+	require.NoError(t, err)
+	err = service.onBlock(ctx, wsb, r)
+	require.Equal(t, true, IsInvalidBlock(err))
+}
+
 func TestOnBlock_CallNewPayloadAndForkchoiceUpdated(t *testing.T) {
 	params.SetupTestConfigCleanup(t)
 	config := params.BeaconConfig()
@@ -1972,3 +2031,23 @@ func TestOnBlock_ProcessBlocksParallel(t *testing.T) {
 		service.cfg.ForkChoiceStore = protoarray.New(0, 0, [32]byte{'a'})
 	}
 }
+
+func Test_verifyBlkFinalizedSlot_invalidBlock(t *testing.T) {
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+
+	fcs := protoarray.New(0, 0, [32]byte{'a'})
+	opts := []Option{
+		WithDatabase(beaconDB),
+		WithStateGen(stategen.New(beaconDB)),
+		WithForkChoiceStore(fcs),
+	}
+	service, err := NewService(ctx, opts...)
+	require.NoError(t, err)
+	service.store.SetFinalizedCheckptAndPayloadHash(&ethpb.Checkpoint{Epoch: 1}, [32]byte{'a'})
+	blk := util.HydrateBeaconBlock(&ethpb.BeaconBlock{Slot: 1})
+	wb, err := wrapper.WrappedBeaconBlock(blk)
+	require.NoError(t, err)
+	err = service.verifyBlkFinalizedSlot(wb)
+	require.Equal(t, true, IsInvalidBlock(err))
+}

beacon-chain/sync/BUILD.bazel

@@ -76,7 +76,6 @@ go_library(
         "//beacon-chain/p2p/encoder:go_default_library",
         "//beacon-chain/p2p/peers:go_default_library",
         "//beacon-chain/p2p/types:go_default_library",
-        "//beacon-chain/powchain:go_default_library",
         "//beacon-chain/state:go_default_library",
         "//beacon-chain/state/stategen:go_default_library",
         "//cache/lru:go_default_library",

beacon-chain/sync/pending_blocks_queue.go

@@ -12,7 +12,6 @@ import (
 	"github.com/prysmaticlabs/prysm/beacon-chain/blockchain"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
 	p2ptypes "github.com/prysmaticlabs/prysm/beacon-chain/p2p/types"
-	"github.com/prysmaticlabs/prysm/beacon-chain/powchain"
 	"github.com/prysmaticlabs/prysm/config/params"
 	"github.com/prysmaticlabs/prysm/consensus-types/interfaces"
 	types "github.com/prysmaticlabs/prysm/consensus-types/primitives"
@@ -156,10 +155,6 @@ func (s *Service) processPendingBlocks(ctx context.Context) error {
 			err = s.validateBeaconBlock(ctx, b, blkRoot)
 			switch {
 			case errors.Is(ErrOptimisticParent, err): // Ok to continue process block with parent that is an optimistic candidate.
-			case errors.Is(blockchain.ErrUndefinedExecutionEngineError, err):
-				// don't mark the block as bad with an undefined EE error.
-				log.Debugf("Could not validate block due to undefined ee error %d: %v", b.Block().Slot(), err)
-				continue
 			case err != nil:
 				log.Debugf("Could not validate block from slot %d: %v", b.Block().Slot(), err)
 				s.setBadBlock(ctx, blkRoot)
@@ -170,11 +165,12 @@ func (s *Service) processPendingBlocks(ctx context.Context) error {
 			}
 
 			if err := s.cfg.chain.ReceiveBlock(ctx, b, blkRoot); err != nil {
-				if !errors.Is(err, powchain.ErrHTTPTimeout) {
-					log.Debugf("Could not process block from slot %d: %v", b.Block().Slot(), err)
+				if blockchain.IsInvalidBlock(err) {
 					tracing.AnnotateError(span, err)
 					s.setBadBlock(ctx, blkRoot)
 				}
+				log.Debugf("Could not process block from slot %d: %v", b.Block().Slot(), err)
+
 				// In the next iteration of the queue, this block will be removed from
 				// the pending queue as it has been marked as a 'bad' block.
 				span.End()

beacon-chain/sync/subscriber_beacon_blocks.go

@@ -3,11 +3,9 @@ package sync
 import (
 	"context"
 
-	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/beacon-chain/blockchain"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/transition/interop"
-	"github.com/prysmaticlabs/prysm/beacon-chain/powchain"
 	"github.com/prysmaticlabs/prysm/config/features"
 	"github.com/prysmaticlabs/prysm/consensus-types/wrapper"
 	ethpb "github.com/prysmaticlabs/prysm/proto/prysm/v1alpha1"
@@ -33,7 +31,7 @@ func (s *Service) beaconBlockSubscriber(ctx context.Context, msg proto.Message)
 	}
 
 	if err := s.cfg.chain.ReceiveBlock(ctx, signed, root); err != nil {
-		if !errors.Is(err, powchain.ErrHTTPTimeout) && !errors.Is(err, blockchain.ErrUndefinedExecutionEngineError) {
+		if blockchain.IsInvalidBlock(err) {
 			interop.WriteBlockToDisk(signed, true /*failed*/)
 			s.setBadBlock(ctx, root)
 		}

beacon-chain/sync/validate_beacon_blocks.go

@@ -8,7 +8,6 @@ import (
 	"github.com/libp2p/go-libp2p-core/peer"
 	pubsub "github.com/libp2p/go-libp2p-pubsub"
 	"github.com/pkg/errors"
-	"github.com/prysmaticlabs/prysm/beacon-chain/blockchain"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/feed"
 	blockfeed "github.com/prysmaticlabs/prysm/beacon-chain/core/feed/block"
@@ -241,7 +240,7 @@ func (s *Service) validateBeaconBlock(ctx context.Context, blk interfaces.Signed
 	}
 
 	if err = s.validateBellatrixBeaconBlock(ctx, parentState, blk.Block()); err != nil {
-		if errors.Is(err, ErrOptimisticParent) || errors.Is(blockchain.ErrUndefinedExecutionEngineError, err) {
+		if errors.Is(err, ErrOptimisticParent) {
 			return err
 		}
 		// for other kinds of errors, set this block as a bad block.