Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

evm interface overhaul #6744

Closed
debris opened this issue Oct 13, 2017 · 12 comments · Fixed by #9360
Closed

evm interface overhaul #6744

debris opened this issue Oct 13, 2017 · 12 comments · Fixed by #9360
Assignees
Labels
F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust. Q9-epic 🌪 Can only be fixed by John Skeet.
Milestone

Comments

@debris
Copy link
Collaborator

debris commented Oct 13, 2017

Current evm interface has been great for us, but as the ethereum virtual machine gets more complicated, we need to revisit how it's done. We need to introduce changes which improve code readability and not decrease the performance.

assumptions:

  • evm should never do recursive calls
  • executive should not use crossbeam to reset the stack
  • we should get rid of Externalities interface
  • evm should be executed in a loop. the result of the execution should contain state changes, information if execution is done and what should be executed in the next iteration of the loop
  • evm should not use callbacks
  • executive public interface should not be changed
  • builtins should be separated from executive

I think we could target 1.10 with those changes, so 3 months from now.

cc @paritytech/core-devs

@debris debris added F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust. Q9-epic 🌪 Can only be fixed by John Skeet. labels Oct 13, 2017
@debris debris added this to the 1.10 & ... milestone Oct 13, 2017
@tomusdrw
Copy link
Collaborator

Is it also possible to clearly separate builtins from executive?

@debris
Copy link
Collaborator Author

debris commented Oct 13, 2017

@tomusdrw I've added it to the list

@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@5chdn 5chdn modified the milestones: 1.11, 1.12 Mar 1, 2018
@5chdn 5chdn modified the milestones: 1.12, 1.13 Apr 24, 2018
@pepyakin
Copy link
Contributor

I thought that these wasm vm use-cases might be useful to keep in mind if and when performing the overhaul:

For now, the wasm engine performs validation of the wasm code each time before executing a contract. The validation run time depends on the code size. Since it depends only on a module itself and nothing else (i.e pure), it doesn't make sense to perform validation each times. It's sufficient to validate once and cache the result of the validation somewhere. Basically it's just a tri-state: not validated, invalid and valid.

Apart from that, there is an instrumentation (such as gas metering) of the wasm code that is also performed on each run. It's not so pure as validation, since it might depend on the exact algorithms used (i.e you can do metering by counting each instruction or by summing all instructions in a block. Same result but different algos), but IMO still something worthwhile caching.

And, of course, wasm→native compilation: recompiling each time before the execution might be a no-go and definitely needs some sort of caching.

So this boils down to the need of caching some data by the vm: ranging from a single flag to a big byte blob.

@sorpaas
Copy link
Collaborator

sorpaas commented Jun 17, 2018

I would like to take this refactoring, if that's okay.

I think I'll start with "evm should be executed in a loop"/"executive should not use crossbeam to reset the stack" refactoring -- changing CALL/CREATE to trap execution, loop to the next callstack item, and then feed result back in. We probably won't gain any memory improvement in this process, because it's basically just moving callstack from program stack to heap. But after that, we can introduce a "runtime" notion that is global to all current callstack items. This allows us to optimize RETURNDATA buffer to avoid allocating additional buffers per callstack.

Regarding "the result of the execution should contain state changes" -- if we don't write state changes directly to the merkle trie but to a hashmap cache, it gives a significant advantage for parallel transaction execution even without ethereum/EIPs#648. But this will introduce a little bit memory overhead.

(And full disclaimer, the above design is similar to what we have in SputnikVM. 😄)

I'm not sure how we can get rid of Externalities interface. @debris @tomusdrw Would it be possible to provide more backgrounds on that?

@sorpaas sorpaas self-assigned this Jun 17, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Jun 17, 2018

Another refactoring that might be beneficial is to use c style enum #[repr(u8)] for instruction instead of a raw u8. This allows us to type-check match branches.

@sorpaas
Copy link
Collaborator

sorpaas commented Jun 19, 2018

@pepyakin mentioned an issue that if we eliminate recursive calls for vm, it might break some of the wasm usage:

  1. To eliminate recursive calls (and get rid of crossbeam), we would need the vm to be able to "pause" execution, during which it execute subsequent calls and feed result back in.
  2. Currently, we cannot pause execution of wasm. This can be implemented in wasmi (interpreter pauses wasmi-labs/wasmi#85). If we do that, our current implementation will work fine.
  3. However, the issue is that in the future, we may want to switch from using a wasm interpreter to a wasm compiler. The compiler might not support pausing execution. So if we eliminate recursive calls, it might not be "future-proof" -- we may make it really hard when doing the wasm interpreter-to-compiler migration. This might not be good.
  4. Another way to handle this is that we allocate stack dynamically, using something like (https://github.com/alexcrichton/stacker). In this way, we get rid of crossbeam but keep recursive calls. A possible drawback on this is that it's not quite portable -- there're only support for win/linux/macos.

@debris @tomusdrw Do you have any thoughts on this?

@tomusdrw
Copy link
Collaborator

Can't we keep recursive calls for wasm but get rid of them for EVM? It probably means maintaining two different interfaces for the VMs (larger surface for bugs), but might still be worth it to boost EVM perf?

@sorpaas
Copy link
Collaborator

sorpaas commented Jun 19, 2018

Yeah sounds good. I'll try to get the design based on that.

@insipx
Copy link
Contributor

insipx commented Jul 3, 2018

Hey guys,

I'm actually working with the Parity EVM implementation as a library in a project of mine. Was wondering if there was any possibility of adding a public 'step hook' to the VM. This could even be similar to the current trace functionality, but could be used for real-time debugging. IE, instead of executing the entirety of the code, the user has the choice of executing only one instruction at a time.

created an separate issue for this, since I don't think it's very related here: #9035

@tomusdrw
Copy link
Collaborator

tomusdrw commented Jul 5, 2018

@InsidiousMind maybe just block the thread in the tracer?

@sorpaas
Copy link
Collaborator

sorpaas commented Jul 5, 2018

@InsidiousMind @tomusdrw I think there is a way to do that. We basically just need to add an additional "initialize" lifecycle to Interpreter, and then replace the while loop by a step function. With proper inlining the performance shouldn't be of difference too much. (And shameless self-advertising -- SputnikVM can already step individual opcodes as of today. 😄)

I mostly got the design for callstack. What I was planning to do is to add in Ext::call/create an extra parameter can_trap: bool. For EVM, we set can_trap = true, and for WASM we set can_trap = false. With can_trap, the Ext would return a Trap struct error which EVM should propagate outside of Interpreter. Executive will then handle Trap and manage the callstack, and call Interpreter::feed_return_data when it's available.

This would actually require step functionality, which is a prerequisite for VM resume.

@sorpaas
Copy link
Collaborator

sorpaas commented Jul 24, 2018

Just some update on removing crossbeam/recursive stack: This turns out to be a much bigger refactoring than I thought. In addition, I'm having some local commit mess while trying to pull master... So I'm planning to split it and submit smaller PRs. Here's the list of refactoring to be done before we can change executive to be resumable:

  • Make VmFactory creation not dependent on Ext, but only on Schedule and depth.
  • Make Vm::new not fail-able, otherwise the control flow for resumable in Executive would be really complicated.
  • Move VmFactory reference out of State. Having Factories::trie and Factories::accountdb there makes sense, but VmFactory is totally independent on State.
  • Handle ReturnData out of Externalities::ret. Otherwise making reference of call stack to work is really difficult. ( Remove pass-by-reference return data value from executive  #9211)
  • Refactor Tracer. I think we would need to move it so that we only need reference on it after Vm is initialized. That's currently one of the major pain point for the refactoring.

For the actual refactoring on removing crossbeam/recursive stack:

  • Change Vm interface to have an extra Vm::resume, which takes a ResumeValue. On wasm, this will just be unimplemented!().
  • In Externalities, have two other versions of call and create which return Trap instead of directly executing the VM.
  • Split Executive. Have CallCreateExecutive which works on call/create level, and TransactExecutive which works on transaction level. CallCreateExecutive is resumable, and have a consume function (which takes self instead of &mut self) that changes it to non-resumable. TransactExecutive is a wrapper on CallCreateExecutive.

This is as far as where I'm now at right now. We may also need some additional refactorings along the way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F6-refactor 📚 Code needs refactoring. M4-core ⛓ Core client code / Rust. Q9-epic 🌪 Can only be fixed by John Skeet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants