chore(engine): splitted up NotifyAndVerifyNewPayload (#2495)

Signed-off-by: Alberto Benegiamo <[email protected]> Co-authored-by: Friðrik Ásmundsson <[email protected]>
berachain · Feb 15, 2025 · ac596fd · ac596fd
1 parent 2dc5822
commit ac596fd
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 151 deletions.
diff --git a/cmd/beacond/types.go b/cmd/beacond/types.go
diff --git a/execution/engine/engine.go b/execution/engine/engine.go
@@ -130,11 +130,8 @@ func (ee *Engine) NotifyForkchoiceUpdate(
 	return payloadID, latestValidHash, nil
 }
 
-// VerifyAndNotifyNewPayload verifies the new payload and notifies the
-// execution client.
-//
-//nolint:funlen
-func (ee *Engine) VerifyAndNotifyNewPayload(
+// NotifyNewPayload notifies the execution client of the new payload.
+func (ee *Engine) NotifyNewPayload(
 	ctx context.Context,
 	req *ctypes.NewPayloadRequest,
 ) error {
@@ -145,14 +142,6 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 		req.Optimistic,
 	)
 
-	// First we verify the block hash and versioned hashes are valid.
-	//
-	// TODO: is this required? Or will the EL handle this for us during
-	// new payload?
-	if err := req.HasValidVersionedAndBlockHashes(); err != nil {
-		return err
-	}
-
 	// Otherwise we will send the payload to the execution client.
 	lastValidHash, err := ee.ec.NewPayload(
 		ctx,
@@ -164,11 +153,18 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 	// We abstract away some of the complexity and categorize status codes
 	// to make it easier to reason about.
 	switch {
-	// If we get accepted or syncing, we are going to optimistically
-	// say that the block is valid, this is utilized during syncing
-	// to allow the beacon-chain to continue processing blocks, while
-	// its execution client is fetching things over it's p2p layer.
+	case err == nil:
+		ee.metrics.markNewPayloadValid(
+			req.ExecutionPayload.GetBlockHash(),
+			req.ExecutionPayload.GetParentHash(),
+			req.Optimistic,
+		)
+
 	case errors.Is(err, engineerrors.ErrSyncingPayloadStatus):
+		// If we get accepted or syncing, we are going to optimistically
+		// say that the block is valid, this is utilized during syncing
+		// to allow the beacon-chain to continue processing blocks, while
+		// its execution client is fetching things over it's p2p layer.
 		ee.metrics.markNewPayloadSyncingPayloadStatus(
 			req.ExecutionPayload.GetBlockHash(),
 			req.ExecutionPayload.GetParentHash(),
@@ -206,20 +202,14 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 			req.Optimistic,
 			err,
 		)
-
 		err = errors.Join(err, engineerrors.ErrPreDefinedJSONRPC)
-	case err != nil:
+
+	default:
 		ee.metrics.markNewPayloadUndefinedError(
 			req.ExecutionPayload.GetBlockHash(),
 			req.Optimistic,
 			err,
 		)
-	default:
-		ee.metrics.markNewPayloadValid(
-			req.ExecutionPayload.GetBlockHash(),
-			req.ExecutionPayload.GetParentHash(),
-			req.Optimistic,
-		)
 	}
 
 	// Under the optimistic condition, we are fine ignoring the error. This

diff --git a/node-core/components/chain_service.go b/node-core/components/chain_service.go
@@ -25,7 +25,6 @@ import (
 	"github.com/berachain/beacon-kit/beacon/blockchain"
 	"github.com/berachain/beacon-kit/chain"
 	"github.com/berachain/beacon-kit/config"
-	"github.com/berachain/beacon-kit/execution/client"
 	"github.com/berachain/beacon-kit/execution/deposit"
 	"github.com/berachain/beacon-kit/execution/engine"
 	"github.com/berachain/beacon-kit/log/phuslu"
@@ -41,7 +40,6 @@ type ChainServiceInput struct {
 
 	ChainSpec             chain.Spec
 	Cfg                   *config.Config
-	EngineClient          *client.EngineClient
 	ExecutionEngine       *engine.Engine
 	LocalBuilder          LocalBuilder
 	Logger                *phuslu.Logger

diff --git a/state-transition/core/mocks/execution_engine.mock.go b/state-transition/core/mocks/execution_engine.mock.go
diff --git a/state-transition/core/state_processor_payload.go b/state-transition/core/state_processor_payload.go
@@ -158,14 +158,21 @@ func (sp *StateProcessor) validateStatefulPayload(
 	}
 
 	parentBeaconBlockRoot := blk.GetParentBlockRoot()
-	if err = sp.executionEngine.VerifyAndNotifyNewPayload(
-		ctx, ctypes.BuildNewPayloadRequest(
-			payload,
-			body.GetBlobKzgCommitments().ToVersionedHashes(),
-			&parentBeaconBlockRoot,
-			optimisticEngine,
-		),
-	); err != nil {
+	payloadReq := ctypes.BuildNewPayloadRequest(
+		payload,
+		body.GetBlobKzgCommitments().ToVersionedHashes(),
+		&parentBeaconBlockRoot,
+		optimisticEngine,
+	)
+
+	// First we verify the block hash and versioned hashes are valid.
+	// TODO: is this required? Or will the EL handle this for us during
+	// new payload?
+	if err = payloadReq.HasValidVersionedAndBlockHashes(); err != nil {
+		return err
+	}
+
+	if err = sp.executionEngine.NotifyNewPayload(ctx, payloadReq); err != nil {
 		return err
 	}
 

diff --git a/state-transition/core/types.go b/state-transition/core/types.go
@@ -53,9 +53,8 @@ type Withdrawals interface {
 
 // ExecutionEngine is the interface for the execution engine.
 type ExecutionEngine interface {
-	// VerifyAndNotifyNewPayload verifies the new payload and notifies the
-	// execution client.
-	VerifyAndNotifyNewPayload(
+	// NotifyNewPayload notifies the execution client of the new payload.
+	NotifyNewPayload(
 		ctx context.Context,
 		req *ctypes.NewPayloadRequest,
 	) error