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

internal/ethapi: use block GasLimit if gas is nil in DoEstimateGas #29509

Closed
wants to merge 1 commit into from

Conversation

darioush
Copy link
Contributor

Hi. Prior to the refactor made in https://github.com/ethereum/go-ethereum/pull/28600/files, when args.Gas == nil, the block's GasLimit was used as the upper limit for gas estimation: https://github.com/ethereum/go-ethereum/pull/28600/files#diff-c426ecd2f7d247753b9ea8c1cc003f21fa412661c1f967d203d4edf8163da344L1208-L1216

After the mentioned PR, when args.Gas == nil, in case the default set in args.ToMessage (usually globalGasCap) is greater than the block's GasLimit, this can result in an estimation higher than the block GasLimit as it would be overwritten here: https://github.com/ethereum/go-ethereum/pull/28600/files#diff-66f4d55a856f5b04d4c6e1ee99582788b0510add7bd64c7332b948c2aeadbfdfR57-R60

This PR restores the previous behavior by setting the header's GasLimit in args prior to calling CallDefaults or ToMessage. Thanks for your consideration.

@fjl fjl added this to the 1.14.1 milestone Apr 30, 2024
@holiman holiman self-requested a review May 2, 2024 09:05
@karalabe
Copy link
Member

karalabe commented May 2, 2024

The code you linked has the same behavior before and after though.

Original:

	if args.Gas != nil && uint64(*args.Gas) >= params.TxGas {
		hi = uint64(*args.Gas)

I.e. if the input already specified the global gas cap as the cap, then the block capping did not run. So that PR didn't change the behaviour.

@karalabe
Copy link
Member

karalabe commented May 2, 2024

@karalabe
Copy link
Member

karalabe commented May 2, 2024

FWIW, this fix is definitely not good, because it just adds one more hack to undo a bad call. We need to pass the call args to esitmate without setting anything on the gas, like the original code intended.

@karalabe
Copy link
Member

karalabe commented May 2, 2024

Going to close this PR because it's the wrong fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants