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: EstimateGas should use LatestBlockNumber by default #24363

Merged
merged 3 commits into from
May 11, 2023

Conversation

zhiqiangxu
Copy link
Contributor

We met a problem that if the args TransactionArgs depends on the latest committed block N, PendingBlockNumber is still N-1, which causes the estimation to fail.

LatestBlockNumber is a better default value IMO.

@zhiqiangxu zhiqiangxu closed this Feb 9, 2022
@csquan
Copy link

csquan commented Dec 8, 2022

yes,why this code had not merge into master?

@csquan
Copy link

csquan commented Dec 8, 2022

I think this really problem, should use LatestBlockNumber rather than PendingBlockNumber can avoid error.

@zhiqiangxu zhiqiangxu reopened this Dec 8, 2022
@zhiqiangxu
Copy link
Contributor Author

I think this really problem, should use LatestBlockNumber rather than PendingBlockNumber can avoid error.

Reopened to hear the advice of the official team :)

@csquan
Copy link

csquan commented Dec 8, 2022

if use pendingblocknumber,then in code:
block, err := b.BlockByNumberOrHash(ctx, blockNrOrHash)
you may get error or "block not found".

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!

@holiman holiman added this to the 1.11.7 milestone May 11, 2023
@holiman holiman requested a review from s1na as a code owner May 11, 2023 09:05
@holiman holiman merged commit 0b66d47 into ethereum:master May 11, 2023
2 checks passed
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
…reum#24363)

* EstimateGas should use LatestBlockNumber by default

* graphql: default to use latest for gas estimation

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
antonydenyer pushed a commit to antonydenyer/go-ethereum that referenced this pull request Jul 28, 2023
…reum#24363)

* EstimateGas should use LatestBlockNumber by default

* graphql: default to use latest for gas estimation

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
MoonShiesty pushed a commit to MoonShiesty/go-ethereum that referenced this pull request Aug 30, 2023
…reum#24363)

* EstimateGas should use LatestBlockNumber by default

* graphql: default to use latest for gas estimation

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
DarianShawn pushed a commit to dogechain-lab/dbsc that referenced this pull request Sep 3, 2023
…20)

### Description

upstream PR:
[go-ethereum#24363](ethereum/go-ethereum#24363)

We met a problem that if the `args TransactionArgs` depends on the
latest committed block N, `PendingBlockNumber` is still N-1, which
causes the estimation to fail.

`LatestBlockNumber` is a better default value IMO.


ethereum/go-ethereum#24363 (comment)
:

```
if use pendingblocknumber,then in code:
block, err := b.BlockByNumberOrHash(ctx, blockNrOrHash)
you may get error or "block not found".
```
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…reum#24363)

* EstimateGas should use LatestBlockNumber by default

* graphql: default to use latest for gas estimation

---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

None yet

4 participants