Revert "fix(engine api): [1/3] error on engine API errors (#2485)"

This reverts commit d708c71.

beacon/blockchain/execution_engine.go

@@ -22,7 +22,6 @@ package blockchain
 
 import (
 	"context"
-	"fmt"
 
 	ctypes "github.com/berachain/beacon-kit/consensus-types/types"
 	contypes "github.com/berachain/beacon-kit/consensus/types"
@@ -39,32 +38,35 @@ func (s *Service) sendPostBlockFCU(
 	ctx context.Context,
 	st *statedb.StateDB,
 	blk *contypes.ConsensusBlock,
-) error {
+) {
 	lph, err := st.GetLatestExecutionPayloadHeader()
 	if err != nil {
-		return fmt.Errorf("failed getting latest payload: %w", err)
+		s.logger.Error(
+			"failed to get latest execution payload in postBlockProcess",
+			"error", err,
+		)
+		return
 	}
 
 	// Send a forkchoice update without payload attributes to notify
 	// EL of the new head.
 	beaconBlk := blk.GetBeaconBlock()
-	_, _, err = s.executionEngine.NotifyForkchoiceUpdate(
+	if _, _, err = s.executionEngine.NotifyForkchoiceUpdate(
 		ctx,
 		// TODO: Switch to New().
-		ctypes.BuildForkchoiceUpdateRequestNoAttrs(
-			&engineprimitives.ForkchoiceStateV1{
-				HeadBlockHash:      lph.GetBlockHash(),
-				SafeBlockHash:      lph.GetParentHash(),
-				FinalizedBlockHash: lph.GetParentHash(),
-			},
-			s.chainSpec.ActiveForkVersionForSlot(beaconBlk.GetSlot()),
-		),
-	)
-	if err != nil {
-		return fmt.Errorf("failed forkchoice update, head %s: %w",
-			lph.GetBlockHash().String(),
-			err,
+		ctypes.
+			BuildForkchoiceUpdateRequestNoAttrs(
+				&engineprimitives.ForkchoiceStateV1{
+					HeadBlockHash:      lph.GetBlockHash(),
+					SafeBlockHash:      lph.GetParentHash(),
+					FinalizedBlockHash: lph.GetParentHash(),
+				},
+				s.chainSpec.ActiveForkVersionForSlot(beaconBlk.GetSlot()),
+			),
+	); err != nil {
+		s.logger.Error(
+			"failed to send forkchoice update without attributes",
+			"error", err,
 		)
 	}
-	return nil
 }

beacon/blockchain/finalize_block.go

@@ -108,9 +108,7 @@ func (s *Service) FinalizeBlock(
 		s.logger.Error("failed to processPruning", "error", err)
 	}
 
-	if err = s.sendPostBlockFCU(ctx, st, consensusBlk); err != nil {
-		return nil, fmt.Errorf("sendPostBlockFCU failed: %w", err)
-	}
+	go s.sendPostBlockFCU(ctx, st, consensusBlk)
 
 	return valUpdates, nil
 }
@@ -155,6 +153,11 @@ func (s *Service) executeStateTransition(
 	defer s.metrics.measureStateTransitionDuration(startTime)
 
 	// Notes about context attributes:
+	// - `OptimisticEngine`: set to true since this is called during
+	// FinalizeBlock. We want to assume the payload is valid. If it
+	// ends up not being valid later, the node will simply AppHash,
+	// which is completely fine. This means we were syncing from a
+	// bad peer, and we would likely AppHash anyways.
 	// - VerifyPayload: set to true. When we are NOT synced to the tip,
 	// process proposal does NOT get called and thus we must ensure that
 	// NewPayload is called to get the execution client the payload.
@@ -165,10 +168,10 @@ func (s *Service) executeStateTransition(
 	// of validators in their process proposal call and thus
 	// the "verification aspect" of this NewPayload call is
 	// actually irrelevant at this point.
-	// - VerifyRandao: set to false. We skip randao validation in FinalizeBlock
+	// VerifyRandao: set to false. We skip randao validation in FinalizeBlock
 	// since either
-	//   1. we validated it during ProcessProposal at the head of the chain OR
-	//   2. we are bootstrapping and implicitly trust that the randao was validated by
+	// 1. we validated it during ProcessProposal at the head of the chain OR
+	// 2. we are bootstrapping and implicitly trust that the randao was validated by
 	//    the super majority during ProcessProposal of the given block height.
 	txCtx := transition.NewTransitionCtx(
 		ctx,
@@ -178,7 +181,8 @@ func (s *Service) executeStateTransition(
 		WithVerifyPayload(true).
 		WithVerifyRandao(false).
 		WithVerifyResult(false).
-		WithMeterGas(true)
+		WithMeterGas(true).
+		WithOptimisticEngine(true)
 
 	return s.stateProcessor.Transition(
 		txCtx,

beacon/blockchain/process_proposal.go

@@ -332,6 +332,8 @@ func (s *Service) verifyStateRoot(
 	startTime := time.Now()
 	defer s.metrics.measureStateRootVerificationTime(startTime)
 
+	// We run with a non-optimistic engine here to ensure
+	// that the proposer does not try to push through a bad block.
 	txCtx := transition.NewTransitionCtx(
 		ctx,
 		consensusTime,
@@ -340,7 +342,8 @@ func (s *Service) verifyStateRoot(
 		WithVerifyPayload(true).
 		WithVerifyRandao(true).
 		WithVerifyResult(true).
-		WithMeterGas(false)
+		WithMeterGas(false).
+		WithOptimisticEngine(false)
 
 	_, err := s.stateProcessor.Transition(txCtx, st, blk)
 	return err

beacon/validator/block_builder.go

@@ -370,7 +370,8 @@ func (s *Service) computeStateRoot(
 	startTime := time.Now()
 	defer s.metrics.measureStateRootComputationTime(startTime)
 
-	// TODO: Think about how this would affect the proposer when
+	// TODO: We should think about how having optimistic
+	// engine enabled here would affect the proposer when
 	// the payload in their block has come from a remote builder.
 	txCtx := transition.NewTransitionCtx(
 		ctx,
@@ -380,7 +381,8 @@ func (s *Service) computeStateRoot(
 		WithVerifyPayload(false).
 		WithVerifyRandao(false).
 		WithVerifyResult(false).
-		WithMeterGas(false)
+		WithMeterGas(false).
+		WithOptimisticEngine(true)
 
 	if _, err := s.stateProcessor.Transition(txCtx, st, blk); err != nil {
 		return common.Root{}, err

consensus-types/types/payload_requests.go

@@ -39,18 +39,23 @@ type NewPayloadRequest struct {
 	VersionedHashes []common.ExecutionHash
 	// ParentBeaconBlockRoot is the root of the parent beacon block.
 	ParentBeaconBlockRoot *common.Root
+	// Optimistic is a flag that indicates if the payload should be
+	// optimistically deemed valid. This is useful during syncing.
+	Optimistic bool
 }
 
 // BuildNewPayloadRequest builds a new payload request.
 func BuildNewPayloadRequest(
 	executionPayload *ExecutionPayload,
 	versionedHashes []common.ExecutionHash,
 	parentBeaconBlockRoot *common.Root,
+	optimistic bool,
 ) *NewPayloadRequest {
 	return &NewPayloadRequest{
 		ExecutionPayload:      executionPayload,
 		VersionedHashes:       versionedHashes,
 		ParentBeaconBlockRoot: parentBeaconBlockRoot,
+		Optimistic:            optimistic,
 	}
 }
 

consensus-types/types/payload_requests_test.go

@@ -37,18 +37,21 @@ func TestBuildNewPayloadRequest(t *testing.T) {
 		executionPayload      = (&types.ExecutionPayload{}).Empty(version.Deneb1())
 		versionedHashes       []common.ExecutionHash
 		parentBeaconBlockRoot = common.Root{}
+		optimistic            = false
 	)
 
 	request := types.BuildNewPayloadRequest(
 		executionPayload,
 		versionedHashes,
 		&parentBeaconBlockRoot,
+		optimistic,
 	)
 
 	require.NotNil(t, request)
 	require.Equal(t, executionPayload, request.ExecutionPayload)
 	require.Equal(t, versionedHashes, request.VersionedHashes)
 	require.Equal(t, &parentBeaconBlockRoot, request.ParentBeaconBlockRoot)
+	require.Equal(t, optimistic, request.Optimistic)
 }
 
 func TestBuildForkchoiceUpdateRequest(t *testing.T) {
@@ -97,12 +100,14 @@ func TestHasValidVersionedAndBlockHashesPayloadError(t *testing.T) {
 		executionPayload      = (&types.ExecutionPayload{}).Empty(version.Deneb1())
 		versionedHashes       = []common.ExecutionHash{}
 		parentBeaconBlockRoot = common.Root{}
+		optimistic            = false
 	)
 
 	request := types.BuildNewPayloadRequest(
 		executionPayload,
 		versionedHashes,
 		&parentBeaconBlockRoot,
+		optimistic,
 	)
 
 	err := request.HasValidVersionedAndBlockHashes()
@@ -115,12 +120,14 @@ func TestHasValidVersionedAndBlockHashesMismatchedHashes(t *testing.T) {
 		executionPayload      = (&types.ExecutionPayload{}).Empty(version.Deneb1())
 		versionedHashes       = []common.ExecutionHash{{}}
 		parentBeaconBlockRoot = common.Root{}
+		optimistic            = false
 	)
 
 	request := types.BuildNewPayloadRequest(
 		executionPayload,
 		versionedHashes,
 		&parentBeaconBlockRoot,
+		optimistic,
 	)
 
 	err := request.HasValidVersionedAndBlockHashes()

execution/engine/engine.go

@@ -93,16 +93,16 @@ func (ee *Engine) NotifyForkchoiceUpdate(
 		)
 
 	case errors.IsAny(err, engineerrors.ErrSyncingPayloadStatus):
-		// We bubble up syncing as an error, to be able to stop
-		// bootstrapping from progressing in CL while EL is syncing.
+		// We do not bubble the error up, since we want to handle it
+		// in the same way as the other cases.
 		ee.metrics.markForkchoiceUpdateSyncing(req.State, err)
-		return nil, nil, err
+		return payloadID, nil, nil
 
 	case errors.Is(err, engineerrors.ErrInvalidPayloadStatus):
 		// If we get invalid payload status, we will need to find a valid
 		// ancestor block and force a recovery.
 		ee.metrics.markForkchoiceUpdateInvalid(req.State, err)
-		return nil, nil, ErrBadBlockProduced
+		return payloadID, latestValidHash, ErrBadBlockProduced
 
 	case jsonrpc.IsPreDefinedError(err):
 		// JSON-RPC errors are predefined and should be handled as such.
@@ -124,14 +124,16 @@ func (ee *Engine) NotifyForkchoiceUpdate(
 			"safe_eth1_hash", req.State.SafeBlockHash,
 			"finalized_eth1_hash", req.State.FinalizedBlockHash,
 		)
-		return nil, nil, ErrNilPayloadOnValidResponse
+		return payloadID, latestValidHash, ErrNilPayloadOnValidResponse
 	}
 
 	return payloadID, latestValidHash, nil
 }
 
 // VerifyAndNotifyNewPayload verifies the new payload and notifies the
 // execution client.
+//
+//nolint:funlen
 func (ee *Engine) VerifyAndNotifyNewPayload(
 	ctx context.Context,
 	req *ctypes.NewPayloadRequest,
@@ -140,6 +142,7 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 	ee.metrics.markNewPayloadCalled(
 		req.ExecutionPayload.GetBlockHash(),
 		req.ExecutionPayload.GetParentHash(),
+		req.Optimistic,
 	)
 
 	// First we verify the block hash and versioned hashes are valid.
@@ -169,19 +172,28 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 		ee.metrics.markNewPayloadSyncingPayloadStatus(
 			req.ExecutionPayload.GetBlockHash(),
 			req.ExecutionPayload.GetParentHash(),
+			req.Optimistic,
 		)
 
 	case errors.IsAny(err, engineerrors.ErrAcceptedPayloadStatus):
 		ee.metrics.markNewPayloadAcceptedPayloadStatus(
 			req.ExecutionPayload.GetBlockHash(),
 			req.ExecutionPayload.GetParentHash(),
+			req.Optimistic,
 		)
 
 	case errors.Is(err, engineerrors.ErrInvalidPayloadStatus):
 		ee.metrics.markNewPayloadInvalidPayloadStatus(
 			req.ExecutionPayload.GetBlockHash(),
+			req.Optimistic,
 		)
 
+		// We want to return bad block irrespective of
+		// if we are running in optimistic mode or not.
+		//
+		// TODO: should we still nillify the error in optimistic mode?
+		return ErrBadBlockProduced
+
 	case jsonrpc.IsPreDefinedError(err):
 		// Protect against possible nil value.
 		if lastValidHash == nil {
@@ -191,20 +203,42 @@ func (ee *Engine) VerifyAndNotifyNewPayload(
 		ee.metrics.markNewPayloadJSONRPCError(
 			req.ExecutionPayload.GetBlockHash(),
 			*lastValidHash,
+			req.Optimistic,
 			err,
 		)
 
 		err = errors.Join(err, engineerrors.ErrPreDefinedJSONRPC)
 	case err != nil:
 		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
+	// is mainly to allow us to safely call the execution client
+	// during abci.FinalizeBlock. If we are in abci.FinalizeBlock and
+	// we get an error here, we make the assumption that
+	// abci.ProcessProposal
+	// has deemed that the BeaconBlock containing the given ExecutionPayload
+	// was marked as valid by an honest majority of validators, and we
+	// don't want to halt the chain because of an error here.
+	//
+	// The practical reason we want to handle this edge case
+	// is to protect against an awkward shutdown condition in which an
+	// execution client dies between the end of abci.ProcessProposal
+	// and the beginning of abci.FinalizeBlock. Without handling this case
+	// it would cause a failure of abci.FinalizeBlock and a
+	// "CONSENSUS FAILURE!!!!" at the CometBFT layer.
+	if req.Optimistic {
+		return nil
+	}
 	return err
 }