Merge pull request #1794 from MetaMask/nonce-tracker-fixes

Nonce tracker fixes

CHANGELOG.md

@@ -6,6 +6,7 @@
 - Now redirects from known malicious sites faster.
 - Added a link to our new support page to the help screen.
 - Fixed bug where a new transaction would be shown over the current transaction, creating a possible timing attack against user confirmation.
+- Fixed bug in nonce tracker where an incorrect nonce would be calculated.
 
 ## 3.9.0 2017-7-12
 

app/scripts/controllers/transactions.js

@@ -3,6 +3,7 @@ const async = require('async')
 const extend = require('xtend')
 const ObservableStore = require('obs-store')
 const ethUtil = require('ethereumjs-util')
+const pify = require('pify')
 const TxProviderUtil = require('../lib/tx-utils')
 const createId = require('../lib/random-id')
 const NonceTracker = require('../lib/nonce-tracker')
@@ -481,35 +482,51 @@ module.exports = class TransactionController extends EventEmitter {
 
   // checks the network for signed txs and
   // if confirmed sets the tx status as 'confirmed'
-  _checkPendingTxs () {
-    var signedTxList = this.getFilteredTxList({status: 'submitted'})
-    if (!signedTxList.length) return
-    signedTxList.forEach((txMeta) => {
-      var txHash = txMeta.hash
-      var txId = txMeta.id
-      if (!txHash) {
-        const errReason = {
-          errCode: 'No hash was provided',
-          message: 'We had an error while submitting this transaction, please try again.',
-        }
-        return this.setTxStatusFailed(txId, errReason)
+  async _checkPendingTxs () {
+    const signedTxList = this.getFilteredTxList({status: 'submitted'})
+    // in order to keep the nonceTracker accurate we block it while updating pending transactions
+    const nonceGlobalLock = await this.nonceTracker.getGlobalLock()
+    try {
+      await Promise.all(signedTxList.map((txMeta) => this._checkPendingTx(txMeta)))
+    } catch (err) {
+      console.error('TransactionController - Error updating pending transactions')
+      console.error(err)
+    }
+    nonceGlobalLock.releaseLock()
+  }
+
+  async _checkPendingTx (txMeta) {
+    const txHash = txMeta.hash
+    const txId = txMeta.id
+    // extra check in case there was an uncaught error during the
+    // signature and submission process
+    if (!txHash) {
+      const errReason = {
+        errCode: 'No hash was provided',
+        message: 'We had an error while submitting this transaction, please try again.',
       }
-      this.query.getTransactionByHash(txHash, (err, txParams) => {
-        if (err || !txParams) {
-          if (!txParams) return
-          txMeta.err = {
-            isWarning: true,
-            errorCode: err,
-            message: 'There was a problem loading this transaction.',
-          }
-          this.updateTx(txMeta)
-          return log.error(err)
-        }
-        if (txParams.blockNumber) {
-          this.setTxStatusConfirmed(txId)
+      this.setTxStatusFailed(txId, errReason)
+      return
+    }
+    // get latest transaction status
+    let txParams
+    try {
+      txParams = await pify((cb) => this.query.getTransactionByHash(txHash, cb))()
+      if (!txParams) return
+      if (txParams.blockNumber) {
+        this.setTxStatusConfirmed(txId)
+      }
+    } catch (err) {
+      if (err || !txParams) {
+        txMeta.err = {
+          isWarning: true,
+          errorCode: err,
+          message: 'There was a problem loading this transaction.',
         }
-      })
-    })
+        this.updateTx(txMeta)
+        log.error(err)
+      }
+    }
   }
 
 }

app/scripts/lib/nonce-tracker.js

@@ -1,4 +1,6 @@
 const EthQuery = require('eth-query')
+const assert = require('assert')
+const Mutex = require('await-semaphore').Mutex
 
 class NonceTracker {
 
@@ -9,22 +11,34 @@ class NonceTracker {
     this.lockMap = {}
   }
 
+  async getGlobalLock () {
+    const globalMutex = this._lookupMutex('global')
+    // await global mutex free
+    const releaseLock = await globalMutex.acquire()
+    return { releaseLock }
+  }
+
   // releaseLock must be called
   // releaseLock must be called after adding signed tx to pending transactions (or discarding)
   async getNonceLock (address) {
-    // await lock free
-    await this.lockMap[address]
-    // take lock
-    const releaseLock = this._takeLock(address)
+    // await global mutex free
+    await this._globalMutexFree()
+    // await lock free, then take lock
+    const releaseLock = await this._takeMutex(address)
     // calculate next nonce
     // we need to make sure our base count
     // and pending count are from the same block
     const currentBlock = await this._getCurrentBlock()
     const pendingTransactions = this.getPendingTransactions(address)
-    const baseCount = await this._getTxCount(address, currentBlock)
-    const nextNonce = parseInt(baseCount) + pendingTransactions.length
+    const pendingCount = pendingTransactions.length
+    assert(Number.isInteger(pendingCount), 'nonce-tracker - pendingCount is an integer')
+    const baseCountHex = await this._getTxCount(address, currentBlock)
+    const baseCount = parseInt(baseCountHex, 16)
+    assert(Number.isInteger(baseCount), 'nonce-tracker - baseCount is an integer')
+    const nextNonce = baseCount + pendingCount
+    assert(Number.isInteger(nextNonce), 'nonce-tracker - nextNonce is an integer')
     // return next nonce and release cb
-    return { nextNonce: nextNonce.toString(16), releaseLock }
+    return { nextNonce, releaseLock }
   }
 
   async _getCurrentBlock () {
@@ -35,16 +49,6 @@ class NonceTracker {
     })
   }
 
-  _takeLock (lockId) {
-    let releaseLock = null
-    // create and store lock
-    const lock = new Promise((resolve, reject) => { releaseLock = resolve })
-    this.lockMap[lockId] = lock
-    // setup lock teardown
-    lock.then(() => delete this.lockMap[lockId])
-    return releaseLock
-  }
-
   async _getTxCount (address, currentBlock) {
     const blockNumber = currentBlock.number
     return new Promise((resolve, reject) => {
@@ -54,6 +58,27 @@ class NonceTracker {
     })
   }
 
+  async _globalMutexFree () {
+    const globalMutex = this._lookupMutex('global')
+    const release = await globalMutex.acquire()
+    release()
+  }
+
+  async _takeMutex (lockId) {
+    const mutex = this._lookupMutex(lockId)
+    const releaseLock = await mutex.acquire()
+    return releaseLock
+  }
+
+  _lookupMutex (lockId) {
+    let mutex = this.lockMap[lockId]
+    if (!mutex) {
+      mutex = new Mutex()
+      this.lockMap[lockId] = mutex
+    }
+    return mutex
+  }
+
 }
 
 module.exports = NonceTracker

package.json

@@ -47,6 +47,7 @@
   },
   "dependencies": {
     "async": "^1.5.2",
+    "await-semaphore": "^0.1.1",
     "babel-runtime": "^6.23.0",
     "bip39": "^2.2.0",
     "bluebird": "^3.5.0",