Merge pull request #814 from ethereumjs/cleanup-tangerine-whistle

[VM] cleanup some changes made in the TangerineWhistle PR

Author: holgerd77

packages/vm/lib/evm/eei.ts

@@ -583,6 +583,14 @@ export default class EEI {
     return this._state.accountIsEmpty(address)
   }
 
+  /**
+   * Returns true if account exists in the state trie (it can be empty). Returns false if the account is `null`.
+   * @param address - Address of account
+   */
+  async accountExists(address: Buffer): Promise<boolean> {
+    return this._state.accountExists(address)
+  }
+
   private _getReturnCode(results: EVMResult) {
     // This preserves the previous logic, but seems to contradict the EEI spec
     // https://github.com/ewasm/design/blob/38eeded28765f3e193e12881ea72a6ab807a3371/eth_interface.md

packages/vm/lib/evm/opFns.ts

@@ -676,16 +676,13 @@ export const handlers: { [k: string]: OpHandler } = {
     if (runState._common.gteHardfork('spuriousDragon')) {
       // We are at or after Spurious Dragon
       // Call new account gas: account is DEAD and we transfer nonzero value
-      if ((await runState.stateManager.accountIsEmpty(toAddressBuf)) && !value.isZero()) {
+      if ((await runState.eei.isAccountEmpty(toAddressBuf)) && !value.isZero()) {
         runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
       }
-    } else if (!(await runState.stateManager.accountExists(toAddressBuf))) {
-      // We are before Spurious Dragon
+    } else if (!(await runState.eei.accountExists(toAddressBuf))) {
+      // We are before Spurious Dragon and the account does not exist.
       // Call new account gas: account does not exist (it is not in the state trie, not even as an "empty" account)
-      const accountDoesNotExist = !(await runState.stateManager.accountExists(toAddressBuf))
-      if (accountDoesNotExist) {
-        runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
-      }
+      runState.eei.useGas(new BN(runState._common.param('gasPrices', 'callNewAccount')))
     }
 
     if (!value.isZero()) {

packages/vm/lib/state/cache.ts

@@ -51,30 +51,39 @@ export default class Cache {
     }
   }
 
+  /**
+   * Returns true if the key was deleted and thus existed in the cache earlier
+   * @param key - trie key to lookup
+   */
+  keyIsDeleted(key: Buffer): boolean {
+    const keyStr = key.toString('hex')
+    const it = this._cache.find(keyStr)
+    if (it.node) {
+      return it.value.deleted
+    }
+    return false
+  }
+
   /**
    * Looks up address in underlying trie.
    * @param address - Address of account
-   * @param create - Create emtpy account if non-existent
    */
-  async _lookupAccount(address: Buffer, create: boolean = true): Promise<Account | undefined> {
+  async _lookupAccount(address: Buffer): Promise<Account> {
     const raw = await this._trie.get(address)
-    if (raw || create) {
-      const account = new Account(raw)
-      return account
-    }
+    const account = new Account(raw)
+    return account
   }
 
   /**
    * Looks up address in cache, if not found, looks it up
    * in the underlying trie.
    * @param key - Address of account
-   * @param create - Create emtpy account if non-existent
    */
-  async getOrLoad(key: Buffer, create: boolean = true): Promise<Account | undefined> {
+  async getOrLoad(key: Buffer): Promise<Account> {
     let account = this.lookup(key)
 
     if (!account) {
-      account = await this._lookupAccount(key, create)
+      account = await this._lookupAccount(key)
       if (account) {
         this._update(key, account as Account, false, false)
       }
@@ -114,7 +123,7 @@ export default class Cache {
         it.next()
       } else if (it.value && it.value.deleted) {
         it.value.modified = false
-        it.value.deleted = false
+        it.value.deleted = true
         it.value.val = new Account().serialize()
         await this._trie.del(Buffer.from(it.key, 'hex'))
         next = it.hasNext

packages/vm/lib/state/interface.ts

@@ -10,7 +10,7 @@ export interface StorageDump {
 export interface StateManager {
   copy(): StateManager
   getAccount(address: Buffer): Promise<Account>
-  putAccount(address: Buffer, account: Account | null): Promise<void>
+  putAccount(address: Buffer, account: Account): Promise<void>
   deleteAccount(address: Buffer): Promise<void>
   touchAccount(address: Buffer): void
   putContractCode(address: Buffer, value: Buffer): Promise<void>

packages/vm/lib/state/stateManager.ts

@@ -71,7 +71,7 @@ export default class DefaultStateManager implements StateManager {
    * @param address - Address of the `account` to get
    */
   async getAccount(address: Buffer): Promise<Account> {
-    const account = (await this._cache.getOrLoad(address)) as Account
+    const account = await this._cache.getOrLoad(address)
     return account
   }
 
@@ -89,6 +89,11 @@ export default class DefaultStateManager implements StateManager {
     this.touchAccount(address)
   }
 
+  /**
+   * Deletes an [`@ethereumjs/account`](https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/account)
+   * from state under the provided `address`. The account will also be removed from the state trie.
+   * @param address - Address of the account which should be deleted
+   */
   async deleteAccount(address: Buffer) {
     this._cache.del(address)
     this.touchAccount(address)
@@ -115,7 +120,7 @@ export default class DefaultStateManager implements StateManager {
     const codeHash = keccak256(value)
 
     if (codeHash.equals(KECCAK256_NULL)) {
-      //return
+      return
     }
 
     const account = await this.getAccount(address)
@@ -489,7 +494,7 @@ export default class DefaultStateManager implements StateManager {
    */
   async accountExists(address: Buffer): Promise<boolean> {
     const account = await this._cache.lookup(address)
-    if (account) {
+    if (account && !this._cache.keyIsDeleted(address)) {
       return true
     }
     if (await this._cache._trie.get(address)) {

packages/vm/tests/api/evm/eei.js

@@ -0,0 +1,44 @@
+const tape = require('tape')
+const { zeroAddress } = require('ethereumjs-util')
+const EEI = require('../../../dist/evm/eei').default
+const StateManager = require('../../../dist/state/stateManager').default
+const Account = require('@ethereumjs/account').default
+
+const ZeroAddress = Buffer.from("0000000000000000000000000000000000000000", 'hex')
+
+tape('EEI', t => {
+
+  t.test('should return false on non-existing accounts', async st => {
+    const eei = new EEI(undefined, new StateManager()) // create a dummy EEI (no VM, no EVM, etc.)
+    st.notOk(await eei.accountExists(ZeroAddress))
+    st.ok(await eei.isAccountEmpty(ZeroAddress))
+    st.end()
+  })
+
+  
+  t.test('should return false on non-existing accounts which once existed in state but are now gone', async st => {
+    const eei = new EEI(undefined, new StateManager()) // create a dummy EEI (no VM, no EVM, etc.)
+    // create empty account
+    await eei._state.putAccount(ZeroAddress, new Account())
+    st.ok(await eei.accountExists(ZeroAddress))
+    st.ok(await eei.isAccountEmpty(ZeroAddress))
+    // now put a non-empty account
+    await eei._state.putAccount(ZeroAddress, new Account({nonce: '0x01', balance: '0x01'}))
+    st.ok(await eei.accountExists(ZeroAddress))
+    st.notOk(await eei.isAccountEmpty(ZeroAddress))
+    st.end()
+  }) 
+
+  t.test('should return true on existing accounts', async st => {
+    const eei = new EEI(undefined, new StateManager()) // create a dummy EEI (no VM, no EVM, etc.)
+    // create empty account
+    await eei._state.putAccount(ZeroAddress, new Account())
+    st.ok(await eei.accountExists(ZeroAddress)) // sanity check: account exists before we delete it
+    st.ok(await eei.isAccountEmpty(ZeroAddress)) // it is obviously empty
+    await eei._state.deleteAccount(ZeroAddress) // delete the account
+    st.notOk(await eei.accountExists(ZeroAddress)) // account should not exist
+    st.ok(await eei.isAccountEmpty(ZeroAddress)) // account is empty
+    st.end()
+  })
+
+})