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.

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

remove incorrect block gossip validation condition #4044

Merged
merged 2 commits into from
Aug 29, 2022
Merged

remove incorrect block gossip validation condition #4044

merged 2 commits into from
Aug 29, 2022

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Aug 29, 2022

This was excessively aggressive and could cause a deadlock in recovery of EL sync verification.

It was an implementation of https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/p2p-interface.md#beacon_block:

  • If exection_payload verification of block's parent by an execution node is not complete:
    • [REJECT] The block's parent (defined by block.parent_root) passes all validation (excluding execution node verification of the block.body.execution_payload).

However, because Nimbus always completes execution_payload verification by the time a block reaches the DAG, this can't ever happen. The first thing Nimbus does is to ask the EL, and wait for the EL's reply either ACCEPTED or SYNCING, because optimistic sync requires this anyway. So by the time it reaches here, the only relevant condition is

  • otherwise:
    • [IGNORE] The block's parent (defined by block.parent_root) passes all validation (including execution node verification of the block.body.execution_payload).

which is already handled.

@github-actions
Copy link

Unit Test Results

       12 files  ±0       860 suites  ±0   1h 4m 10s ⏱️ - 1m 3s
  1 982 tests ±0    1 835 ✔️ ±0  147 💤 ±0  0 ±0 
10 662 runs  ±0  10 472 ✔️ ±0  190 💤 ±0  0 ±0 

Results for commit 61e57e6. ± Comparison against base commit b60456f.

@zah zah merged commit 2545d1d into unstable Aug 29, 2022
@zah zah deleted the DbS branch August 29, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants