From 55494f06e52c2663e35c2ceebaaa0f23d1e03379 Mon Sep 17 00:00:00 2001
From: andri lim <jangko128@gmail.com>
Date: Fri, 24 Jan 2020 19:52:55 +0700
Subject: [PATCH] move 'validateTransaction' from GST into 'processTransaction'

---
 nimbus/p2p/executor.nim          | 25 +++----------------
 nimbus/vm_state_transactions.nim | 42 ++++++++++++++++++++++----------
 nimbus/vm_types.nim              |  1 +
 tests/test_generalstate_json.nim | 19 ---------------
 4 files changed, 34 insertions(+), 53 deletions(-)

diff --git a/nimbus/p2p/executor.nim b/nimbus/p2p/executor.nim
index 040b4e441..9644a6e00 100644
--- a/nimbus/p2p/executor.nim
+++ b/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
-
-  block:
-    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 gasUsed = 0.GasInt
+  if validateTransaction(vmState, tx, sender, fork):
+    gasUsed = tx.gasLimit
     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)
diff --git a/nimbus/vm_state_transactions.nim b/nimbus/vm_state_transactions.nim
index 932a2854d..14cf84ed0 100644
--- a/nimbus/vm_state_transactions.nim
+++ b/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
-    #transaction.gasPrice <= (1 shl 34) and
-    limitAndValue <= account.balance and
-    tx.accountNonce == account.nonce and
-    account.balance >= gasCost
+  if vmState.cumulativeGasUsed + tx.gasLimit > vmState.blockHeader.gasLimit:
+    debug "invalid tx: block header gasLimit reached",
+      maxLimit=vmState.blockHeader.gasLimit,
+      gasUsed=vmState.cumulativeGasUsed,
+      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)
-
-  if gas < 0:
-    debug "not enough gas to perform calculation", gas=gas
-    return
+  assert gas >= 0
 
   vmState.setupTxContext(
     origin = sender,
diff --git a/nimbus/vm_types.nim b/nimbus/vm_types.nim
index 0be161bb5..782597670 100644
--- a/nimbus/vm_types.nim
+++ b/nimbus/vm_types.nim
@@ -35,6 +35,7 @@ type
     txGasPrice*    : GasInt
     gasCosts*      : GasCosts
     fork*          : Fork
+    gasCost*       : Uint256
 
   AccessLogs* = ref object
     reads*: Table[string, string]
diff --git a/tests/test_generalstate_json.nim b/tests/test_generalstate_json.nim
index 009ae5cf0..7f49bddf6 100644
--- a/tests/test_generalstate_json.nim
+++ b/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