Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor evm and prepare for ewasm integration #424

Closed
wants to merge 5 commits into from
Closed

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Jan 23, 2019

Work in progress:

  • Move evm-related code to subdir lib/vm
  • Introduce class, which aims to implement EEI to facilitate integration of ewasm later on. Currently EEI is not yet compatible with the spec.
  • Use EEI in opFns instead of runState object for most cases
  • Introduce Memory class and use it in memLoad and memStore
  • Limit Memory's size
  • Introduce Stack class
  • Move hardfork logic out of opFns, e.g. whether operation is allowed in current hardfork, or gas price of operations which are specific to HF
  • Make opFns all sync and delegate async fetching of data either to runCode or to EEI
  • Tests
  • ...

@holgerd77
Copy link
Member

Speechless. But...ahm... Cool. 🙂

Really important that you take in the opfns.js file with renaming (currently shown as deleted) for later review.

@s1na
Copy link
Contributor Author

s1na commented Jan 23, 2019

Sorry, I should have first created an issue to discuss before suddenly creating a big PR 😓 I can close it right away if you don't think its a good idea...

Yeah, I'm not sure why github is not showing the diff (and thinks lib/vm/opFns.js is a new file). This would make the review difficult.

@holgerd77
Copy link
Member

No, that's ok, go ahead. Seems to be really targeted on what we want to achieve on the road to ewasm. 😀

@cdetrio
Copy link
Contributor

cdetrio commented Jan 23, 2019

cool!

One issue with ewasm integration is async vs sync around the EEI and js host functions (https://github.com/ewasm/design/blob/master/interface_questions.md#ewasm-interface-methods-synchronous-vs-asynchronous). What's your preferred workaround for that?

@cdetrio
Copy link
Contributor

cdetrio commented Jan 23, 2019

btw, I think another fix for the sync vs async issue would be to rewrite merkle-patricia-tree so that its fully synchronous (no callbacks/async).

@axic
Copy link
Member

axic commented Jan 23, 2019

btw, I think another fix for the sync vs async issue would be to rewrite merkle-patricia-tree so that its fully synchronous (no callbacks/async).

That was one of the original ideas, but seemed even more tedious than doing a wrapper.

@holgerd77
Copy link
Member

I think that's what the guys from Rainblock have done in their fork?

@axic
Copy link
Member

axic commented Jan 23, 2019

Nice, didn't know about rainblock: https://github.com/RainBlock/merkle-patricia-tree

@jwasinger
Copy link
Contributor

Nice, didn't know about rainblock: https://github.com/RainBlock/merkle-patricia-tree

wow that's great.

@s1na
Copy link
Contributor Author

s1na commented Jan 24, 2019

I don't really have a satisfactory workaround in mind 😅 I was thinking of just using some easy (but not so elegant and performant) mechanism at the beginning for turning the async funcs into sync ones, like deasync, and then iterate on that.

Rainblock's sync trie is definitely an interesting idea!

Do you think it'd be possible to use coroutines to switch between the wasm module and the environment interface? E.g. wrap the wasm module and the functions it imports (getAccount) in a coroutine, when one of the functions is called, yield to EEI which calls async func and in the callback yields again to the first coroutine. Not sure it makes sense, but I could try doing a small experiment. (Update: Fibers doesn't work in browser, es6 generators don't allow yielding in callbacks)

@s1na
Copy link
Contributor Author

s1na commented Jan 24, 2019

Here's an example for using deasync.

Update: Doesn't work in browser (when using browserify).

@s1na
Copy link
Contributor Author

s1na commented Jan 25, 2019

Thanks for pointing this out Casey, it sent me down a fascinating rabbit hole (through which I learned a lot)! Here's my current thinking on this:

  1. Precompiles are the initial focus of ewasm (from what I understand), and at least the existing ones don't use operations that are async in js.
  2. First step could be to only support sync operations and trap if the wasm code calls any async api.

After that, ideally wasm-js-api would support async imports but that seems farther away. In the meanwhile (and here I'm only repeating what this doc says):

  1. Track progress on SAB being re-enabled. If it seems likely that it will be re-enabled (which the thread suggests), this could be a good approach.
  2. Experiment warming a sync cache with data that a contract needs by repetitively running the ewasm contract (it's inefficient, but could work for some simpler contracts).
  3. Experiment with using a sync trie. My hesitation for this is that, it would touch a loot of code (all higher layer storage abstractions, incl. ethereumjs-account, stateManager, cache, and anywhere their methods are invoked), or at least I haven't been able to find a solution to limit the necessary code changes. I'm also not sure how it would affect the performance, and whether a sync trie would cause other tasks to starve.

However all I've said are based on my incomplete understanding. I'd love to hear from @axic and @cdetrio if this "plan" makes sense, or what approach you'd prefer.

@axic
Copy link
Member

axic commented Jan 25, 2019

Your assessment is correct, for precompiles only there none of the async issues come into play.

Track progress on SAB being re-enabled.

I personally would avoid any workarounds like SAB, so this is out of scope for me.

Experiment warming a sync cache with data that a contract needs by repetitively running the ewasm contract

This is the approach I think is the best for now. It also works with our hope to do some kind of stateless client prototype, where all the touched accounts have to be supplied prior to execution and in that case everything can be preloaded.

Experiment with using a sync trie.

I feel this approach would only work with a fresh prototype, as you've mentioned it would be a material change in current ethereumjs-vm. Also before changing ethereumjs-vm I would prefer the speed change to be evaluated, whether it is for the better or the worse.

@axic
Copy link
Member

axic commented Jan 31, 2019

@s1na please rebase

@holgerd77
Copy link
Member

@s1na Can we close this here?

@ryanio ryanio deleted the refactor/vm branch October 28, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants