From 03435c7beb2fa84a979997844f95216aa6c2d795 Mon Sep 17 00:00:00 2001 From: andri lim Date: Thu, 18 Apr 2019 07:56:57 +0700 Subject: [PATCH] gst and processTransaction unification --- GeneralStateTests.md | 30 +++++----- nimbus/p2p/executor.nim | 86 +++++++++++++++-------------- nimbus/tracer.nim | 3 - nimbus/vm_state_transactions.nim | 14 +++-- nimbus/vm_types.nim | 3 +- tests/test_generalstate_failing.nim | 25 +++++---- tests/test_generalstate_json.nim | 25 +++++---- 7 files changed, 98 insertions(+), 88 deletions(-) diff --git a/GeneralStateTests.md b/GeneralStateTests.md index 5b2fa96ba..466c7cda2 100644 --- a/GeneralStateTests.md +++ b/GeneralStateTests.md @@ -521,7 +521,7 @@ OK: 5/5 Fail: 0/5 Skip: 0/5 + CallContractToCreateContractWhichWouldCreateContractInInitCode.jsonOK + CallRecursiveContract.json OK + CallTheContractToCreateEmptyContract.json OK - NotEnoughCashContractCreation.json Skip ++ NotEnoughCashContractCreation.json OK OutOfGasContractCreation.json Skip OutOfGasPrefundedContractCreation.json Skip + ReturnTest.json OK @@ -532,7 +532,7 @@ OK: 5/5 Fail: 0/5 Skip: 0/5 + TransactionCreateStopInInitcode.json OK + TransactionCreateSuicideInInitcode.json OK ``` -OK: 14/18 Fail: 0/18 Skip: 4/18 +OK: 15/18 Fail: 0/18 Skip: 3/18 ## stLogTests ```diff + log0_emptyMem.json OK @@ -1287,7 +1287,7 @@ OK: 0/16 Fail: 0/16 Skip: 16/16 OK: 321/327 Fail: 0/327 Skip: 6/327 ## stRandom2 ```diff - 201503110226PYTHON_DUP6.json Skip ++ 201503110226PYTHON_DUP6.json OK + randomStatetest.json OK + randomStatetest384.json OK + randomStatetest385.json OK @@ -1515,7 +1515,7 @@ OK: 321/327 Fail: 0/327 Skip: 6/327 + randomStatetest646.json OK randomStatetest647.json Skip ``` -OK: 222/227 Fail: 0/227 Skip: 5/227 +OK: 223/227 Fail: 0/227 Skip: 4/227 ## stRecursiveCreate ```diff + recursiveCreate.json OK @@ -2090,9 +2090,9 @@ OK: 62/67 Fail: 0/67 Skip: 5/67 + ContractStoreClearsSuccess.json OK + CreateMessageReverted.json OK + CreateMessageSuccess.json OK - CreateTransactionReverted.json Skip ++ CreateTransactionReverted.json OK + CreateTransactionSuccess.json OK - EmptyTransaction.json Skip ++ EmptyTransaction.json OK EmptyTransaction2.json Skip + EmptyTransaction3.json OK + HighGasLimit.json OK @@ -2102,10 +2102,10 @@ OK: 62/67 Fail: 0/67 Skip: 5/67 + InternlCallStoreClearsOOG.json OK + InternlCallStoreClearsSucces.json OK + Opcodes_TransactionInit.json OK - OverflowGasRequire.json Skip ++ OverflowGasRequire.json OK + OverflowGasRequire2.json OK - RefundOverflow.json Skip - RefundOverflow2.json Skip ++ RefundOverflow.json OK ++ RefundOverflow2.json OK + StoreClearsAndInternlCallStoreClearsOOG.json OK + StoreClearsAndInternlCallStoreClearsSuccess.json OK + StoreGasOnCreate.json OK @@ -2120,18 +2120,18 @@ OK: 62/67 Fail: 0/67 Skip: 5/67 + TransactionFromCoinbaseHittingBlockGasLimit.json OK + TransactionFromCoinbaseHittingBlockGasLimit1.json OK + TransactionFromCoinbaseNotEnoughFounds.json OK - TransactionNonceCheck.json Skip - TransactionNonceCheck2.json Skip ++ TransactionNonceCheck.json OK ++ TransactionNonceCheck2.json OK + TransactionSendingToEmpty.json OK TransactionSendingToZero.json Skip + TransactionToAddressh160minusOne.json OK + TransactionToItself.json OK - TransactionToItselfNotEnoughFounds.json Skip - UserTransactionGasLimitIsTooLowWhenZeroCost.json Skip ++ TransactionToItselfNotEnoughFounds.json OK ++ UserTransactionGasLimitIsTooLowWhenZeroCost.json OK UserTransactionZeroCost.json Skip UserTransactionZeroCostWithData.json Skip ``` -OK: 30/44 Fail: 0/44 Skip: 14/44 +OK: 39/44 Fail: 0/44 Skip: 5/44 ## stTransitionTest ```diff + createNameRegistratorPerTxsAfter.json OK @@ -2520,4 +2520,4 @@ OK: 5/133 Fail: 0/133 Skip: 128/133 OK: 0/130 Fail: 0/130 Skip: 130/130 ---TOTAL--- -OK: 1480/2334 Fail: 0/2334 Skip: 854/2334 +OK: 1491/2334 Fail: 0/2334 Skip: 843/2334 diff --git a/nimbus/p2p/executor.nim b/nimbus/p2p/executor.nim index d0b09e418..e3d590e42 100644 --- a/nimbus/p2p/executor.nim +++ b/nimbus/p2p/executor.nim @@ -13,31 +13,48 @@ proc processTransaction*(tx: Transaction, sender: EthAddress, vmState: BaseVMSta trace "Sender", sender trace "txHash", rlpHash = tx.rlpHash - let upfrontGasCost = tx.gasLimit.u256 * tx.gasPrice.u256 - var balance = vmState.readOnlyStateDb().getBalance(sender) - if balance < upfrontGasCost: - return tx.gasLimit + var gasUsed = tx.gasLimit - let recipient = tx.getRecipient() - let isCollision = vmState.readOnlyStateDb().hasCodeOrNonce(recipient) + 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 - var computation = setupComputation(vmState, tx, sender, recipient, forkOverride) - if computation.isNil: - return 0 + let upfrontGasCost = tx.gasLimit.u256 * tx.gasPrice.u256 + var balance = vmState.readOnlyStateDb().getBalance(sender) + if balance < upfrontGasCost: break + let recipient = tx.getRecipient() + let isCollision = vmState.readOnlyStateDb().hasCodeOrNonce(recipient) + + var computation = setupComputation(vmState, tx, sender, recipient, forkOverride) + if computation.isNil: + gasUsed = 0 + break + + vmState.mutateStateDB: + db.incNonce(sender) + db.subBalance(sender, upfrontGasCost) + + if tx.isContractCreation and isCollision: break + if execComputation(computation): + gasUsed = computation.refundGas(tx, sender) + + if computation.isSuicided(vmState.blockHeader.coinbase): + gasUsed = 0 + + vmState.cumulativeGasUsed += gasUsed + + # miner fee + let txFee = gasUsed.u256 * tx.gasPrice.u256 vmState.mutateStateDB: - db.incNonce(sender) - db.subBalance(sender, upfrontGasCost) + db.addBalance(vmState.blockHeader.coinbase, txFee) - if tx.isContractCreation and isCollision: - return tx.gasLimit - - result = tx.gasLimit - if execComputation(computation): - result = computation.refundGas(tx, sender) - - if computation.isSuicided(vmState.blockHeader.coinbase): - return 0 + result = gasUsed type # TODO: these types need to be removed @@ -58,7 +75,7 @@ func createBloom*(receipts: openArray[Receipt]): Bloom = bloom.value = bloom.value or logsBloom(receipt.logs).value result = bloom.value.toByteArrayBE -proc makeReceipt(vmState: BaseVMState, cumulativeGasUsed: GasInt, fork = FkFrontier): Receipt = +proc makeReceipt(vmState: BaseVMState, fork = FkFrontier): Receipt = if fork < FkByzantium: result.stateRootOrStatus = hashOrStatus(vmState.accountDb.rootHash) else: @@ -66,7 +83,7 @@ proc makeReceipt(vmState: BaseVMState, cumulativeGasUsed: GasInt, fork = FkFront let vmStatus = true # success or failure result.stateRootOrStatus = hashOrStatus(vmStatus) - result.cumulativeGasUsed = cumulativeGasUsed + result.cumulativeGasUsed = vmState.cumulativeGasUsed result.logs = vmState.getAndClearLogEntries() result.bloom = logsBloom(result.logs).value.toByteArrayBE @@ -89,26 +106,15 @@ proc processBlock*(chainDB: BaseChainDB, header: BlockHeader, body: BlockBody, v trace "Has transactions", blockNumber = header.blockNumber, blockHash = header.blockHash vmState.receipts = newSeq[Receipt](body.transactions.len) - var cumulativeGasUsed = GasInt(0) + vmState.cumulativeGasUsed = 0 for txIndex, tx in body.transactions: - if cumulativeGasUsed + tx.gasLimit > header.gasLimit: - vmState.mutateStateDB: - db.addBalance(header.coinbase, 0.u256) - # TODO: do we need to break or continue execution? + var sender: EthAddress + if tx.getSender(sender): + let gasUsed = processTransaction(tx, sender, vmState) else: - var sender: EthAddress - if tx.getSender(sender): - let gasUsed = processTransaction(tx, sender, vmState) - cumulativeGasUsed += gasUsed - - # miner fee - let txFee = gasUsed.u256 * tx.gasPrice.u256 - vmState.mutateStateDB: - db.addBalance(header.coinbase, txFee) - else: - debug "Could not get sender", txIndex, tx - return ValidationResult.Error - vmState.receipts[txIndex] = makeReceipt(vmState, cumulativeGasUsed) + debug "Could not get sender", txIndex, tx + return ValidationResult.Error + vmState.receipts[txIndex] = makeReceipt(vmState) var mainReward = blockReward if header.ommersHash != EMPTY_UNCLE_HASH: diff --git a/nimbus/tracer.nim b/nimbus/tracer.nim index 53961d4d2..a373a47f5 100644 --- a/nimbus/tracer.nim +++ b/nimbus/tracer.nim @@ -103,9 +103,6 @@ proc traceTransaction*(db: BaseChainDB, header: BlockHeader, beforeRoot = stateDb.rootHash gasUsed = processTransaction(tx, sender, vmState) - vmState.mutateStateDB: - let txFee = gasUsed.u256 * tx.gasPrice.u256 - db.addBalance(header.coinbase, txFee) if idx == txIndex: after.captureAccount(stateDb, sender, senderName) diff --git a/nimbus/vm_state_transactions.nim b/nimbus/vm_state_transactions.nim index 63ee083f0..1810cc86c 100644 --- a/nimbus/vm_state_transactions.nim +++ b/nimbus/vm_state_transactions.nim @@ -14,15 +14,17 @@ import 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 gasCost = transaction.gasLimit.u256 * transaction.gasPrice.u256 + let + account = vmState.readOnlyStateDB.getAccount(sender) + gasLimit = transaction.gasLimit.u256 + limitAndValue = gasLimit + transaction.value + gasCost = gasLimit * transaction.gasPrice.u256 transaction.gasLimit >= transaction.intrinsicGas and #transaction.gasPrice <= (1 shl 34) and - limitAndValue <= readOnlyDB.getBalance(sender) and - transaction.accountNonce == readOnlyDB.getNonce(sender) and - readOnlyDB.getBalance(sender) >= gasCost + limitAndValue <= account.balance and + transaction.accountNonce == account.nonce and + account.balance >= gasCost proc setupComputation*(vmState: BaseVMState, tx: Transaction, sender, recipient: EthAddress, forkOverride=none(Fork)) : BaseComputation = let fork = diff --git a/nimbus/vm_types.nim b/nimbus/vm_types.nim index c75bdfa4f..7790871a0 100644 --- a/nimbus/vm_types.nim +++ b/nimbus/vm_types.nim @@ -23,7 +23,8 @@ type tracer* : TransactionTracer logEntries* : seq[Log] receipts* : seq[Receipt] - accountDb* : AccountStateDB + accountDb* : AccountStateDB + cumulativeGasUsed*: GasInt AccessLogs* = ref object reads*: Table[string, string] diff --git a/tests/test_generalstate_failing.nim b/tests/test_generalstate_failing.nim index bd74c4151..cd04a6eef 100644 --- a/tests/test_generalstate_failing.nim +++ b/tests/test_generalstate_failing.nim @@ -21,26 +21,15 @@ func allowedFailingGeneralStateTest*(folder, name: string): bool = "doubleSelfdestructTest2.json", # Spurious Dragon failed GST - "CallContractToCreateContractOOG.json", - "NotEnoughCashContractCreation.json", + "CallContractToCreateContractOOG.json", "OutOfGasContractCreation.json", "OutOfGasPrefundedContractCreation.json", - "201503110226PYTHON_DUP6.json", "RevertOpcodeInInit.json", "RevertOpcodeWithBigOutputInInit.json", "failed_tx_xcf416c53.json", "suicideCoinbase.json", - "CreateTransactionReverted.json", - "EmptyTransaction.json", "EmptyTransaction2.json", - "OverflowGasRequire.json", - "RefundOverflow.json", - "RefundOverflow2.json", "SuicidesMixingCoinbase.json", - "TransactionNonceCheck.json", - "TransactionNonceCheck2.json", - "TransactionToItselfNotEnoughFounds.json", - "UserTransactionGasLimitIsTooLowWhenZeroCost.json", "UserTransactionZeroCost.json", "UserTransactionZeroCostWithData.json", "createNameRegistratorPerTxsNotEnoughGasAfter.json", @@ -51,6 +40,18 @@ func allowedFailingGeneralStateTest*(folder, name: string): bool = "NonZeroValue_CALLCODE_ToEmpty.json", "NonZeroValue_CALLCODE_ToOneStorageKey.json", "TransactionSendingToZero.json" + + #"NotEnoughCashContractCreation.json", + #"201503110226PYTHON_DUP6.json", + #"CreateTransactionReverted.json", + #"EmptyTransaction.json", + #"OverflowGasRequire.json", + #"RefundOverflow.json", + #"RefundOverflow2.json", + #"TransactionNonceCheck.json", + #"TransactionNonceCheck2.json", + #"TransactionToItselfNotEnoughFounds.json", + #"UserTransactionGasLimitIsTooLowWhenZeroCost.json", # Homestead recursives #["ContractCreationSpam.json", diff --git a/tests/test_generalstate_json.nim b/tests/test_generalstate_json.nim index ee0b00f38..11b1f4e47 100644 --- a/tests/test_generalstate_json.nim +++ b/tests/test_generalstate_json.nim @@ -107,19 +107,22 @@ proc testFixtureIndexes(tester: Tester, testStatusIMPL: var TestStatus) = # 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 recipient = tester.tx.getRecipient() + let miner = tester.header.coinbase + let touchedAccounts = [sender, miner, recipient] + for account in touchedAccounts: + debug "state clearing", account + if db.accountExists(account) and db.isEmptyAccount(account): + db.deleteAccount(account) + return - if gasUsed + tester.tx.gasLimit <= tester.header.gasLimit: - vmState.mutateStateDB: - gasUsed = tester.tx.processTransaction(sender, vmState, some(tester.fork)) - db.addBalance(tester.header.coinbase, gasUsed.u256 * tester.tx.gasPrice.u256) - else: - debug "invalid tx: block header gasLimit reached", - blockGasLimit=tester.header.gasLimit, - gasUsed=gasUsed, - txGasLimit=tester.tx.gasLimit - vmState.mutateStateDB: - db.addBalance(tester.header.coinbase, 0.u256) + gasUsed = tester.tx.processTransaction(sender, vmState, some(tester.fork)) proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus, debugMode = false, supportedForks: set[Fork] = supportedForks) =