fixes coinbase suicide bugs

BlockchainTests.md

@@ -228,7 +228,7 @@ OK: 96/96 Fail: 0/96 Skip: 0/96
 + OverflowGasRequire.json                                         OK
 + RefundOverflow.json                                             OK
 + RefundOverflow2.json                                            OK
-  SuicidesMixingCoinbase.json                                     Skip
++ SuicidesMixingCoinbase.json                                     OK
 + TransactionFromCoinbaseHittingBlockGasLimit1.json               OK
 + TransactionFromCoinbaseNotEnoughFounds.json                     OK
 + TransactionNonceCheck.json                                      OK
@@ -277,8 +277,8 @@ OK: 96/96 Fail: 0/96 Skip: 0/96
 + randomStatetest619.json                                         OK
   randomStatetest94.json                                          Skip
 + simpleSuicide.json                                              OK
-  suicideCoinbase.json                                            Skip
++ suicideCoinbase.json                                            OK
-  suicideCoinbaseState.json                                       Skip
++ suicideCoinbaseState.json                                       OK
 + suicideStorageCheck.json                                        OK
 + suicideStorageCheckVCreate.json                                 OK
 + suicideStorageCheckVCreate2.json                                OK
@@ -286,7 +286,7 @@ OK: 96/96 Fail: 0/96 Skip: 0/96
 + transactionFromNotExistingAccount.json                          OK
 + txCost-sec73.json                                               OK
 ```
-OK: 63/67 Fail: 0/67 Skip: 4/67
+OK: 66/67 Fail: 0/67 Skip: 1/67
 ## bcTotalDifficultyTest
 ```diff
 + lotsOfBranchesOverrideAtTheEnd.json                             OK
@@ -404,4 +404,4 @@ OK: 20/20 Fail: 0/20 Skip: 0/20
 OK: 5/5 Fail: 0/5 Skip: 0/5
 
 ---TOTAL---
-OK: 312/318 Fail: 0/318 Skip: 6/318
+OK: 315/318 Fail: 0/318 Skip: 3/318

GeneralStateTests.md

@@ -1364,13 +1364,13 @@ OK: 0/16 Fail: 0/16 Skip: 16/16
 + randomStatetest9.json                                           OK
 + randomStatetest90.json                                          OK
 + randomStatetest92.json                                          OK
-  randomStatetest94.json                                          Skip
++ randomStatetest94.json                                          OK
 + randomStatetest95.json                                          OK
 + randomStatetest96.json                                          OK
 + randomStatetest97.json                                          OK
 + randomStatetest98.json                                          OK
 ```
-OK: 322/327 Fail: 0/327 Skip: 5/327
+OK: 323/327 Fail: 0/327 Skip: 4/327
 ## stRandom2
 ```diff
 + 201503110226PYTHON_DUP6.json                                    OK
@@ -1963,7 +1963,7 @@ OK: 7/7 Fail: 0/7 Skip: 0/7
 + static_RETURN_Bounds.json                                       OK
 + static_RETURN_BoundsOOG.json                                    OK
 + static_RawCallGasAsk.json                                       OK
-  static_Return50000_2.json                                       Skip
++ static_Return50000_2.json                                       OK
 + static_ReturnTest.json                                          OK
 + static_ReturnTest2.json                                         OK
 + static_RevertDepth2.json                                        OK
@@ -2137,7 +2137,7 @@ OK: 7/7 Fail: 0/7 Skip: 0/7
 + static_refund_CallToSuicideNoStorage.json                       OK
 + static_refund_CallToSuicideTwice.json                           OK
 ```
-OK: 270/284 Fail: 0/284 Skip: 14/284
+OK: 271/284 Fail: 0/284 Skip: 13/284
 ## stSystemOperationsTest
 ```diff
 + ABAcalls0.json                                                  OK
@@ -2201,14 +2201,14 @@ OK: 270/284 Fail: 0/284 Skip: 14/284
 + suicideCaller.json                                              OK
 + suicideCallerAddresTooBigLeft.json                              OK
 + suicideCallerAddresTooBigRight.json                             OK
-  suicideCoinbase.json                                            Skip
++ suicideCoinbase.json                                            OK
 + suicideNotExistingAccount.json                                  OK
 + suicideOrigin.json                                              OK
 + suicideSendEtherPostDeath.json                                  OK
 + suicideSendEtherToMe.json                                       OK
 + testRandomTest.json                                             OK
 ```
-OK: 56/67 Fail: 0/67 Skip: 11/67
+OK: 57/67 Fail: 0/67 Skip: 10/67
 ## stTransactionTest
 ```diff
 + ContractStoreClearsOOG.json                                     OK
@@ -2239,7 +2239,7 @@ OK: 56/67 Fail: 0/67 Skip: 11/67
 + SuicidesAndInternlCallSuicidesOOG.json                          OK
 + SuicidesAndInternlCallSuicidesSuccess.json                      OK
 + SuicidesAndSendMoneyToItselfEtherDestroyed.json                 OK
-  SuicidesMixingCoinbase.json                                     Skip
++ SuicidesMixingCoinbase.json                                     OK
 + SuicidesStopAfterSuicide.json                                   OK
 + TransactionDataCosts652.json                                    OK
 + TransactionFromCoinbaseHittingBlockGasLimit.json                OK
@@ -2256,7 +2256,7 @@ OK: 56/67 Fail: 0/67 Skip: 11/67
 + UserTransactionZeroCost.json                                    OK
 + UserTransactionZeroCostWithData.json                            OK
 ```
-OK: 43/44 Fail: 0/44 Skip: 1/44
+OK: 44/44 Fail: 0/44 Skip: 0/44
 ## stTransitionTest
 ```diff
 + createNameRegistratorPerTxsAfter.json                           OK
@@ -2644,4 +2644,5 @@ OK: 133/133 Fail: 0/133 Skip: 0/133
 ```
 OK: 130/130 Fail: 0/130 Skip: 0/130
 
-OK: 2336/2447 Fail: 0/2447 Skip: 111/2447
+---TOTAL---
+OK: 2343/2447 Fail: 0/2447 Skip: 104/2447

nimbus/p2p/executor.nim

@@ -14,7 +14,6 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
   trace "txHash", rlpHash = tx.rlpHash
 
   var gasUsed = tx.gasLimit
-  var coinBaseSuicide = false
 
   block:
     if vmState.cumulativeGasUsed + gasUsed > vmState.blockHeader.gasLimit:
@@ -32,8 +31,8 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
     let recipient = tx.getRecipient()
     let isCollision = vmState.readOnlyStateDb().hasCodeOrNonce(recipient)
 
-    var computation = setupComputation(vmState, tx, sender, recipient, fork)
+    var c = setupComputation(vmState, tx, sender, recipient, fork)
-    if computation.isNil: # OOG in setupComputation
+    if c.isNil: # OOG in setupComputation
       gasUsed = 0
       break
 
@@ -42,21 +41,19 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta
       db.subBalance(sender, upfrontGasCost)
 
     if tx.isContractCreation and isCollision: break
-    execComputation(computation)
+    execComputation(c)
-    if not computation.shouldBurnGas:
+    if not c.shouldBurnGas:
-      gasUsed = computation.refundGas(tx, sender)
+      gasUsed = c.refundGas(tx, sender)
-
-    coinBaseSuicide = computation.isSuicided(vmState.blockHeader.coinbase)
 
   vmState.cumulativeGasUsed += gasUsed
 
-  # miner fee
   vmState.mutateStateDB:
-    if not coinBaseSuicide:
+    # miner fee
-      let txFee = gasUsed.u256 * tx.gasPrice.u256
+    let txFee = gasUsed.u256 * tx.gasPrice.u256
-      db.addBalance(vmState.blockHeader.coinbase, txFee)
+    db.addBalance(vmState.blockHeader.coinbase, txFee)
-    else:
+
-      db.addBalance(vmState.blockHeader.coinbase, 0.u256)
+    for deletedAccount in vmState.suicides:
+      db.deleteAccount deletedAccount
 
     if fork >= FkSpurious:
       vmState.touchedAccounts.incl(vmState.blockHeader.coinbase)

nimbus/vm/computation.nim

@@ -222,10 +222,9 @@ proc registerAccountForDeletion*(c: Computation, beneficiary: EthAddress) =
 proc addLogEntry*(c: Computation, log: Log) {.inline.} =
   c.logEntries.add(log)
 
-iterator accountsForDeletion*(c: Computation): EthAddress =
+proc getSuicides*(c: Computation): HashSet[EthAddress] =
   if c.isSuccess:
-    for address in c.suicides:
+    result = c.suicides
-      yield address
 
 proc getGasRefund*(c: Computation): GasInt =
   if c.isSuccess:
@@ -243,6 +242,10 @@ proc getGasRemaining*(c: Computation): GasInt =
   else:
     result = c.gasMeter.gasRemaining
 
+proc refundSelfDestruct*(c: Computation) =
+  let cost = gasFees[c.fork][RefundSelfDestruct]
+  c.gasMeter.refundGas(cost * c.suicides.len)
+
 proc collectTouchedAccounts*(c: Computation) =
   ## Collect all of the accounts that *may* need to be deleted based on EIP161:
   ## https://github.com/ethereum/EIPs/blob/master/EIPS/eip-161.md

nimbus/vm_state_transactions.nim

@@ -67,19 +67,12 @@ proc execComputation*(c: Computation) =
   else:
     c.applyMessage(Call)
 
-  c.vmState.mutateStateDB:
-    var suicidedCount = 0
-    for deletedAccount in c.accountsForDeletion:
-      db.deleteAccount deletedAccount
-      inc suicidedCount
-
-    # FIXME: hook this into actual RefundSelfDestruct
-    const RefundSelfDestruct = 24_000
-    c.gasMeter.refundGas(RefundSelfDestruct * suicidedCount)
-
   if c.fork >= FkSpurious:
     c.collectTouchedAccounts()
 
+  c.refundSelfDestruct()
+  c.vmState.suicides = c.getSuicides()
+  
   c.vmstate.status = c.isSuccess
   if c.isSuccess:
     c.vmState.addLogs(c.logEntries)

nimbus/vm_types.nim

@@ -26,6 +26,7 @@ type
     accountDb*     : AccountStateDB
     cumulativeGasUsed*: GasInt
     touchedAccounts*: HashSet[EthAddress]
+    suicides*      : HashSet[EthAddress]
     status*        : bool
 
   AccessLogs* = ref object

tests/test_allowed_to_fail.nim

@@ -155,12 +155,7 @@ func skipBCTests*(folder: string, name: string): bool =
     "randomStatetest94.json", # pre istanbul
 
     # BC huge memory consumption
-    "DelegateCallSpam.json",
+    "DelegateCallSpam.json"
-
-    # pre istanbul failing
-    "SuicidesMixingCoinbase.json",
-    "suicideCoinbase.json",
-    "suicideCoinbaseState.json"
   ]
 
   result =  name in allowedFailingBCTests

tests/test_generalstate_json.nim

@@ -6,7 +6,7 @@
 # at your option. This file may not be copied, modified, or distributed except according to those terms.
 
 import
-  unittest2, strformat, strutils, tables, json, times, os,
+  unittest2, strformat, strutils, tables, json, times, os, sets,
   stew/ranges/typedranges, nimcrypto, options,
   eth/[rlp, common], eth/trie/[db, trie_defs], chronicles,
   ./test_helpers, ./test_allowed_to_fail,
@@ -131,6 +131,23 @@ proc testFixtureIndexes(tester: Tester, testStatusIMPL: var TestStatus) =
 
   gasUsed = tester.tx.processTransaction(sender, vmState, tester.fork)
 
+  # This is necessary due to the manner in which the state tests are
+  # generated. State tests are generated from the BlockChainTest tests
+  # in which these transactions are included in the larger context of a
+  # block and thus, the mechanisms which would touch/create/clear the
+  # coinbase account based on the mining reward are present during test
+  # generation, but not part of the execution, thus we must artificially
+  # create the account in VMs prior to the state clearing rules,
+  # as well as conditionally cleaning up the coinbase account when left
+  # empty in VMs after the state clearing rules came into effect.
+  let miner = tester.header.coinbase
+  if miner in vmState.suicides:
+    vmState.mutateStateDB:
+      db.addBalance(miner, 0.u256)
+      if tester.fork >= FkSpurious:
+        if db.isEmptyAccount(miner):
+          db.deleteAccount(miner)
+
 proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus,
                  trace = false, debugMode = false, supportedForks: set[Fork] = supportedForks) =
   var tester: Tester