all: remove noop vm config flags #23111
Conversation
|
|
| EWASMInterpreter string | ||
|
|
||
| // Type of the EVM interpreter ("" for default) | ||
| EVMInterpreter string |
There was a problem hiding this comment.
Given that the config file will be rejected if we remove these, I think for now we should keep these and add a doc that they are deprecated/unused. We can remove them in a future PR if Felix changes the toml parser to accept invalid fields (or somehow more gracefully deprecate ones).
core/vm/evm.go
Outdated
| } | ||
| return interpreter.Run(contract, input, readOnly) | ||
| } | ||
| if !evm.interpreter.CanRun(contract.Code) { |
There was a problem hiding this comment.
That CanRun function has been introduced to check that it was WASM code. If we no longer support multiple interpreters, then it should go as well, since in EVM, any bytecode is considered valid.
There was a problem hiding this comment.
I went ahead and deleted the Interpreter interface and the run func defined in evm.go too (in addition to CanRun). But happy to revert these changes and only remove CanRun
|
#23118 is merged, please rebase and make use of it |
ae7124d to
f6c13d7
Compare
|
@holiman rebased and tried it on a synced node, no issue jumped out |
| case "ethconfig.Config.EVMInterpreter": | ||
| return true | ||
| case "ethconfig.Config.EWASMInterpreter": | ||
| return true | ||
| default: | ||
| return false |
There was a problem hiding this comment.
Are you sure this is the 'namespace' they wind up in? I tried a geth --vm.ewasm=foo dumpconfig > foo.conf, and they wound up like this:
[Eth]
EWASMInterpreter = "foo"
EVMInterpreter = ""
There was a problem hiding this comment.
Yeah the name is coming from reflect.Type.Name() of each config struct, which uses the packageName.typeName as its format. This is what I get when I use a similar config as your example:
WARN [07-06|09:28:46.630] Config field is deprecated and won't have an effect name=ethconfig.Config.EVMInterpreter
* all: rm external interpreter and ewasm config * core/vm: rm Interpreter interface * cmd/geth: deprecate interpreter config fields
* all: rm external interpreter and ewasm config * core/vm: rm Interpreter interface * cmd/geth: deprecate interpreter config fields
This reverts commit 5441a8f. # Conflicts: # cmd/utils/flags.go # core/vm/evm.go # eth/ethconfig/gen_config.go # eth/tracers/tracer_test.go # params/config.go
These PRs #17687, #18457, #18084 introduced the configuration for alternative interpreters (e.g. wasm or external evm-c based). They're no-op as of now and the feature PRs that use these config flags, namely #17954 and #16957 have been on hold for a long time and are not being actively pursued AFAIK.
If needed these flags can be re-introduced along with the actual logic at a later point.
I plan to look into whether the
Interpreterinterface implemented here https://github.com/ethereum/go-ethereum/pull/17093/files is useful without these external VMs and remove it otherwise.