refactor aspects of transaction processing out of GeneralStateTests handler; get p2p/chain runnning through block 48680; combine/deduplicate two different but equivalent methods of caulculating transaction intrinsic gas cost; fix BaseTransaction references to just Transaction references in hitherto unreferenced stubbed out source so it builds; clean up some imports from refactoring (#161)

nimbus/p2p/chain.nim

@@ -1,6 +1,8 @@
 import ../db/[db_chain, state_db], eth_common, chronicles, ../vm_state, ../vm_types, ../transaction, ranges,
   ../vm/[computation, interpreter_dispatch, message], ../constants, stint, nimcrypto, ../utils/addresses,
-  eth_trie/memdb, eth_trie, rlp
+  ../vm_state_transactions,
+  eth_trie/memdb, eth_trie, rlp,
+  sugar
 
 
 type
@@ -31,13 +33,6 @@ method getSuccessorHeader*(c: Chain, h: BlockHeader, output: var BlockHeader): b
 method getBlockBody*(c: Chain, blockHash: KeccakHash): BlockBodyRef =
   result = nil
 
-proc dataGas(data: openarray[byte]): GasInt =
-  for i in data:
-    if i == 0:
-      result += 4
-    else:
-      result += 68
-
 proc processTransaction(db: var AccountStateDB, t: Transaction, sender: EthAddress, head: BlockHeader, chainDB: BaseChainDB): UInt256 =
   ## Process the transaction, write the results to db.
   ## Returns amount of ETH to be rewarded to miner
@@ -47,6 +42,9 @@ proc processTransaction(db: var AccountStateDB, t: Transaction, sender: EthAddre
   db.setNonce(sender, db.getNonce(sender) + 1)
   var transactionFailed = false
 
+  #t.dump
+
+  # TODO: combine/refactor re validate
   let upfrontGasCost = t.gasLimit.u256 * t.gasPrice.u256
   let upfrontCost = upfrontGasCost + t.value
   var balance = db.getBalance(sender)
@@ -65,26 +63,53 @@ proc processTransaction(db: var AccountStateDB, t: Transaction, sender: EthAddre
   if transactionFailed:
     return
 
-  var gasUsed = 21000.GasInt
-  if t.payload.len != 0:
-    gasUsed += dataGas(t.payload)
+  var gasUsed = t.payload.intrinsicGas.GasInt
+  #echo "gasUsed is ", gasUsed
 
   if t.isContractCreation:
     # gasUsed += 32000 # This appears in Homestead.
     echo "Contract creation"
 
+  # TODO: check the 200x constant up front
   if gasUsed > t.gasLimit:
     echo "Transaction failed. Out of gas."
     transactionFailed = true
   else:
     if t.isContractCreation:
-      db.setCode(generateAddress(sender, t.accountNonce), t.payload.toRange)
-      let msg = newMessage(t.gasLimit, t.gasPrice, t.to, sender, t.value, @[], t.payload)
+      # TODO: split out into separate function?; interleaving the cases obfuscates
       let vmState = newBaseVMState(head, chainDB)
+
+      # TODO: setupComputation
+      let msg = newMessage(t.gasLimit - gasUsed, t.gasPrice, t.to, sender, t.value, @[], t.payload)
       var c = newBaseComputation(vmState, head.blockNumber, msg)
-      c.executeOpcodes()
-      echo "isError: ", c.isError
-      # TODO: Handle failure, and gas consumption
+
+      if execComputation(c, vmState):
+        let contractAddress = generateAddress(sender, t.accountNonce)
+        db.setCode(contractAddress, c.output.toRange)
+        db.addBalance(contractAddress, t.value)
+
+        # XXX: copy/pasted from GST fixture
+        # TODO: more merging/refactoring/etc
+        # also a couple lines can collapse because variable used once
+        # once verified in GST fixture
+        let
+          gasRemaining = c.gasMeter.gasRemaining.u256
+          gasRefunded = c.gasMeter.gasRefunded.u256
+          gasUsed2 = t.gasLimit.u256 - gasRemaining
+          gasRefund = min(gasRefunded, gasUsed2 div 2)
+          gasRefundAmount = (gasRefund + gasRemaining) * t.gasPrice.u256
+        #echo "gasRemaining is ", gasRemaining, " and gasRefunded = ", gasRefunded, " and gasUsed2 = ", gasUsed2, " and gasRefund = ", gasRefund, " and gasRefundAmount = ", gasRefundAmount
+        let codeCost = (200 * c.output.len).u256
+        db.addBalance(sender, (t.gasLimit.u256 - gasUsed2 - codeCost)*t.gasPrice.u256)
+        echo c.output.len, " bytes: ", c.output
+        return (gasUsed2 + codeCost) * t.gasPrice.u256
+
+      else:
+        # FIXME: don't do this revert, but rather only subBalance correctly
+        # the if transactionfailed at end is what is supposed to pick it up
+        db.addBalance(sender, t.value)
+        echo "isError: ", c.isError
+        return upfrontGasCost
 
     else:
       let code = db.getCode(t.to)
@@ -150,6 +175,7 @@ method persistBlocks*(c: Chain, headers: openarray[BlockHeader], bodies: openarr
             assert(false, "Could not get sender")
 
     var mainReward = blockReward + gasReward
+    #echo "mainReward = ", mainReward , " with blockReward = ", blockReward, " and gasReward = ", gasReward
 
     if headers[i].ommersHash != EMPTY_UNCLE_HASH:
       let h = c.db.persistUncles(bodies[i].uncles)
@@ -160,6 +186,7 @@ method persistBlocks*(c: Chain, headers: openarray[BlockHeader], bodies: openarr
         uncleReward = uncleReward * blockReward
         uncleReward = uncleReward div 8.u256
         stateDb.addBalance(bodies[i].uncles[u].coinbase, uncleReward)
+        #echo "adding via unclehash to mainReward: ", (blockReward div 32.u256)
         mainReward += blockReward div 32.u256
 
     # Reward beneficiary

nimbus/vm_state_transactions.nim

@@ -6,11 +6,61 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  strformat, tables,
+  ranges/typedranges, sequtils, strformat, tables,
+  eth_common,
   ./constants, ./errors, ./vm/computation,
-  ./transaction, ./vm_types, ./vm_state, ./block_types, ./db/db_chain, ./utils/header
+  ./transaction, ./vm_types, ./vm_state, ./block_types, ./db/[db_chain, state_db], ./utils/header,
+  ./vm/interpreter
 
-method executeTransaction(vmState: var BaseVMState, transaction: BaseTransaction): (BaseComputation, BlockHeader) {.base.}=
+func intrinsicGas*(data: openarray[byte]): GasInt =
+  result = 21_000
+  for i in data:
+    if i == 0:
+      result += 4
+    else:
+      result += 68
+
+proc validateTransaction*(vmState: BaseVMState, transaction: Transaction, sender: EthAddress): bool =
+  # XXX: https://github.com/status-im/nimbus/issues/35#issuecomment-391726518
+  # XXX: lots of avoidable u256 construction
+  var readOnlyDB = vmState.readOnlyStateDB
+  let limitAndValue = transaction.gasLimit.u256 + transaction.value
+  let gas_cost = transaction.gasLimit.u256 * transaction.gasPrice.u256
+
+  transaction.gasLimit >= transaction.payload.intrinsicGas and
+    transaction.gasPrice <= (1 shl 34) and
+    limitAndValue <= readOnlyDB.getBalance(sender) and
+    transaction.accountNonce == readOnlyDB.getNonce(sender) and
+    readOnlyDB.getBalance(sender) >= gas_cost
+
+proc setupComputation*(header: BlockHeader, vmState: var BaseVMState, transaction: Transaction, sender: EthAddress) : BaseComputation =
+  let message = newMessage(
+      gas = transaction.gasLimit - transaction.payload.intrinsicGas,
+      gasPrice = transaction.gasPrice,
+      to = transaction.to,
+      sender = sender,
+      value = transaction.value,
+      data = transaction.payload,
+      code = vmState.readOnlyStateDB.getCode(transaction.to).toSeq,
+      options = newMessageOptions(origin = sender,
+                                  createAddress = transaction.to))
+
+  result = newBaseComputation(vmState, header.blockNumber, message)
+  result.precompiles = initTable[string, Opcode]()
+  doAssert result.isOriginComputation
+
+proc execComputation*(computation: var BaseComputation, vmState: BaseVMState): bool =
+  try:
+    computation.executeOpcodes()
+    #[vmState.mutateStateDB:
+      for deletedAccount in computation.getAccountsForDeletion:
+        db.deleteAccount deletedAccount]#
+
+    result = not computation.isError
+  except ValueError:
+    result = false
+
+method executeTransaction(vmState: var BaseVMState, transaction: Transaction): (BaseComputation, BlockHeader) {.base.}=
   # Execute the transaction in the vm
   # TODO: introduced here: https://github.com/ethereum/py-evm/commit/21c57f2d56ab91bb62723c3f9ebe291d0b132dde
   # Refactored/Removed here: https://github.com/ethereum/py-evm/commit/cc991bf
@@ -18,7 +68,7 @@ method executeTransaction(vmState: var BaseVMState, transaction: BaseTransaction
   raise newException(ValueError, "Must be implemented by subclasses")
 
 
-method addTransaction*(vmState: var BaseVMState, transaction: BaseTransaction, computation: BaseComputation, b: Block): (Block, Table[string, string]) =
+method addTransaction*(vmState: var BaseVMState, transaction: Transaction, computation: BaseComputation, b: Block): (Block, Table[string, string]) =
   # Add a transaction to the given block and
   # return `trieData` to store the transaction data in chaindb in VM layer
   # Update the bloomFilter, transaction trie and receipt trie roots, bloom_filter,
@@ -49,7 +99,7 @@ method addTransaction*(vmState: var BaseVMState, transaction: BaseTransaction, c
 
 method applyTransaction*(
     vmState: var BaseVMState,
-    transaction: BaseTransaction,
+    transaction: Transaction,
     b: Block,
     isStateless: bool): (BaseComputation, Block, Table[string, string]) =
   # Apply transaction to the given block

tests/test_generalstate_json.nim

@@ -6,13 +6,13 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  unittest, strformat, strutils, sequtils, tables, json, ospaths, times,
+  unittest, strformat, strutils, tables, json, ospaths, times,
   byteutils, ranges/typedranges, nimcrypto/[keccak, hash],
   rlp, eth_trie/[types, memdb], eth_common,
   eth_keys,
   ./test_helpers,
   ../nimbus/[constants, errors],
-  ../nimbus/[vm_state, vm_types],
+  ../nimbus/[vm_state, vm_types, vm_state_transactions],
   ../nimbus/utils/header,
   ../nimbus/vm/interpreter,
   ../nimbus/db/[db_chain, state_db]
@@ -23,47 +23,6 @@ suite "generalstate json tests":
   jsonTest("GeneralStateTests", testFixture)
 
 
-proc validateTransaction(vmState: BaseVMState, transaction: Transaction, sender: EthAddress): bool =
-  # XXX: https://github.com/status-im/nimbus/issues/35#issuecomment-391726518
-  # XXX: lots of avoidable u256 construction
-  var readOnlyDB = vmState.readOnlyStateDB
-  let limitAndValue = transaction.gasLimit.u256 + transaction.value
-  let gas_cost = transaction.gasLimit.u256 * transaction.gasPrice.u256
-
-  transaction.gasLimit >= transaction.getFixtureIntrinsicGas and
-    transaction.gasPrice <= (1 shl 34) and
-    limitAndValue <= readOnlyDB.getBalance(sender) and
-    transaction.accountNonce == readOnlyDB.getNonce(sender) and
-    readOnlyDB.getBalance(sender) >= gas_cost
-
-proc setupComputation(header: BlockHeader, vmState: var BaseVMState, transaction: Transaction, sender: EthAddress) : BaseComputation =
-  let message = newMessage(
-      gas = transaction.gasLimit - transaction.getFixtureIntrinsicGas,
-      gasPrice = transaction.gasPrice,
-      to = transaction.to,
-      sender = sender,
-      value = transaction.value,
-      data = transaction.payload,
-      code = vmState.readOnlyStateDB.getCode(transaction.to).toSeq,
-      options = newMessageOptions(origin = sender,
-                                  createAddress = transaction.to))
-
-  # doAssert not message.isCreate
-  result = newBaseComputation(vmState, header.blockNumber, message)
-  result.precompiles = initTable[string, Opcode]()
-  doAssert result.isOriginComputation
-
-proc execComputation(computation: var BaseComputation, vmState: var BaseVMState): bool =
-  try:
-    computation.executeOpcodes()
-    vmState.mutateStateDB:
-      for deletedAccount in computation.getAccountsForDeletion:
-        db.deleteAccount deletedAccount
-
-    result = not computation.isError
-  except ValueError:
-    result = false
-
 proc testFixtureIndexes(header: BlockHeader, pre: JsonNode, transaction: Transaction, sender: EthAddress, expectedHash: string) =
   var vmState = newBaseVMState(header, newBaseChainDB(newMemoryDb()))
   vmState.mutateStateDB:
@@ -101,6 +60,8 @@ proc testFixtureIndexes(header: BlockHeader, pre: JsonNode, transaction: Transac
       gasRefundAmount = (gasRefund + gasRemaining) * transaction.gasPrice.u256
 
     vmState.mutateStateDB:
+      for deletedAccount in computation.getAccountsForDeletion:
+        db.deleteAccount deletedAccount
       if header.coinbase notin computation.getAccountsForDeletion:
         db.subBalance(header.coinbase, gasRefundAmount)
         db.addBalance(header.coinbase, gas_cost)

tests/test_helpers.nim

@@ -6,7 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  os, macros, json, sequtils, strformat, strutils, parseutils, ospaths, tables,
+  os, macros, json, strformat, strutils, parseutils, ospaths, tables,
   byteutils, eth_common, eth_keys, ranges/typedranges,
   ../nimbus/[vm_state, constants],
   ../nimbus/db/[db_chain, state_db],
@@ -247,17 +247,3 @@ proc getFixtureTransactionSender*(j: JsonNode): EthAddress =
   else:
     # XXX: appropriate failure mode; probably raise something
     discard
-
-proc getFixtureIntrinsicGas*(transaction: Transaction) : auto =
-  # Py-EVM has _get_homestead_intrinsic_gas and _get_frontier_intrinsic_gas
-  # Using former.
-
-  # TODO: refactor and pull from nimbus/vm/interpreter/gas_costs.nim
-  let
-    gasTransaction = 21_000
-    gasTXDataZero = 4
-    gasTXDataNonZero = 68
-    numZeroBytes = transaction.payload.count(0)
-    gasCosts = forkToSchedule(FkHomestead)
-
-  result = gasTransaction + gasTXDataZero * numZeroBytes + gasTXDataNonZero * (transaction.payload.len - numZeroBytes)