-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Update EIP-4844: Remove system contract concept #5353
Conversation
All tests passed; auto-merging...(pass) eip-4844.md
|
The commit 3c5e72b (as a parent of 85fc2d3) contains errors. Please inspect the Run Summary for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting that we'd add a blob_basefee
field, not a total blob_txs
counter. Since the counter does not capture the fee-adjustment result of previous history, only the total number of blobs.
In EIP-1559 we weigh recent history higher than older history, but now it's basically averaging the usage all-time, which is not the same as the exponential adjustment we have in EIP-1559; adjustments will be smaller and smaller the longer the fork is active.
And the ExecutionPayloadHeader
and ExecutionPayload
type in the CL spec will need to match the EL definition of the block.
After re-reading the EIP, and the
EIP-1559 responds based on the distribution of blobs throughout history, whereas the current EIP draft handles excess blobs over all time, making excess blobs have long-term effects on the price. With these effects, we get two unfortunate side-effects:
Also, the EIP describes " |
I think that if we had gone with an EIP 1559 based on excess (so what this EIP does) from the start, no one would seriously consider adding a weird rule where you penalize excess more if it's spread out across blocks than if it's concentrated in a single block (which is what status quo EIP 1559 gives us in practice). The complexity is just not worth it, and the value of having a clean simple invariant of what total usage is targeted is really high. More concretely, the value that the clean invariant of the excess-based rule gives us includes:
|
In favor of less complexity, let's go ahead with this PR. Pruning/batching incentives are interesting to me, but don't have to block this PR, and may just make it worse. The change from |
EIPS/eip-4844.md
Outdated
```python | ||
def calc_excess_blob_txs(parent: Header, blobs: int) -> int: | ||
adjusted = parent.blob_txs + blobs | ||
return adjusted - min(TARGET_BLOB_TXS_PER_BLOCK, adjusted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is TARGET_BLOB_TXS_PER_BLOCK
the same as TARGET_BLOBS_PER_BLOCK
constant? If that's the case, can we define only one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the EIP and this Draft I notice the above.
@0xGabi thanks for the review. I was previously considering renaming the constant but decided against it in this PR. |
998a512
to
d2e558c
Compare
Co-authored-by: vbuterin <v@buterin.com> Co-authored-by: lightclient <lightclient@protonmail.com>
8a7c759
to
235b2e2
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from an inplementer's POV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more small recommended changes, then this should be good to merge.
ad20e07
to
9b22385
Compare
Co-authored-by: Ansgar Dietrichs <adietrichs@gmail.com> Co-authored-by: lightclient <lightclient@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -290,8 +328,8 @@ For early draft implementations, we simply change `get_blob_gas(pre_state)` to a | |||
### Gas price update rule (Full version) | |||
|
|||
We propose a simple independent EIP-1559-style targeting rule to compute the gas cost of the transaction. | |||
We use the `TOTAL_BLOB_TXS_STORAGE_SLOT` storage slot of the `SYSTEM_STATE_ADDRESS` address | |||
to store persistent data needed to compute the cost. | |||
We use the `blobs` header field to store persistent data needed to compute the cost. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this blobs
here excess_blobs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in #5629, thanks for catching
* remove system contract concept * store excess blob count in header instead of adjusted-total Co-authored-by: vbuterin <v@buterin.com> Co-authored-by: lightclient <lightclient@protonmail.com> * revert previous name change * update again * should account for blobs, not blob txs * fix * apply review feedback Co-authored-by: Ansgar Dietrichs <adietrichs@gmail.com> Co-authored-by: lightclient <lightclient@protonmail.com> Co-authored-by: vbuterin <v@buterin.com> Co-authored-by: Ansgar Dietrichs <adietrichs@gmail.com>
This PR removes the "system contract" concept from EIP-4844. Although the approach has several pros, it tends to be a sticking point in the discussions around EIP-4844 due to the fact that we don't currently have the concept of a system contract with storage and that concept would need to be implemented into clients - whereas the header extension approach has already been performed to add
baseFee
to the header in EIP-1559.Additionally, it resolves an issue in the original draft that conflated blobs and blob txs.
Will leave in draft until some consensus among authors is reached.