StateManager interface simplification

mattdean-digicatapult · holgerd77 · commit c346a0446402 · 2018-11-15T13:41:25.000+01:00
Simplifies the stateManager interface used by the VM by removing dependencies on the following methods:
* getAccountBalance
* putAccountBalance
* copy
diff --git a/lib/opFns.js b/lib/opFns.js
@@ -219,11 +219,11 @@ module.exports = {
     }
 
     // otherwise load account then return balance
-    stateManager.getAccountBalance(address, function (err, value) {
+    stateManager.getAccount(address, function (err, account) {
       if (err) {
         return cb(err)
       }
-      cb(null, new BN(value))
+      cb(null, new BN(account.balance))
     })
   },
   ORIGIN: function (runState) {
@@ -702,7 +702,6 @@ module.exports = {
     })
   },
   STATICCALL: function (gasLimit, toAddress, inOffset, inLength, outOffset, outLength, runState, done) {
-    var stateManager = runState.stateManager
     var value = new BN(0)
     toAddress = addressToBuffer(toAddress)
 
@@ -723,22 +722,15 @@ module.exports = {
       outLength: outLength
     }
 
-    stateManager.accountIsEmpty(toAddress, function (err, empty) {
-      if (err) {
-        done(err)
-        return
-      }
-
-      try {
-        checkCallMemCost(runState, options, localOpts)
-        checkOutOfGas(runState, options)
-      } catch (e) {
-        done(e.error)
-        return
-      }
+    try {
+      checkCallMemCost(runState, options, localOpts)
+      checkOutOfGas(runState, options)
+    } catch (e) {
+      done(e.error)
+      return
+    }
 
-      makeCall(runState, options, localOpts, done)
-    })
+    makeCall(runState, options, localOpts, done)
   },
   RETURN: function (offset, length, runState) {
     runState.returnValue = memLoad(runState, offset, length)
@@ -790,9 +782,17 @@ module.exports = {
         runState.stopped = true
 
         var newBalance = new BN(contract.balance).add(new BN(toAccount.balance))
-        async.series([
-          stateManager.putAccountBalance.bind(stateManager, selfdestructToAddress, newBalance),
-          stateManager.putAccountBalance.bind(stateManager, contractAddress, new BN(0))
+        async.waterfall([
+          stateManager.getAccount.bind(stateManager, selfdestructToAddress),
+          function (account, cb) {
+            account.balance = newBalance
+            stateManager.putAccount(selfdestructToAddress, account, cb)
+          },
+          stateManager.getAccount.bind(stateManager, contractAddress),
+          function (account, cb) {
+            account.balance = new BN(0)
+            stateManager.putAccount(contractAddress, account, cb)
+          }
         ], function (err) {
           // The reason for this is to avoid sending an array of results
           cb(err)
@@ -970,6 +970,7 @@ function makeCall (runState, callOptions, localOpts, cb) {
   callOptions.block = runState.block
   callOptions.static = callOptions.static || false
   callOptions.selfdestruct = runState.selfdestruct
+  callOptions.storageReader = runState.storageReader
 
   // increment the runState.depth
   callOptions.depth = runState.depth + 1
@@ -1057,10 +1058,7 @@ function isCreateOpCode (opName) {
 
 function getContractStorage (runState, address, key, cb) {
   if (runState._common.gteHardfork('constantinople')) {
-    async.parallel({
-      original: function (cb) { runState.originalState.getContractStorage(address, key, cb) },
-      current: function (cb) { runState.stateManager.getContractStorage(address, key, cb) }
-    }, cb)
+    runState.storageReader.getContractStorage(address, key, cb)
   } else {
     runState.stateManager.getContractStorage(address, key, cb)
   }
diff --git a/lib/runCall.js b/lib/runCall.js
@@ -3,6 +3,7 @@ const async = require('async')
 const ethUtil = require('ethereumjs-util')
 const BN = ethUtil.BN
 const exceptions = require('./exceptions.js')
+const StorageReader = require('./storageReader')
 
 const ERROR = exceptions.ERROR
 
@@ -48,6 +49,7 @@ module.exports = function (opts, cb) {
   var delegatecall = opts.delegatecall || false
   var isStatic = opts.static || false
   var salt = opts.salt || null
+  var storageReader = opts.storageReader || new StorageReader(stateManager)
 
   txValue = new BN(txValue)
 
@@ -149,7 +151,7 @@ module.exports = function (opts, cb) {
     }
     var newBalance = new BN(account.balance).sub(txValue)
     account.balance = newBalance
-    stateManager.putAccountBalance(ethUtil.toBuffer(caller), newBalance, cb)
+    stateManager.putAccount(ethUtil.toBuffer(caller), account, cb)
   }
 
   function addTxValue (cb) {
@@ -205,7 +207,8 @@ module.exports = function (opts, cb) {
       block: block,
       depth: depth,
       selfdestruct: selfdestruct,
-      static: isStatic
+      static: isStatic,
+      storageReader: storageReader
     }
 
     // run Code through vm
diff --git a/lib/runCode.js b/lib/runCode.js
@@ -18,6 +18,7 @@ const Block = require('ethereumjs-block')
 const lookupOpInfo = require('./opcodes.js')
 const opFns = require('./opFns.js')
 const exceptions = require('./exceptions.js')
+const StorageReader = require('./storageReader')
 const setImmediate = require('timers').setImmediate
 const BN = utils.BN
 
@@ -64,7 +65,7 @@ module.exports = function (opts, cb) {
   var runState = {
     blockchain: self.blockchain,
     stateManager: stateManager,
-    originalState: stateManager.copy(),
+    storageReader: opts.storageReader || new StorageReader(stateManager),
     returnValue: false,
     stopped: false,
     vmError: false,
diff --git a/lib/runTx.js b/lib/runTx.js
@@ -5,6 +5,7 @@ const BN = utils.BN
 const Bloom = require('./bloom.js')
 const Block = require('ethereumjs-block')
 const Account = require('ethereumjs-account')
+const StorageReader = require('./storageReader')
 
 /**
  * Process a transaction. Run the vm. Transfers eth. Checks balances.
@@ -39,6 +40,7 @@ module.exports = function (opts, cb) {
   var gasLimit
   var results
   var basefee
+  var storageReader = new StorageReader(self.stateManager)
 
   // tx is required
   if (!tx) {
@@ -141,7 +143,8 @@ module.exports = function (opts, cb) {
       to: tx.to,
       value: tx.value,
       data: tx.data,
-      block: block
+      block: block,
+      storageReader: storageReader
     }
 
     if (tx.to.toString('hex') === '') {
@@ -196,7 +199,7 @@ module.exports = function (opts, cb) {
           .add(new BN(fromAccount.balance))
         fromAccount.balance = finalFromBalance
 
-        self.stateManager.putAccountBalance(utils.toBuffer(tx.from), finalFromBalance, next)
+        self.stateManager.putAccount(utils.toBuffer(tx.from), fromAccount, next)
       }
 
       var minerAccount
diff --git a/lib/stateManager.js b/lib/stateManager.js
@@ -52,29 +52,6 @@ proto.putAccount = function (address, account, cb) {
   cb()
 }
 
-proto.getAccountBalance = function (address, cb) {
-  var self = this
-  self.getAccount(address, function (err, account) {
-    if (err) {
-      return cb(err)
-    }
-    cb(null, account.balance)
-  })
-}
-
-proto.putAccountBalance = function (address, balance, cb) {
-  var self = this
-
-  self.getAccount(address, function (err, account) {
-    if (err) {
-      return cb(err)
-    }
-
-    account.balance = balance
-    self.putAccount(address, account, cb)
-  })
-}
-
 // sets the contract code on the account
 proto.putContractCode = function (address, value, cb) {
   var self = this
@@ -307,6 +284,7 @@ proto.accountIsEmpty = function (address, cb) {
       return cb(err)
     }
 
+    // should be replaced by account.isEmpty() once updated
     cb(null, account.nonce.toString('hex') === '' && account.balance.toString('hex') === '' && account.codeHash.toString('hex') === utils.KECCAK256_NULL_S)
   })
 }
diff --git a/lib/storageReader.js b/lib/storageReader.js
@@ -0,0 +1,40 @@
+module.exports = StorageReader
+
+function StorageReader (stateManager) {
+  this._stateManager = stateManager
+  this._storageCache = new Map()
+}
+
+const proto = StorageReader.prototype
+
+proto.getContractStorage = function getContractStorage (address, key, cb) {
+  const self = this
+  const addressHex = address.toString('hex')
+  const keyHex = key.toString('hex')
+
+  self._stateManager.getContractStorage(address, key, function (err, current) {
+    if (err) return cb(err)
+
+    let map = null
+    if (!self._storageCache.has(addressHex)) {
+      map = new Map()
+      self._storageCache.set(addressHex, map)
+    } else {
+      map = self._storageCache.get(addressHex)
+    }
+
+    let original = null
+
+    if (map.has(keyHex)) {
+      original = map.get(keyHex)
+    } else {
+      map.set(keyHex, current)
+      original = current
+    }
+
+    cb(null, {
+      original,
+      current
+    })
+  })
+}
diff --git a/tests/api/storageReader.js b/tests/api/storageReader.js
@@ -0,0 +1,82 @@
+const { promisify } = require('util')
+const tape = require('tape')
+const StorageReader = require('../../lib/storageReader')
+
+const mkStateManagerMock = () => {
+  let i = 0
+  return {
+    getContractStorage: function (address, key, cb) {
+      cb(null, i++)
+    }
+  }
+}
+
+tape('StateManager', (t) => {
+  t.test('should get value from stateManager', async (st) => {
+    const storageReader = new StorageReader(mkStateManagerMock())
+    const getContractStorage = promisify((...args) => storageReader.getContractStorage(...args))
+    const {
+      original,
+      current
+    } = await getContractStorage(Buffer.from([1]), Buffer.from([1, 1]))
+
+    st.equal(original, 0)
+    st.equal(current, 0)
+    st.end()
+  })
+
+  t.test('should retrieve original from cache', async (st) => {
+    const storageReader = new StorageReader(mkStateManagerMock())
+    const getContractStorage = promisify((...args) => storageReader.getContractStorage(...args))
+    await getContractStorage(Buffer.from([1]), Buffer.from([1, 1]))
+    const {
+      original,
+      current
+    } = await getContractStorage(Buffer.from([1]), Buffer.from([1, 1]))
+
+    st.equal(original, 0)
+    st.equal(current, 1)
+    st.end()
+  })
+
+  t.test('should cache keys separately', async (st) => {
+    const storageReader = new StorageReader(mkStateManagerMock())
+    const getContractStorage = promisify((...args) => storageReader.getContractStorage(...args))
+    await getContractStorage(Buffer.from([1]), Buffer.from([1, 1]))
+    const {
+      original,
+      current
+    } = await getContractStorage(Buffer.from([1]), Buffer.from([1, 2]))
+
+    st.equal(original, 1)
+    st.equal(current, 1)
+    st.end()
+  })
+
+  t.test('should cache addresses separately', async (st) => {
+    const storageReader = new StorageReader(mkStateManagerMock())
+    const getContractStorage = promisify((...args) => storageReader.getContractStorage(...args))
+    await getContractStorage(Buffer.from([1]), Buffer.from([1, 1]))
+    const {
+      original,
+      current
+    } = await getContractStorage(Buffer.from([2]), Buffer.from([1, 1]))
+
+    st.equal(original, 1)
+    st.equal(current, 1)
+    st.end()
+  })
+
+  t.test('should forward errors from stateManager.getContractStorage', async (st) => {
+    const storageReader = new StorageReader({
+      getContractStorage: (address, key, cb) => cb(new Error('test'))
+    })
+    const getContractStorage = promisify((...args) => storageReader.getContractStorage(...args))
+
+    await getContractStorage(Buffer.from([2]), Buffer.from([1, 1]))
+      .then(() => t.fail('should have returned error'))
+      .catch((e) => t.equal(e.message, 'test'))
+
+    st.end()
+  })
+})
