Skip to content

Commit

Permalink
Fix safe optimistic import (#4340)
Browse files Browse the repository at this point in the history 
* Fix safe optimistic import

* fix issues

* correct comment text

Co-authored-by: Lion - dapplion <[email protected]>

* propagate the parent of the segment from sanity checks

* fixes

Co-authored-by: Lion - dapplion <[email protected]>
  • Loading branch information
@g11tech @dapplion
g11tech and dapplion authored Jul 26, 2022
1 parent 3320c20 commit 384e393
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 45 deletions.
6 changes: 4 additions & 2 deletions packages/beacon-node/src/chain/blocks/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,19 @@ export async function processBlocks(
}

try {
const {relevantBlocks, parentSlots} = verifyBlocksSanityChecks(chain, blocks, opts);
const {relevantBlocks, parentSlots, parentBlock} = verifyBlocksSanityChecks(chain, blocks, opts);

// No relevant blocks, skip verifyBlocksInEpoch()
if (relevantBlocks.length === 0) {
if (relevantBlocks.length === 0 || parentBlock === null) {
// parentBlock can only be null if relevantBlocks are empty
return;
}

// Fully verify a block to be imported immediately after. Does not produce any side-effects besides adding intermediate
// states in the state cache through regen.
const {postStates, executionStatuses, proposerBalanceDeltas} = await verifyBlocksInEpoch(
chain,
parentBlock,
relevantBlocks,
opts
);
Expand Down
5 changes: 3 additions & 2 deletions packages/beacon-node/src/chain/blocks/verifyBlock.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {CachedBeaconStateAllForks, computeEpochAtSlot} from "@lodestar/state-transition";
import {allForks, bellatrix} from "@lodestar/types";
import {toHexString} from "@chainsafe/ssz";
import {IForkChoice, ExecutionStatus} from "@lodestar/fork-choice";
import {IForkChoice, ExecutionStatus, ProtoBlock} from "@lodestar/fork-choice";
import {IChainForkConfig} from "@lodestar/config";
import {ILogger} from "@lodestar/utils";
import {IMetrics} from "../../metrics/index.js";
Expand Down Expand Up @@ -43,6 +43,7 @@ export type VerifyBlockModules = {
*/
export async function verifyBlocksInEpoch(
chain: VerifyBlockModules,
parentBlock: ProtoBlock,
blocks: allForks.SignedBeaconBlock[],
opts: BlockProcessOpts & ImportBlockOpts
): Promise<{
Expand Down Expand Up @@ -88,7 +89,7 @@ export async function verifyBlocksInEpoch(
verifyBlocksSignatures(chain, preState0, blocks, opts),

// Execution payloads
verifyBlocksExecutionPayload(chain, blocks, preState0, abortController.signal, opts),
verifyBlocksExecutionPayload(chain, parentBlock, blocks, preState0, abortController.signal, opts),
]);

if (mergeBlockFound !== null) {
Expand Down
122 changes: 87 additions & 35 deletions packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
} from "@lodestar/state-transition";
import {bellatrix, allForks} from "@lodestar/types";
import {toHexString} from "@chainsafe/ssz";
import {IForkChoice, ExecutionStatus, assertValidTerminalPowBlock} from "@lodestar/fork-choice";
import {IForkChoice, ExecutionStatus, assertValidTerminalPowBlock, ProtoBlock} from "@lodestar/fork-choice";
import {IChainForkConfig} from "@lodestar/config";
import {ErrorAborted, ILogger} from "@lodestar/utils";
import {IExecutionEngine} from "../../execution/engine/index.js";
Expand All @@ -33,6 +33,7 @@ type VerifyBlockModules = {
*/
export async function verifyBlocksExecutionPayload(
chain: VerifyBlockModules,
parentBlock: ProtoBlock,
blocks: allForks.SignedBeaconBlock[],
preState0: CachedBeaconStateAllForks,
signal: AbortSignal,
Expand All @@ -41,14 +42,90 @@ export async function verifyBlocksExecutionPayload(
const executionStatuses: ExecutionStatus[] = [];
let mergeBlockFound: bellatrix.BeaconBlock | null = null;

// Error in the same way as verifyBlocksSanityChecks if empty blocks
if (blocks.length === 0) {
throw Error("Empty partiallyVerifiedBlocks");
}

// For a block with SYNCING status (called optimistic block), it's okay to import with
// SYNCING status as EL could switch into syncing
//
// 1. On intial startup/restart
// 2. When some reorg might have occured and EL doesn't has a parent root
// (observed on devnets)
// 3. Because of some unavailable (and potentially invalid) root but there is no way
// of knowing if this is invalid/unavailable. For unavailable block, some proposer
// will (sooner or later) build on the available parent head which will
// eventually win in fork-choice as other validators vote on VALID blocks.
//
// Once EL catches up again and respond VALID, the fork choice will be updated which
// will either validate or prune invalid blocks
//
// We need to track and keep updating if its safe to optimistically import these blocks.
// The following is how we determine for a block if its safe:
//
// (but we need to modify this check for this segment of blocks because it checks if the
// parent of any block imported in forkchoice is post-merge and currently we could only
// have blocks[0]'s parent imported in the chain as this is no longer one by one verify +
// import.)
//
//
// When to import such blocks:
// From: https://github.com/ethereum/consensus-specs/pull/2844
// A block MUST NOT be optimistically imported, unless either of the following
// conditions are met:
//
// 1. Parent of the block has execution
//
// Since with the sync optimizations, the previous block might not have been in the
// forkChoice yet, so the below check could fail for safeSlotsToImportOptimistically
//
// Luckily, we can depend on the preState0 to see if we are already post merge w.r.t
// the blocks we are importing.
//
// Or in other words if
// - block status is syncing
// - and we are not in a post merge world and is parent is not optimistically safe
// - and we are syncing close to the chain head i.e. clock slot
// - and parent is optimistically safe
//
// then throw error
//
//
// - if we haven't yet imported a post merge ancestor in forkchoice i.e.
// - and we are syncing close to the clockSlot, i.e. merge Transition could be underway
//
//
// 2. The current slot (as per the system clock) is at least
// SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the slot of the block being
// imported.
// This means that the merge transition could be underway and we can't afford to import
// a block which is not fully validated as it could affect liveliness of the network.
//
//
// For this segment of blocks:
// We are optimistically safe with respec to this entire block segment if:
// - all the blocks are way behind the current slot
// - or we have already imported a post-merge parent of first block of this chain in forkchoice
const lastBlock = blocks[blocks.length - 1];
let isOptimisticallySafe =
parentBlock.executionStatus !== ExecutionStatus.PreMerge ||
lastBlock.message.slot + opts.safeSlotsToImportOptimistically < chain.clock.currentSlot;

for (const block of blocks) {
// If blocks are invalid in consensus the main promise could resolve before this loop ends.
// In that case stop sending blocks to execution engine
if (signal.aborted) {
throw new ErrorAborted("verifyBlockExecutionPayloads");
}

const {executionStatus} = await verifyBlockExecutionPayload(chain, block, preState0, opts);
const {executionStatus} = await verifyBlockExecutionPayload(chain, block, preState0, opts, isOptimisticallySafe);
// It becomes optimistically safe for following blocks if a post-merge block is deemed fit
// for import. If it would not have been safe verifyBlockExecutionPayload would throw error
// and we would not be here.
if (executionStatus !== ExecutionStatus.PreMerge) {
isOptimisticallySafe = true;
}
executionStatuses.push(executionStatus);

const isMergeTransitionBlock =
Expand Down Expand Up @@ -122,7 +199,8 @@ export async function verifyBlockExecutionPayload(
chain: VerifyBlockModules,
block: allForks.SignedBeaconBlock,
preState0: CachedBeaconStateAllForks,
opts: BlockProcessOpts
opts: BlockProcessOpts,
isOptimisticallySafe: boolean
): Promise<{executionStatus: ExecutionStatus}> {
/** Not null if execution is enabled */
const executionPayloadEnabled =
Expand Down Expand Up @@ -168,43 +246,17 @@ export async function verifyBlockExecutionPayload(
// Accepted and Syncing have the same treatment, as final validation of block is pending
case ExecutePayloadStatus.ACCEPTED:
case ExecutePayloadStatus.SYNCING: {
// It's okay to ignore SYNCING status as EL could switch into syncing
// 1. On intial startup/restart
// 2. When some reorg might have occured and EL doesn't has a parent root
// (observed on devnets)
// 3. Because of some unavailable (and potentially invalid) root but there is no way
// of knowing if this is invalid/unavailable. For unavailable block, some proposer
// will (sooner or later) build on the available parent head which will
// eventually win in fork-choice as other validators vote on VALID blocks.
// Once EL catches up again and respond VALID, the fork choice will be updated which
// will either validate or prune invalid blocks
//
// When to import such blocks:
// From: https://github.com/ethereum/consensus-specs/pull/2844
// A block MUST NOT be optimistically imported, unless either of the following
// conditions are met:
//
// 1. Parent of the block has execution
// 2. The justified checkpoint has execution enabled
// 3. The current slot (as per the system clock) is at least
// SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY ahead of the slot of the block being
// imported.

const parentRoot = toHexString(block.message.parentRoot);
const parentBlock = chain.forkChoice.getBlockHex(parentRoot);
const justifiedBlock = chain.forkChoice.getJustifiedBlock();

// Check if the entire segment was deemed safe or, this block specifically itself if not in
// the safeSlotsToImportOptimistically window of current slot, then we can import else
// we need to throw and not import his block
if (
!parentBlock ||
// Following condition is the !(Not) of the safe import condition
(parentBlock.executionStatus === ExecutionStatus.PreMerge &&
justifiedBlock.executionStatus === ExecutionStatus.PreMerge &&
block.message.slot + opts.safeSlotsToImportOptimistically > chain.clock.currentSlot)
!isOptimisticallySafe &&
block.message.slot + opts.safeSlotsToImportOptimistically >= chain.clock.currentSlot
) {
throw new BlockError(block, {
code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
execStatus: ExecutePayloadStatus.UNSAFE_OPTIMISTIC_STATUS,
errorMessage: `not safe to import ${execResult.status} payload within ${opts.safeSlotsToImportOptimistically} of currentSlot, status=${execResult.status}`,
errorMessage: `not safe to import ${execResult.status} payload within ${opts.safeSlotsToImportOptimistically} of currentSlot`,
});
}

Expand Down
19 changes: 13 additions & 6 deletions packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {computeStartSlotAtEpoch} from "@lodestar/state-transition";
import {IChainForkConfig} from "@lodestar/config";
import {IForkChoice} from "@lodestar/fork-choice";
import {IForkChoice, ProtoBlock} from "@lodestar/fork-choice";
import {allForks, Slot} from "@lodestar/types";
import {toHexString} from "@lodestar/utils";
import {IBeaconClock} from "../clock/interface.js";
Expand All @@ -23,13 +23,14 @@ export function verifyBlocksSanityChecks(
chain: {forkChoice: IForkChoice; clock: IBeaconClock; config: IChainForkConfig},
blocks: allForks.SignedBeaconBlock[],
opts: ImportBlockOpts
): {relevantBlocks: allForks.SignedBeaconBlock[]; parentSlots: Slot[]} {
): {relevantBlocks: allForks.SignedBeaconBlock[]; parentSlots: Slot[]; parentBlock: ProtoBlock | null} {
if (blocks.length === 0) {
throw Error("Empty partiallyVerifiedBlocks");
}

const relevantBlocks: allForks.SignedBeaconBlock[] = [];
const parentSlots: Slot[] = [];
let parentBlock: ProtoBlock | null = null;

for (const block of blocks) {
const blockSlot = block.message.slot;
Expand Down Expand Up @@ -57,16 +58,16 @@ export function verifyBlocksSanityChecks(

let parentBlockSlot: Slot;

// When importing a block segment, only the first NON-IGNORED block must be known to the fork-choice.
if (relevantBlocks.length > 0) {
parentBlockSlot = relevantBlocks[relevantBlocks.length - 1].message.slot;
} else {
// Parent is known to the fork-choice
// When importing a block segment, only the first NON-IGNORED block must be known to the fork-choice.
const parentRoot = toHexString(block.message.parentRoot);
const parentBlock = chain.forkChoice.getBlockHex(parentRoot);
parentBlock = chain.forkChoice.getBlockHex(parentRoot);
if (!parentBlock) {
throw new BlockError(block, {code: BlockErrorCode.PARENT_UNKNOWN, parentRoot});
} else {
// Parent is known to the fork-choice
parentBlockSlot = parentBlock.slot;
}
}
Expand Down Expand Up @@ -95,5 +96,11 @@ export function verifyBlocksSanityChecks(
parentSlots.push(parentBlockSlot);
}

return {relevantBlocks, parentSlots};
// Just assert to be over cautious and for purposes to be more explicit for someone
// going through the code segment
if (parentBlock === null && relevantBlocks.length > 0) {
throw Error(`Internal error, parentBlock should not be null for relevantBlocks=${relevantBlocks.length}`);
}

return {relevantBlocks, parentSlots, parentBlock};
}

0 comments on commit 384e393

Please sign in to comment.