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

Update EIP-4844: add data_gas_used to header #7062

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

lightclient
Copy link
Member

At the moment, EIP-4844 uses the excess_data_gas from a block's parent header in order to determine the data gas price of transactions in the current block. This is a bit different than EIP-1559 which first ensures the header's base_fee is correctly derived from the parent header, then proceeds to use that value for further gas price calculation.

tldr; excess_data_gas is pricing for next block, while base_fee is pricing for current block -- this PR aligns them so that both represent pricing information for the current block

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels May 24, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 24, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title 4844: add data_gas_used to header Update EIP-4844: add data_gas_used to header May 24, 2023
@lightclient
Copy link
Member Author

note, just an idea, wanted to see what it actually looked like to do this

EIPS/eip-4844.md Outdated Show resolved Hide resolved
@roberto-bayardo
Copy link
Contributor

I find this easier to understand. (...I've lost track of how many times I've gotten confused over parent vs current block for gathering appropriate data for blob pricing in the current spec.) Not sure how we trade that off against requiring another header field though.

@lightclient
Copy link
Member Author

Not sure how we trade that off against requiring another header field though.

IMO there is very little difference between adding one additional header field vs. two.

yperbasis
yperbasis previously approved these changes May 26, 2023
Copy link
Member

@yperbasis yperbasis left a comment

Choose a reason for hiding this comment

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

Yes, this is less confusing and error-prone.

@lightclient
Copy link
Member Author

Had to rebase - changes should be the same though.

@lightclient lightclient force-pushed the add-data-gas-used branch 3 times, most recently from a13bda1 to 939170c Compare May 30, 2023 13:56
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

looks good and clear! 👍

g11tech
g11tech previously approved these changes May 31, 2023
adietrichs
adietrichs previously approved these changes Jun 1, 2023
Copy link
Member

@adietrichs adietrichs left a comment

Choose a reason for hiding this comment

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

left some whitespace nitpicks, but LGTM.

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Show resolved Hide resolved
EIPS/eip-4844.md Show resolved Hide resolved
EIPS/eip-4844.md Show resolved Hide resolved
Co-authored-by: Ansgar Dietrichs <adietrichs@gmail.com>
@lightclient
Copy link
Member Author

applied suggestions from @adietrichs, going to undraft now

@lightclient lightclient marked this pull request as ready for review June 1, 2023 14:10
@lightclient lightclient requested a review from eth-bot as a code owner June 1, 2023 14:10
@lightclient lightclient closed this Jun 1, 2023
@lightclient lightclient reopened this Jun 1, 2023
@eth-bot eth-bot enabled auto-merge (squash) June 1, 2023 14:17
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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants