diff --git a/BlockchainTests.md b/BlockchainTests.md index 80eae95f3..610a918e6 100644 --- a/BlockchainTests.md +++ b/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 - suicideCoinbaseState.json Skip ++ suicideCoinbase.json OK ++ 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 diff --git a/GeneralStateTests.md b/GeneralStateTests.md index f7668cb1f..caa48fcf1 100644 --- a/GeneralStateTests.md +++ b/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 diff --git a/nimbus/p2p/executor.nim b/nimbus/p2p/executor.nim index 597dbb398..5d6f9244d 100644 --- a/nimbus/p2p/executor.nim +++ b/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) - if computation.isNil: # OOG in setupComputation + var c = setupComputation(vmState, tx, sender, recipient, fork) + 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) - if not computation.shouldBurnGas: - gasUsed = computation.refundGas(tx, sender) - - coinBaseSuicide = computation.isSuicided(vmState.blockHeader.coinbase) + execComputation(c) + if not c.shouldBurnGas: + gasUsed = c.refundGas(tx, sender) vmState.cumulativeGasUsed += gasUsed - # miner fee vmState.mutateStateDB: - if not coinBaseSuicide: - let txFee = gasUsed.u256 * tx.gasPrice.u256 - db.addBalance(vmState.blockHeader.coinbase, txFee) - else: - db.addBalance(vmState.blockHeader.coinbase, 0.u256) + # miner fee + let txFee = gasUsed.u256 * tx.gasPrice.u256 + db.addBalance(vmState.blockHeader.coinbase, txFee) + + for deletedAccount in vmState.suicides: + db.deleteAccount deletedAccount if fork >= FkSpurious: vmState.touchedAccounts.incl(vmState.blockHeader.coinbase) diff --git a/nimbus/vm/computation.nim b/nimbus/vm/computation.nim index be86d734e..23c55ab08 100644 --- a/nimbus/vm/computation.nim +++ b/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: - yield address + result = c.suicides 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 diff --git a/nimbus/vm_state_transactions.nim b/nimbus/vm_state_transactions.nim index 8bc7f1cf9..83d51def4 100644 --- a/nimbus/vm_state_transactions.nim +++ b/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) diff --git a/nimbus/vm_types.nim b/nimbus/vm_types.nim index 377aeb9f8..ff7472e38 100644 --- a/nimbus/vm_types.nim +++ b/nimbus/vm_types.nim @@ -26,6 +26,7 @@ type accountDb* : AccountStateDB cumulativeGasUsed*: GasInt touchedAccounts*: HashSet[EthAddress] + suicides* : HashSet[EthAddress] status* : bool AccessLogs* = ref object diff --git a/tests/test_allowed_to_fail.nim b/tests/test_allowed_to_fail.nim index 97e713a13..cde30de60 100644 --- a/tests/test_allowed_to_fail.nim +++ b/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", - - # pre istanbul failing - "SuicidesMixingCoinbase.json", - "suicideCoinbase.json", - "suicideCoinbaseState.json" + "DelegateCallSpam.json" ] result = name in allowedFailingBCTests diff --git a/tests/test_generalstate_json.nim b/tests/test_generalstate_json.nim index 39968762f..009ae5cf0 100644 --- a/tests/test_generalstate_json.nim +++ b/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