Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix safe optimistic import #4340

Merged
merged 5 commits into from
Jul 26, 2022
Merged

Fix safe optimistic import #4340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b3fe288
Fix safe optimistic import
g11tech Jul 23, 2022
c6c08f8
fix issues
g11tech Jul 23, 2022
e2b8d05
correct comment text
g11tech Jul 26, 2022
c7ea53e
propagate the parent of the segment from sanity checks
g11tech Jul 26, 2022
d14a1ff
fixes
g11tech Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/beacon-node/src/chain/blocks/index.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this function ensure that there are more than 0 blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be somewhere from where its called? because it didn't give any type issues, but will return early to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a throw just to be safe

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
Open in desktop
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};
}