core/vm: add switches to select evm+ewasm interpreters #17687

gballet · 2018-09-17T15:21:32Z

Interpreter initialization is left to the PRs implementing them.

There is a commented-out part that needs to be uncommented in order to initialize the proper interpreter when need be; for instance, in the case of ewasm and assuming that --ewasm.interpreter is evmc:path then the code should be something like:

params := strings.Split(, ":")
switch params[0] {
case "evmc":
  if len(params) != 2 {
  panic("Missing vm path in evmc:path argument to ewasm.interpreter")
  }
  vmPath := params[1]
  evm.interpreters = append(evm.interpreters, NewEVMC(evm, vmPath))
  break;
  ...
}

and the panic should be removed.

chfast · 2018-09-17T15:46:21Z

Is there a way to run tests with an external EVMC VM? E.g. --evm.interpreter evmc:aleth-interpreter.so? Can go test ./tests/... accept custom flags?

chfast · 2018-09-17T15:47:34Z

EVMC supports also custom options. Can will add additional flag, e.g. --evmc.options option1=x;option2=y?

karalabe · 2018-09-17T15:51:37Z

cmd/utils/flags.go

+	EWASMFlag = cli.BoolFlag{
+		Name:  "ewasm",
+		Usage: "Enable eWASM capabilities on this chain",
+	}


This should be defined by a hard fork rule, not a CLI flag.

karalabe · 2018-09-17T15:52:50Z

cmd/utils/flags.go

+	EVMInterpreterFlag = cli.StringFlag{
+		Name:  "evm.interpreter",
+		Usage: "Path to the evm interpreter",
+		Value: "geth",


Please name these --vm.evm and --vm.ewasm. The defaults should be empty, which geth can interpret by running the built in VMs.

Also add (default = built in implementation)

karalabe · 2018-09-17T15:55:38Z

core/vm/interpreter.go

@@ -39,6 +39,13 @@ type Config struct {
 	// may be left uninitialised and will be set to the default
 	// table.
 	JumpTable [256]operation
+
+	// Enable support for EWASM contracts
+	EWASMEnabled bool


Whether ewasm is enabled or not is not a configuration value, it's a chain fork rule.

karalabe · 2018-09-17T15:55:59Z

core/vm/interpreter.go

+	EWASMEnabled bool
+	// Type of the EWASM interpreter ("wagon" or "evmc")
+	EWASMInterpreter string
+	// Type of the EVM interpreter ("geth" or "evmc")


Please get rid of geth and wagon. Empty means built in, non empty means use something else.

karalabe · 2018-09-17T15:56:47Z

params/config.go

@@ -84,16 +84,16 @@ var (
 	//
 	// This configuration is intentionally not using keyed fields to force anyone
 	// adding flags to the config to also have to set these fields.
-	AllEthashProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil}
+	AllEthashProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil, big.NewInt(0)}


Please use nil. big.NewInt(0) enables EWASM at block 0.

karalabe · 2018-09-17T15:56:58Z

params/config.go


 	// AllCliqueProtocolChanges contains every protocol change (EIPs) introduced
 	// and accepted by the Ethereum core developers into the Clique consensus.
 	//
 	// This configuration is intentionally not using keyed fields to force anyone
 	// adding flags to the config to also have to set these fields.
-	AllCliqueProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, nil, &CliqueConfig{Period: 0, Epoch: 30000}}
+	AllCliqueProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, nil, &CliqueConfig{Period: 0, Epoch: 30000}, big.NewInt(0)}


Please use nil. big.NewInt(0) enables EWASM at block 0.

karalabe · 2018-09-17T15:57:22Z

params/config.go


-	TestChainConfig = &ChainConfig{big.NewInt(1), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil}
+	TestChainConfig = &ChainConfig{big.NewInt(1), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil, big.NewInt(0)}


Please use nil. big.NewInt(0) enables EWASM at block 0.

karalabe · 2018-09-17T15:57:47Z

params/config.go

@@ -123,6 +123,9 @@ type ChainConfig struct {
 	// Various consensus engines
 	Ethash *EthashConfig `json:"ethash,omitempty"`
 	Clique *CliqueConfig `json:"clique,omitempty"`
+
+	// EWASM-related switches
+	EWASMBlock *big.Int `json:"ewasmBlock,omitempty"`


Please move this up after the rest of the fork definitions.

params/config.go

karalabe · 2018-09-17T15:59:27Z

params/config.go

@@ -269,6 +277,9 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, head *big.Int) *Confi
 	if isForkIncompatible(c.ConstantinopleBlock, newcfg.ConstantinopleBlock, head) {
 		return newCompatError("Constantinople fork block", c.ConstantinopleBlock, newcfg.ConstantinopleBlock)
 	}
+	if isForkIncompatible(c.EWASMBlock, newcfg.EWASMBlock, head) {
+		return newCompatError("Constantinople fork block", c.EWASMBlock, newcfg.EWASMBlock)


"Constantinople" -> "EWASM"

gballet · 2018-09-17T16:35:53Z

Is there a way to run tests with an external EVMC VM? E.g. --evm.interpreter evmc:aleth-interpreter.so? Can go test ./tests/... accept custom flags?

you can write those tests

EVMC supports also custom options. Can will add additional flag, e.g. --evmc.options option1=x;option2=y?

I would say that's part of the EVMC pr: if you have --vm.evm=path:option1:option2 that should work .

ghost

Can't wait to implement eWASM on mainnet 😂

karalabe · 2018-09-17T19:29:19Z

core/vm/interpreter.go

+
+	// Type of the EWASM interpreter ("wagon" or "evmc")
+	EWASMInterpreter string
+	// Type of the EVM interpreter ("geth" or "evmc")


Please remove "wagon" and "geth"

karalabe · 2018-09-18T08:10:39Z

params/config.go

@@ -119,6 +119,7 @@ type ChainConfig struct {

 	ByzantiumBlock      *big.Int `json:"byzantiumBlock,omitempty"`      // Byzantium switch block (nil = no fork, 0 = already on byzantium)
 	ConstantinopleBlock *big.Int `json:"constantinopleBlock,omitempty"` // Constantinople switch block (nil = no fork, 0 = already activated)
+	EWASMBlock          *big.Int `json:"ewasmBlock,omitempty"`          // EWASM-related switches


Lets keep the comment consistent with the other ones.

// EWASM switch block (nil = no fork, 0 = already activated)

karalabe · 2018-09-18T08:12:47Z

cmd/utils/flags.go

+
+	EWASMInterpreterFlag = cli.StringFlag{
+		Name:  "vm.ewasm",
+		Usage: "Path to the ewasm interpreter",


External EWASM configuration (default = built in interpreter)

karalabe · 2018-09-18T08:12:56Z

cmd/utils/flags.go

+	}
+	EVMInterpreterFlag = cli.StringFlag{
+		Name:  "vm.evm",
+		Usage: "Path to the evm interpreter",


External EVM configuration (default = built in interpreter)

axic · 2018-09-18T09:01:37Z

eth/config.go

@@ -121,6 +121,11 @@ type Config struct {

 	// Miscellaneous options
 	DocRoot string `toml:"-"`
+
+	// Type of the EWASM interpreter ("" for detault, or e.g. "evmc:path:option1:...")


Does this needs to be colon separated or can anything be passed after evmc:path:.... ?

anything can be passed

You can use the same comments as for the flag.

External eWASM configuration and External EVM configuration

I wouldn't mention the config string here yet, since it's not even implemented, so we don't know what it will be. We do know it won't be the one you listed though (no evmc).

axic · 2018-09-19T12:25:10Z

params/config.go

@@ -269,6 +275,9 @@ func (c *ChainConfig) checkCompatible(newcfg *ChainConfig, head *big.Int) *Confi
 	if isForkIncompatible(c.ConstantinopleBlock, newcfg.ConstantinopleBlock, head) {
 		return newCompatError("Constantinople fork block", c.ConstantinopleBlock, newcfg.ConstantinopleBlock)
 	}
+	if isForkIncompatible(c.EWASMBlock, newcfg.EWASMBlock, head) {
+		return newCompatError("eWASM fork block", c.EWASMBlock, newcfg.EWASMBlock)


Can you use ewasm instead of eWASM as that is the current naming style?

@axic is this the current or the definitive naming style? 😄

Current, could change.

But there is agreement eWASM is not the way to go :)

Interpreter initialization is left to the PRs implementing them. Options for external interpreters are passed after a colon in the `--vm.ewasm` and `--vm.evm` switches.

karalabe

LGTM

Interpreter initialization is left to the PRs implementing them. Options for external interpreters are passed after a colon in the `--vm.ewasm` and `--vm.evm` switches.

chfast · 2018-09-21T20:29:15Z

eth/backend.go

@@ -149,7 +149,11 @@ func New(ctx *node.ServiceContext, config *Config) (*Ethereum, error) {
 		rawdb.WriteDatabaseVersion(chainDb, core.BlockChainVersion)
 	}
 	var (
-		vmConfig    = vm.Config{EnablePreimageRecording: config.EnablePreimageRecording}
+		vmConfig = vm.Config{


Sorry, but you missed 64 other places where vm.Config{} is created. I'm not even sure if the new incoming transactions are going to be executed with the selected VM.

@chfast those other location correspond to tests and simulations and we want to use the native interpreter in them. Incoming transactions will be executed by the VM, if you don't believe us check it for yourself.

I believe you. I just remember I had to change this in two places. Maybe it was about tests.

gballet requested review from holiman and karalabe as code owners September 17, 2018 15:21

gballet mentioned this pull request Sep 17, 2018

EVMC support #17050

Closed

karalabe reviewed Sep 17, 2018

View reviewed changes

params/config.go Show resolved Hide resolved

karalabe reviewed Sep 17, 2018

View reviewed changes

ghost approved these changes Sep 17, 2018

View reviewed changes

karalabe reviewed Sep 17, 2018

View reviewed changes

karalabe reviewed Sep 18, 2018

View reviewed changes

axic reviewed Sep 18, 2018

View reviewed changes

karalabe added this to the 1.8.16 milestone Sep 19, 2018

axic reviewed Sep 19, 2018

View reviewed changes

core/vm: add switches to select evm+ewasm interpreters

0c84df5

Interpreter initialization is left to the PRs implementing them. Options for external interpreters are passed after a colon in the `--vm.ewasm` and `--vm.evm` switches.

gballet force-pushed the vm-selector-switches branch from 4e4c89b to 0c84df5 Compare September 19, 2018 13:10

karalabe approved these changes Sep 20, 2018

View reviewed changes

karalabe merged commit da29332 into ethereum:master Sep 20, 2018

chfast reviewed Sep 21, 2018

View reviewed changes

gballet mentioned this pull request Sep 25, 2018

core: ewasm support #16957

Closed

10 tasks

s1na mentioned this pull request Jun 25, 2021

all: remove noop vm config flags #23111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/vm: add switches to select evm+ewasm interpreters #17687

core/vm: add switches to select evm+ewasm interpreters #17687

gballet commented Sep 17, 2018

chfast commented Sep 17, 2018

chfast commented Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

karalabe Sep 17, 2018

gballet commented Sep 17, 2018

ghost left a comment

karalabe Sep 17, 2018

karalabe Sep 19, 2018

karalabe Sep 18, 2018

karalabe Sep 18, 2018

karalabe Sep 18, 2018

axic Sep 18, 2018

gballet Sep 18, 2018

karalabe Sep 19, 2018

axic Sep 19, 2018

gballet Sep 19, 2018

axic Sep 19, 2018

karalabe left a comment

chfast Sep 21, 2018 •

edited

Loading

gballet Sep 23, 2018

chfast Sep 23, 2018


		TestChainConfig = &ChainConfig{big.NewInt(1), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil}
		TestChainConfig = &ChainConfig{big.NewInt(1), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, new(EthashConfig), nil, big.NewInt(0)}

core/vm: add switches to select evm+ewasm interpreters #17687

core/vm: add switches to select evm+ewasm interpreters #17687

Conversation

gballet commented Sep 17, 2018

chfast commented Sep 17, 2018

chfast commented Sep 17, 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

gballet commented Sep 17, 2018

ghost 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

karalabe left a comment

Choose a reason for hiding this comment

chfast Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast Sep 21, 2018 •

edited

Loading