core/vm: Simplify error handling in interpreter loop #23952
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evmone benchmark suite with go 1.16 (obsolete). |
chfast
commented
Nov 22, 2021
core/vm/instructions.go
Outdated
| return res, nil | ||
| } | ||
| interpreter.returnData = nil |
There was a problem hiding this comment.
In general I'm not happy how returnData is handled after these changes, but wanted to see big impact all changes have. Also please note that returning []byte from instructions is not needed any more.
There was a problem hiding this comment.
In CREATE and CREATE2 the res is either returned bytecode or revert return data, depending on the error. So we need handle ErrExecutionReverted separately. But I think it would be better to reset returnData = nil before evm.Create() and only assign it in the end in case of ErrExecutionReverted.
I can also move the returnData change to separate PR.
There was a problem hiding this comment.
I left it with some better comments.
chfast
commented
Nov 22, 2021
axic
reviewed
Nov 22, 2021
core/vm/errors.go
Outdated
| @@ -36,6 +36,7 @@ var ( | |||
| ErrGasUintOverflow = errors.New("gas uint64 overflow") | |||
| ErrInvalidCode = errors.New("invalid code: must not begin with 0xef") | |||
| ErrNonceUintOverflow = errors.New("nonce uint64 overflow") | |||
| errStopToken = errors.New("stop token") | |||
There was a problem hiding this comment.
Lowercase names are not visible outside of a go package
There was a problem hiding this comment.
I think it's ok. Perhaps it should be changed like this
Suggested change
| errStopToken = errors.New("stop token") | |
| // errStopToken is an internal stop token which is never returned to outside callers. | |
| errStopToken = errors.New("stop token") |
|
So I built this branch and synced goerli from block 5,416,168 to the current head (5900905) without seeing an issue |
chfast
force-pushed
the
evm_loop_err
branch
2 times, most recently
from
0bc56c9
to
782e923
Compare
holiman
reviewed
Nov 24, 2021
| } | ||
| return nil, nil | ||
|
|
||
| if err == errStopToken { |
There was a problem hiding this comment.
more go-idiomatic
Suggested change
| if err == errStopToken { | |
| if errors.Is(err, errStopToken) { |
But maybe your variant is more performant?
There was a problem hiding this comment.
The errors.Is() is not inlined (https://godbolt.org/z/n81qdKTzn), but this does not matter because this check is outside of the loop.
There was a problem hiding this comment.
My slight preference is to leave == because of other usages in EVM, e.g. suberr == ErrCodeStoreOutOfGas.
There was a problem hiding this comment.
There is no need to use errors.Is here. The error is purely internal, and this check is performance-critical. errors.Is is appropriate when dealing with unknown and potentially nested errors.
|
Gave this a test on the existing benchmarks. I had expected it to have a bit of impact on |
evmone benchmark suite on go 1.17 |
|
Yes, more like it! |
chfast
commented
Nov 24, 2021
| return res, nil | ||
| } | ||
| interpreter.returnData = nil // clear dirty return data buffer |
There was a problem hiding this comment.
This is not covered by tests. Reported in ethereum/tests#993.
core/vm/errors.go
Outdated
| @@ -36,6 +36,7 @@ var ( | |||
| ErrGasUintOverflow = errors.New("gas uint64 overflow") | |||
| ErrInvalidCode = errors.New("invalid code: must not begin with 0xef") | |||
| ErrNonceUintOverflow = errors.New("nonce uint64 overflow") | |||
| errStopToken = errors.New("stop token") | |||
It would be nice to have some other hardware to confirm the numbers. |
|
|
JacekGlen
pushed a commit
to JacekGlen/go-ethereum
that referenced
this pull request
May 26, 2022
* core/vm: break loop on any error * core/vm: move ErrExecutionReverted to opRevert() * core/vm: use "stop token" to stop the loop * core/vm: unconditionally pc++ in the loop * core/vm: set return data in instruction impls
yperbasis
added a commit
to erigontech/erigon
that referenced
this pull request
Dec 21, 2022
Cherry-pick ethereum/go-ethereum#23952 Co-authored-by: Paweł Bylica <chfast@gmail.com>
This was referenced Oct 11, 2023
19 tasks
gzliudan
pushed a commit
to gzliudan/XDPoSChain
that referenced
this pull request
Mar 1, 2024
* core/vm: break loop on any error * core/vm: move ErrExecutionReverted to opRevert() * core/vm: use "stop token" to stop the loop * core/vm: unconditionally pc++ in the loop * core/vm: set return data in instruction impls
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simplifies interpreter loop exit condition to just
err != nil. This catches:REVERT),errStopToken).Also
operation.jumpscheck is removed by settingpc - 1inside the jump instruction and doingpc++in the loop unconditionally.Finally, instructions manipulating return data are handling this themselves. This eliminates another
operation.returnscheck.