Merge pull request #607 from ethereumjs/istanbul-tests

[WIP] Add state tests for Istanbul

.circleci/config.yml

@@ -82,6 +82,15 @@ jobs:
       - run:
           name: testStatePetersburg
           command: npm run testStatePetersburg
+  test_state_istanbul:
+    <<: *defaults
+    steps:
+      - attach_workspace:
+          at: ~/project
+      - *restore_node_modules
+      - run:
+          name: testStateIstanbul
+          command: npm run testStateIstanbul
   test_blockchain:
     <<: *defaults
     steps:
@@ -91,6 +100,15 @@ jobs:
       - run:
           name: testBlockchain
           command: npm run testBlockchain
+  test_blockchain_petersburg:
+    <<: *defaults
+    steps:
+      - attach_workspace:
+          at: ~/project
+      - *restore_node_modules
+      - run:
+          name: testBlockchainPetersburg
+          command: npm run testBlockchainPetersburg
   coveralls:
     <<: *defaults
     steps:
@@ -123,9 +141,15 @@ workflows:
       - test_state_petersburg:
           requires:
             - install
+      - test_state_istanbul:
+          requires:
+            - install
       - test_blockchain:
           requires:
             - install
+      - test_blockchain_petersburg:
+          requires:
+            - install
       - coveralls:
           requires:
             - install

lib/evm/eei.ts

@@ -33,7 +33,6 @@ export interface Env {
 export interface RunResult {
   logs: any // TODO: define type for Log (each log: [Buffer(address), [Buffer(topic0), ...]])
   returnValue?: Buffer
-  gasRefund: BN
   /**
    * A map from the accounts that have self-destructed to the addresses to send their funds to
    */
@@ -67,7 +66,6 @@ export default class EEI {
     this._result = {
       logs: [],
       returnValue: undefined,
-      gasRefund: new BN(0),
       selfdestruct: {},
     }
   }
@@ -90,17 +88,17 @@ export default class EEI {
    * @param amount - Amount of gas refunded
    */
   refundGas(amount: BN): void {
-    this._result.gasRefund.iadd(amount)
+    this._evm._refund.iadd(amount)
   }
 
   /**
    * Reduces amount of gas to be refunded by a positive value.
    * @param amount - Amount to subtract from gas refunds
    */
   subRefund(amount: BN): void {
-    this._result.gasRefund.isub(amount)
-    if (this._result.gasRefund.ltn(0)) {
-      this._result.gasRefund = new BN(0)
+    this._evm._refund.isub(amount)
+    if (this._evm._refund.ltn(0)) {
+      this._evm._refund = new BN(0)
       trap(ERROR.REFUND_EXHAUSTED)
     }
   }
@@ -163,10 +161,6 @@ export default class EEI {
    * input data passed with the message call instruction or transaction.
    */
   getCallDataSize(): BN {
-    if (this._env.callData.length === 1 && this._env.callData[0] === 0) {
-      return new BN(0)
-    }
-
     return new BN(this._env.callData.length)
   }
 
@@ -354,9 +348,7 @@ export default class EEI {
   async _selfDestruct(toAddress: Buffer): Promise<void> {
     // only add to refund if this is the first selfdestruct for the address
     if (!this._result.selfdestruct[this._env.address.toString('hex')]) {
-      this._result.gasRefund = this._result.gasRefund.addn(
-        this._common.param('gasPrices', 'selfdestructRefund'),
-      )
+      this.refundGas(new BN(this._common.param('gasPrices', 'selfdestructRefund')))
     }
 
     this._result.selfdestruct[this._env.address.toString('hex')] = toAddress
@@ -491,11 +483,6 @@ export default class EEI {
       this._result.logs = this._result.logs.concat(results.execResult.logs)
     }
 
-    // add gasRefund
-    if (results.execResult.gasRefund) {
-      this._result.gasRefund = this._result.gasRefund.add(results.execResult.gasRefund)
-    }
-
     // this should always be safe
     this.useGas(results.gasUsed)
 
@@ -553,11 +540,6 @@ export default class EEI {
       this._result.logs = this._result.logs.concat(results.execResult.logs)
     }
 
-    // add gasRefund
-    if (results.execResult.gasRefund) {
-      this._result.gasRefund = this._result.gasRefund.add(results.execResult.gasRefund)
-    }
-
     // this should always be safe
     this.useGas(results.gasUsed)
 

lib/evm/evm.ts

@@ -10,7 +10,7 @@ import {
 import Account from 'ethereumjs-account'
 import { ERROR, VmError } from '../exceptions'
 import PStateManager from '../state/promisified'
-import { getPrecompile, PrecompileFunc } from './precompiles'
+import { getPrecompile, PrecompileFunc, ripemdPrecompileAddress } from './precompiles'
 import TxContext from './txContext'
 import Message from './message'
 import EEI from './eei'
@@ -61,14 +61,14 @@ export interface ExecResult {
    * Array of logs that the contract emitted
    */
   logs?: any[]
-  /**
-   * Amount of gas to refund from deleting storage values
-   */
-  gasRefund?: BN
   /**
    * A map from the accounts that have self-destructed to the addresses to send their funds to
    */
   selfdestruct?: { [k: string]: Buffer }
+  /**
+   * Total amount of gas to be refunded from all nested calls.
+   */
+  gasRefund?: BN
 }
 
 export interface NewContractEvent {
@@ -96,12 +96,17 @@ export default class EVM {
   _state: PStateManager
   _tx: TxContext
   _block: any
+  /**
+   * Amount of gas to refund from deleting storage values
+   */
+  _refund: BN
 
   constructor(vm: any, txContext: TxContext, block: any) {
     this._vm = vm
     this._state = this._vm.pStateManager
     this._tx = txContext
     this._block = block
+    this._refund = new BN(0)
   }
 
   /**
@@ -120,20 +125,14 @@ export default class EVM {
     } else {
       result = await this._executeCreate(message)
     }
+    // TODO: Move `gasRefund` to a tx-level result object
+    // instead of `ExecResult`.
+    result.execResult.gasRefund = this._refund.clone()
 
     const err = result.execResult.exceptionError
     if (err) {
       result.execResult.logs = []
       await this._state.revert()
-      if (message.isCompiled) {
-        // Empty precompiled contracts need to be deleted even in case of OOG
-        // because the bug in both Geth and Parity led to deleting RIPEMD precompiled in this case
-        // see https://github.com/ethereum/go-ethereum/pull/3341/files#diff-2433aa143ee4772026454b8abd76b9dd
-        // We mark the account as touched here, so that is can be removed among other touched empty accounts (after tx finalization)
-        if (err.error === ERROR.OUT_OF_GAS) {
-          await this._touchAccount(message.to)
-        }
-      }
     } else {
       await this._state.commit()
     }
@@ -289,6 +288,7 @@ export default class EVM {
       eei._result.selfdestruct = message.selfdestruct
     }
 
+    const oldRefund = this._refund.clone()
     const interpreter = new Interpreter(this._vm, eei)
     const interpreterRes = await interpreter.run(message.code as Buffer, opts)
 
@@ -303,9 +303,10 @@ export default class EVM {
       result = {
         ...result,
         logs: [],
-        gasRefund: new BN(0),
         selfdestruct: {},
       }
+      // Revert gas refund if message failed
+      this._refund = oldRefund
     }
 
     return {

lib/evm/opFns.ts

@@ -959,9 +959,7 @@ function updateSstoreGas(runState: RunState, found: any, value: Buffer) {
       // If original value is not 0
       if (current.length === 0) {
         // If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
-        runState.eei._result.gasRefund.isub(
-          new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
-        )
+        runState.eei.subRefund(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))
       } else if (value.length === 0) {
         // If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
         runState.eei.refundGas(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))

lib/evm/precompiles/09-blake2f.ts

@@ -404,7 +404,7 @@ export default function(opts: PrecompileInput): ExecResult {
   const f = lastByte === 1
 
   const gasUsed = new BN(opts._common.param('gasPrices', 'blake2Round'))
-  gasUsed.imuln(rounds)
+  gasUsed.imul(new BN(rounds))
   if (opts.gasLimit.lt(gasUsed)) {
     return OOGResult(opts.gasLimit)
   }

lib/evm/precompiles/index.ts

@@ -13,10 +13,11 @@ interface Precompiles {
   [key: string]: PrecompileFunc
 }
 
+const ripemdPrecompileAddress = '0000000000000000000000000000000000000003'
 const precompiles: Precompiles = {
   '0000000000000000000000000000000000000001': p1,
   '0000000000000000000000000000000000000002': p2,
-  '0000000000000000000000000000000000000003': p3,
+  [ripemdPrecompileAddress]: p3,
   '0000000000000000000000000000000000000004': p4,
   '0000000000000000000000000000000000000005': p5,
   '0000000000000000000000000000000000000006': p6,
@@ -29,4 +30,4 @@ function getPrecompile(address: string): PrecompileFunc {
   return precompiles[address]
 }
 
-export { precompiles, getPrecompile, PrecompileFunc, PrecompileInput }
+export { precompiles, getPrecompile, PrecompileFunc, PrecompileInput, ripemdPrecompileAddress }

lib/runTx.ts

@@ -154,10 +154,10 @@ async function _runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> {
   // Caculate the total gas used
   results.gasUsed = results.gasUsed.add(basefee)
   // Process any gas refund
-  results.gasRefund = results.execResult.gasRefund
-  if (results.gasRefund) {
-    if (results.gasRefund.lt(results.gasUsed.divn(2))) {
-      results.gasUsed.isub(results.gasRefund)
+  const gasRefund = evm._refund
+  if (gasRefund) {
+    if (gasRefund.lt(results.gasUsed.divn(2))) {
+      results.gasUsed.isub(gasRefund)
     } else {
       results.gasUsed.isub(results.gasUsed.divn(2))
     }

lib/state/stateManager.ts

@@ -8,6 +8,7 @@ import Common from 'ethereumjs-common'
 import { genesisStateByName } from 'ethereumjs-common/dist/genesisStates'
 import Account from 'ethereumjs-account'
 import Cache from './cache'
+import { ripemdPrecompileAddress } from '../evm/precompiles'
 
 /**
  * Storage values of an account
@@ -102,11 +103,22 @@ export default class StateManager {
     // if (toAccount.balance.toString('hex') === '00') {
     // if they have money or a non-zero nonce or code, then write to tree
     this._cache.put(address, account)
-    this._touched.add(address.toString('hex'))
+    this.touchAccount(address)
     // self._trie.put(addressHex, account.serialize(), cb)
     cb()
   }
 
+  /**
+   * Marks an account as touched, according to the definition
+   * in [EIP-158](https://github.com/ethereum/EIPs/issues/158).
+   * This happens when the account is triggered for a state-changing
+   * event. Touched accounts that are empty will be cleared
+   * at the end of the tx.
+   */
+  touchAccount(address: Buffer): void {
+    this._touched.add(address.toString('hex'))
+  }
+
   /**
    * Adds `value` to the state trie as code, and sets `codeHash` on the account
    * corresponding to `address` to reference this.
@@ -275,7 +287,7 @@ export default class StateManager {
         const contract = this._cache.get(address)
         contract.stateRoot = storageTrie.root
         this.putAccount(address, contract, cb)
-        this._touched.add(address.toString('hex'))
+        this.touchAccount(address)
       })
     })
   }
@@ -372,6 +384,13 @@ export default class StateManager {
     if (!touched) {
       throw new Error('Reverting to invalid state checkpoint failed')
     }
+    // Exceptional case due to consensus issue in Geth and Parity.
+    // See [EIP-716](https://github.com/ethereum/EIPs/issues/716) for context.
+    // The RIPEMD precompile has to remain *touched* even when the call reverts,
+    // and be considered for deletion.
+    if (this._touched.has(ripemdPrecompileAddress)) {
+      touched.add(ripemdPrecompileAddress)
+    }
     this._touched = touched
     this._checkpointCount--
 

package.json

@@ -18,8 +18,10 @@
     "testStateByzantium": "npm run build:dist && node ./tests/tester -s --fork='Byzantium' --dist",
     "testStateConstantinople": "npm run build:dist && node ./tests/tester -s --fork='Constantinople' --dist",
     "testStatePetersburg": "npm run build:dist && node ./tests/tester -s --fork='Petersburg' --dist",
+    "testStateIstanbul": "npm run build:dist && node ./tests/tester -s --fork='Istanbul' --dist",
     "testBuildIntegrity": "npm run build:dist && node ./tests/tester -s --dist --test='stackOverflow'",
-    "testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'",
+    "testBlockchain": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Istanbul' --dist --excludeDir='GeneralStateTests'",
+    "testBlockchainPetersburg": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --fork='Petersburg' --dist --excludeDir='GeneralStateTests'",
     "testBlockchainGeneralStateTests": "npm run build:dist && node --stack-size=1500 ./tests/tester -b --dist --dir='GeneralStateTests'",
     "testAPI": "npm run build:dist && tape './tests/api/**/*.js'",
     "testAPI:browser": "npm run build:dist && karma start karma.conf.js",
@@ -51,11 +53,11 @@
     "async-eventemitter": "^0.2.2",
     "core-js-pure": "^3.0.1",
     "ethereumjs-account": "^3.0.0",
-    "ethereumjs-block": "~2.2.0",
-    "ethereumjs-blockchain": "^4.0.1",
+    "ethereumjs-block": "^2.2.1",
+    "ethereumjs-blockchain": "^4.0.2",
     "ethereumjs-common": "^1.3.2",
     "ethereumjs-tx": "^2.1.1",
-    "ethereumjs-util": "^6.1.0",
+    "ethereumjs-util": "~6.1.0",
     "fake-merkle-patricia-tree": "^1.0.1",
     "functional-red-black-tree": "^1.0.1",
     "merkle-patricia-tree": "^2.3.2",
@@ -74,7 +76,7 @@
     "@types/node": "^11.13.4",
     "browserify": "^16.2.3",
     "coveralls": "^3.0.0",
-    "ethereumjs-testing": "git+https://github.com/ethereumjs/ethereumjs-testing.git#v1.2.7",
+    "ethereumjs-testing": "git+https://github.com/ethereumjs/ethereumjs-testing.git#v1.3.0",
     "husky": "^2.1.0",
     "karma": "^4.0.1",
     "karma-browserify": "^6.0.0",

tests/GeneralStateTestsRunner.js

@@ -62,7 +62,7 @@ function runTestCase (options, testData, t, cb) {
       testUtil.setupPreConditions(state, testData, done)
     },
     function (done) {
-      var tx = testUtil.makeTx(testData.transaction)
+      var tx = testUtil.makeTx(testData.transaction, options.forkConfig.toLowerCase())
       block = testUtil.makeBlockFromEnv(testData.env)
       tx._homestead = true
       tx.enableHomestead = true

tests/api/index.js

@@ -87,8 +87,8 @@ tape('VM with blockchain', (t) => {
 
   t.test('should run blockchain with mocked runBlock', async (st) => {
     const vm = setupVM({ chain: 'goerli' })
-    const genesis = new Block(Buffer.from(testData.genesisRLP.slice(2), 'hex'))
-    const block = new Block(Buffer.from(testData.blocks[0].rlp.slice(2), 'hex'))
+    const genesis = new Block(Buffer.from(testData.genesisRLP.slice(2), 'hex'), { common: vm._common })
+    const block = new Block(Buffer.from(testData.blocks[0].rlp.slice(2), 'hex'), { common: vm._common })
 
     await putGenesisP(vm.blockchain, genesis)
     st.equal(vm.blockchain.meta.genesis.toString('hex'), testData.genesisBlockHeader.hash.slice(2))
@@ -114,8 +114,8 @@ tape('VM with blockchain', (t) => {
 
   t.test('should run blockchain with blocks', async (st) => {
     const vm = setupVM({ chain: 'goerli' })
-    const genesis = new Block(Buffer.from(testData.genesisRLP.slice(2), 'hex'))
-    const block = new Block(Buffer.from(testData.blocks[0].rlp.slice(2), 'hex'))
+    const genesis = new Block(Buffer.from(testData.genesisRLP.slice(2), 'hex'), { common: vm._common })
+    const block = new Block(Buffer.from(testData.blocks[0].rlp.slice(2), 'hex'), { common: vm._common })
 
     await putGenesisP(vm.blockchain, genesis)
     st.equal(vm.blockchain.meta.genesis.toString('hex'), testData.genesisBlockHeader.hash.slice(2))