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

Implement Pectra Eip-7623 #2634

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Implement Pectra Eip-7623 #2634

wants to merge 11 commits into from

Conversation

bzawisto
Copy link
Contributor

@bzawisto bzawisto commented Jan 3, 2025

See: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7623.md
This has become a part of the devnet-5.

@bzawisto bzawisto marked this pull request as draft January 3, 2025 11:15
@bzawisto bzawisto added the pectra The Prague/Electra protocol upgrade label Jan 3, 2025
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

The pectra-devnet-5 tests has a bug where they apply the final floor cost adjustment before applying gas refunds while it should be done before.

if (rev >= EVMC_PRAGUE) {
// EIP-7623: Increase calldata cost
const intx::uint128 floor_cost = fee::kGTransaction + (zero_bytes + non_zero_bytes * 4) * fee::kTotalCostFloorPerToken;
gas = std::max(gas, floor_cost);
Copy link
Member

Choose a reason for hiding this comment

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

This is not exactly correct. The floor cost is not a part of intrinsic cost. However, the transaction gas limit must be big enough to cover the floor cost just in case.

After the transaction execution you need another modification: if the gas used by the transaction is lower than the floor cost, bump it to the floor cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pectra The Prague/Electra protocol upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants