Implemented most of the stubbed-out state handling instructions #59

zah · 2018-06-27T01:27:01Z

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.

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.

zah · 2018-07-04T20:42:57Z

This is in a relatively good shape for merging now.

Please note that quite a lot of tests are failing now, but this seems to be the result of properly implementing the "post" state checker. Previously, a test was considered successful just because it managed to execute to the end without crashing.

mratsim

Looks good to me. I prefer the new syntax to the old db(readOnly = true) macro

A few remarks:

I feel like the code will get littered by toOpenArray (which I like), I hope that move/lent types and destructors will alleviate this because the compiler should infer that copy is not needed.
I think TODO is better than XXX due to github highlight, have to check if FIXME works also so that we have granularity if needed
due to quasiBoolean and https://github.com/status-im/nimbus/issues/63, the code doesn't compile on latest devel. I see a partial fix for and/or/xor code, if you have time I'd like a complete fix. Otherwise we can merge as is, as the "fix" will be removed anyway when I merge my branch.
writePaddedResult is complex and inefficient (2 nested if, plus zero-padding that is actually unneeded). I use a much simpler version in my branch. In fact I noticed that Py-EVM overengineered when implementing memory ops:

mratsim · 2018-07-05T10:37:39Z

nimbus/db/db_chain.nim


 proc getCanonicalHead*(self: BaseChainDB): BlockHeader =
  let k = canonicalHeadHashKey()
-  if k notin self.db:
+  if k.toOpenArray notin self.db:
    raise newException(CanonicalHeadNotFound,
                      "No canonical head set for this chain")
  return self.getBlockHeaderByHash(self.getHash(k))


Is there really a cost from passing a simple k to notin or getHash?
If so shouldn't it be getHash(k.toOpenarray)

Passing just k wouldn't compile. notin is expanded to a contains call, which is part of the abstract interface of the database.

mratsim · 2018-07-05T10:41:04Z

nimbus/db/state_db.nim

-  # db.trie[address] = rlp.encode[Account](account)
-  discard # TODO
-
+  db.trie.put createRangeFromAddress(address), rlp.encode(account)


rlp.encode(account).toOpenArray here?

The Trie currently requires ByteRanges keys and values, because it needs to create alternative views over the passed in range (it needs to examine the range bit by bit or nibble by nibble). The Nim type system must be improved before it will be possible to create such views over openarray inputs. I'll create an RFC about it soon.

mratsim · 2018-07-05T10:45:40Z

nimbus/db/state_db.nim

+  # XXX: This is too expensive. Similar to `createRangeFromAddress`
+  # Converts a number to hex big-endian representation including
+  # prefix and leading zeros:
+  @(keccak256.digest(slot.toByteArrayBE).data).toRange


This should use toByteRange_Unnecessary defined below.

mratsim · 2018-07-05T10:48:14Z

nimbus/vm/code_stream.nim

+  let last = min(c.pc + n, c.bytes.len)
+  let toWrite = last - c.pc
+  for i in 0 ..< toWrite : result_bytes[i] = c.bytes[last - i - 1]
+  for j in toWrite ..< 32: result_bytes[j] = 0


This is unnecessary, Uint256/stack objects are initialized with all 0.

Fair enough, but we can add your trick about using noinit on the result here perhaps.

mratsim · 2018-07-05T10:50:07Z

nimbus/vm/code_stream.nim

+
+  let last = min(c.pc + n, c.bytes.len)
+  let toWrite = last - c.pc
+  for i in 0 ..< toWrite : result_bytes[i] = c.bytes[last - i - 1]


Are there cases where endianness is important?

If yes, it should at least have a # TODO: implement endianness aware or be implemented right now because that might lead to hard to debug issues otherwise.

Yes, I wanted to add a static assert about this actually (wanted to see how you are detecting the endianness in stint).

I just use when system.cpuEndian == littleEndian

mratsim · 2018-07-05T10:52:33Z

nimbus/vm/interpreter/opcodes_impl/comparison.nim

@@ -18,11 +18,17 @@ quasiBoolean(sgt, `>`, signed=true) # Signed Greater Comparison

 quasiBoolean(eq, `==`) # Equality


Unfortunately, the quasiBoolean macro is also broken for < and == on latest devel

mratsim · 2018-07-05T10:54:59Z

nimbus/vm/interpreter/opcodes_impl/context.nim

+      presentBytes = 0
+
+    for i in presentBytes ..< 32: bytes[i] = 0
+    computation.stack.push(bytes)


This seems very complex. In my own implementation rewrite I use the following:

op callDataLoad, inline = false, startPos: ## 0x35, Get input data of current environment let start = startPos.toInt # If the data does not take 32 bytes, pad with zeros let endRange = min(computation.msg.data.len - 1, start + 31) let padding = start + 31 - endRange var value: array[32, byte] # We rely on value being initialized with 0 by default value[padding ..< 32] = computation.msg.data.toOpenArray(start, endRange) push: value

This is not safe. The original code was written in convoluted way, because startPos may be larger than INT_MAX.

And again, noinit can speed up the original code to avoid double writes to the same bytes.

mratsim · 2018-07-05T10:59:20Z

nimbus/vm/memory.nim

+  let endPos = startPos + numberOfBytes
+  assert endPos < memory.len
+  for i in startPos ..< endPos:
+    memory.bytes[i] = paddingValue


This is unnecessary if we use openarray because memory.extend already extends with 0.

mratsim · 2018-07-05T11:05:25Z

nimbus/vm/interpreter/opcodes_impl/context.nim

+    mem.writePaddingBytes(memPos + presentElements,
+                          len - presentElements,
+                          paddingValue)
+


Since extend already pads with 0, this can be simplified, the following is extracted from my implementation of codecopy and avoids 2 if-else nesting and the need of writePaddingBytes

# If the data does not take 32 bytes, pad with zeros let lim = min(computation.code.bytes.len, copyPos + len) let padding = copyPos + len - lim # Note: when extending, extended memory is zero-ed, we only need to offset with padding value computation.memory.write(memPos): computation.code.bytes.toOpenArray(copyPos+padding, copyPos+lim)

Full implementation with comments, I noticed that Py-EVM is overengineering here:

op codecopy, inline = false, memStartPos, copyStartPos, size: ## 0x39, Copy code running in current environment to memory. let (memPos, copyPos, len) = (memStartPos.toInt, copyStartPos.toInt, size.toInt) computation.gasMeter.consumeGas( computation.gasCosts[CodeCopy].m_handler(memPos, copyPos, len), reason="CodeCopy fee") computation.memory.extend(memPos, len) # TODO: here Py-EVM is doing something very complex, increasing a program counter in the "CodeStream" type. # while Geth, Parity and the Yellow paper are just copying bytes? # https://github.com/ethereum/py-evm/blob/090b29141d1d80c4b216cfa7ab889115df3c0da0/evm/vm/logic/context.py#L96-L97 # https://github.com/paritytech/parity/blob/98b7c07171cd320f32877dfa5aa528f585dc9a72/ethcore/evm/src/interpreter/mod.rs#L581-L582 # https://github.com/ethereum/go-ethereum/blob/947e0afeb3bce9c52548979daddd1e00aa0d7ba8/core/vm/instructions.go#L478-L479 # If the data does not take 32 bytes, pad with zeros let lim = min(computation.code.bytes.len, copyPos + len) let padding = copyPos + len - lim # Note: when extending, extended memory is zero-ed, we only need to offset with padding value computation.memory.write(memPos): computation.code.bytes.toOpenArray(copyPos+padding, copyPos+lim)

Fair enough again, but perhaps expand shouldn't pad with zero. This way, there will be a single write over the expanded range.

I think there is an off-by-one error in your code. The second index passed to toOpenArray is inclusive, so there should be lim - 1 somewhere in there.

Also, this is seductively simple, but the getting the old code right was quite tricky. Notice how there is a check if presentElements > 0. This was because dataPos may be outside the bounds of code.bytes. You code is not handling this case properly as far as I can tell.

I've extracted the discussion to #67. That can be tackled later and might make a good first issue for someone new to Nimbus. (We would need an easier way to test mem opcodes probably)

mratsim · 2018-07-05T11:06:43Z

nimbus/vm/interpreter/opcodes_impl/storage.nim

+  if found:
+    computation.stack.push value
+  else:
+    # XXX: raise exception?


I prefer TODO to XXX due to Github highlight

zah · 2018-07-05T16:03:10Z

nimbus/vm/interpreter/opcodes_impl/stack_ops.nim

-        let paddedValue = `value`.padRight(`size`, 0.byte)
-        `computation`.stack.push(paddedValue)
-
+      `computation`.stack.push `computation`.code.readVmWord(`size`)


This is not ported yet in the other branch.

* move forks constants, rename errors * Move vm/utils to vm/interpreter/utils * initial opcodes refactoring * Add refactored Comparison & Bitwise Logic Operations * Add sha3 and address, simplify macro, support pop 0 * balance, origin, caller, callValue * fix gas copy opcodes gas costs, add callDataLoad/Size/Copy, CodeSize/Copy and gas price opcode * Update with 30s, 40s, 50s opcodes + impl of balance + stack improvement * add push, dup, swap, log, create and call operations * finish opcode implementation * Add the new dispatching logic * Pass the opcode test * Make test_vm_json compile * halt execution without exceptions for Return, Revert, selfdestruct (fix #62) * Properly catch and recover from EVM exceptions (stack underflow ...) * Fix byte op * Fix jump regressions * Update for latest devel, don't import old dispatch code as quasiBoolean macro is broken by latest devel * Fix sha3 regression on empty memory slice and until end of range slice * Fix padding / range error on expXY_success (gas computation left) * update logging procs * Add tracing - expXY_success is not a regression, sload stub was accidentally passing the test * Reuse the same stub as OO implementation * Delete previous opcode implementation * Delete object oriented fork code * Delete exceptions that were used as control flows * delete base.nim 🔥, yet another OO remnants * Delete opcode table * Enable omputed gotos and compile-time gas fees * Revert const gasCosts -> generates SIGSEGV * inline push, swap and dup opcodes * loggers are now template again, why does this pass new tests? * Trigger CI rebuild after rocksdb fix status-im/nim-rocksdb#5 * Address review comment on "push" + VMTests in debug mode (not release) * Address review comment: don't tag fork by default, make opcode impl grepable * Static compilation fixes after rebasing * fix the initialization of the VM database * add a missing import * Deactivate balance and sload test following #59 * Reactivate stack check (deactivated in #59, necessary to pass tests) * Merge remaining opcodes implementation from #59 * Merge callDataLoad and codeCopy fixes, todo simplify see #67

zah requested review from yglukhov, coffeepots and mratsim June 27, 2018 01:27

zah force-pushed the state-ops-implementation branch from 14f0407 to 026f1d6 Compare July 4, 2018 19:51

zah and others added 10 commits July 4, 2018 23:05

Implemented most of the stubbed out state handling instructions

aa65703

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

bcbcbc1

trie put and del are void now

3ca7752

getBlockTransactionData and getReceipts

df8261e

Working code for extcodesize0.json

bcf6547

fix origin.json

e61d42d

fix calldatasize1

2ac1679

fix calldataloadSizeTooHighPartial

cc8b1d4

fix calldataloadSizeTooHigh

4264036

more efficient PushX implementation

c8287a9

zah force-pushed the state-ops-implementation branch from 026f1d6 to c8287a9 Compare July 4, 2018 20:05

fix and, or, xor

7054b8b

mratsim approved these changes Jul 5, 2018

View reviewed changes

mratsim merged commit 18b7bbb into master Jul 5, 2018

mratsim mentioned this pull request Jul 5, 2018

Refactor interpreter dispatch #65

Merged

zah commented Jul 5, 2018

View reviewed changes

mratsim added a commit that referenced this pull request Jul 5, 2018

Deactivate balance and sload test following #59

763489c

mratsim added a commit that referenced this pull request Jul 5, 2018

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 a commit that referenced this pull request Jul 5, 2018

Merge remaining opcodes implementation from #59

42fd80c

zah deleted the state-ops-implementation branch July 12, 2018 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented most of the stubbed-out state handling instructions #59

Implemented most of the stubbed-out state handling instructions #59

zah commented Jun 27, 2018

zah commented Jul 4, 2018

mratsim left a comment

mratsim Jul 5, 2018

zah Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 5, 2018

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 5, 2018 •

edited

Loading

mratsim Jul 5, 2018

zah Jul 5, 2018 •

edited

Loading

mratsim Jul 5, 2018

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 5, 2018

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 5, 2018

zah Jul 5, 2018

mratsim Jul 5, 2018

mratsim Jul 5, 2018

zah Jul 5, 2018

		@@ -18,11 +18,17 @@ quasiBoolean(sgt, `>`, signed=true) # Signed Greater Comparison

		quasiBoolean(eq, `==`) # Equality

Implemented most of the stubbed-out state handling instructions #59

Implemented most of the stubbed-out state handling instructions #59

Conversation

zah commented Jun 27, 2018

zah commented Jul 4, 2018

mratsim 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

zah Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Jul 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zah Jul 5, 2018 •

edited

Loading

zah Jul 5, 2018 •

edited

Loading