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

Modify Merge Gossip Block Validation Conditions #2621

Merged

Conversation

ethDreamer
Copy link
Contributor

This PR addresses the first validation condition as it is currently written:

  • If the merge is complete with respect to the head state -- i.e. is_merge_complete(state) -- then validate the following:
    • [REJECT] The block's execution payload must be non-empty -- i.e. execution_payload != ExecutionPayload()

As mentioned by @paulhauner in #2618, this condition might cause problems when there are forks in the POW chain at the transition boundary, potentially causing clients to initially reject and fail to propagate blocks that might be ultimately destined to become a part of the canonical chain.

It seems likely that this condition was originally intended to apply to all descendants of the transition block. Therefore it should be validated against the BeaconState associated with the block being imported rather than the head state. If that is true, the validation conditions simplify to this.

@paulhauner
Copy link
Contributor

I think we could just remove that first condition all together, the second condition should cover it alone.

@djrtwo
Copy link
Contributor

djrtwo commented Sep 24, 2021

Meaning, retain "If the execution is enabled for the block"?

Ah, I see what you mean. The next few field conditions actually implicitly validate that the block is non-empty (e.g. execution_payload.timestamp == compute_time_at_slot(state, block.slot))

@djrtwo
Copy link
Contributor

djrtwo commented Sep 24, 2021

I was going to write the following yesterday but didn't click send. Although I think the below is correct, validating non-empty is of dubious value, requiring much more complexity to state correctly.


This is more accurately the "block pre-state" (which would either have a latest_execution_payload_header or not, thus dictating is_merge_complete), right? We need to make sure we don't imply that the state must be transitioned using the block just received to validate this condition.

And what we want to say in text that "if this block or any ancestor of this block turned on execution, then this block needs to be validated against the execution rules".

suggestion below:

- If the execution is enabled for the block or any ancestor of the block -- i.e. `is_execution_enabled(state, block.body)` where `state` is the pre-state of `block` --

@djrtwo
Copy link
Contributor

djrtwo commented Sep 24, 2021

I made the change to just remove to the top condition 👍

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.

3 participants