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 gas validations from CL #2699

Merged
merged 3 commits into from
Nov 1, 2021
Merged

remove gas validations from CL #2699

merged 3 commits into from
Nov 1, 2021

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Oct 29, 2021

I think that having is_valid_gas_limit violates the separation of layers for zero gain. By hoisting this logic into CL while retaining the validations in EL, we now create two places where specs need to be maintained, client logic built and tested, and for updates to be made in the event of changes.

Sometimes, cross-layers will be unavoidable, but this seems like a place we can and should avoid

I'd also argue that the following can/should be removed as well but I have less of a strong stance because this logic is incredible simple and unlikely to change. That said, there is minimal value in having them in CL.

        assert payload.parent_hash == state.latest_execution_payload_header.block_hash
        assert payload.block_number == state.latest_execution_payload_header.block_number + uint64(1)

Edit: As pointed out by @mkalinin we need to ensure that the EL sub-chain is consistent with the outer CL chain so the parent_hash validations are left in

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Generally agree with removing it. We could put a sanity limit in place, but it might be more trouble for what it's worth (if the block data is not too large, it can be passed to execution engine, and dropped as invalid there).

specs/merge/beacon-chain.md Show resolved Hide resolved
@djrtwo
Copy link
Contributor Author

djrtwo commented Oct 29, 2021

Thanks for the review! Will leave this up a bit for some more eyes before merging

Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

I agree with the arguments. My original intention of bringing on these changes was optimisation for the future stateless world. Apparently this optimisation was premature and now I think that even in a stateless world these checks should be done by EL as they are related to the execution.

The block hash check is very important and we must never remove it. This is what enforces mapping of one block tree on another, i.e. one CL block to one EL block (ok, many-to-one to be strict) instead of many-to-many.

The block number check is enshrined into a block header validity that is validated by EL and is redundant. But it's cheap and it's a kind of axiom that isn't a subject to change. So, we can leave it here or remove it for conformance with proposed change. I don't have a strong opinion in that regard.

The proposal looks good to me. One thing, we should remove the gas_limit check from p2p-interface as well

Copy link
Contributor

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

I think you also can remove GAS_LIMIT_DENOMINATOR and MIN_GAS_LIMIT constants

@djrtwo
Copy link
Contributor Author

djrtwo commented Oct 31, 2021

The block hash check is very important and we must never remove it. This is what enforces mapping of one block tree on another, i.e. one CL block to one EL block (ok, many-to-one to be strict) instead of many-to-many.

If we assume that EL does a consistency check on the fact that a blockchain is formed -- which is even implicit if using the parent_hash it pulls and executes on top of the correct pre-state -- then I don't understand why CL also performing this check that we gain anything?

We are removing gas parent-to-child consistency checks from CL because EL does it.
What is the difference here by removing the parent_hash consistency check if EL does it?

@ralexstokes
Copy link
Member

ralexstokes commented Oct 31, 2021

just going to chime in that i agree w/ the changes in this PR and put my vote in to remove as much as possible.

to that end, i'd support removing the parent hash check and the block number check

(i think the only "coupling" we should aim for will be pulling the execution block's header into the beacon state to facilitate efficient light verification and possibly an affordance to communicate "gas/sec" to the EL, but that is another line of work separate to this PR)

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

left comment above. generally approve!

@djrtwo
Copy link
Contributor Author

djrtwo commented Oct 31, 2021

One thing, we should remove the gas_limit check from p2p-interface as well

Done @mkalinin

you also can remove GAS_LIMIT_DENOMINATOR and MIN_GAS_LIMIT constants

Done @terencechain

Thank you!

@mkalinin
Copy link
Collaborator

mkalinin commented Nov 1, 2021

What is the difference here by removing the parent_hash consistency check if EL does it?

We can defer all checks to EL except for this one. If there is no prent_hash check enforced by CL then CL's blockchain may jump between different EL blockchains. The inconsistency of the block tree versions between the layers makes lock-step sync degenerate as there is no guarantee that EL has all inputs for the child block in the case when the parent one has been imported successfully.

@djrtwo
Copy link
Contributor Author

djrtwo commented Nov 1, 2021

okay, I see what you are getting at. We want CL to know the blockchain datastructure is respected so that we don't accidentally trigger validation of a disjoint chain

CL could point to a disjoint chain and EL would validate from that point and maybe even blindly accept the transition but it was from a different chain

We should leave it in

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.

6 participants