Refactor interpreter dispatch #65

mratsim · 2018-07-03T15:30:29Z

This completely removes the need of an OO approach in the EVM as in Py-EVM to use a more classical "switch" as in Geth, Parity and Cpp-Ethereum (and NimVM).

It is a follow-up to #49 and #52.

Summary:

1300 lines added, 2200 deleted
29 files deleted
Arguably much easier to maintain
Very probably much better performance
Needed step if we want a Nimbus EVM stand-alone (like EVMJIT, Hera)

Fixes:

Fix Boolean opcodes do not compile on latest devel #63 boolean opcodes do not compile on latest devel. The new implementation compiles.
Fix EVM is using exceptions for non-exceptional situations #62 current EVM is using exceptions for control flow. The new implementation just breaks out of the opcode execution loop.
Help on Remove unnecessary ref object #14, several ref objects allocated in tight loops, especially "Opcode", were removed.

Expected benefits:

Maintenance is significantly improved for complex opcodes:
- CALL needed to be special-cased depending on forks https://github.com/status-im/nimbus/blob/6f28d1186675f648e0587f44e730b9dc23291bab/nimbus/vm/interpreter/opcodes_impl/call.nim#L19-L41
- CREATE and SELFDESTRUCT and more generally all procs with a different logic depending on hardforks beyond "simple" gas changes introduced dispatch issues https://github.com/status-im/nimbus/blob/6f28d1186675f648e0587f44e730b9dc23291bab/nimbus/vm/interpreter/opcodes_impl/system_ops.nim#L19-L24
- Previous dispatching logic used exceptions for control flow (EVM is using exceptions for non-exceptional situations #62) in Return, Revert and SelfDestruct: https://github.com/status-im/nimbus/blob/6f28d1186675f648e0587f44e730b9dc23291bab/nimbus/vm/computation.nim#L247-L291
- Reimplementation of opcodes in single file of 750 lines instead of 18 files totaling 1220 lines
- No inheritance instances to track.
Performance
- CALL, CREATE, SELFDESTRUCT special-casing require slower runtime dispatch
- The heap allocated opcode type has been killed, which significantly reduce GC pressure as it avoids allocation at each opcode read https://github.com/status-im/nimbus/blob/6f28d1186675f648e0587f44e730b9dc23291bab/nimbus/vm_types.nim#L40-L45. VM Forks that would have been created on the heap for each smart contract CALL will also be avoided
- Computed Gotos as researched here are the fastest portable interpreter dispatch technique with speed benefits of about 20%. See research notes in the wiki: https://github.com/status-im/nimbus/wiki/Interpreter-optimization-resources
Nimbus EVM as standalone
- Some interest has been expressed to use Nimbus EVM as a standalone library with EVM-C interface, that can be called from any language. To avoid having to load the Nim runtime, inheritance and all GC-ed types should be avoided in that case

Note on regressions

There are regressions on arith, boolean and mktx tests that test that the CALL opcode which is stubbed.

Similarly the expXY_success test passes even though it uses SLOAD. It passes because SLOAD is stubbed with a non-zero value.

Note on new jump tests passed specific jump tests

I'm not too sure why I pass new jump/control flow tests, the new tests were enabled by this commit https://github.com/status-im/nimbus/pull/65/commits/63b6fc251f77146b60b85bf032429cd5489f9e33, where I changed log proc back to templates as in master (I had some import/symbol visibility issue with templates during branch implementation).

The following tests are passing with -d:release and failing in debug mode:

BlockNumberDynamicJumpiOutsideBoundary
DynamicJumpiOutsideBoundary
jumpiOutsideBoundary
kv1

CI failure:

~~Pending - status-im/nim-rocksdb#3~~ Solved

zah · 2018-07-04T18:02:17Z

nimbus/vm/forks/f20150730_frontier/frontier_vm_state.nim

-
-# proc _executeFrontierTransaction*(vmState: FrontierVMState;
-#                                  transaction: FrontierTransaction): FrontierComputation =
-#   transaction.validate()


The deleted code here seems quite complicated. Was it translated in any way in the new implementation?

No, I feel that the code is much too complex and it has been refactored/deleted in Py-EVM since our port from January so it's better if we rewrite it from scratch.

Notice how there is no execute_frontier_transaction in the following screenshot and how all methods are about 10 lines long:

zah · 2018-07-04T18:23:44Z

nimbus/vm/interpreter/opcodes_impl.nim

+
+op add, FkFrontier, inline = true, lhs, rhs:
+  ## 0x01, Addition
+  push: lhs + rhs


The push construct seems a bit unnatural, because it suggests that an arbitrary block may be given, but is this actually true? You could have defined it as a simple dirty template accepting a single expression and making use of the computation variable.

No need for the whole replacePush business.

Yeah, I did too much macros recently and completely forgot about dirty templates

Fixed in https://github.com/status-im/nimbus/pull/65/commits/3151bba0d13f9f92c4fb874a4ab50b26269c293e

zah · 2018-07-04T18:26:00Z

nimbus/vm/interpreter/opcodes_impl.nim

@@ -0,0 +1,759 @@
+# Nimbus


The fact that all opcode implementations were moved will make merging this with the other state-ops branch very hard. Any suggestions about how should we solve this problem?

I feel like most of the changes in https://github.com/status-im/nimbus/tree/state-ops-implementation are pretty self-contained.

State-ops can be merged first, and I rebase and port changes over.
I also have to carry over the changes from https://github.com/status-im/nimbus/tree/blocknumber-size-refactoring

zah · 2018-07-04T18:35:00Z

nimbus/vm/interpreter_dispatch.nim

+let FrontierOpDispatch {.compileTime.}: array[Op, NimNode] = block:
+  fill_enum_table_holes(Op, newIdentNode"invalidInstruction"):
+    [
+      Stop: newIdentNode "toBeReplacedByBreak",


We should try to avoid programming constructs that break grepping, "go to definition" and ntags. The previous opcode table was nicer in this regard, because it used the actual proc names and I believe we can keep this property.

Can you explain in more details how the different forks will feature different opcode implementations? My previous understanding was that this is a rare occurrence and we'll handle it by defining separate op code implementation only where differences exist (and these separate implementations will have different proc names). Any shared code may be defined as helper templates.

EDIT: I noticed how genCall is defined, but I still think the whole suffix-appending approach is probably unnecessary (the op macro doesn't need the ForkName parameter).

I agree, I will suffix the changed ops with their EIP (like Py-EVM) or fork (might be more dev friendly) but the default one will have an easy name.

I also agree with the grepping issue, I was often wondering where ceil32 was defined and it was defined by ceilXX(32)

Fixed in https://github.com/status-im/nimbus/pull/65/commits/00baaf384e996657c08221f5efb94e209b1f61b6

zah · 2018-07-04T18:38:16Z

nimbus/vm/interpreter/opcodes_impl/system_ops.nim

-proc selfdestructEIP150(computation) =
-  let beneficiary = stack.popAddress()
-  # TODO: with
-  # with computation.vm_state.state_db(read_only=True) as state_db:


I find these TODO items and commented code quite useful. We should try to keep them.

Yes I find them useful as well, I try to keep them:

Create: https://github.com/status-im/nimbus/blob/00baaf384e996657c08221f5efb94e209b1f61b6/nimbus/vm/interpreter/opcodes_impl.nim#L492

Call: https://github.com/status-im/nimbus/blob/00baaf384e996657c08221f5efb94e209b1f61b6/nimbus/vm/interpreter/opcodes_impl.nim#L664

I preferred to delete it to reduce cognitive load given that:

the comment didn't match with the current Nim syntax,

the op is relatively straightforward,

I can always use git history to get it back.

and I intend to finish this as soon as this PR and state ops are merged.

Merge note: currently cannot compile due to `quasiBoolean` (#63). This will be solved by https://github.com/status-im/nimbus/pull/65 ---- * Implemented most of the stubbed out state handling instructions The code compiles, but still fails at the moment due to incorrect initialization of the VM. Don't merge yet. More commits will be pushed in the coming days. * Fixed crash * trie put and del are void now * getBlockTransactionData and getReceipts * Working code for extcodesize0.json * fix origin.json * fix calldatasize1 * fix calldataloadSizeTooHighPartial * fix calldataloadSizeTooHigh * more efficient PushX implementation * fix and, or, xor

…Copy and gas price opcode

#62)

…an macro is broken by latest devel

…repable

mratsim · 2018-07-05T13:45:38Z

I've rebased on the new master. I still have to port to opcode_impl.nim some of the new opcode implementations that include the #59 additions.

@zah I'm getting Illegal storage accesses with the new hexaryTree:

[Suite] parse bytecode
  [OK] accepts bytes
  [OK] next returns the correct opcode
  [OK] peek returns next opcode without changing location
  [OK] stop opcode is returned when end reached
  [OK] [] returns opcode
  [OK] isValidOpcode invalidates after PUSHXX
  [OK] isValidOpcode 0
  [OK] isValidOpcode 1
  [OK] right number of bytes invalidates

[Suite] gasMeter
  [OK] consume spends
  [OK] consume errors
  [OK] return refund works correctly

[Suite] memory
  [OK] write
  [OK] write rejects valyes beyond memory size
  [OK] extends appropriately extends memory
  [OK] read returns correct bytes

[Suite] stack
    test_stack.nim(30, 16): Expect Failed, unexpected exception was thrown.
  [FAILED] push only valid
  [OK] push does not allow stack to exceed 1024
  [OK] dup does not allow stack to exceed 1024
  [OK] pop returns latest stack item
  [OK] swap correct
  [OK] dup correct
  [OK] pop raises InsufficientStack appropriately
  [OK] swap raises InsufficientStack appropriately
  [OK] dup raises InsufficientStack appropriately
  [OK] binary operations raises InsufficientStack appropriately

[Suite] opcodes
  [OK] add
Traceback (most recent call last)
test_opcode.nim(79)      test_opcode
test_opcode.nim(45)      testCode
interpreter_dispatch.nim(227) executeOpcodes
interpreter_dispatch.nim(194) frontierVM
opcodes_impl.nim(195)    balance
vm_state.nim(122)        readOnlyStateDB
db_chain.nim(231)        getStateDb
state_db.nim(26)         newAccountStateDB
hexary.nim(55)           initHexaryTrie
assign.nim(124)          genericAssign
assign.nim(100)          genericAssignAux
assign.nim(24)           genericAssignAux
assign.nim(21)           genericAssignAux
assign.nim(106)          genericAssignAux
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

zah · 2018-07-05T20:54:26Z

I'm a bit worried about the separation of VM/VmState that is no longer present, but we can always restore it in the future if necessary. I'm still looking forward to see how the fork-specific opcode implementations will work out (we've discussed on Slack that these may be mapped to an additional static parameter for the forked instructions).

All in all, let's merge this, so the rest of the team doesn't accumulate too many conflicts.

coffeepots

LGTM. Originally wasn't sure about combining all the ops into one module but after looking at it, I think it will be easier to work with.

mratsim added optimization refactoring labels Jul 3, 2018

mratsim force-pushed the refactor-interpreter-dispatch branch from 81a9297 to 63b6fc2 Compare July 4, 2018 11:01

mratsim requested review from coffeepots, yglukhov and zah July 4, 2018 11:22

mratsim changed the title ~~[WIP] Refactor interpreter dispatch~~ Refactor interpreter dispatch Jul 4, 2018

zah reviewed Jul 4, 2018

View reviewed changes

mratsim added 21 commits July 5, 2018 14:47

move forks constants, rename errors

14c9b90

Move vm/utils to vm/interpreter/utils

d3e6dc2

initial opcodes refactoring

bfd4e0e

Add refactored Comparison & Bitwise Logic Operations

d2aa102

Add sha3 and address, simplify macro, support pop 0

0315c99

balance, origin, caller, callValue

19df472

fix gas copy opcodes gas costs, add callDataLoad/Size/Copy, CodeSize/…

26c9352

…Copy and gas price opcode

Update with 30s, 40s, 50s opcodes + impl of balance + stack improvement

82f53e0

add push, dup, swap, log, create and call operations

49556c9

finish opcode implementation

e765405

Add the new dispatching logic

2dc0376

Pass the opcode test

90c014c

Make test_vm_json compile

1befcff

halt execution without exceptions for Return, Revert, selfdestruct (fix

a8c2c9a

#62)

Properly catch and recover from EVM exceptions (stack underflow ...)

cf08fe7

Fix byte op

1221c73

Fix jump regressions

eae2a9e

Update for latest devel, don't import old dispatch code as quasiBoole…

9f3d9e9

…an macro is broken by latest devel

Fix sha3 regression on empty memory slice and until end of range slice

54bb55d

Fix padding / range error on expXY_success (gas computation left)

f0164e9

update logging procs

2e86b9f

mratsim added 14 commits July 5, 2018 15:01

Reuse the same stub as OO implementation

8e31b88

Delete previous opcode implementation

2348ea0

Delete object oriented fork code

816b6bc

Delete exceptions that were used as control flows

b677c1f

delete base.nim 🔥, yet another OO remnants

18e682d

Delete opcode table

5226b58

Enable omputed gotos and compile-time gas fees

b4b5543

Revert const gasCosts -> generates SIGSEGV

b5020ff

inline push, swap and dup opcodes

1a5b358

loggers are now template again, why does this pass new tests?

2a3ae3a

Trigger CI rebuild after rocksdb fix status-im/nim-rocksdb#5

6221296

Address review comment on "push" + VMTests in debug mode (not release)

27545b8

Address review comment: don't tag fork by default, make opcode impl g…

b08de18

…repable

Static compilation fixes after rebasing

6d53c46

mratsim force-pushed the refactor-interpreter-dispatch branch from 00baaf3 to 6d53c46 Compare July 5, 2018 13:43

zah and others added 4 commits July 5, 2018 19:38

fix the initialization of the VM database

65330cd

add a missing import

fe312a4

Deactivate balance and sload test following #59

763489c

Reactivate stack check (deactivated in #59, necessary to pass tests)

549d722

mratsim mentioned this pull request Jul 5, 2018

Tests & implementation of CallDataLoad and CodeCopy #67

Closed

mratsim added 2 commits July 5, 2018 21:24

Merge remaining opcodes implementation from #59

42fd80c

Merge callDataLoad and codeCopy fixes, todo simplify see #67

bd17353

zah approved these changes Jul 5, 2018

View reviewed changes

yglukhov approved these changes Jul 6, 2018

View reviewed changes

coffeepots approved these changes Jul 6, 2018

View reviewed changes

mratsim merged commit 4b5eada into master Jul 6, 2018

mratsim mentioned this pull request Jul 6, 2018

Remove usage of nim reserved keywords #13

Closed

zah deleted the refactor-interpreter-dispatch branch July 12, 2018 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor interpreter dispatch #65

Refactor interpreter dispatch #65

mratsim commented Jul 3, 2018 •

edited

Loading

zah Jul 4, 2018

mratsim Jul 5, 2018

zah Jul 4, 2018 •

edited

Loading

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 4, 2018

mratsim Jul 5, 2018

zah Jul 4, 2018

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 4, 2018

mratsim Jul 5, 2018

mratsim commented Jul 5, 2018

zah commented Jul 5, 2018

coffeepots left a comment

Refactor interpreter dispatch #65

Refactor interpreter dispatch #65

Conversation

mratsim commented Jul 3, 2018 • edited Loading

Fixes:

Expected benefits:

Note on regressions

Note on new jump tests passed specific jump tests

CI failure:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Jul 4, 2018 • edited Loading

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

mratsim commented Jul 5, 2018

zah commented Jul 5, 2018

coffeepots left a comment

Choose a reason for hiding this comment

mratsim commented Jul 3, 2018 •

edited

Loading

zah Jul 4, 2018 •

edited

Loading