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

Changes from 2 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
133 changes: 99 additions & 34 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 @@ -4,6 +4,8 @@ import {
isBellatrixBlockBodyType,
isMergeTransitionBlock as isMergeTransitionBlockFn,
isExecutionEnabled,
isMergeTransitionComplete,
BeaconStateBellatrix,
} from "@lodestar/state-transition";
import {bellatrix, allForks} from "@lodestar/types";
import {toHexString} from "@chainsafe/ssz";
Expand Down Expand Up @@ -41,14 +43,102 @@ export async function verifyBlocksExecutionPayload(
const executionStatuses: ExecutionStatus[] = [];
let mergeBlockFound: bellatrix.BeaconBlock | null = null;

// 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.)
//
// 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
//
// 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 are are not yet in post merge world (preState0's isMergeTransitionComplete
// is false) and
// - 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:
// - we have transitioned to post-merge world or
// - 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 =
(isBellatrixStateType(preState0) && isMergeTransitionComplete(preState0 as BeaconStateBellatrix)) ||
lastBlock.message.slot + opts.safeSlotsToImportOptimistically < chain.clock.currentSlot;

if (!isOptimisticallySafe) {
const firstBlock = blocks[0];
const parentRoot = toHexString(firstBlock.message.parentRoot);
const parentBlock = chain.forkChoice.getBlockHex(parentRoot);
Copy link
Contributor

@dapplion dapplion Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You could return the parent block of the segment in sanity checks

lodestar/packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts

Line 98 in 3320c20

return {relevantBlocks, parentSlots};

and re-use here

if (!parentBlock) {
throw new BlockError(firstBlock, {
code: BlockErrorCode.PARENT_UNKNOWN,
parentRoot,
});
}

// isOptimisticallySafe if we have already imported a post-merge parent of this block
if (parentBlock.executionStatus !== ExecutionStatus.PreMerge) {
isOptimisticallySafe = true;
}
}

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 safefor following blocks if a post-merge block is deemed fit
g11tech marked this conversation as resolved.
Show resolved Hide resolved
// for import. If it would not have been safe verifyBlockExecutionPayload would throw error
// and we would not be here.
isOptimisticallySafe ||= executionStatus !== ExecutionStatus.PreMerge;
Copy link
Contributor

@dapplion dapplion Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

While the ||= operand is pretty cool, can you use a regular if statement since it's more readable?

executionStatuses.push(executionStatus);

const isMergeTransitionBlock =
Expand Down Expand Up @@ -122,7 +212,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 +259,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