Merge branch 'cleanups' of https://github.com/jangko/nimbus into jangko-cleanups

nimbus/p2p/chain.nim

@@ -1,5 +1,5 @@
 import ../db/[db_chain, state_db], eth/common, chronicles, ../vm_state, ../vm_types, ../transaction, ranges,
-  ../vm/[computation, interpreter_dispatch, message], ../constants, stint, nimcrypto,
+  ../vm/[computation, message], ../constants, stint, nimcrypto,
   ../vm_state_transactions, sugar, ../utils, eth/trie/db, ../tracer, ./executor
 
 type

nimbus/p2p/executor.nim

@@ -3,7 +3,7 @@ import options,
   ../db/[db_chain, state_db],
   ../utils, ../constants, ../transaction,
   ../vm_state, ../vm_types, ../vm_state_transactions,
-  ../vm/[computation, interpreter_dispatch, message],
+  ../vm/[computation, message],
   ../vm/interpreter/vm_forks
 
 proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMState, forkOverride=none(Fork)): GasInt =
@@ -12,18 +12,6 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
   trace "Sender", sender
   trace "txHash", rlpHash = tx.rlpHash
 
-  # TODO: we have identical `fork` code in setupComputation.
-  # at later stage, we need to get rid of it
-  # and apply changes in eth_*, debug_* RPC,
-  # macro assembler and premix tool set.
-  # at every place where setupComputation and
-  # processTransaction are used.
-  let fork =
-    if forkOverride.isSome:
-      forkOverride.get
-    else:
-      vmState.blockNumber.toFork
-
   let upfrontGasCost = tx.gasLimit.u256 * tx.gasPrice.u256
   var balance = vmState.readOnlyStateDb().getBalance(sender)
   if balance < upfrontGasCost:
@@ -43,24 +31,10 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
   if tx.isContractCreation and isCollision:
     return tx.gasLimit
 
-  var snapshot = vmState.snapshot()
-  defer: snapshot.dispose()
-
-  var contractOK = true
   result = tx.gasLimit
-
   if execComputation(computation):
-    if tx.isContractCreation:
-      contractOK = computation.writeContract(fork)
     result = computation.refundGas(tx, sender)
 
-  if not contractOK and fork == FkHomestead:
-    snapshot.revert()
-    # consume all gas
-    result = tx.gasLimit
-  else:
-    snapshot.commit()
-  
   if computation.isSuicided(vmState.blockHeader.coinbase):
     return 0
 

nimbus/rpc/p2p.nim

@@ -14,7 +14,7 @@ import
   ../transaction, ../config, ../vm_state, ../constants, ../vm_types,
   ../vm_state_transactions, ../utils,
   ../db/[db_chain, state_db, storage_types],
-  rpc_types, rpc_utils, ../vm/[message, computation, interpreter_dispatch]
+  rpc_types, rpc_utils, ../vm/[message, computation]
 
 #[
   Note:

nimbus/vm/computation.nim

@@ -112,61 +112,12 @@ proc commit*(snapshot: var ComputationSnapshot) {.inline.} =
 proc dispose*(snapshot: var ComputationSnapshot) {.inline.} =
   snapshot.snapshot.dispose()
 
-proc transferBalance(computation: var BaseComputation, opCode: static[Op]) =
-  if computation.msg.depth >= MaxCallDepth:
-    raise newException(StackDepthError, "Stack depth limit reached")
-
-  let senderBalance = computation.vmState.readOnlyStateDb().
-                      getBalance(computation.msg.sender)
-
-  if senderBalance < computation.msg.value:
-    raise newException(InsufficientFunds,
-      &"Insufficient funds: {senderBalance} < {computation.msg.value}")
-
-  when opCode in {Call, Create}:
-    computation.vmState.mutateStateDb:
-      db.subBalance(computation.msg.sender, computation.msg.value)
-      db.addBalance(computation.msg.storageAddress, computation.msg.value)
-
-proc applyMessage(computation: var BaseComputation, opCode: static[Op]): bool =
-  var snapshot = computation.snapshot()
-  defer: snapshot.dispose()
-
-  when opCode in {CallCode, Call, Create}:
-    try:
-      computation.transferBalance(opCode)
-    except VMError:
-      snapshot.revert()
-      debug "transferBalance failed", msg = computation.error.info
-      return
-
-  if computation.gasMeter.gasRemaining < 0:
-    snapshot.commit()
-    return
-
-  try:
-    # Run code
-    # We cannot use the normal dispatching function `executeOpcodes`
-    # within `interpreter_dispatch.nim` due to a cyclic dependency.
-    computation.opcodeExec(computation)
-    snapshot.commit()
-  except VMError:
-    snapshot.revert(true)
-    debug "VMError applyMessage failed",
-      msg = computation.error.info,
-      depth = computation.msg.depth
-  except EVMError:
-    snapshot.revert() # TODO: true or false?
-    debug "EVMError applyMessage failed",
-      msg = computation.error.info,
-      depth = computation.msg.depth
-  except ValueError:
-    snapshot.revert(true)
-    debug "ValueError applyMessage failed",
-      msg = computation.error.info,
-      depth = computation.msg.depth
-      
-  result = not computation.isError
+proc getFork*(computation: BaseComputation): Fork =
+  result =
+    if computation.forkOverride.isSome:
+      computation.forkOverride.get
+    else:
+      computation.vmState.blockNumber.toFork
 
 proc writeContract*(computation: var BaseComputation, fork: Fork): bool =
   result = true
@@ -192,19 +143,65 @@ proc writeContract*(computation: var BaseComputation, fork: Fork): bool =
     if fork < FkHomestead: computation.output = @[]
     result = false
 
-proc generateChildComputation*(fork: Fork, computation: var BaseComputation, childMsg: Message): BaseComputation =
-  var childComp = newBaseComputation(
-      computation.vmState,
-      computation.vmState.blockNumber,
-      childMsg,
-      some(fork))
+proc transferBalance(computation: var BaseComputation, opCode: static[Op]): bool =
+  if computation.msg.depth >= MaxCallDepth:
+    debug "Stack depth limit reached", depth=computation.msg.depth
+    return false
 
-  # Copy the fork op code executor proc (assumes child computation is in the same fork)
-  childComp.opCodeExec = computation.opCodeExec
+  let senderBalance = computation.vmState.readOnlyStateDb().
+                      getBalance(computation.msg.sender)
 
-  return childComp
+  if senderBalance < computation.msg.value:
+    debug "insufficient funds", available=senderBalance, needed=computation.msg.value
+    return false
 
-proc addChildComputation(fork: Fork, computation: var BaseComputation, child: BaseComputation) =
+  when opCode in {Call, Create}:
+    computation.vmState.mutateStateDb:
+      db.subBalance(computation.msg.sender, computation.msg.value)
+      db.addBalance(computation.msg.storageAddress, computation.msg.value)
+
+  result = true
+
+proc executeOpcodes*(computation: var BaseComputation) {.gcsafe.}
+
+proc applyMessage*(computation: var BaseComputation, opCode: static[Op]): bool =
+  var snapshot = computation.snapshot()
+  defer: snapshot.dispose()
+
+  when opCode in {CallCode, Call, Create}:
+    if not computation.transferBalance(opCode):
+      snapshot.revert()
+      return
+
+  if computation.gasMeter.gasRemaining < 0:
+    snapshot.commit()
+    return
+
+  try:
+    executeOpcodes(computation)
+    result = not computation.isError
+  except VMError:
+    result = false
+    debug "applyMessage VM Error",
+      msg = getCurrentExceptionMsg(),
+      depth = computation.msg.depth
+  except ValueError:
+    result = false
+    debug "applyMessage Value Error",
+      msg = getCurrentExceptionMsg(),
+      depth = computation.msg.depth
+
+  if result and computation.msg.isCreate:
+    var fork = computation.getFork
+    let contractFailed = not computation.writeContract(fork)
+    result = not(contractFailed and fork == FkHomestead)
+
+  if result:
+    snapshot.commit()
+  else:
+    snapshot.revert(true)
+
+proc addChildComputation(computation: var BaseComputation, child: BaseComputation) =
   if child.isError:
     if child.msg.isCreate:
       computation.returnData = child.output
@@ -222,32 +219,10 @@ proc addChildComputation(fork: Fork, computation: var BaseComputation, child: Ba
     computation.logEntries.add child.logEntries
   computation.children.add(child)
 
-proc getFork*(computation: BaseComputation): Fork =
-  result =
-    if computation.forkOverride.isSome:
-      computation.forkOverride.get
-    else:
-      computation.vmState.blockNumber.toFork
-
 proc applyChildComputation*(parentComp, childComp: var BaseComputation, opCode: static[Op]) =
   ## Apply the vm message childMsg as a child computation.
-  let fork = parentComp.getFork
-
-  var snapshot = parentComp.snapshot()
-  defer: snapshot.dispose()
-  var contractOK = true
-
-  if applyMessage(childComp, opCode):
-    if childComp.msg.isCreate:
-      contractOK = childComp.writeContract(fork)
-
-  if not contractOK and fork == FkHomestead:
-    # consume all gas
-    snapshot.revert(true)
-  else:
-    snapshot.commit()
-
-  fork.addChildComputation(parentComp, childComp)
+  discard childComp.applyMessage(opCode)
+  parentComp.addChildComputation(childComp)
 
 proc registerAccountForDeletion*(c: var BaseComputation, beneficiary: EthAddress) =
   if c.msg.storageAddress in c.accountsToDelete:
@@ -261,14 +236,10 @@ proc addLogEntry*(c: var BaseComputation, log: Log) {.inline.} =
 
 # many methods are basically TODO, but they still return valid values
 # in order to test some existing code
-func getAccountsForDeletion*(c: BaseComputation): seq[EthAddress] =
-  # TODO
-  if c.isError:
-    result = @[]
-  else:
-    result = @[]
+iterator accountsForDeletion*(c: BaseComputation): EthAddress =
+  if not c.isError:
     for account in c.accountsToDelete.keys:
-      result.add(account)
+      yield account
 
 proc getGasRefund*(c: BaseComputation): GasInt =
   if c.isError:
@@ -302,3 +273,5 @@ proc traceError*(c: BaseComputation) =
 
 proc prepareTracer*(c: BaseComputation) =
   c.vmState.tracer.prepare(c.msg.depth)
+
+include interpreter_dispatch

nimbus/vm/interpreter.nim

@@ -10,11 +10,11 @@ import
   ./interpreter/vm_forks
 
 import # Used in vm_types. Beware of recursive dependencies
-  ./code_stream, ./computation, ./stack, ./message, interpreter_dispatch
+  ./code_stream, ./computation, ./stack, ./message
 
 export
   opcode_values, gas_meter,
   vm_forks
 
 export
-  code_stream, computation, stack, message, interpreter_dispatch
+  code_stream, computation, stack, message

nimbus/vm/interpreter/opcodes_impl.nim

@@ -572,7 +572,11 @@ proc setupCreate(computation: var BaseComputation, memPos, len: int, value: Uint
     )
 
   childMsg.sender = computation.msg.storageAddress
-  result = generateChildComputation(computation.getFork, computation, childMsg)
+  result = newBaseComputation(
+    computation.vmState,
+    computation.vmState.blockNumber,
+    childMsg,
+    some(computation.getFork))
 
 op create, inline = false, value, startPosition, size:
   ## 0xf0, Create a new account with associated code.
@@ -739,7 +743,12 @@ template genCall(callName: untyped, opCode: Op): untyped =
     when opCode == CallCode:
       childMsg.storageAddress = computation.msg.storageAddress
 
-    var childComp = generateChildComputation(computation.getFork, computation, childMsg)
+    var childComp = newBaseComputation(
+      computation.vmState,
+      computation.vmState.blockNumber,
+      childMsg,
+      some(computation.getFork))
+
     result = (childComp, memOutPos, memOutLen)
 
   op callName, inline = false:

nimbus/vm/interpreter_dispatch.nim

@@ -11,13 +11,13 @@ import
   ./interpreter/[opcode_values, opcodes_impl, vm_forks, gas_costs, gas_meter, utils/macros_gen_opcodes],
   ./code_stream,
   ../vm_types, ../errors, precompiles,
-  ./stack, ./computation, terminal # Those are only needed for logging
+  ./stack, terminal # Those are only needed for logging
 
 logScope:
   topics = "vm opcode"
 
 func invalidInstruction*(computation: var BaseComputation) {.inline.} =
-  raise newException(ValueError, "Invalid instruction, received an opcode not implemented in the current fork.")
+  raise newException(InvalidInstruction, "Invalid instruction, received an opcode not implemented in the current fork.")
 
 let FrontierOpDispatch {.compileTime.}: array[Op, NimNode] = block:
   fill_enum_table_holes(Op, newIdentNode("invalidInstruction")):
@@ -256,24 +256,14 @@ proc homesteadVM(computation: var BaseComputation) =
   if not computation.execPrecompiles:
     genHomesteadDispatch(computation)
 
-proc updateOpcodeExec*(computation: var BaseComputation, fork: Fork) =
+proc executeOpcodes(computation: var BaseComputation) =
+  # TODO: Optimise getting fork and updating opCodeExec only when necessary
+  let fork = computation.getFork
+
   case fork
   of FkFrontier..FkThawing:
-    computation.opCodeExec = frontierVM
     computation.frontierVM()
   of FkHomestead..FkSpurious:
-    computation.opCodeExec = homesteadVM
     computation.homesteadVM()
   else:
     raise newException(VMError, "Unknown or not implemented fork: " & $fork)
-
-proc executeOpcodes*(computation: var BaseComputation) =
-  # TODO: Optimise getting fork and updating opCodeExec only when necessary
-  let fork = computation.getFork
-  try:
-    computation.updateOpcodeExec(fork)
-  except VMError:
-    computation.error = Error(info: getCurrentExceptionMsg())
-    debug "VM Error executeOpcodes() failed", error = getCurrentExceptionMsg()
-  except EVMError:
-    debug "EVM Error executeOpcodes() failed", error = getCurrentExceptionMsg()

nimbus/vm_state_transactions.nim

@@ -64,28 +64,18 @@ proc setupComputation*(vmState: BaseVMState, tx: Transaction, sender, recipient:
   doAssert result.isOriginComputation
 
 proc execComputation*(computation: var BaseComputation): bool =
-  var snapshot = computation.snapshot()
-  defer: snapshot.dispose()
+  if computation.msg.isCreate:
+    result = computation.applyMessage(Create)
+  else:
+    result = computation.applyMessage(Call)
 
   computation.vmState.mutateStateDB:
-    db.subBalance(computation.msg.origin, computation.msg.value)
-    db.addBalance(computation.msg.storageAddress, computation.msg.value)
-
-  try:
-    computation.executeOpcodes()
-    computation.vmState.mutateStateDB:
-      for deletedAccount in computation.getAccountsForDeletion:
+    for deletedAccount in computation.accountsForDeletion:
       db.deleteAccount deletedAccount
-    result = not computation.isError
-  except ValueError:
-    result = false
-    debug "execComputation() error", msg = getCurrentExceptionMsg()
 
   if result:
-    snapshot.commit()
     computation.vmState.addLogs(computation.logEntries)
   else:
-    snapshot.revert()
     if computation.tracingEnabled: computation.traceError()
 
 proc refundGas*(computation: BaseComputation, tx: Transaction, sender: EthAddress): GasInt =

nimbus/vm_types.nim

@@ -44,8 +44,6 @@ type
     accounts*: HashSet[EthAddress]
     storageKeys*: seq[HashSet[Uint256]]
 
-  OpcodeExecutor* = proc(computation: var BaseComputation)
-
   BaseComputation* = ref object of RootObj
     # The execution computation
     vmState*:               BaseVMState
@@ -62,7 +60,6 @@ type
     accountsToDelete*:      Table[EthAddress, EthAddress]
     opcodes*:               Table[Op, proc(computation: var BaseComputation){.nimcall.}]
     gasCosts*:              GasCosts # TODO - will be hidden at a lower layer
-    opCodeExec*:            OpcodeExecutor
     forkOverride*:          Option[Fork]
     logEntries*:            seq[Log]
 
@@ -71,13 +68,6 @@ type
     burnsGas*:              bool
     erasesReturnData*:      bool
 
-  Opcode* = ref object of RootObj
-    # TODO can't use a stack-allocated object because
-    # "BaseComputation is not a concrete type"
-    # TODO: We can probably remove this.
-    kind*: Op
-    runLogic*:  proc(computation: var BaseComputation)
-
   GasMeter* = object
     gasRefunded*: GasInt
     startGas*: GasInt

tests/test_vm_json.nim

@@ -7,13 +7,9 @@
 
 import
   unittest, strformat, strutils, sequtils, tables, json, ospaths, times,
-  byteutils, ranges/typedranges,
-  eth/[rlp, common], eth/trie/db,
-  ./test_helpers,
-  ../nimbus/[constants, errors],
-  ../nimbus/[vm_state, vm_types],
-  ../nimbus/utils,
-  ../nimbus/vm/interpreter,
+  byteutils, ranges/typedranges, eth/[rlp, common], eth/trie/db,
+  ./test_helpers, ../nimbus/vm/interpreter,
+  ../nimbus/[constants, errors, vm_state, vm_types, utils],
   ../nimbus/db/[db_chain, state_db]
 
 proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus)
@@ -60,7 +56,10 @@ proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus) =
                                   createAddress = toAddress))
 
   var computation = newBaseComputation(vmState, header.blockNumber, message)
+  try:
     computation.executeOpcodes()
+  except VMError:
+    computation.error = Error(info: getCurrentExceptionMsg())
 
   if not fixture{"post"}.isNil:
     # Success checks
@@ -94,5 +93,5 @@ proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus) =
   else:
     # Error checks
     check(computation.isError)
-      # TODO postState = fixture{"pre"}
-
+    if not fixture{"pre"}.isNil:
+      verifyStateDb(fixture{"pre"}, computation.vmState.readOnlyStateDB)