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

Enforce terminal PoW block to be on the cusp #2522

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

mkalinin
Copy link
Contributor

What's done

Execution client must stop importing PoW blocks beyond the terminal PoW block. This PR introduces the corresponding change to this requirement.

Rationale

Suppose the beacon chain client goes offline before transition_total_difficulty has been reached and stays offline while the network meets this threshold. This may happen either due to misconfiguration or software bug and results in the inability of the execution client to switch to the proof-of-stake and is following the wrong chain. Stopping block import after terminal PoW block would prevent potential security issues in this case.

Credits to @dankrad who made this point during Eth2 Call 68.

@@ -90,7 +90,8 @@ def get_execution_payload(state: BeaconState,
execution_engine: ExecutionEngine) -> ExecutionPayload:
if not is_merge_complete(state):
pow_block = get_pow_chain_head()
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 making an assumption that the execution-engine is rejecting blocks past valid terminal PoW blocks, otherwise (if for example a child is the head past the terminal pow block) this function might never see is_valid_terminal_pow_block as true.

I'm not certain if this should be noted here, but if we keep the semantics of get_pow_chain_head it must be a hard requiremnt somewhere in the spec that the PoW chain rejects beyond terminal difficulty. Otherwise you'd have to do get_pow_chain_at_difficulty(terminal_difficulty) -> Union[Block, None] if this is not a strict requirement under the hood

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is making an assumption that the execution-engine is rejecting blocks past valid terminal PoW blocks

That was the intention. The execution-layer must have a hard requirement on not importing PoW blocks beyond the terminal one which implies that the head won't move beyond the terminal PoW block. However, I do see the value in getting rid of this assumption on the beacon chain side.

Otherwise you'd have to do get_pow_chain_at_difficulty(terminal_difficulty) -> Union[Block, None]

Looks reasonable and this function may be implemented without the corresponding query to the execution client. Beacon chain client may follow the PoW chain in the same fashion as it does in order to obtain Eth1Data and filter the PoW block tree to get terminal PoW block.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks good! one quick discussion point

@mkalinin
Copy link
Contributor Author

In the most recent change there was a conflict between Union from typing module and Union from ssz_typing. I took the path with less changes and did from typing import Union as PyUnion, another option is to have from eth2spec.utils.ssz.ssz_typing import Union as SSZUnion but it also requires the change in the beacon-chain.md.

Not sure which one is better and leaving it up you, cc @djrtwo @protolambda @hwwhww

@hwwhww
Copy link
Contributor

hwwhww commented Jul 20, 2021

@mkalinin (Sorry, I just saw it!)

PyUnion[PowBlock, None]

For this case, we can use Python typing Optional[SomeType] to replace PyUnion[SomeType, None]. It has been used in phase0 chain validator guide and DAS spec.

@hwwhww hwwhww added the Bellatrix CL+EL Merge label Jul 20, 2021
@mkalinin
Copy link
Contributor Author

@mkalinin (Sorry, I just saw it!)

PyUnion[PowBlock, None]

For this case, we can use Python typing Optional[SomeType] to replace PyUnion[SomeType, None]. It has been used in phase0 chain validator guide and DAS spec.

Thanks a lot for directions. Fixed!

@djrtwo djrtwo merged commit 947ca90 into ethereum:dev Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bellatrix CL+EL Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants