Migrate opFns to typescript, modify loop/interpreter structure #506

s1na · 2019-05-09T08:45:45Z

Migrates opFns to typescript, which necessitated some changes (that were also suggested by @holgerd77 in V4 tracker #479) in the interaction between Interpreter, Loop and the EEI:
Loop is now greatly simplified: it simply takes a code buffer, goes through it until an exception happens or the code finishes. The opcode handlers and the Loop itself interact with outside through EEI (there are some exceptions which still use stateManager directly, but I suggest we modify them later). The result Loop.run is whether there were any exceptions in running the code.
Interpreter instantiates EEI and passes it to Loop, and also processes the results after each message being executed. After these changes EEI has become more stateful (it stores results, gasLeft and some other stuff locally). But as the name suggests, it should only be an interface, and the logic and state should be moved elsewhere (Interpreter in this case) in future.
Loop logic for detecting when to stop has been changed and everything happens through traps (dropped runState.stopped and checking returnValue).

Now I'm rather happy with Loop (apart from its name!! 😄), but Interpreter and EEI still need changes. E.g. processing results, handling calls and creates, etc.

lgtm-com · 2019-05-09T08:51:06Z

This pull request introduces 1 alert when merging 31e9838 into 37a4de1 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

Comment posted by LGTM.com

holgerd77

Looks good.

holgerd77 · 2019-05-09T09:33:44Z

lib/evm/eei.ts

-      this._runState.gasLeft = new BN(0)
+    this._gasLeft.isub(amount)
+    if (this._gasLeft.ltn(0)) {
+      this._gasLeft = new BN(0)
      trap(ERROR.OUT_OF_GAS)
    }
  }


Doesn't need to be addressed in this PR but still have the impression, that gas handling should rather have it's own gas.ts module since this doesn't really belong to the environment but is rather some internal state component like Stack or Memory. This would be likely also similarly useful for VM users if they could hook into that class and get step-by-step gas usage info or can manipulate.

Also one indicator: gasLeft doesn't really look right in the constructor here, this is also used only in very isolated places and not throughout the whole class.

On-top note: but please resist the temptation of just doing it because I repeat the idea 2-3 times if you don't think it's a good idea. 😛

To be honest I'm not sure what you have in mind to put in the gas.ts module :) The useGas and refundGas methods (maybe like this)?

this is also used only in very isolated places and not throughout the whole class.

It's mostly used in opFns (and also used for instruction base fee in Loop). On purpose I avoided doing gas metering in EEI.

Thanks for digging this out from Py-EVM, yes I was actually thinking about something like this. We can do this later though if agreed upon, don't let this distract you here. I would assume this would be useful if e.g. extended with events which could be used to real-time display gas usage in IDEs e.g..

Great, I think this is doable. One possibility (for next versions) is to take inspiration from go-ethereum's gas table bring most gas calculation logic there (possibly even hardfork-specific gas costs).

holgerd77 · 2019-05-09T09:36:45Z

lib/evm/eei.ts

-    const addressBuf = addressToBuffer(address)
-    return this._state.accountIsEmpty(addressBuf)
+  async isAccountEmpty (address: Buffer): Promise<boolean> {
+    return this._state.accountIsEmpty(address)
  }
 }



Ok, some more signature tightening fof account functionality, good.

holgerd77 · 2019-05-09T09:48:15Z

lib/evm/interpreter.ts

+      gas: eei._gasLeft,
+      gasUsed,
+      'return': result.returnValue ? result.returnValue : Buffer.alloc(0)
+    })
  }



Can't completely judge these changes in Interpreter but would give this a go since you're saying that this is only some intermediate step.

Yeah this is mostly temporary. But I'd appreciate comments on how to change it in future (I'm not yet sure myself).

Will give this some thought, maybe @alcuadrado can also join for discussion?

holgerd77 · 2019-05-09T09:54:16Z

lib/evm/loop.ts

-      account: this._runState.eei!._env.contract,
+      depth: this._eei._env.depth,
+      address: this._eei._env.address,
+      account: this._eei._env.contract,
      stateManager: this._runState.stateManager,
      memory: this._runState.memory._store, // Return underlying array for backwards-compatibility
      memoryWordCount: this._runState.memoryWordCount


Whole file: yes, this looks actually much cleaner and this state removal and just execution focus looks more fitting to the purpose this class should probably have, great!

holgerd77 · 2019-05-09T10:08:15Z

lib/evm/opFns.ts

-module.exports = {
-  STOP: function (runState) {
-    runState.stopped = true
+export const handlers: {[k: string]: OpHandler} = {


Is this mainly to closely stick to the current structure of the module or does this have other benefits or - more broadly - implications?

Related question: is making opcodes sub-classable or extendable or similar a separate step? Have you this on the plan for this release/change round?

Well, no major benefit, I only typed what was there, didn't make structural modifications. I think it definitely makes sense to revisit this and maybe take inspiration from Chris's comments to improve the handlers.

is making opcodes sub-classable or extendable or similar a separate step?

Once users can customize Loop and use their customized version to run transactions, they can override getOpHandler and replace the implementation for any of the opcodes. I imagine this should be a good first step. I'm open however to ideas on how to make this even further extensible.

Ok, thanks for the explanation.

holgerd77 · 2019-05-09T10:13:23Z

lib/exceptions.ts

@@ -8,7 +8,8 @@ export enum ERROR {
  REVERT = 'revert',
  STATIC_STATE_CHANGE = 'static state change',
  INTERNAL_ERROR = 'internal error',
-  CREATE_COLLISION = 'create collision'
+  CREATE_COLLISION = 'create collision',
+  STOP = 'stop'
 }

 export class VmError {


Whole file: generally looks good, various variable initialization clean ups and naming improvements, good.

holgerd77 · 2019-05-09T10:13:55Z

lib/evm/opFns.ts

 const MASK_160 = new BN(1).shln(160).subn(1)

 // Find Ceil(`this` / `num`)
-BN.prototype.divCeil = function divCeil (num) {
-  var dm = this.divmod(num)
+function divCeil (a: BN, b: BN) {


Makes sense to have this less intrusive.

s1na added 5 commits May 7, 2019 15:11

Change opFns filetype to ts

12a98ee

Migrate opFns to ts, mv eei init to interpreter

9df0927

Fix linting error

26a8c99

Annotate op handlers with type

3469087

Cleanup loop and interpreter

31e9838

holgerd77 approved these changes May 9, 2019

View reviewed changes

s1na merged commit a53d432 into v4 May 13, 2019

s1na deleted the ts/opFns branch May 13, 2019 07:38

s1na mentioned this pull request May 13, 2019

V4 tracker #479

Merged

23 tasks

evertonfraga added the package: vm label Feb 27, 2020

snyk-bot mentioned this pull request Jul 5, 2021

[Snyk] Upgrade solc from 0.5.17 to 0.8.5 terrorizer1980/ethereumjs-vm#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate opFns to typescript, modify loop/interpreter structure #506

Migrate opFns to typescript, modify loop/interpreter structure #506

s1na commented May 9, 2019

lgtm-com bot commented May 9, 2019

holgerd77 left a comment

holgerd77 May 9, 2019

holgerd77 May 9, 2019

holgerd77 May 9, 2019

s1na May 9, 2019

holgerd77 May 10, 2019

s1na May 10, 2019

holgerd77 May 9, 2019

holgerd77 May 9, 2019

s1na May 9, 2019

holgerd77 May 10, 2019

holgerd77 May 9, 2019

holgerd77 May 9, 2019

s1na May 9, 2019

holgerd77 May 10, 2019

holgerd77 May 9, 2019

holgerd77 May 9, 2019

Migrate opFns to typescript, modify loop/interpreter structure #506

Migrate opFns to typescript, modify loop/interpreter structure #506

Conversation

s1na commented May 9, 2019

lgtm-com bot commented May 9, 2019

holgerd77 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

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