all: improve EstimateGas API #20830
all: improve EstimateGas API #20830
Conversation
rjl493456442
requested review from
gballet,
holiman,
karalabe and
zsfelfoldi
as code owners
core/error.go
Outdated
| ErrInsufficientBalanceForFee = errors.New("insufficient balance to pay fee") | ||
|
|
||
| // ErrGasOverflow is returned when calculating gas usage. | ||
| ErrGasOverflow = errors.New("gas overflow") |
There was a problem hiding this comment.
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
ErrGasUintOverflowandErrGasOverflow? Seems like it would be easy to mix those two up, and I'm not sure what (if any) the consequence would be?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
|
Overall a really nice change, good job! |
|
Adding a reference, this fixes #20714 |
|
I've now started benchmrks for
from block ~7M. papertrail startup logs: |
rjl493456442
force-pushed
the
evm-execution-result
branch
from
db1ef6e to
ef81461
Compare
|
@MariusVanDerWijden Please take another look |
| @@ -882,6 +882,23 @@ func (s *PublicBlockChainAPI) Call(ctx context.Context, args CallArgs, blockNrOr | |||
| return result.Return(), nil | |||
| } | |||
|
|
|||
| type estimateGasError struct { | |||
There was a problem hiding this comment.
I think this error type should be exported, s.th. users can use it to cast an error into it and read the fields
There was a problem hiding this comment.
These are internal packages, they can't be imported either way.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Sync from ~7M to head done, see #20761 (comment) |
|
Sorry @rjl493456442 , my PR caused a conflict, so it needs a rebase now |
rjl493456442
force-pushed
the
evm-execution-result
branch
from
ef81461 to
59e545a
Compare
internal/ethapi/api.go
Outdated
| if len(result.Revert()) > 0 { | ||
| ret, err := abi.UnpackRevert(result.Revert()) | ||
| if err != nil { | ||
| revert = "0x" + common.Bytes2Hex(result.Revert()) |
There was a problem hiding this comment.
Please use hexutil, trying to get rid of the common byte hex converters.
| @@ -75,6 +68,41 @@ type Message interface { | |||
| Data() []byte | |||
| } | |||
|
|
|||
| // ExecutionResult includes all output after executing given evm | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| if len(result.Revert()) > 0 { | ||
| ret, err := abi.UnpackRevert(result.Revert()) | ||
| if err != nil { | ||
| errMsg += fmt.Sprintf(" (0x%x)", result.Revert()) |
There was a problem hiding this comment.
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
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
EstimateGasAPI to return the concreterevert reason if the transaction always fails