Merge branch 'master' into inpage-provider-fixes

CHANGELOG.md

@@ -5,6 +5,11 @@
 - Make eth_sign deprecation warning less noisy
 - Fix bug with network version serialization over synchronous RPC
 
+## 3.9.11 2017-8-24
+
+- Fix nonce calculation bug that would sometimes generate very wrong nonces.
+- Give up resubmitting a transaction after 3500 blocks.
+
 ## 3.9.10 2017-8-23
 
 - Improve nonce calculation, to prevent bug where people are unable to send transactions reliably.

app/manifest.json

@@ -1,7 +1,7 @@
 {
   "name": "MetaMask",
   "short_name": "Metamask",
-  "version": "3.9.10",
+  "version": "3.9.11",
   "manifest_version": 2,
   "author": "https://metamask.io",
   "description": "Ethereum Browser Extension",

app/scripts/controllers/transactions.js

@@ -40,6 +40,10 @@ module.exports = class TransactionController extends EventEmitter {
           err: undefined,
         })
       },
+      giveUpOnTransaction: (txId) => {
+        const msg = `Gave up submitting after 3500 blocks un-mined.`
+        this.setTxStatusFailed(txId, msg)
+      },
     })
     this.query = new EthQuery(this.provider)
     this.txProviderUtil = new TxProviderUtil(this.provider)
@@ -451,4 +455,4 @@ module.exports = class TransactionController extends EventEmitter {
     })
     this.memStore.updateState({ unapprovedTxs, selectedAddressTxList })
   }
-}
+}

app/scripts/lib/nonce-tracker.js

@@ -28,12 +28,26 @@ class NonceTracker {
     const releaseLock = await this._takeMutex(address)
     // evaluate multiple nextNonce strategies
     const nonceDetails = {}
-    const localNonceResult = await this._getlocalNextNonce(address)
-    nonceDetails.local = localNonceResult.details
     const networkNonceResult = await this._getNetworkNextNonce(address)
-    nonceDetails.network = networkNonceResult.details
+    const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address)
+    const nextNetworkNonce = networkNonceResult.nonce
+    const highestLocalNonce = highestLocallyConfirmed
+    const highestSuggested = Math.max(nextNetworkNonce, highestLocalNonce)
+
+    const pendingTxs = this.getPendingTransactions(address)
+    const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0
+
+    nonceDetails.params = {
+      highestLocalNonce,
+      highestSuggested,
+      nextNetworkNonce,
+    }
+    nonceDetails.local = localNonceResult
+    nonceDetails.network = networkNonceResult
+
     const nextNonce = Math.max(networkNonceResult.nonce, localNonceResult.nonce)
     assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`)
+
     // return nonce and release cb
     return { nextNonce, nonceDetails, releaseLock }
   }
@@ -74,38 +88,17 @@ class NonceTracker {
     // and pending count are from the same block
     const currentBlock = await this._getCurrentBlock()
     const blockNumber = currentBlock.blockNumber
-    const baseCountHex = await this.ethQuery.getTransactionCount(address, blockNumber)
-    const baseCount = parseInt(baseCountHex, 16)
+    const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber || 'latest')
+    const baseCount = baseCountBN.toNumber()
     assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`)
-    const nonceDetails = { blockNumber, baseCountHex, baseCount }
+    const nonceDetails = { blockNumber, baseCount }
     return { name: 'network', nonce: baseCount, details: nonceDetails }
   }
 
-  async _getlocalNextNonce (address) {
-    let nextNonce
-    // check our local tx history for the highest nonce (if any)
+  _getHighestLocallyConfirmed (address) {
     const confirmedTransactions = this.getConfirmedTransactions(address)
-    const pendingTransactions = this.getPendingTransactions(address)
-    const transactions = confirmedTransactions.concat(pendingTransactions)
-    const highestConfirmedNonce = this._getHighestNonce(confirmedTransactions)
-    const highestPendingNonce = this._getHighestNonce(pendingTransactions)
-    const highestNonce = this._getHighestNonce(transactions)
-
-    const haveHighestNonce = Number.isInteger(highestNonce)
-    if (haveHighestNonce) {
-      // next nonce is the nonce after our last
-      nextNonce = highestNonce + 1
-    } else {
-      // no local tx history so next must be first (zero)
-      nextNonce = 0
-    }
-    const nonceDetails = { highestNonce, haveHighestNonce, highestConfirmedNonce, highestPendingNonce }
-    return { name: 'local', nonce: nextNonce, details: nonceDetails }
-  }
-
-  _getPendingTransactionCount (address) {
-    const pendingTransactions = this.getPendingTransactions(address)
-    return this._reduceTxListToUniqueNonces(pendingTransactions).length
+    const highest = this._getHighestNonce(confirmedTransactions)
+    return Number.isInteger(highest) ? highest + 1 : 0
   }
 
   _reduceTxListToUniqueNonces (txList) {
@@ -122,11 +115,30 @@ class NonceTracker {
   }
 
   _getHighestNonce (txList) {
-    const nonces = txList.map((txMeta) => parseInt(txMeta.txParams.nonce, 16))
+    const nonces = txList.map((txMeta) => {
+      const nonce = txMeta.txParams.nonce
+      assert(typeof nonce, 'string', 'nonces should be hex strings')
+      return parseInt(nonce, 16)
+    })
     const highestNonce = Math.max.apply(null, nonces)
     return highestNonce
   }
 
+  _getHighestContinuousFrom (txList, startPoint) {
+    const nonces = txList.map((txMeta) => {
+      const nonce = txMeta.txParams.nonce
+      assert(typeof nonce, 'string', 'nonces should be hex strings')
+      return parseInt(nonce, 16)
+    })
+
+    let highest = startPoint
+    while (nonces.includes(highest)) {
+      highest++
+    }
+
+    return { name: 'local', nonce: highest, details: { startPoint, highest } }
+  }
+
   // this is a hotfix for the fact that the blockTracker will
   // change when the network changes
   _getBlockTracker () {

app/scripts/lib/pending-tx-tracker.js

@@ -1,6 +1,7 @@
 const EventEmitter = require('events')
 const EthQuery = require('ethjs-query')
 const sufficientBalance = require('./util').sufficientBalance
+const RETRY_LIMIT = 3500 // Retry 3500 blocks, or about 1 day.
 /*
 
   Utility class for tracking the transactions as they
@@ -28,6 +29,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter {
     this.getBalance = config.getBalance
     this.getPendingTransactions = config.getPendingTransactions
     this.publishTransaction = config.publishTransaction
+    this.giveUpOnTransaction = config.giveUpOnTransaction
   }
 
   //  checks if a signed tx is in a block and
@@ -100,6 +102,10 @@ module.exports = class PendingTransactionTracker extends EventEmitter {
     if (balance === undefined) return
     if (!('retryCount' in txMeta)) txMeta.retryCount = 0
 
+    if (txMeta.retryCount > RETRY_LIMIT) {
+      return this.giveUpOnTransaction(txMeta.id)
+    }
+
     // if the value of the transaction is greater then the balance, fail.
     if (!sufficientBalance(txMeta.txParams, balance)) {
       const insufficientFundsError = new Error('Insufficient balance during rebroadcast.')
@@ -160,4 +166,4 @@ module.exports = class PendingTransactionTracker extends EventEmitter {
     }
     nonceGlobalLock.releaseLock()
   }
-}
+}

app/scripts/migrations/019.js

@@ -0,0 +1,83 @@
+
+const version = 19
+
+/*
+
+This migration sets transactions as failed
+whos nonce is too high
+
+*/
+
+const clone = require('clone')
+
+module.exports = {
+  version,
+
+  migrate: function (originalVersionedData) {
+    const versionedData = clone(originalVersionedData)
+    versionedData.meta.version = version
+    try {
+      const state = versionedData.data
+      const newState = transformState(state)
+      versionedData.data = newState
+    } catch (err) {
+      console.warn(`MetaMask Migration #${version}` + err.stack)
+    }
+    return Promise.resolve(versionedData)
+  },
+}
+
+function transformState (state) {
+  const newState = state
+  const transactions = newState.TransactionController.transactions
+
+  newState.TransactionController.transactions = transactions.map((txMeta, _, txList) => {
+    if (txMeta.status !== 'submitted') return txMeta
+
+    const confirmedTxs = txList.filter((tx) => tx.status === 'confirmed')
+    .filter((tx) => tx.txParams.from === txMeta.txParams.from)
+    .filter((tx) => tx.metamaskNetworkId.from === txMeta.metamaskNetworkId.from)
+    const highestConfirmedNonce = getHighestNonce(confirmedTxs)
+
+    const pendingTxs = txList.filter((tx) => tx.status === 'submitted')
+    .filter((tx) => tx.txParams.from === txMeta.txParams.from)
+    .filter((tx) => tx.metamaskNetworkId.from === txMeta.metamaskNetworkId.from)
+    const highestContinuousNonce = getHighestContinuousFrom(pendingTxs, highestConfirmedNonce)
+
+    const maxNonce = Math.max(highestContinuousNonce, highestConfirmedNonce)
+
+    if (parseInt(txMeta.txParams.nonce, 16) > maxNonce + 1) {
+      txMeta.status = 'failed'
+      txMeta.err = {
+        message: 'nonce too high',
+        note: 'migration 019 custom error',
+      }
+    }
+    return txMeta
+  })
+  return newState
+}
+
+function getHighestContinuousFrom (txList, startPoint) {
+  const nonces = txList.map((txMeta) => {
+    const nonce = txMeta.txParams.nonce
+    return parseInt(nonce, 16)
+  })
+
+  let highest = startPoint
+  while (nonces.includes(highest)) {
+    highest++
+  }
+
+  return highest
+}
+
+function getHighestNonce (txList) {
+  const nonces = txList.map((txMeta) => {
+  const nonce = txMeta.txParams.nonce
+    return parseInt(nonce || '0x0', 16)
+  })
+  const highestNonce = Math.max.apply(null, nonces)
+  return highestNonce
+}
+

app/scripts/migrations/index.js

@@ -29,4 +29,5 @@ module.exports = [
   require('./016'),
   require('./017'),
   require('./018'),
+  require('./019'),
 ]

test/unit/nonce-tracker-test.js

@@ -18,10 +18,10 @@ describe('Nonce Tracker', function () {
         nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x1')
       })
 
-      it('should work', async function () {
+      it('should return 4', async function () {
         this.timeout(15000)
         const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926')
-        assert.equal(nonceLock.nextNonce, '4', 'nonce should be 4')
+        assert.equal(nonceLock.nextNonce, '4', `nonce should be 4 got ${nonceLock.nextNonce}`)
         await nonceLock.releaseLock()
       })
 
@@ -41,7 +41,7 @@ describe('Nonce Tracker', function () {
       it('should return 0', async function () {
         this.timeout(15000)
         const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926')
-        assert.equal(nonceLock.nextNonce, '0', 'nonce should be 0')
+        assert.equal(nonceLock.nextNonce, '0', `nonce should be 0 returned ${nonceLock.nextNonce}`)
         await nonceLock.releaseLock()
       })
     })
@@ -55,7 +55,7 @@ describe('Nonce Tracker', function () {
           txParams: { nonce: '0x01' },
         }, { count: 5 })
 
-        nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs)
+        nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x0')
       })
 
       it('should return nonce after those', async function () {
@@ -69,14 +69,14 @@ describe('Nonce Tracker', function () {
     describe('when local confirmed count is higher than network nonce', function () {
       beforeEach(function () {
         const txGen = new MockTxGen()
-        confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 2 })
-        nonceTracker = generateNonceTrackerWith([], confirmedTxs)
+        confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 3 })
+        nonceTracker = generateNonceTrackerWith([], confirmedTxs, '0x1')
       })
 
       it('should return nonce after those', async function () {
         this.timeout(15000)
         const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926')
-        assert.equal(nonceLock.nextNonce, '2', `nonce should be 2 got ${nonceLock.nextNonce}`)
+        assert.equal(nonceLock.nextNonce, '3', `nonce should be 3 got ${nonceLock.nextNonce}`)
         await nonceLock.releaseLock()
       })
     })
@@ -125,6 +125,43 @@ describe('Nonce Tracker', function () {
         await nonceLock.releaseLock()
       })
     })
+
+    describe('when there are pending nonces non sequentially over the network nonce.', function () {
+      beforeEach(function () {
+        const txGen = new MockTxGen()
+        txGen.generate({ status: 'submitted' }, { count: 5 })
+        // 5 over that number
+        pendingTxs = txGen.generate({ status: 'submitted' }, { count: 5 })
+        nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x00')
+      })
+
+      it('should return nonce after network nonce', async function () {
+        this.timeout(15000)
+        const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926')
+        assert.equal(nonceLock.nextNonce, '0', `nonce should be 0 got ${nonceLock.nextNonce}`)
+        await nonceLock.releaseLock()
+      })
+    })
+
+    describe('When all three return different values', function () {
+      beforeEach(function () {
+        const txGen = new MockTxGen()
+        const confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 10 })
+        const pendingTxs = txGen.generate({
+          status: 'submitted',
+          nonce: 100,
+        }, { count: 1 })
+                                         // 0x32 is 50 in hex:
+        nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x32')
+      })
+
+      it('should return nonce after network nonce', async function () {
+        this.timeout(15000)
+        const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926')
+        assert.equal(nonceLock.nextNonce, '50', `nonce should be 50 got ${nonceLock.nextNonce}`)
+        await nonceLock.releaseLock()
+      })
+    })
   })
 })