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

Add EIP: Uncouple blob count between CL and EL #8735

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

ralexstokes
Copy link
Member

ATTENTION: ERC-RELATED PULL REQUESTS NOW OCCUR IN ETHEREUM/ERCS

--

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@eth-bot
Copy link
Collaborator

eth-bot commented Jul 12, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Jul 12, 2024
@eth-bot eth-bot changed the title Add EIP to uncouple blob counts between CL and EL Add EIP: Uncouple blob count between CL and EL Jul 12, 2024
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 12, 2024
Copy link

The commit 965c047 (as a parent of e4a8338) contains errors.
Please inspect the Run Summary for details.

EIPS/eip-7733.md Outdated Show resolved Hide resolved
EIPS/eip-7733.md Outdated
|--- |--- |
| `FORK_TIMESTAMP` | TBD |

### Background
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this should be moved to Motivation or partially to Rationale

EIPS/eip-7733.md Outdated

Even if the EL keeps a fixed target value based on the max, removing the max implies the EL would not know what the target
value should be. To address this lack of information, this EIP proposes the CL sends the current target value to the EL
with each provided payload over the Engine API. The EL block header will also need to be extended with this target value
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS, target_blob_gas_per_block is used to compute excess_blob_gas. Would it be possible to send the target as a part of PayloadAttributes so the EL can compute the excess and make CL validate that the excess is computed correctly given the target and parent’s excess?

Copy link
Member Author

@ralexstokes ralexstokes Jul 15, 2024

Choose a reason for hiding this comment

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

the CL part of this change is here: ethereum/consensus-specs#3800 where the target is sent w/ the payload attributes

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, i wasn’t clear enough on the CL validation part, it seems to be possible to me to not include target_blob_gas_per_block into EL block by doing the following:

  1. Send this parameter to payload build process so EL can correctly compute excess_blob_gas
  2. Make CL validate that the computation was done correctly by utilising a simple math: assert payload.excess_blob_gas == min(0, state.latest_execution_payload_header.excess_blob_gas + target_blob_gas_per_block - blob_gas_used)

Copy link
Member Author

Choose a reason for hiding this comment

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

would you still commit the target_blob_gas in the EL header?

it seems to break the ability for the EL to independently verify the excess_blob_gas (and by extension the blob base fee) if it doesn't have access to the target value

if we keep the header extension then your change is just moving this check from the EL to the CL, which is fine in principle although I could argue violates the separation of concerns of "blob (max) limit check on the CL for DoS reasons" and "blob fee accounting on the EL"

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems to break the ability for the EL to independently verify the excess_blob_gas (and by extension the blob base fee) if it doesn't have access to the target value

I’d argue that EL doesn’t need to have this ability as long as CL is responsible for verifying the excess_blob_gas computation. If we put it this way then we have no change to the EL block structure with a bit of cross-layer logic duplication: EL and CL will have to compute excess_blob_gas. But I think this duplication is fine as the excess_blob_gas formula is unlikely to change.

@ralexstokes ralexstokes changed the title Add EIP: Uncouple blob count between CL and EL EIP-7742: Uncouple blob count between CL and EL Jul 15, 2024
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jul 15, 2024
@eth-bot eth-bot changed the title EIP-7742: Uncouple blob count between CL and EL Add EIP: Uncouple blob count between CL and EL Jul 15, 2024
@ralexstokes ralexstokes changed the title Add EIP: Uncouple blob count between CL and EL EIP-7742: Uncouple blob count between CL and EL Jul 15, 2024
@eth-bot eth-bot changed the title EIP-7742: Uncouple blob count between CL and EL Add EIP: Uncouple blob count between CL and EL Jul 15, 2024
@eth-bot eth-bot enabled auto-merge (squash) July 15, 2024 17:04
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit c02b4d2 into ethereum:master Jul 15, 2024
27 of 30 checks passed
@ralexstokes ralexstokes deleted the blob-uncoupling branch July 15, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e-consensus Waiting on editor consensus e-review Waiting on editor to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants