Request block by root: Return early and improve logging.

* Update electra core processing error handling

* Add test for IsExecutionRequestError

* Add TestProcessOperationsWithNilRequests

* gazelle

---------

Co-authored-by: Preston Van Loon <preston@pvl.dev>

beacon-chain/blockchain/BUILD.bazel

@@ -43,6 +43,7 @@ go_library(
         "//beacon-chain/cache:go_default_library",
         "//beacon-chain/core/altair:go_default_library",
         "//beacon-chain/core/blocks:go_default_library",
+        "//beacon-chain/core/electra:go_default_library",
         "//beacon-chain/core/epoch/precompute:go_default_library",
         "//beacon-chain/core/feed:go_default_library",
         "//beacon-chain/core/feed/state:go_default_library",

beacon-chain/blockchain/receive_block.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/pkg/errors"
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/electra"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed"
 	statefeed "github.com/prysmaticlabs/prysm/v5/beacon-chain/core/feed/state"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/helpers"
@@ -468,7 +469,7 @@ func (s *Service) validateStateTransition(ctx context.Context, preState state.Be
 	stateTransitionStartTime := time.Now()
 	postState, err := transition.ExecuteStateTransition(ctx, preState, signed)
 	if err != nil {
-		if ctx.Err() != nil {
+		if ctx.Err() != nil || electra.IsExecutionRequestError(err) {
 			return nil, err
 		}
 		return nil, invalidBlock{error: err}

beacon-chain/core/electra/BUILD.bazel

@@ -8,6 +8,7 @@ go_library(
         "consolidations.go",
         "deposits.go",
         "effective_balance_updates.go",
+        "error.go",
         "registry_updates.go",
         "transition.go",
         "transition_no_verify_sig.go",
@@ -55,13 +56,16 @@ go_test(
         "deposit_fuzz_test.go",
         "deposits_test.go",
         "effective_balance_updates_test.go",
+        "error_test.go",
         "export_test.go",
         "registry_updates_test.go",
+        "transition_no_verify_sig_test.go",
         "transition_test.go",
         "upgrade_test.go",
         "validator_test.go",
         "withdrawals_test.go",
     ],
+    data = glob(["testdata/**"]),
     embed = [":go_default_library"],
     deps = [
         "//beacon-chain/core/helpers:go_default_library",
@@ -86,6 +90,7 @@ go_test(
         "@com_github_ethereum_go_ethereum//common:go_default_library",
         "@com_github_ethereum_go_ethereum//common/hexutil:go_default_library",
         "@com_github_google_gofuzz//:go_default_library",
+        "@com_github_pkg_errors//:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_sirupsen_logrus//hooks/test:go_default_library",
     ],

beacon-chain/core/electra/error.go

@@ -0,0 +1,16 @@
+package electra
+
+import "github.com/pkg/errors"
+
+type execReqErr struct {
+	error
+}
+
+// IsExecutionRequestError returns true if the error has `execReqErr`.
+func IsExecutionRequestError(e error) bool {
+	if e == nil {
+		return false
+	}
+	var d execReqErr
+	return errors.As(e, &d)
+}

beacon-chain/core/electra/error_test.go

@@ -0,0 +1,45 @@
+package electra
+
+import (
+	"testing"
+
+	"github.com/pkg/errors"
+)
+
+func TestIsExecutionRequestError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{
+			name: "nil error",
+			err:  nil,
+			want: false,
+		},
+		{
+			name: "random error",
+			err:  errors.New("some error"),
+			want: false,
+		},
+		{
+			name: "execution request error",
+			err:  execReqErr{errors.New("execution request failed")},
+			want: true,
+		},
+		{
+			name: "wrapped execution request error",
+			err:  errors.Wrap(execReqErr{errors.New("execution request failed")}, "wrapped"),
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := IsExecutionRequestError(tt.err)
+			if got != tt.want {
+				t.Errorf("IsExecutionRequestError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}

beacon-chain/core/electra/transition_no_verify_sig.go

@@ -2,7 +2,6 @@ package electra
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/blocks"
@@ -82,16 +81,31 @@ func ProcessOperations(
 	if err != nil {
 		return nil, errors.Wrap(err, "could not get execution requests")
 	}
+	for _, d := range requests.Deposits {
+		if d == nil {
+			return nil, errors.New("nil deposit request")
+		}
+	}
 	st, err = ProcessDepositRequests(ctx, st, requests.Deposits)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not process deposit requests")
+		return nil, execReqErr{errors.Wrap(err, "could not process deposit requests")}
+	}
+	for _, w := range requests.Withdrawals {
+		if w == nil {
+			return nil, errors.New("nil withdrawal request")
+		}
 	}
 	st, err = ProcessWithdrawalRequests(ctx, st, requests.Withdrawals)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not process withdrawal requests")
+		return nil, execReqErr{errors.Wrap(err, "could not process withdrawal requests")}
+	}
+	for _, c := range requests.Consolidations {
+		if c == nil {
+			return nil, errors.New("nil consolidation request")
+		}
 	}
 	if err := ProcessConsolidationRequests(ctx, st, requests.Consolidations); err != nil {
-		return nil, fmt.Errorf("could not process consolidation requests: %w", err)
+		return nil, execReqErr{errors.Wrap(err, "could not process consolidation requests")}
 	}
 	return st, nil
 }

beacon-chain/core/electra/transition_no_verify_sig_test.go

@@ -0,0 +1,61 @@
+package electra_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/core/electra"
+	"github.com/prysmaticlabs/prysm/v5/consensus-types/blocks"
+	enginev1 "github.com/prysmaticlabs/prysm/v5/proto/engine/v1"
+	ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
+	"github.com/prysmaticlabs/prysm/v5/testing/require"
+	"github.com/prysmaticlabs/prysm/v5/testing/util"
+)
+
+func TestProcessOperationsWithNilRequests(t *testing.T) {
+	tests := []struct {
+		name      string
+		modifyBlk func(blockElectra *ethpb.SignedBeaconBlockElectra)
+		errMsg    string
+	}{
+		{
+			name: "Nil deposit request",
+			modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) {
+				blk.Block.Body.ExecutionRequests.Deposits = []*enginev1.DepositRequest{nil}
+			},
+			errMsg: "nil deposit request",
+		},
+		{
+			name: "Nil withdrawal request",
+			modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) {
+				blk.Block.Body.ExecutionRequests.Withdrawals = []*enginev1.WithdrawalRequest{nil}
+			},
+			errMsg: "nil withdrawal request",
+		},
+		{
+			name: "Nil consolidation request",
+			modifyBlk: func(blk *ethpb.SignedBeaconBlockElectra) {
+				blk.Block.Body.ExecutionRequests.Consolidations = []*enginev1.ConsolidationRequest{nil}
+			},
+			errMsg: "nil consolidation request",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			st, ks := util.DeterministicGenesisStateElectra(t, 128)
+			blk, err := util.GenerateFullBlockElectra(st, ks, util.DefaultBlockGenConfig(), 1)
+			require.NoError(t, err)
+
+			tc.modifyBlk(blk)
+
+			b, err := blocks.NewSignedBeaconBlock(blk)
+			require.NoError(t, err)
+
+			require.NoError(t, st.SetSlot(1))
+
+			_, err = electra.ProcessOperations(context.Background(), st, b.Block())
+			require.ErrorContains(t, tc.errMsg, err)
+		})
+	}
+}

beacon-chain/sync/pending_attestations_queue_test.go

@@ -53,7 +53,7 @@ func TestProcessPendingAtts_NoBlockRequestBlock(t *testing.T) {
 	a := &ethpb.AggregateAttestationAndProof{Aggregate: &ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Root: make([]byte, 32)}}}}
 	r.blkRootToPendingAtts[[32]byte{'A'}] = []ethpb.SignedAggregateAttAndProof{&ethpb.SignedAggregateAttestationAndProof{Message: a}}
 	require.NoError(t, r.processPendingAtts(context.Background()))
-	require.LogsContain(t, hook, "Requesting block by root")
+	require.LogsContain(t, hook, "Requesting block by root: Not found")
 }
 
 func TestProcessPendingAtts_HasBlockSaveUnAggregatedAtt(t *testing.T) {

beacon-chain/sync/pending_blocks_queue.go

@@ -292,8 +292,6 @@ func (s *Service) sendBatchRootRequest(ctx context.Context, roots [][32]byte, ra
 		r := roots[i]
 		if s.seenPendingBlocks[r] || s.cfg.chain.BlockBeingSynced(r) {
 			roots = append(roots[:i], roots[i+1:]...)
-		} else {
-			log.WithField("blockRoot", fmt.Sprintf("%#x", r)).Debug("Requesting block by root")
 		}
 	}
 	s.pendingQueueLock.RUnlock()

beacon-chain/sync/rpc_beacon_blocks_by_root.go

@@ -11,6 +11,7 @@ import (
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/p2p/types"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/sync/verify"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/verification"
+	fieldparams "github.com/prysmaticlabs/prysm/v5/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/v5/config/params"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/blocks"
 	"github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces"
@@ -46,11 +47,40 @@ func (s *Service) sendBeaconBlocksRequest(ctx context.Context, requests *types.B
 		}
 		return nil
 	})
+
+	log := log.WithField("peer", id)
+
+	blkByRoot := make(map[[fieldparams.RootLength]byte]interfaces.ReadOnlySignedBeaconBlock)
 	for _, blk := range blks {
+		blkRoot, err := blk.Block().HashTreeRoot()
+		if err != nil {
+			return errors.Wrap(err, "block hash tree root")
+		}
+
+		blkByRoot[blkRoot] = blk
+	}
+
+	if err != nil {
+		log = log.WithError(err)
+	}
+
+	for requestedRoot := range requestedRoots {
+		log := log.WithField("root", fmt.Sprintf("%#x", requestedRoot))
+		if _, ok := blkByRoot[requestedRoot]; ok {
+			log.Debug("Requesting block by root: Success")
+			continue
+		}
+
+		log.Debug("Requesting block by root: Not found")
+	}
+
+	// The following part deals with blobs (if any).
+	for _, blk := range blkByRoot {
 		// Skip blocks before deneb because they have no blob.
 		if blk.Version() < version.Deneb {
 			continue
 		}
+
 		blkRoot, err := blk.Block().HashTreeRoot()
 		if err != nil {
 			return err
@@ -66,6 +96,7 @@ func (s *Service) sendBeaconBlocksRequest(ctx context.Context, requests *types.B
 			return err
 		}
 	}
+
 	return err
 }
 

changelog/kasey_warn-hist-state-rep.md

@@ -0,0 +1,2 @@
+### Added
+- WARN log message on node startup advising of the upcoming deprecation of the --enable-historical-state-representation feature flag.

changelog/manu_blob_request_log.md

@@ -0,0 +1,4 @@
+### Changed
+
+- `sendBatchRootRequest`: Remove `Requesting block by root` log.
+- `sendBeaconBlocksRequest`: Add `Requesting block by root: {Success,Not found}` log with peer ID.

changelog/tt_fix_electra_core_processing.md

@@ -0,0 +1,3 @@
+### Changed
+
+- Update electra core processing to not mark block bad if execution request error.

config/features/config.go

@@ -166,6 +166,7 @@ func applyHoleskyFeatureFlags(ctx *cli.Context) {
 // ConfigureBeaconChain sets the global config based
 // on what flags are enabled for the beacon-chain client.
 func ConfigureBeaconChain(ctx *cli.Context) error {
+	warnDeprecationUpcoming(ctx)
 	complainOnDeprecatedFlags(ctx)
 	cfg := &Flags{}
 	if ctx.Bool(devModeFlag.Name) {
@@ -336,6 +337,22 @@ func complainOnDeprecatedFlags(ctx *cli.Context) {
 	}
 }
 
+var upcomingDeprecationExtra = map[string]string{
+	enableHistoricalSpaceRepresentation.Name: "The node needs to be resynced after flag removal.",
+}
+
+func warnDeprecationUpcoming(ctx *cli.Context) {
+	for _, f := range upcomingDeprecation {
+		if ctx.IsSet(f.Names()[0]) {
+			extra := "Please remove this flag from your configuration."
+			if special, ok := upcomingDeprecationExtra[f.Names()[0]]; ok {
+				extra += " " + special
+			}
+			log.Warnf("--%s is pending deprecation and will be removed in the next release. %s", f.Names()[0], extra)
+		}
+	}
+}
+
 func logEnabled(flag cli.DocGenerationFlag) {
 	var name string
 	if names := flag.Names(); len(names) > 0 {

config/features/deprecated_flags.go

@@ -120,6 +120,10 @@ var deprecatedFlags = []cli.Flag{
 	deprecatedEnableQuic,
 }
 
+var upcomingDeprecation = []cli.Flag{
+	enableHistoricalSpaceRepresentation,
+}
+
 // deprecatedBeaconFlags contains flags that are still used by other components
 // and therefore cannot be added to deprecatedFlags
 var deprecatedBeaconFlags = []cli.Flag{

config/features/flags.go

@@ -205,7 +205,7 @@ var E2EValidatorFlags = []string{
 }
 
 // BeaconChainFlags contains a list of all the feature flags that apply to the beacon-chain client.
-var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []cli.Flag{
+var BeaconChainFlags = combinedFlags([]cli.Flag{
 	devModeFlag,
 	disableExperimentalState,
 	writeSSZStateTransitionsFlag,
@@ -218,7 +218,6 @@ var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []c
 	disablePeerScorer,
 	disableBroadcastSlashingFlag,
 	enableSlasherFlag,
-	enableHistoricalSpaceRepresentation,
 	disableStakinContractCheck,
 	SaveFullExecutionPayloads,
 	enableStartupOptimistic,
@@ -236,7 +235,18 @@ var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []c
 	DisableCommitteeAwarePacking,
 	EnableDiscoveryReboot,
 	enableExperimentalAttestationPool,
-}...)...)
+}, deprecatedBeaconFlags, deprecatedFlags, upcomingDeprecation)
+
+func combinedFlags(flags ...[]cli.Flag) []cli.Flag {
+	if len(flags) == 0 {
+		return []cli.Flag{}
+	}
+	collected := flags[0]
+	for _, f := range flags[1:] {
+		collected = append(collected, f...)
+	}
+	return collected
+}
 
 // E2EBeaconChainFlags contains a list of the beacon chain feature flags to be tested in E2E.
 var E2EBeaconChainFlags = []string{