Merge pull request #447 from status-im/tx_validation

move 'validateTransaction' from GST into 'processTransaction'

nimbus/p2p/executor.nim

@@ -13,29 +13,12 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
   trace "Sender", sender
   trace "txHash", rlpHash = tx.rlpHash
 
-  var gasUsed = tx.gasLimit
+  var gasUsed = 0.GasInt
-
+  if validateTransaction(vmState, tx, sender, fork):
-  block:
+    gasUsed = tx.gasLimit
-    if vmState.cumulativeGasUsed + gasUsed > vmState.blockHeader.gasLimit:
-      debug "invalid tx: block header gasLimit reached",
-        blockGasLimit=vmState.blockHeader.gasLimit,
-        gasUsed=gasUsed,
-        txGasLimit=tx.gasLimit
-      gasUsed = 0
-      break
-
-    let upfrontGasCost = tx.gasLimit.u256 * tx.gasPrice.u256
-    var balance = vmState.readOnlyStateDb().getBalance(sender)
-    if balance < upfrontGasCost: break
-
     var c = setupComputation(vmState, tx, sender, fork)
-    if c.isNil: # OOG in setupComputation
-      gasUsed = 0
-      break
-
     vmState.mutateStateDB:
-      db.subBalance(sender, upfrontGasCost)
+      db.subBalance(sender, vmState.gasCost)
-
     execComputation(c)
     if not c.shouldBurnGas:
       gasUsed = c.refundGas(tx, sender)

nimbus/vm_state_transactions.nim

@@ -12,26 +12,42 @@ import
   ./vm/[computation, interpreter]
 
 proc validateTransaction*(vmState: BaseVMState, tx: Transaction, sender: EthAddress, fork: Fork): bool =
-  # XXX: https://github.com/status-im/nimbus/issues/35#issuecomment-391726518
-  # XXX: lots of avoidable u256 construction
   let
     account = vmState.readOnlyStateDB.getAccount(sender)
     gasLimit = tx.gasLimit.u256
-    limitAndValue = gasLimit + tx.value
-    gasCost = gasLimit * tx.gasPrice.u256
 
-  tx.gasLimit >= tx.intrinsicGas(fork) and
+  if vmState.cumulativeGasUsed + tx.gasLimit > vmState.blockHeader.gasLimit:
-    #transaction.gasPrice <= (1 shl 34) and
+    debug "invalid tx: block header gasLimit reached",
-    limitAndValue <= account.balance and
+      maxLimit=vmState.blockHeader.gasLimit,
-    tx.accountNonce == account.nonce and
+      gasUsed=vmState.cumulativeGasUsed,
-    account.balance >= gasCost
+      addition=tx.gasLimit
+    return
+
+  vmState.gasCost = gasLimit * tx.gasPrice.u256
+  let totalCost = vmState.gasCost + tx.value
+  if totalCost > account.balance:
+    debug "invalid tx: not enough cash",
+      available=account.balance,
+      require=totalCost
+    return
+
+  if tx.gasLimit < tx.intrinsicGas(fork):
+    debug "invalid tx: not enough gas to perform calculation",
+      available=tx.gasLimit,
+      require=tx.intrinsicGas(fork)
+    return
+
+  if tx.accountNonce != account.nonce:
+    debug "invalid tx: account nonce mismatch",
+      txNonce=tx.accountnonce,
+      accountNonce=account.nonce
+    return
+
+  result = true
 
 proc setupComputation*(vmState: BaseVMState, tx: Transaction, sender: EthAddress, fork: Fork) : Computation =
   var gas = tx.gasLimit - tx.intrinsicGas(fork)
-
+  assert gas >= 0
-  if gas < 0:
-    debug "not enough gas to perform calculation", gas=gas
-    return
 
   vmState.setupTxContext(
     origin = sender,

nimbus/vm_types.nim

@@ -35,6 +35,7 @@ type
     txGasPrice*    : GasInt
     gasCosts*      : GasCosts
     fork*          : Fork
+    gasCost*       : Uint256
 
   AccessLogs* = ref object
     reads*: Table[string, string]

tests/test_generalstate_json.nim

@@ -110,25 +110,6 @@ proc testFixtureIndexes(tester: Tester, testStatusIMPL: var TestStatus) =
       let success = expectedLogsHash == actualLogsHash and obtainedHash == tester.expectedHash
       tester.dumpDebugData(vmState, sender, gasUsed, success)
 
-  if not validateTransaction(vmState, tester.tx, sender, tester.fork):
-    vmState.mutateStateDB:
-      # pre-EIP158 (e.g., Byzantium) should ensure currentCoinbase exists
-      # in later forks, don't create at all
-      db.addBalance(tester.header.coinbase, 0.u256)
-
-      # TODO: this feels not right to be here
-      # perhaps the entire validateTransaction block
-      # should be moved into processTransaction
-      if tester.fork >= FkSpurious:
-        let miner = tester.header.coinbase
-        let touchedAccounts = [miner]
-        for account in touchedAccounts:
-          debug "state clearing", account
-          if db.accountExists(account) and db.isEmptyAccount(account):
-            db.deleteAccount(account)
-
-    return
-
   gasUsed = tester.tx.processTransaction(sender, vmState, tester.fork)
 
   # This is necessary due to the manner in which the state tests are