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 ExecutionPayloadHeader #2463

Closed
wants to merge 1 commit into from
Closed

Remove ExecutionPayloadHeader #2463

wants to merge 1 commit into from

Conversation

JustinDrake
Copy link
Contributor

The complexity around ExecutionPayloadHeader feels unnecessary given that the BeaconState can easily store ExecutionPayload.transactions (the storage overhead is small/tiny). Note that even when the transactions are accompanied by witnesses in the context of statelessness the overhead remains small. More speculatively, it may be valuable down the road for the beacon state to readily have access to the latest transactions.

Heads up: I have a bunch of other cleanups and polishing to this merge doc but want to get this fairly substantive change out first.

The complexity around `ExecutionPayloadHeader` feels unnecessary given that the `BeaconState` can easily store `ExecutionPayload.transactions` (the storage overhead is small/tiny). Note that even when the transactions are accompanied by witnesses in the context of statelessness the overhead remains small. More speculatively, it may be valuable down the road for the beacon state to readily have access to the latest transactions.

*Heads up*: I have a bunch of other cleanups and polishing to this merge doc but want to get this fairly substantive change out first.
@JustinDrake JustinDrake added the Bellatrix CL+EL Merge label Jun 1, 2021
@JustinDrake JustinDrake requested a review from mkalinin June 1, 2021 15:27
Copy link
Contributor

@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.

It might make sense in terms of simplification as we already have BeaconBlock, BeaconBlockHeader, BeaconBlockBody, PowBlockHeader, PowBlockBody. Adding one more unnecessary header might looks like an overhead. Also, execution_payload_header.transactions_root is a different kind of root than pow_block_header.transactions_root which may lead to a sort of confusion.

But I am a bit hesitant of adding unnecessary payload (though, negligible wrt overall state size) to the beacon state. Do we have an estimation on the max size of transactions depending on the gas limit after statelessness got released? 10MB or 100MB for 25M gas limit?

@JustinDrake
Copy link
Contributor Author

Do we have an estimation on the max size of transactions depending on the gas limit after statelessness got released? 10MB or 100MB for 25M gas limit?

Non-zero calldata is 16 gas per byte so the worst-case block size pre-statelessness is (25M gas)/(16 gas/byte) = 1.5MB. My rough conservative heuristic is ~10x overhead for naive Merkle witnesses. The fancier algebraic non-Merkle stateless solutions have way less than 10x overhead thanks to batching. Also the way one maxes out the witness size for a given block is by accessing state, not using call data. So all in all I expect the worst case total size would be lower than 10MB for 25M gas. cc @dankrad

@djrtwo
Copy link
Contributor

djrtwo commented Jun 3, 2021

The main problem I see with this, especially in the context of statelessness (larger blocks), is that this state object changes every slot.

Many client implementations store many states (e.g. 1 epoch worth) in memory and rely heavily on diffs/deduplication due to most of the state (the validator set) remaining almost entirely stable diff to diff.

The fact that this block changes every slot then puts much higher diff each state. And in the event that this is 5 to 10MB during stateless, then you have hundreds of more MB to store in memory if you store an epoch or more of state diffs in memory.

@dankrad
Copy link
Contributor

dankrad commented Jun 3, 2021

In terms of numbers: One 32-byte state access costs ca. 200 bytes using verkle tries, so if we increase costs to roughly 800 gas per access that's around 5 gas/byte or a max of 5 MB at 25M gas.

I find it very strange to put a payload into the state and would be against it. Danny has added good arguments why it might be inefficient.

@JustinDrake JustinDrake closed this Jun 8, 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.

4 participants