Skip to content

core: fix wrong refundGas when gasUsed < floorDataGas #31682

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

core: fix wrong refundGas when gasUsed < floorDataGas #31682

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/state_transition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ func (st *stateTransition) execute() (*ExecutionResult, error) {
if rules.IsPrague {
// After EIP-7623: Data-heavy transactions pay the floor gas.
if st.gasUsed() < floorDataGas {
gasRefund = 0
Copy link
Member

@rjl493456442 rjl493456442 Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably not correct to blindly reset the refund counter to zero.

e.g., the used gas is 100, refund is 10, floor is 95;

the used gas will be reset to 95.

If (usedGas:95, refund: 0) is returned to gas estimator, then the peak gas usage will be (95 + params.CallStipend) * 64 / 63 which is lower than (100 + params.CallStipend) * 64 / 63.

Without sufficient gas allowance, the evm execution will be aborted due to oog at some point. And the gas estimation might be affected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about your example, it won't be triggered as 100 > 95. Say we have:

  • initialGas = 200
  • gasRemaining = 20
  • gasRefund = 10

As gasRefund is added before the gasUsed check, we have:

  • gasRemaining = 20 + 10 = 30
  • gasUsed = initialGas - gasRemaining = 170

Say floor is 180, then 170 < 180 so no refund should be reported:

  • gasRemaining = initialGas - floor = 200 - 180 = 20
  • gasRefund = 0
  • gasUsed = 200 - 20 = 180 = floor

effectively paying for the floor cost as well as returning the remaining up to initialGas. If we don't reset refundGas to 0, then it will still be 10, which breaks the gas accounting, that is, 180 gas used + 20 gas remaining + 10 gas refund = 210 > 200 initial gas

Copy link
Member

Choose a reason for hiding this comment

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

twist your example a bit,

  • initialGas = 200
  • gasRemaining = 20
  • gasRefund = 10

As gasRefund is added before the gasUsed check, we have:

  • gasRemaining = 20 + 10 = 30
  • gasUsed = initialGas - gasRemaining = 170

Say floor is 175, then 170 < 175

  • gasRemaining = initialGas - floor = 200 - 175 = 25
  • gasUsed = floor

if we return gasUsed: 175, refund: 0, then the peak gas usage is interpreted as 175, rather than 180.
This peak gas usage is essential to accurately estimate the gas usage, also the reason why we do the
binary search instead of running the eth_call for one time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then, do we let it as is, even when other clients do it differently? I'm not quite sure what to do here, or what the possible fix would be.

prev := st.gasRemaining
st.gasRemaining = st.initialGas - floorDataGas
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR, but I'm trying to figure out why we don't also subtract the base cost (21000) here.

if t := st.evm.Config.Tracer; t != nil && t.OnGasChange != nil {
Expand Down