all: improve EstimateGas API by rjl493456442 · Pull Request #20830 · ethereum/go-ethereum

rjl493456442 · 2020-03-28T04:46:02Z

There are actually two types of error will be returned when
a tranaction/message call is executed: (a) consensus error
(b) evm internal error. The former should be converted to
a consensus issue, e.g. The sender doesn't enough asset to
purchase the gas it specifies. The latter is allowed since
evm itself is a blackbox and internal error is allowed to happen.

This PR emphasizes the difference by introducing a executionResult
structure. The evm error is embedded inside. So if any error
returned, it indicates consensus issue happens.

And also this PR improve the EstimateGas API to return the concrete
revert reason if the transaction always fails

holiman · 2020-03-28T20:45:24Z

I get a bit confused, because at another place (core/vm/errors.go) you have also defined ErrGasUintOverflow. So I guess two questions:

Why are some vm errors defined here, and some in core/vm/errors? What is the difference between the two categories?

Why do we have both ErrGasUintOverflow and ErrGasOverflow? Seems like it would be easy to mix those two up, and I'm not sure what (if any) the consequence would be?

The errors defined here is for transaction pre-checking. If any error returned, it can lead to "BAD BLOCK".

The errors defined in core/vm/errors is for evm internal error. It's allowed behavior and will eventually be packed into ExecutionResult.

The former refers to some critical issues, the latter refers to evm execution status.
So it would be better to define them separately for better distinguish.

I think the separation is a good idea, but maybe it could be made more obvious, for example if the errors in core/vm were called EvmErrStackOverflow instead of ErrStackOverflow ?
I'm not sure..

holiman · 2020-03-28T20:54:45Z

Overall a really nice change, good job!

holiman · 2020-04-01T07:52:18Z

Adding a reference, this fixes #20714

holiman · 2020-04-01T13:16:25Z

I've now started benchmrks for

core/vm: use a callcontext struct #20761 on bench01 and
all: improve EstimateGas API #20830 on bench02

from block ~7M.
The benchmarks are not not particularly to intended to benchmark speeds, more to sanity-check that they can chug through a couple of million blocks
without any faults.

ansible-playbook configure_bench01-02.yaml -t wipe,reinit,prep,bench  \
   -e "s3file={{ block7M }}" \
   -e "img_a=holiman/geth-experimental:latest" \
   -e "img_b=holiman/geth-gary:latest" \
   -e "syncmode=full"

papertrail startup logs:

Apr 01 15:14:17 bench01.ethdevops.io geth INFO [04-01|13:14:16.962] Starting peer-to-peer node instance=Geth/v1.9.12-unstable-4d236fb1-20200327/linux-amd64/go1.13.9
Apr 01 15:14:21 bench02.ethdevops.io geth INFO [04-01|13:14:21.696] Starting peer-to-peer node instance=Geth/v1.9.13-unstable-db1ef6e7-20200331/linux-amd64/go1.13.9

holiman

LGTM

rjl493456442 · 2020-04-06T02:47:33Z

@MariusVanDerWijden Please take another look

MariusVanDerWijden · 2020-04-06T08:30:07Z

I think this error type should be exported, s.th. users can use it to cast an error into it and read the fields

These are internal packages, they can't be imported either way.

I know, but I think this error should be moved to an external package (abi or bind) s.th. users can read the revert reason without having to parse the error string

This error is mostly for the API(as Felix required, ErrorCode should also be implemented). The internal shouldn't call this function directly since it's for API.

holiman · 2020-04-07T07:11:08Z

Sync from ~7M to head done, see #20761 (comment)

holiman · 2020-04-07T12:38:11Z

Sorry @rjl493456442 , my PR caused a conflict, so it needs a rebase now

karalabe · 2020-04-20T13:35:54Z

Please use hexutil, trying to get rid of the common byte hex converters.

karalabe · 2020-04-20T13:37:09Z

Wondering if we should move this to vm? Seems a bit clearer to type vm.ExecutionResult vs. core.EvexutionResult. Is there some dependency loop that would make this undoable?

The reason to put this definition here is: after raw evm execution(we get the ret and vmerr), we still need to apply some additional rules(e.g. refund gas), so the usedGas field has to be updated in the core scope.

Btw now all my VPN nodes are blocked :P. Now I can't login my discord account(also the reason for missing standup). Sorry about it.

karalabe · 2020-04-20T13:38:44Z

I think %#x does the prefixing so you don't have to explicitly specify 0x

There are actually two types of error will be returned when a tranaction/message call is executed: (a) consensus error (b) evm internal error. The former should be converted to a consensus issue, e.g. The sender doesn't enough asset to purchase the gas it specifies. The latter is allowed since evm itself is a blackbox and internal error is allowed to happen. This PR emphasizes the difference by introducing a executionResult structure. The evm error is embedded inside. So if any error returned, it indicates consensus issue happens. And also this PR improve the `EstimateGas` API to return the concrete revert reason if the transaction always fails

rjl493456442 requested review from gballet, holiman, karalabe and zsfelfoldi as code owners March 28, 2020 04:46

holiman reviewed Mar 28, 2020

View reviewed changes

holiman reviewed Mar 30, 2020

View reviewed changes

Comment thread core/vm/errors.go Outdated

Comment thread internal/ethapi/api.go Outdated

holiman mentioned this pull request Apr 1, 2020

core/vm: use a callcontext struct #20761

Merged

holiman approved these changes Apr 3, 2020

View reviewed changes

MariusVanDerWijden reviewed Apr 3, 2020

View reviewed changes

Comment thread accounts/abi/bind/backends/simulated.go Outdated

MariusVanDerWijden approved these changes Apr 3, 2020

View reviewed changes

Comment thread accounts/abi/bind/backends/simulated.go Outdated

rjl493456442 force-pushed the evm-execution-result branch from db1ef6e to ef81461 Compare April 6, 2020 02:46

MariusVanDerWijden reviewed Apr 6, 2020

View reviewed changes

rjl493456442 force-pushed the evm-execution-result branch from ef81461 to 59e545a Compare April 7, 2020 12:42

holiman added the status:triage label Apr 14, 2020

adamschmideg assigned fjl and karalabe Apr 14, 2020

adamschmideg removed the status:triage label Apr 14, 2020

karalabe reviewed Apr 20, 2020

View reviewed changes

karalabe added this to the 1.9.14 milestone Apr 20, 2020

rjl493456442 added 3 commits April 20, 2020 21:53

all: polish

38cbee9

accounts/abi/bind/backends: add tests

e390d4f

JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Nov 28, 2024

all: improve EstimateGas API (ethereum#20830)

5c28d8b

JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Dec 20, 2024

all: improve EstimateGas API (ethereum#20830)

43f3432

gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Dec 21, 2024

all: improve EstimateGas API (ethereum#20830)

7491a7b

JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Dec 23, 2024

all: simplify function TransitionDb (ethereum#20830)

e8dcabb

gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Dec 28, 2024

all: simplify function TransitionDb (ethereum#20830)

48d1f22

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025

accounts/abi: remove function Unpack2 (ethereum#20830)

91f6259

MariusVanDerWijden mentioned this pull request Jan 17, 2026

gas estimation fails for calls that do not revert on internal failures #21746

Open

Conversation

rjl493456442 commented Mar 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

holiman commented Mar 28, 2020

Uh oh!

Uh oh!

Uh oh!

holiman commented Apr 1, 2020

Uh oh!

holiman commented Apr 1, 2020

Uh oh!

holiman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rjl493456442 commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holiman commented Apr 7, 2020

Uh oh!

holiman commented Apr 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karalabe Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rjl493456442 Apr 7, 2020 •

edited

Loading

karalabe Apr 20, 2020 •

edited

Loading