-
Couldn't load subscription status.
- Fork 21.5k
core/vm, cmd/evm: implement eof validation #30418
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
Conversation
e1feb9c to
6ebbbd9
Compare
6ebbbd9 to
666abfe
Compare
666abfe to
6cfe117
Compare
|
Added benchmarks, based on some of the worst-cases from the consensus-tests + fuzzing vectors. |
a80b89a to
5099626
Compare
|
Sped it up quite a bit, the slowest vector is now improved by a factor of 10 :) |
33cc41e to
8196b46
Compare
| } | ||
| } | ||
|
|
||
| func BenchmarkEOFValidation2(b *testing.B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the benchmark-function in this file appears to be non-functional, should we nuke them or salvage them, @MariusVanDerWijden ?
example
[user@work vm]$ go test . -run - -bench EOFV
--- FAIL: BenchmarkEOFValidation
validate_test.go:353: callf into non-returning section: section 1
--- FAIL: BenchmarkEOFValidation2
validate_test.go:403: callf into non-returning section: section 1
--- FAIL: BenchmarkEOFValidation3
validate_test.go:452: callf into non-returning section: section 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to salvage
50dcc67 to
8783059
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Let's move the new utility commands into cmd/evm. It's better to use an existing command. |
|
Would also be nice to remove some complexity in the validation code by moving statements out of if have, want := currentStackMax, int(metadata[section].outputs)+int(newSection.inputs)-int(newSection.outputs); have != want {should become wantStack := int(metadata[section].outputs)+int(newSection.inputs)-int(newSection.outputs)
if currentStackMax != wantStack {And perhaps we can find a way to reduce conversions. |
9465e6d to
822b345
Compare
Done
Done Also fixed up the |
cmd/evm/eofparse.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filepath.Walk is sequential, there was no need to use atomics here
cmd/evm/eofparse.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, filepath.Walk behaves fine if you point it directly at a file, as opposed to a dir. So no need to special case that, like the code did earlier
71c9fdd to
7638f64
Compare
cmd/eofdump: benchmarks of eof validation speeds core/vm: move eof instructions to separate file core/vm: unexport fields Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
core/vm: some clarifications in the eof code core/vm: clarifications + minor speedup core/vm: clarifications + lint + minor speedup core/vm, core/asm: support eof in asm instruction iteration core/vm: comment out unused core/vm: remove gasfunctions
…analysis Using the immediates-map in jumpdest analysis leads to less error prone code (only one place to update it) and it improves the worst case from ``` BenchmarkJumpdestOpEOFAnalysis/EOFCREATE-24 724 1471215 ns/op 835.23 MB/s ``` to ``` BenchmarkJumpdestOpEOFAnalysis/EOFCREATE-24 1710 693609 ns/op 1771.60 MB/s ``` which is a quite significant 2x improvement and brings them in line with the average case
…sts, last fix Changes include: - unexporting errors, removing error-to-code mapping - new test vectors from execution-spec-tests v [email protected] - remaining fix from @shemnon in MariusVanDerWijden#56 - core/vm: address review-comments - simplify code - cmd/evm: move eofdump/eofparse into `evm` binary - Also makes eofdump read from stdin if hex is not provided, and makes the output of eofdump a bit more eye-friendly. - core/vm: refactor check in eof control-flow validation - core/vm: refactor some control flow checks for readability
7638f64 to
03d36ac
Compare
The bulk of this PR is authored by @lightclient , in the original EOF-work. More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in #29518, and also @shemnon has contributed with fixes. This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff. In this PR: - [x] Validation of eof containers, lifted from #29518, along with test-vectors from consensus-tests and fuzzing, to ensure that the move did not lose any functionality. - [x] Definition of eof opcodes, which is a prerequisite for validation - [x] Addition of `undefined` to a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined". - [x] benchmarks of eof validation speed --------- Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
The bulk of this PR is authored by @lightclient , in the original EOF-work. More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes. This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff. In this PR: - [x] Validation of eof containers, lifted from ethereum#29518, along with test-vectors from consensus-tests and fuzzing, to ensure that the move did not lose any functionality. - [x] Definition of eof opcodes, which is a prerequisite for validation - [x] Addition of `undefined` to a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined". - [x] benchmarks of eof validation speed --------- Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
The bulk of this PR is authored by @lightclient , in the original EOF-work. More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes. This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff. In this PR: - [x] Validation of eof containers, lifted from ethereum#29518, along with test-vectors from consensus-tests and fuzzing, to ensure that the move did not lose any functionality. - [x] Definition of eof opcodes, which is a prerequisite for validation - [x] Addition of `undefined` to a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined". - [x] benchmarks of eof validation speed --------- Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
The bulk of this PR is authored by @lightclient , in the original EOF-work. More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes. This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff. In this PR: - [x] Validation of eof containers, lifted from ethereum#29518, along with test-vectors from consensus-tests and fuzzing, to ensure that the move did not lose any functionality. - [x] Definition of eof opcodes, which is a prerequisite for validation - [x] Addition of `undefined` to a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined". - [x] benchmarks of eof validation speed --------- Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
The bulk of this PR is authored by @lightclient , in the original EOF-work. More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes. This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff. In this PR: - [x] Validation of eof containers, lifted from ethereum#29518, along with test-vectors from consensus-tests and fuzzing, to ensure that the move did not lose any functionality. - [x] Definition of eof opcodes, which is a prerequisite for validation - [x] Addition of `undefined` to a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined". - [x] benchmarks of eof validation speed --------- Co-authored-by: lightclient <[email protected]> Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Danno Ferrin <[email protected]>
The bulk of this PR is authored by @lightclient , in the original EOF-work.
More recently, the code has been picked up and reworked for the new EOF specification, by @MariusVanDerWijden , in #29518, and also @shemnon has contributed with fixes.
They are the main authors to be credited for the code in this PR.
This PR is an attempt to start eating the elephant one small bite at a time, by selecting only the eof-validation as a standalone piece which can be merged without interfering too much in the core stuff.
In this PR:
undefinedto a jumptable entry item. I'm not super-happy with this, but for the moment it seems the least invasive way to do it. A better way might be to go back and allowing nil-items or nil execute-functions to denote "undefined".It would have been nice to break it up further,
core/vmis growing very very large. However, it's not easy to puteofin a submodule.So what we might do is move the opcodes, and the jumptable definition to a new low-level module. And in
core/vmkeep the jumptable instantiations, so we can pass the actual jumptables toeof. Theneofdoesn't have to importcore/vm, I think.Even with this smaller piece cherry-picked, there's quite a bit of work to do to improve it, better documentation and test coverage. I'll push commits here and rebase quite a bit.
To do in follow-up PR(s) is to modify the interpreter a bit more in-depth, to operate differently in eof-mode versus non-eof-mode.
Co-authored-by: lightclient [email protected]
Co-authored-by: Marius van der Wijden [email protected]
Co-authored-by: Danno Ferrin [email protected]