-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
core/vm, cmd/evm: implement eof validation #30418
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
core/vm/validate_test.go
Outdated
} | ||
} | ||
|
||
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 |
executedTests atomic.Int32 | ||
passedTests atomic.Int32 | ||
file = ctx.String(refTestFlag.Name) | ||
executedTests int |
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
executedTests.Add(int32(total)) | ||
return err | ||
}) | ||
err := filepath.Walk(file, func(path string, info fs.FileInfo, err error) error { |
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 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:
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".It would have been nice to break it up further,
core/vm
is growing very very large. However, it's not easy to puteof
in a submodule.So what we might do is move the opcodes, and the jumptable definition to a new low-level module. And in
core/vm
keep the jumptable instantiations, so we can pass the actual jumptables toeof
. Theneof
doesn'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]