-
Notifications
You must be signed in to change notification settings - Fork 21k
fix: ErrGasLimitTooHigh conditions for estimateGas and createAccessList #32268
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
base: fusaka-devnet-3
Are you sure you want to change the base?
fix: ErrGasLimitTooHigh conditions for estimateGas and createAccessList #32268
Conversation
…imit estimation to continue estimating with lower caps
eth/ethconfig/config.go
Outdated
@@ -65,7 +65,7 @@ var Defaults = Config{ | |||
Miner: miner.DefaultConfig, | |||
TxPool: legacypool.DefaultConfig, | |||
BlobPool: blobpool.DefaultConfig, | |||
RPCGasCap: 50000000, | |||
RPCGasCap: 30000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxTxGas uint64 = 1 << 24 // Maximum transaction gas limit after eip-7825 (16,777,216).
Please use params.MaxTxGas instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thank you, I think I got mixed up with an older branch! Will fix it:)
@@ -62,6 +62,9 @@ func Estimate(ctx context.Context, call *core.Message, opts *Options, gasCap uin | |||
if call.GasLimit >= params.TxGas { | |||
hi = call.GasLimit | |||
} | |||
if hi >= params.MaxTxGas { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might break L2 projects whose transactions potentially consume more gas in order to amortize L1 costs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have misunderstood the code, but even if the L2s specified gas higher than MaxTxGas
, wouldn't it err with ErrGasLimitTooHigh
and return error for the estimateGas call anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right!
I will ask L2 forks how they plan to integrate the EIP-7825 first, but generally your changes look good to me.
This PR makes 2 changes to how EIP-7825 behaves.
When
eth_estimateGas
oreth_createAccessList
is called without any gas limit in the payload, geth will choose the block's gas limit or theRPCGasCap
, which can be larger than themaxTxGas
.When this happens for
estimateGas
, the gas estimation just errors out and ends, when it should continue doing binary search to find the lowest possible gas limit.This PR will:
Add a check to see if
hi
is larger thanmaxTxGas
and cap it tomaxTxGas
if it's larger. And add a special case handling for gas estimation execute when it errs withErrGasLimitTooHigh
Decrease the
RPCGasCap
to 30M (== maxTxGas) to makecreateAccessList
choose 30M as the highest gas cap for the tx. This can also be fixed by adding a check like this to usemaxTxGas
when the specifiedRPCGasCap
is larger thanmaxTxGas
. I'd be glad to fix it either way!I've created this PR for fusaka-devnet-3 first, and will open another pr to master (or there is some periodic merges to master) if needed.