core/vm: support for various interpreters #17093
Conversation
|
It is now possible to call contract across interpreters. The solution that I have currently chosen is the naive one: simply have a list of interpreters. There is another possible solution: have one interpreter per EVM but call contracts across EVMs (to be renamed "VM" to differentiate between EVM, EVM-C and eWASM). |
gballet
force-pushed
the
interpreter-as-interface
branch
from
d03339c to
0971414
Compare
|
ping |
core/vm/instructions.go
Outdated
| @@ -41,23 +41,23 @@ func opAdd(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *Stac | |||
| x, y := stack.pop(), stack.peek() | |||
| math.U256(y.Add(x, y)) | |||
|
|
|||
| evm.interpreter.intPool.put(x) | |||
| evm.interpreter.(*InterpreterEVM).intPool.put(x) | |||
There was a problem hiding this comment.
We need to find a way to avoid this type assertion in every instruction, maybe by changing the evm parameter to have type *InterpreterEVM.
core/vm/evm.go
Outdated
| if evm.interpreter == nil { | ||
| evm.interpreter = evm.interpreters[len(evm.interpreters)-1] | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is weird because evm.interpreters is always nil (the EVM is created just a few lines above).
gballet
force-pushed
the
interpreter-as-interface
branch
3 times, most recently
from
9939fc5 to
f685634
Compare
core/vm/evm.go
Outdated
| return evm.interpreter.Run(contract, input) | ||
| for _, interpreter := range evm.interpreters { | ||
| if interpreter.CanRun(contract.Code) { | ||
| if evm.Interpreter() != interpreter { |
There was a problem hiding this comment.
Why method call here? Just access evm.interpreter directly.
core/vm/instructions.go
Outdated
| @@ -37,45 +37,45 @@ var ( | |||
| errMaxCodeSizeExceeded = errors.New("evm: max code size exceeded") | |||
| ) | |||
|
|
|||
| func opAdd(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) { | |||
| func opAdd(pc *uint64, interpreter *InterpreterEVM, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) { | |||
There was a problem hiding this comment.
Imho rename interpreter to evm throughout this file. Much cleaner and shorter code.
There was a problem hiding this comment.
I have an issue with this, because it's an EVMInterpreter and not an EVM. It is shorter but confusing. What about evmi ?
core/vm/instructions.go
Outdated
| offset, size := stack.pop(), stack.pop() | ||
| data := memory.Get(offset.Int64(), size.Int64()) | ||
| hash := crypto.Keccak256(data) | ||
| evm := interpreter.evm |
There was a problem hiding this comment.
Are you using this anywhere? Seems like an unused variable? Does this code compile? ;P
There was a problem hiding this comment.
Ah, you did use it to add the preimages. Please use it to access the vmConfig too.
core/vm/interpreter.go
Outdated
| } | ||
|
|
||
| // InterpreterEVM represents an EVM interpreter | ||
| type InterpreterEVM struct { |
There was a problem hiding this comment.
Please call this EVMInterpreter
core/vm/interpreter.go
Outdated
| // CanRun tells if the contract, passed as an argument, can be | ||
| // run by the current interpreter. | ||
| // The EVM interpreter will try to interpret any binary file and | ||
| // as such, should be tried last. |
There was a problem hiding this comment.
Please remove
+// The EVM interpreter will try to interpret any binary file and
+// as such, should be tried last.
That will be up to a consensus descision on how to enable it, most probably by having an explicit flag in the accounts specifying whether the code is EVM or EWASM.
core/vm/errors.go
Outdated
| @@ -26,4 +26,5 @@ var ( | |||
| ErrTraceLimitReached = errors.New("the number of logs reached the specified limit") | |||
| ErrInsufficientBalance = errors.New("insufficient balance for transfer") | |||
| ErrContractAddressCollision = errors.New("contract address collision") | |||
| ErrNoCompatibleInterpreter = errors.New("could not find an interpreter to run this contract's code") | |||
There was a problem hiding this comment.
errors.New("no compatible interpreter")
No need to make it more verbose than that.
gballet
force-pushed
the
interpreter-as-interface
branch
from
e7db76a to
9635a5c
Compare
- Define an Interpreter interface - One contract can call contracts from other interpreter types. - Pass the interpreter to the operands instead of the evm. This is meant to prevent type assertions in operands.
gballet
force-pushed
the
interpreter-as-interface
branch
from
9635a5c to
d0bd10a
Compare
- Define an Interpreter interface - One contract can call contracts from other interpreter types. - Pass the interpreter to the operands instead of the evm. This is meant to prevent type assertions in operands.
This PR turns
Interpreterinto an interface, so that other interpreters can be chosen. This is intended to support PRs #16957 and #17050.