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

Enforce terminal PoW block to be on the cusp #2522

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Enforce terminal PoW block to be on the cusp #2522

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4a050e
Enforce terminal PoW block to be on the cusp
mkalinin Jul 16, 2021
add00ad
Replace get_pow_chain_head with get_pow_block_at_total_difficulty in …
mkalinin Jul 17, 2021
65f6aa1
Replace PyUnion with Optional
mkalinin Jul 20, 2021
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
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ def sundry_functions(cls) -> str:


def get_pow_block(hash: Bytes32) -> PowBlock:
return PowBlock(block_hash=hash, is_valid=True, is_processed=True,
return PowBlock(block_hash=hash, parent_hash=Bytes32(), is_valid=True, is_processed=True,
total_difficulty=uint256(0), difficulty=uint256(0))


Expand Down
9 changes: 6 additions & 3 deletions specs/merge/fork-choice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class TransitionStore(object):
@dataclass
class PowBlock(object):
block_hash: Hash32
parent_hash: Hash32
is_processed: boolean
is_valid: boolean
total_difficulty: uint256
Expand All @@ -99,9 +100,10 @@ Let `get_pow_block(block_hash: Hash32) -> PowBlock` be the function that given t
Used by fork-choice handler, `on_block`.

```python
def is_valid_terminal_pow_block(transition_store: TransitionStore, block: PowBlock) -> bool:
def is_valid_terminal_pow_block(transition_store: TransitionStore, block: PowBlock, parent: PowBlock) -> bool:
is_total_difficulty_reached = block.total_difficulty >= transition_store.transition_total_difficulty
return block.is_valid and is_total_difficulty_reached
is_parent_total_difficulty_valid = parent.total_difficulty < transition_store.transition_total_difficulty
return block.is_valid and is_total_difficulty_reached and is_parent_total_difficulty_valid
```

## Updated fork-choice handlers
Expand Down Expand Up @@ -130,8 +132,9 @@ def on_block(store: Store, signed_block: SignedBeaconBlock, transition_store: Tr
if (transition_store is not None) and is_merge_block(pre_state, block):
# Delay consideration of block until PoW block is processed by the PoW node
pow_block = get_pow_block(block.body.execution_payload.parent_hash)
pow_parent = get_pow_block(pow_block.parent_hash)
assert pow_block.is_processed
assert is_valid_terminal_pow_block(transition_store, pow_block)
assert is_valid_terminal_pow_block(transition_store, pow_block, pow_parent)

# Check the block is valid and compute the post-state
state = pre_state.copy()
Expand Down
3 changes: 2 additions & 1 deletion specs/merge/validator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if not is_valid_terminal_pow_block(transition_store, pow_block):
pow_parent = get_pow_block(pow_block.parent_hash)
if not is_valid_terminal_pow_block(transition_store, pow_block, pow_parent):
# Pre-merge, empty payload
return ExecutionPayload()
else:
Expand Down