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

eth/gasestimator: include blobs in virtual balance computation #29703

Merged
merged 4 commits into from May 7, 2024

Conversation

nand2
Copy link
Contributor

@nand2 nand2 commented May 3, 2024

Fix for the bug descripted on #29702 -- I no longer see the bug.

I am new to go-ethereum codebase, so I may have missed things, please tell me :-)

Also, would you like some test with it?

@fjl fjl changed the title Fix: eth_estimateGas with blobs fails when capped by limited funds eth/gasestimator: include blobs in virtual balance computation May 6, 2024
if opts.Config.IsCancun(opts.Header.Number, opts.Header.Time) && len(call.BlobHashes) > 0 {
blobBalanceUsage := new(big.Int).SetInt64(int64(len(call.BlobHashes) * params.BlobTxBlobGasPerBlob))
blobBalanceUsage.Mul(blobBalanceUsage, call.BlobGasFeeCap)
available.Sub(available, blobBalanceUsage)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need another check for balance > blobBalanceUsage here, otherwise it could go negative.

Copy link
Contributor Author

@nand2 nand2 May 6, 2024

Choose a reason for hiding this comment

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

You're right! Which one of the following options would you use for the error?

  • Using core.ErrInsufficientFunds as error, keeping insufficient funds for gas * price + value as description,
  • Using core.ErrInsufficientFunds as error, changing description to insufficient funds for gas * price + blobGas * blobGasPrice + value as description,
  • creating and using a new error, ErrEip4844InsufficientFunds with insufficient funds for gas * price + blobGas * blobGasPrice + value as description ?

I guess option 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added option 1/ for now, let me know.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl merged commit d6e91e2 into ethereum:master May 7, 2024
2 of 3 checks passed
@fjl fjl added this to the 1.14.1 milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants