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

Jump to bottom

Handle merge block fetch error #4016

Merged
merged 2 commits into from
May 18, 2022
Merged

Handle merge block fetch error #4016

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8629623
Handle merge block fetch error
g11tech May 12, 2022
6a46abf
Log errors on fetch errors for terminal pow
g11tech May 13, 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
4 changes: 2 additions & 2 deletions packages/lodestar/src/chain/blocks/verifyBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,8 @@ export async function verifyBlockStateTransition(
if (isMergeTransitionBlock) {
const mergeBlock = block.message as bellatrix.BeaconBlock;
const powBlockRootHex = toHexString(mergeBlock.body.executionPayload.parentHash);
const powBlock = await chain.eth1.getPowBlock(powBlockRootHex);
const powBlockParent = powBlock && (await chain.eth1.getPowBlock(powBlock.parentHash));
const powBlock = await chain.eth1.getPowBlock(powBlockRootHex).catch((_error) => null);
const powBlockParent = powBlock && (await chain.eth1.getPowBlock(powBlock.parentHash).catch((_error) => null));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very serious error we must know about, at least log it

Copy link
Contributor

Choose a reason for hiding this comment

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

If isMergeTransitionBlock == true and powBlock == null it's impossible for us to accept this block right?

Copy link
Contributor Author

@g11tech g11tech May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

no, we can still import it if executionStatus is SYNCING and we are not safeSlotsToImportOptimistically (128) blocks of the clock slot (i.e. merge has long happened, this is just syncing past merge even if EL doesn't has merge block), which is when this fetch actually throws error.

Another scenario is if TERMINAL_BLOCK_HASH is set, then also powBlock is not required as hashes are compared directly.

assertValidTerminalPowBlock takes care of these checks so we can safely assign it null and let it move to validation.

Logging them is a good idea! will update 👍

Copy link
Contributor

@dapplion dapplion May 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why not move the fn assertValidTerminalPowBlock into the verifyBlock.ts, and do the fetching inside the function? Seems like things will be more straight forward to reason about.

Copy link
Contributor Author

@g11tech g11tech May 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ummm well there are couple of considerations:
i) forkchoice spec tests have spec tests for terminal conditions, so a pure function is better to have
ii) the #3508 will require this validation as per spec once the merge block status changes from SYNCING to VALID.

Lets leave it here for the moment and see if we can place it better when working on the ii) ? 🤔


assertValidTerminalPowBlock(chain.config, mergeBlock, {executionStatus, powBlock, powBlockParent});
}
Expand Down