EVMC support #17050

chfast · 2018-06-21T14:00:34Z

The meat is in Go bindings evmc#41, here included as a git submodule in vendor (to be copy&paste later with some vendor magic, but submodule is good for now because the code under review and is going to change).
Here we just use the Go API from evmc package. Some improvements can be made on both sides in the future.
The minus of having clean Go API is that the state tests execution bumped from 19 s to 24 s.
The geth+aleth-interpreter is running on the mainnet as "aleth" on https://ethstats.net.

Usage:

The path to EVMC VM DLL can be provided with EVMC_PATH env var.
The EVMC options can be passed to the VM via EVMC_OPTIONS env var, e.g. EVMC_OPTIONS='debug=on evm2wasm=true'.

chfast · 2018-06-21T15:38:50Z

cc @gballet (I don't have the rights to make you the reviewer).

axic · 2018-06-21T23:24:58Z

core/vm/evmc.go

+	defer C.free(unsafe.Pointer((centrypoint)))
+
+	C.dlerror()
+	createSymbol = C.dlsym(handle, centrypoint)


Cannot this iterate symbols here and find the first starting with evmc_create_?

The dl library does not have any way to iterate over symbols. We'd have to use ELF library on Linux and something equivalent for MACHO on macOS. Possible in the future, but definitely overkill at the moment.

karalabe · 2018-06-25T08:25:47Z

ethstats/ethstats.go

@@ -380,11 +380,12 @@ func (s *Service) login(conn *websocket.Conn) error {
 		network = fmt.Sprintf("%d", infos.Protocols["les"].(*les.NodeInfo).Network)
 		protocol = fmt.Sprintf("les/%d", les.ClientProtocolVersions[0])
 	}
+	node := strings.Replace(infos.Name, "Geth", "Geth+EVMC", 1)


Please don't modify the reported name here. If yu need a different name, update it where the name is created to include any additional info you'd like.

I could not find where the name comes from, but this is not going to stay.

axic · 2018-06-26T16:46:11Z

core/vm/evmc.go:21:10: fatal error: 'evmc/evmc.h' file not found
#include <evmc/evmc.h>
         ^~~~~~~~~~~~~

Don’t seem to have a submodule, does this rely on having evmc installed system wide?

axic · 2018-06-26T16:57:41Z

To compile: changed the include path in core/vm/evmc.go to ../../evmc/include and added evmc as a submodule in the root directory.

axic · 2018-06-26T17:10:58Z

One issue I find with this that the existence of the requested VM is not checked on startup, e.g. geth --vm invalid will not result in a failure until code is executed.

chfast · 2018-06-27T09:29:55Z

To compile: changed the include path in core/vm/evmc.go to ../../evmc/include and added evmc as a submodule in the root directory.

I will pull your change to make it easier for now. The final solution would be to include evmc Go wrapper in ethereum/evmc repo and use that as a Go package here. No git submodule would be needed then.

One issue I find with this that the existence of the requested VM is not checked on startup, e.g. geth --vm invalid will not result in a failure until code is executed.

Yes. Command line arguments in geth are quite flaky and to pass the path to VM you would have to change ~20 places (not all of them are covered at the moment, I added some workarounds). That requires discussion with Go team.

chfast · 2018-07-13T18:19:07Z

Big update! See the top comment.

axic · 2018-07-25T14:40:38Z

Needs rebase after #17093 was merged.

gballet

Please update the code to implement the Interpreter interface, as it has been added especially to support this use case.

gballet · 2018-06-28T13:57:34Z

.gitmodules

@@ -1,3 +1,6 @@
 [submodule "tests"]
 	path = tests/testdata
 	url = https://github.com/ethereum/tests
+[submodule "evmc"]


This creates a strong dependency on an optional component. Since you have the EVMCPathFlag parameter, you can use it to download. Most clients will not use the evmc at this stage, so forcing them to download the code for it is overkill.

EVMC is only the C header plus the Go binding for it, doesn't include any VM implementation. I think it is needed, due it being a compile time dependency?

My bad, the previous comment was very poorly worded: you should use go's own opinionated package system and add the dependency to vendor/vendor.json with the govendor utility.

gballet · 2018-06-28T14:20:24Z

core/vm/evm.go

@@ -122,7 +122,7 @@ func NewEVM(ctx Context, statedb StateDB, chainConfig *params.ChainConfig, vmCon
 		chainRules:  chainConfig.Rules(ctx.BlockNumber),
 	}

-	evm.interpreter = NewInterpreter(evm, vmConfig)
+	evm.interpreter = NewEVMC(evm, vmConfig)


We should make it depend on a switch, like:

if vmConfig.EVMCPath != "" { evm.interpreter = NewEVMC(evm, vmConfig) } else { evm.interpreter = newInterpreter(evm, vmConfig) }

core/vm/evmc.go

gballet · 2018-07-23T12:54:03Z

cmd/utils/flags.go

@@ -355,6 +355,11 @@ var (
 		Name:  "vmdebug",
 		Usage: "Record information useful for VM and contract debugging",
 	}
+	EVMCPathFlag = cli.StringFlag{
+		Name:  "vm",


This should then be --evmc or --evmc-vm, there are other, non-EVMC compliant VMs.

We can make it nicer by allowing options like:

--vm auto --vm interpreter --vm ewasm --vm /path/to/evmc.so

This flag does not work anyway, so I'm removing it for now.

gballet · 2018-07-25T20:44:00Z

core/vm/evm.go

@@ -139,7 +139,7 @@ func NewEVM(ctx Context, statedb StateDB, chainConfig *params.ChainConfig, vmCon
 		interpreters: make([]Interpreter, 1),
 	}

-	evm.interpreters[0] = NewEVMInterpreter(evm, vmConfig)
+	evm.interpreters[0] = NewEVMC(evm, vmConfig)


Since the merge of #17093, you can support other interpreters; this disables the evm interpreter which is not acceptable. What should happen is that if --evmc-vm is passed as an argument, then an EVMC interpreter should be inserted before. Something like:

if cfg.EVMCPath != nil { old := evm.interpreters evm.interpreters = make([]Interpreter, len(evm.interpreters)+1) evm.interpreters[0] = NewEVMC(evm, vmConfig) copy(evm.interpreters[1:], old) }

gballet · 2018-07-25T20:46:13Z

core/vm/evmc.go

+	return evmcInstance
+}
+
+func NewEVMC(env *EVM, cfg Config) *EVMC {


EVMC should implement the Interpreter interface, see https://github.com/ethereum/go-ethereum/blob/master/core/vm/interpreter.go#L48

axic · 2018-07-27T12:40:37Z

core/vm/evmc.go

+			output, gasLeftU, err = env.Call(host.contract, destination, input, gasU, value)
+		}
+	case evmc.DelegateCall:
+		output, gasLeftU, err = env.DelegateCall(host.contract, destination, input, gasU)


Should any of these panic if static is set or is it handled up in the interface?

static is magically handled inside, but this seems to be bad design. Probably should be changed later on to be handled internally in any VM.

chfast · 2018-08-22T09:56:55Z

@gballet I believe I addressed all your comments.

axic · 2018-09-04T15:20:51Z

#17461 could simplify this a bit.

axic · 2018-09-04T15:22:34Z

core/vm/evmc.go

+
+func getRevision(env *EVM) evmc.Revision {
+	n := env.BlockNumber
+	if env.ChainConfig().IsByzantium(n) {


Is there a constantinople setting now in master, which needs to be supported here?

axic · 2018-09-04T15:25:15Z

Please update to EVMC 5.2.0.

@chfast should support for EVMC_REJECTED implemented in this PR or a subsequent PR?

gballet · 2018-07-29T17:38:40Z

core/vm/evmc.go

+}
+
+func (evm *EVMC) CanRun([]byte) bool {
+	return true


There are two issues here:

The EVMC will always try to execute the contract. There could be another interpreter that supports a format that the EVMC doesn't, and the native Go interpreter should be the default one in any case.

In the case of a mixed network that supports EVM+EWASM contracts, you need to have an external switch to be able to execute hera in mode evm1mode=fallback as discussed here, because right now it will always try to execute everything as WASM and that behavior is controlled by something outside of geth, which is horrible usability-wise.

So determining the proper format should be more advanced in any case.

So it should be false and then never run? EVMC is not for ewasm only, I'm using it with C++ EVM interpreter. I believe if an user specifies the execution as geth --vm myvm.so he really wants to use myvm.so.

You should rethink this CanRun() feature.

the feature has been thought through enough, thank you. If you have geth --vm myvm.so you are stuck with only one interpreter, whereas this supports a mixed chain with different formats.

This will be handled with EVMC 6.

then in EVMC-6, and provided the right command line switch, the return value here should be true. Until then, it should check if the format is wasm and, if not, return false.

This would break geth + aleth-interpreter.

Please be more specific. How would that break geth + aleth ?

I need geth + aleth-interpreter for testing. Aleth is EVM and if we check for wasm bytecode here it will be skip and the execution fallback to Go Interpreter.

Anyway, I'm not aiming to merge this PR now. We should definitely wait for EVMC 6 which should be soon.

ok let's do that. In the mean time, here is the PR that implements all the command line switches: #17687

gballet · 2018-07-29T17:40:18Z

core/vm/evm.go

@@ -139,7 +140,11 @@ func NewEVM(ctx Context, statedb StateDB, chainConfig *params.ChainConfig, vmCon
 		interpreters: make([]Interpreter, 1),
 	}

-	evm.interpreters[0] = NewEVMInterpreter(evm, vmConfig)
+	if len(os.Getenv("EVMC_PATH")) != 0 {


Let's use a command line switch instead, env variables are transient and they lead to multi-steps instructions; it's better to have all non-privacy-sensitive configuration in one place.

furthermore, this de-activates the EVM and that's a no go: the EVM needs to remain as a backup. I suggest:

if vmConfig.evmcPath != 0 { evm.interpreters = append(evm.interpreters, NewEVMC(evm)) } evm.interpreters = append(evm.interpreters, NewEVMInterpreter(evm, vmConfig))

Then line 140 should be replaced with

interpreters: []Interpreter{}

What exactly is geth doing with that list?

I tried the command line switch but this is not doable currently (at least not in this PR) because the VMConfig{} is created in 100 places usually with default value just because the it is required to create an EVM.

@axic it lists the interpreters, so as to be able to find the best format for a given executable. This is how you can mix EVM&EWASM contracts.

core/vm/evmc.go

chfast · 2018-09-11T13:46:40Z

Please update to EVMC 5.2.0.

Done.

@chfast should support for EVMC_REJECTED implemented in this PR or a subsequent PR?

Let's wait for capabilities from EVMC 6.

axic · 2018-09-20T21:34:13Z

Please rebase over #17687 (well, master) so that we can support fallback to EVM.

axic · 2018-09-20T21:39:13Z

core/vm/evmc.go

+		defer func() { evm.readOnly = false }()
+	}
+
+	fmt.Printf("depth: %d kind: %d static: %t\n", evm.env.depth, kind, readOnly)


Debug to be removed?

govendor fetch github.com/ethereum/evmc/bindings/go/evmc@=v5.2.0

chfast · 2018-10-21T21:19:56Z

Replaced with #17954

chfast requested review from holiman and karalabe as code owners June 21, 2018 14:00

chfast mentioned this pull request Jun 21, 2018

Preview of EVMJIT integration #3410

Closed

lrettig mentioned this pull request Jun 21, 2018

geth with EVMC ewasm/testnet#36

Closed

3 tasks

chfast force-pushed the evmc branch from a3a31bf to 46d2b32 Compare June 21, 2018 14:38

axic reviewed Jun 21, 2018

View reviewed changes

gballet self-requested a review June 22, 2018 09:06

karalabe reviewed Jun 25, 2018

View reviewed changes

gballet mentioned this pull request Jun 27, 2018

core/vm: support for various interpreters #17093

Merged

chfast force-pushed the evmc branch from 9bc74b1 to edc419d Compare June 27, 2018 15:21

chfast force-pushed the evmc branch 2 times, most recently from 50a58f3 to c425949 Compare July 13, 2018 18:11

chfast mentioned this pull request Jul 25, 2018

Go bindings ethereum/evmc#41

Merged

chfast force-pushed the evmc branch from c425949 to 70f2327 Compare July 25, 2018 17:31

gballet requested changes Jul 25, 2018

View reviewed changes

chfast force-pushed the evmc branch from 70f2327 to c770117 Compare July 26, 2018 14:10

axic reviewed Jul 27, 2018

View reviewed changes

mratsim mentioned this pull request Aug 9, 2018

Message type - match EVMC status-im/nimbus-eth1#61

Closed

chfast force-pushed the evmc branch 3 times, most recently from f8a704d to f1882d1 Compare August 20, 2018 13:04

axic reviewed Sep 4, 2018

View reviewed changes

gballet requested changes Sep 5, 2018

View reviewed changes

chfast force-pushed the evmc branch from 43d364f to 9c38275 Compare September 11, 2018 13:33

chfast force-pushed the evmc branch from 9c38275 to 3f474f6 Compare September 11, 2018 13:55

ghost approved these changes Sep 17, 2018

View reviewed changes

axic reviewed Sep 20, 2018

View reviewed changes

chfast force-pushed the evmc branch from 3f474f6 to 94d7c41 Compare September 21, 2018 20:52

gballet mentioned this pull request Oct 11, 2018

core/vm: tweak the --vm.ewasm= option format #17895

Closed

chfast added 2 commits October 18, 2018 18:26

vendor: Add EVMC 5.2.0

33bf2f9

govendor fetch github.com/ethereum/evmc/bindings/go/evmc@=v5.2.0

core/vm: Add EVMC support

d7fefbb

chfast force-pushed the evmc branch from 94d7c41 to d7fefbb Compare October 18, 2018 16:26

chfast closed this Oct 21, 2018

chfast deleted the evmc branch January 15, 2019 11:06

chfast mentioned this pull request Jul 3, 2019

Support for EVMC #17954

Closed

EVMC support #17050

EVMC support #17050

Conversation

chfast commented Jun 21, 2018 • edited Loading

Usage:

chfast commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jun 26, 2018

axic commented Jun 26, 2018

axic commented Jun 26, 2018

chfast commented Jun 27, 2018

chfast commented Jul 13, 2018

axic commented Jul 25, 2018

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Aug 22, 2018

axic commented Sep 4, 2018

Choose a reason for hiding this comment

axic commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Sep 11, 2018

axic commented Sep 20, 2018

Choose a reason for hiding this comment

chfast commented Oct 21, 2018

chfast commented Jun 21, 2018 •

edited

Loading