Review bellatrix assembleBody (#4307)

* Review bellatrix assembleBody * fix the prepareExecutionPayload to only return payloadid * simplify prepareForNextSlot * handle the error case properly when payload can't be fetched * unit test fixes * avoid null as prepare response to be more explicit * changing empty payload logging from verbose to yarn Co-authored-by: gajinder <[email protected]>
ChainSafe · Jul 17, 2022 · 3239eb2 · 3239eb2
1 parent f04d270
commit 3239eb2
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 106 deletions.
diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts
@@ -202,7 +202,7 @@ export function getValidatorApi({chain, config, logger, metrics, network, sync}:
 
       timer = metrics?.blockProductionTime.startTimer();
       const block = await assembleBlock(
-        {type, chain, metrics},
+        {type, chain, metrics, logger},
         {
           slot,
           randaoReveal,

diff --git a/packages/beacon-node/src/chain/factory/block/body.ts b/packages/beacon-node/src/chain/factory/block/body.ts
@@ -22,7 +22,7 @@ import {
   isMergeTransitionComplete,
 } from "@lodestar/state-transition";
 import {IChainForkConfig} from "@lodestar/config";
-import {toHex} from "@lodestar/utils";
+import {toHex, ILogger} from "@lodestar/utils";
 
 import {IBeaconChain} from "../../interface.js";
 import {PayloadId, IExecutionEngine, IExecutionBuilder} from "../../../execution/index.js";
@@ -42,7 +42,7 @@ export type AssembledBlockType<T extends BlockType> = T extends BlockType.Full
   : bellatrix.BlindedBeaconBlock;
 
 export async function assembleBody<T extends BlockType>(
-  {type, chain}: {type: T; chain: IBeaconChain},
+  {type, chain, logger}: {type: T; chain: IBeaconChain; logger?: ILogger},
   currentState: CachedBeaconStateAllForks,
   {
     randaoReveal,
@@ -118,39 +118,50 @@ export async function assembleBody<T extends BlockType>(
           feeRecipient
         );
       }
+
       // For MeV boost integration, this is where the execution header will be
       // fetched from the payload id and a blinded block will be produced instead of
       // fullblock for the validator to sign
-
-      const executionPayloadHeader = await prepareExecutionPayloadHeader(
+      (blockBody as bellatrix.BlindedBeaconBlockBody).executionPayloadHeader = await prepareExecutionPayloadHeader(
         chain,
         currentState as CachedBeaconStateBellatrix,
         proposerPubKey
       );
-      (blockBody as bellatrix.BlindedBeaconBlockBody).executionPayloadHeader = executionPayloadHeader;
     } else {
-      // prepareExecutionPayload will throw error via notifyForkchoiceUpdate if
-      // the EL returns Syncing on this request to prepare a payload
+      // try catch payload fetch here, because there is still a recovery path possible if we
+      // are pre-merge. We don't care the same for builder segment as the execution block
+      // will takeover if the builder flow was activated and errors
       try {
-        const payloadId = await prepareExecutionPayload(
+        const prepareRes = await prepareExecutionPayload(
           chain,
           safeBlockHash,
           finalizedBlockHash ?? ZERO_HASH_HEX,
           currentState as CachedBeaconStateBellatrix,
           feeRecipient
         );
-        const executionPayload = payloadId ? await chain.executionEngine.getPayload(payloadId) : null;
-        if (executionPayload === null) throw Error("Empty executionPayload");
-        (blockBody as bellatrix.BeaconBlockBody).executionPayload =
-          executionPayload ?? ssz.bellatrix.ExecutionPayload.defaultValue();
+        (blockBody as bellatrix.BeaconBlockBody).executionPayload = prepareRes.isPremerge
+          ? ssz.bellatrix.ExecutionPayload.defaultValue()
+          : await chain.executionEngine.getPayload(prepareRes.payloadId);
       } catch (e) {
-        // If the state is post merge, the empty/default payload will not be
-        // accepted by the engine. Else we can propose with empty payload
-        // and let someone else build a merge transition payload
-        (blockBody as bellatrix.BeaconBlockBody).executionPayload = ssz.bellatrix.ExecutionPayload.defaultValue();
+        // ok we don't have an execution payload here, so we can assign an empty one
+        // if pre-merge
+
+        if (!isMergeTransitionComplete(currentState as CachedBeaconStateBellatrix)) {
+          logger?.warn(
+            "Fetch payload from the execution failed, however since we are still pre-merge proceeding with an empty one.",
+            {},
+            e as Error
+          );
+          (blockBody as bellatrix.BeaconBlockBody).executionPayload = ssz.bellatrix.ExecutionPayload.defaultValue();
+        } else {
+          // since merge transition is complete, we need a valid payload even if with an
+          // empty (transactions) one. defaultValue isn't gonna cut it!
+          throw e;
+        }
       }
     }
   }
+
   return blockBody as AssembledBodyType<T>;
 }
 
@@ -171,28 +182,46 @@ export async function prepareExecutionPayload(
   finalizedBlockHash: RootHex,
   state: CachedBeaconStateBellatrix,
   suggestedFeeRecipient: string
-): Promise<PayloadId | null> {
-  const parentHash = extractExecutionPayloadParentHash(chain, state);
-  if (parentHash === null) return null;
+): Promise<{isPremerge: true} | {isPremerge: false; payloadId: PayloadId}> {
+  const parentHashRes = getExecutionPayloadParentHash(chain, state);
+  if (parentHashRes.isPremerge) {
+    // Return null only if the execution is pre-merge
+    return {isPremerge: true};
+  }
 
+  const {parentHash} = parentHashRes;
   const timestamp = computeTimeAtSlot(chain.config, state.slot, state.genesisTime);
   const prevRandao = getRandaoMix(state, state.epochCtx.epoch);
 
+  const payloadIdCached = chain.executionEngine.payloadIdCache.get({
+    headBlockHash: toHex(parentHash),
+    finalizedBlockHash,
+    timestamp: numToQuantity(timestamp),
+    prevRandao: toHex(prevRandao),
+    suggestedFeeRecipient,
+  });
+
+  // prepareExecutionPayload will throw error via notifyForkchoiceUpdate if
+  // the EL returns Syncing on this request to prepare a payload
+  // TODO: Handle only this case, DO NOT put a generic try / catch that discards all errors
   const payloadId =
-    chain.executionEngine.payloadIdCache.get({
-      headBlockHash: toHex(parentHash),
-      finalizedBlockHash,
-      timestamp: numToQuantity(timestamp),
-      prevRandao: toHex(prevRandao),
-      suggestedFeeRecipient,
-    }) ??
+    payloadIdCached ??
     (await chain.executionEngine.notifyForkchoiceUpdate(toHex(parentHash), safeBlockHash, finalizedBlockHash, {
       timestamp,
       prevRandao,
       suggestedFeeRecipient,
     }));
-  if (!payloadId) throw new Error("InvalidPayloadId: Null");
-  return payloadId;
+
+  // Should never happen, notifyForkchoiceUpdate() with payload attributes always
+  // returns payloadId
+  if (payloadId === null) {
+    throw Error("notifyForkchoiceUpdate returned payloadId null");
+  }
+
+  // We are only returning payloadId here because prepareExecutionPayload is also called from
+  // prepareNextSlot, which is an advance call to execution engine to start building payload
+  // Actual payload isn't produced till getPayload is called.
+  return {isPremerge: false, payloadId};
 }
 
 async function prepareExecutionPayloadHeader(
@@ -204,25 +233,34 @@ async function prepareExecutionPayloadHeader(
   state: CachedBeaconStateBellatrix,
   proposerPubKey: BLSPubkey
 ): Promise<bellatrix.ExecutionPayloadHeader> {
-  if (!chain.executionBuilder) throw Error("executionBuilder required");
+  if (!chain.executionBuilder) {
+    throw Error("executionBuilder required");
+  }
 
-  const parentHash = extractExecutionPayloadParentHash(chain, state);
-  if (parentHash === null) throw Error(`Invalid parentHash=${parentHash} for builder getPayloadHeader`);
+  const parentHashRes = getExecutionPayloadParentHash(chain, state);
 
+  if (parentHashRes.isPremerge) {
+    // TODO: Is this okay?
+    throw Error("Execution builder disabled pre-merge");
+  }
+
+  const {parentHash} = parentHashRes;
   return chain.executionBuilder.getPayloadHeader(state.slot, parentHash, proposerPubKey);
 }
 
-function extractExecutionPayloadParentHash(
+function getExecutionPayloadParentHash(
   chain: {
     eth1: IEth1ForBlockProduction;
     config: IChainForkConfig;
   },
   state: CachedBeaconStateBellatrix
-): Root | null {
+): {isPremerge: true} | {isPremerge: false; parentHash: Root} {
   // Use different POW block hash parent for block production based on merge status.
   // Returned value of null == using an empty ExecutionPayload value
-  let parentHash: Root;
-  if (!isMergeTransitionComplete(state)) {
+  if (isMergeTransitionComplete(state)) {
+    // Post-merge, normal payload
+    return {isPremerge: false, parentHash: state.latestExecutionPayloadHeader.blockHash};
+  } else {
     if (
       !ssz.Root.equals(chain.config.TERMINAL_BLOCK_HASH, ZERO_HASH) &&
       getCurrentEpoch(state) < chain.config.TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH
@@ -232,19 +270,16 @@ function extractExecutionPayloadParentHash(
           chain.config.TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH
         }, actual: ${getCurrentEpoch(state)}`
       );
+
     const terminalPowBlockHash = chain.eth1.getTerminalPowBlock();
     if (terminalPowBlockHash === null) {
       // Pre-merge, no prepare payload call is needed
-      return null;
+      return {isPremerge: true};
     } else {
       // Signify merge via producing on top of the last PoW block
-      parentHash = terminalPowBlockHash;
+      return {isPremerge: false, parentHash: terminalPowBlockHash};
     }
-  } else {
-    // Post-merge, normal payload
-    parentHash = state.latestExecutionPayloadHeader.blockHash;
   }
-  return parentHash;
 }
 
 /** process_sync_committee_contributions is implemented in syncCommitteeContribution.getSyncAggregate */
diff --git a/packages/beacon-node/src/chain/factory/block/index.ts b/packages/beacon-node/src/chain/factory/block/index.ts
@@ -1,6 +1,7 @@
 import {CachedBeaconStateAllForks, stateTransition} from "@lodestar/state-transition";
 import {allForks, Bytes32, Bytes96, Root, Slot} from "@lodestar/types";
 import {fromHexString} from "@chainsafe/ssz";
+import {ILogger} from "@lodestar/utils";
 
 import {ZERO_HASH} from "../../../constants/index.js";
 import {IMetrics} from "../../../metrics/index.js";
@@ -11,11 +12,12 @@ import {assembleBody, BlockType, AssembledBlockType} from "./body.js";
 type AssembleBlockModules = {
   chain: IBeaconChain;
   metrics: IMetrics | null;
+  logger?: ILogger;
 };
 
 export {BlockType, AssembledBlockType};
 export async function assembleBlock<T extends BlockType>(
-  {chain, metrics, type}: AssembleBlockModules & {type: T},
+  {chain, metrics, logger, type}: AssembleBlockModules & {type: T},
   {
     randaoReveal,
     graffiti,
@@ -38,7 +40,7 @@ export async function assembleBlock<T extends BlockType>(
     proposerIndex,
     parentRoot: parentBlockRoot,
     stateRoot: ZERO_HASH,
-    body: await assembleBody<T>({type, chain}, state, {
+    body: await assembleBody<T>({type, chain, logger}, state, {
       randaoReveal,
       graffiti,
       blockSlot: slot,