From 1f0223d0a0b617ffaf1704210b7ed328d12c48d1 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 19:34:20 -0700 Subject: [PATCH 01/20] Simplify nonce calculation --- app/scripts/lib/nonce-tracker.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 30c59fa46..db5a5327f 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -28,9 +28,9 @@ 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) + const localNonceResult = await this._getLocalNextNonce(address, networkNonceResult) + nonceDetails.local = localNonceResult.details nonceDetails.network = networkNonceResult.details const nextNonce = Math.max(networkNonceResult.nonce, localNonceResult.nonce) assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`) @@ -81,15 +81,16 @@ class NonceTracker { return { name: 'network', nonce: baseCount, details: nonceDetails } } - async _getlocalNextNonce (address) { + async _getLocalNextNonce (address, networkNonce) { let nextNonce // check our local tx history for the highest nonce (if any) 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 highestNonce = Math.max(highestConfirmedNonce, highestPendingNonce) const haveHighestNonce = Number.isInteger(highestNonce) if (haveHighestNonce) { From 5feda433607fc5a762e1e0843598bacda9d11ca9 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 19:34:37 -0700 Subject: [PATCH 02/20] Add failing test for newly identified error state Reproduces #1966 --- test/unit/nonce-tracker-test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 11f99751c..5b0732d44 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -125,6 +125,23 @@ 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() + }) + }) }) }) From c4ab7a57799167904b863b9cb9423885a9c1cc66 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 19:35:49 -0700 Subject: [PATCH 03/20] Linted --- app/scripts/lib/nonce-tracker.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index db5a5327f..d3c76c230 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -86,7 +86,6 @@ class NonceTracker { // check our local tx history for the highest nonce (if any) 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) From 221575a191a0a8b8c4c17465a0530561e1905297 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 20:04:03 -0700 Subject: [PATCH 04/20] Fix new test, break an older maybe wrong one --- app/scripts/lib/nonce-tracker.js | 50 +++++++++++++++----------------- test/unit/nonce-tracker-test.js | 6 ++-- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index d3c76c230..4f2473cf5 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -29,11 +29,16 @@ class NonceTracker { // evaluate multiple nextNonce strategies const nonceDetails = {} const networkNonceResult = await this._getNetworkNextNonce(address) - const localNonceResult = await this._getLocalNextNonce(address, networkNonceResult) + const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address) + const highestConfirmed = Math.max(networkNonceResult.nonce, highestLocallyConfirmed) + const pendingTxs = this.getPendingTransactions(address) + const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestConfirmed) || 0 + nonceDetails.local = localNonceResult.details nonceDetails.network = networkNonceResult.details 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 } } @@ -77,35 +82,14 @@ class NonceTracker { const baseCountHex = await this.ethQuery.getTransactionCount(address, blockNumber) const baseCount = parseInt(baseCountHex, 16) 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, networkNonce) { - 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 highestConfirmedNonce = this._getHighestNonce(confirmedTransactions) - const highestPendingNonce = this._getHighestNonce(pendingTransactions) - const highestNonce = Math.max(highestConfirmedNonce, highestPendingNonce) - - 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 highest } _reduceTxListToUniqueNonces (txList) { @@ -127,6 +111,20 @@ class NonceTracker { return highestNonce } + _getHighestContinuousFrom (txList, startPoint) { + const nonces = txList.map((txMeta) => parseInt(txMeta.txParams.nonce, 16)) + + let highest = startPoint + while (nonces.includes(highest + 1)) { + highest++ + } + + const haveHighestNonce = Number.isInteger(highest) && highest > 0 + const nonce = haveHighestNonce ? highest + 1 : 0 + + return { name: 'local', nonce } + } + // this is a hotfix for the fact that the blockTracker will // change when the network changes _getBlockTracker () { diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 5b0732d44..c5a71d6c8 100644 --- a/test/unit/nonce-tracker-test.js +++ b/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() }) }) From 04d40b114dd12237710f605fe2f0a5f2c337d2cb Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 20:11:37 -0700 Subject: [PATCH 05/20] Got all tests but one passing --- app/scripts/lib/nonce-tracker.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 4f2473cf5..6fcd716f2 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -30,6 +30,7 @@ class NonceTracker { const nonceDetails = {} const networkNonceResult = await this._getNetworkNextNonce(address) const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address) + const highestConfirmed = Math.max(networkNonceResult.nonce, highestLocallyConfirmed) const pendingTxs = this.getPendingTransactions(address) const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestConfirmed) || 0 From 855f4eeacbcf7b3e056cf7956edea2c84fa256d5 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 20:31:03 -0700 Subject: [PATCH 06/20] Pass nonce tests --- app/scripts/lib/nonce-tracker.js | 15 +++++++-------- test/unit/nonce-tracker-test.js | 8 ++++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 6fcd716f2..8c2568c3f 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -30,10 +30,12 @@ class NonceTracker { const nonceDetails = {} const networkNonceResult = await this._getNetworkNextNonce(address) const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address) + const nextNetworkNonce = networkNonceResult.nonce + const highestLocalNonce = highestLocallyConfirmed + const highestSuggested = Math.max(nextNetworkNonce, highestLocalNonce) - const highestConfirmed = Math.max(networkNonceResult.nonce, highestLocallyConfirmed) const pendingTxs = this.getPendingTransactions(address) - const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestConfirmed) || 0 + const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0 nonceDetails.local = localNonceResult.details nonceDetails.network = networkNonceResult.details @@ -90,7 +92,7 @@ class NonceTracker { _getHighestLocallyConfirmed (address) { const confirmedTransactions = this.getConfirmedTransactions(address) const highest = this._getHighestNonce(confirmedTransactions) - return highest + return Number.isInteger(highest) ? highest + 1 : 0 } _reduceTxListToUniqueNonces (txList) { @@ -116,14 +118,11 @@ class NonceTracker { const nonces = txList.map((txMeta) => parseInt(txMeta.txParams.nonce, 16)) let highest = startPoint - while (nonces.includes(highest + 1)) { + while (nonces.includes(highest)) { highest++ } - const haveHighestNonce = Number.isInteger(highest) && highest > 0 - const nonce = haveHighestNonce ? highest + 1 : 0 - - return { name: 'local', nonce } + return { name: 'local', nonce: highest, details: { startPoint, highest } } } // this is a hotfix for the fact that the blockTracker will diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index c5a71d6c8..758bf9eb6 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -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() }) }) From dae7c632d6b7bed74abbe6bbc7b6b43deb2f2f0d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:01:42 -0700 Subject: [PATCH 07/20] Add mysterious failing test --- test/unit/nonce-tracker-test.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 758bf9eb6..5f247b46b 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -142,6 +142,26 @@ describe('Nonce Tracker', function () { await nonceLock.releaseLock() }) }) + + describe('A normal usage condition.', 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, '10', `nonce should be 10 got ${nonceLock.nextNonce}`) + await nonceLock.releaseLock() + }) + }) }) }) From e057c37b337f34556af1734ceb44e7a4d9a7b08c Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:03:29 -0700 Subject: [PATCH 08/20] Corrected test constraints --- test/unit/nonce-tracker-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 5f247b46b..3312a3bd0 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -143,7 +143,7 @@ describe('Nonce Tracker', function () { }) }) - describe('A normal usage condition.', function () { + describe('When all three return different values', function () { beforeEach(function () { const txGen = new MockTxGen() const confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 10 }) @@ -158,7 +158,7 @@ describe('Nonce Tracker', function () { it('should return nonce after network nonce', async function () { this.timeout(15000) const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '10', `nonce should be 10 got ${nonceLock.nextNonce}`) + assert.equal(nonceLock.nextNonce, '50', `nonce should be 50 got ${nonceLock.nextNonce}`) await nonceLock.releaseLock() }) }) From 55c1a259b1ac01e8a8f6b241bac7a3b1cd16f3ec Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:14:46 -0700 Subject: [PATCH 09/20] Fix network nonce parsing --- app/scripts/lib/nonce-tracker.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 8c2568c3f..d49c7f205 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -37,8 +37,14 @@ class NonceTracker { const pendingTxs = this.getPendingTransactions(address) const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0 - nonceDetails.local = localNonceResult.details - nonceDetails.network = networkNonceResult.details + 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}"`) @@ -82,8 +88,9 @@ 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) + const baseString = baseCountBN.toString() + const baseCount = parseInt(baseString) assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`) const nonceDetails = { blockNumber, baseCount } return { name: 'network', nonce: baseCount, details: nonceDetails } From a122ec1f8ba0935d26a45ce0b26be991d222aaad Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:37:07 -0700 Subject: [PATCH 10/20] Use toNumber method --- app/scripts/lib/nonce-tracker.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index d49c7f205..e0e065d82 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -89,8 +89,7 @@ class NonceTracker { const currentBlock = await this._getCurrentBlock() const blockNumber = currentBlock.blockNumber const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber) - const baseString = baseCountBN.toString() - const baseCount = parseInt(baseString) + const baseCount = baseCountBN.toNumber() assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`) const nonceDetails = { blockNumber, baseCount } return { name: 'network', nonce: baseCount, details: nonceDetails } From c620123fab9e1eac8d3038204c9cfb04ca85afb1 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:50:28 -0700 Subject: [PATCH 11/20] Enforce nonces as type string --- app/scripts/lib/nonce-tracker.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index e0e065d82..08f1e1e86 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -115,13 +115,21 @@ 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) => 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) + }) let highest = startPoint while (nonces.includes(highest)) { From ad40fc6b42f21ec12b640cd53bd1987c653d6df0 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 21:54:26 -0700 Subject: [PATCH 12/20] Bump changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2b6572da..1d1558859 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +- Fix nonce calculation bug that would sometimes generate very wrong nonces. + ## 3.9.10 2017-8-23 - Improve nonce calculation, to prevent bug where people are unable to send transactions reliably. From 0ad310e096fe8c13553dc40f65363d296a7e84ce Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 22:29:08 -0700 Subject: [PATCH 13/20] Fail transactions after a day of retries --- app/scripts/controllers/transactions.js | 6 +++++- app/scripts/lib/pending-tx-tracker.js | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 6f49c9633..fb3be6073 100644 --- a/app/scripts/controllers/transactions.js +++ b/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 }) } -} \ No newline at end of file +} diff --git a/app/scripts/lib/pending-tx-tracker.js b/app/scripts/lib/pending-tx-tracker.js index 19720db3f..b90851b58 100644 --- a/app/scripts/lib/pending-tx-tracker.js +++ b/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() } -} \ No newline at end of file +} From 941e5ff1252f792e1572c3fd77d7fff086a647c6 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 22:29:28 -0700 Subject: [PATCH 14/20] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d1558859..3ca49048c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - 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 From dadee1ed79715c7dcdfe1d208e2bd554a1d4da8b Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 23 Aug 2017 22:48:19 -0700 Subject: [PATCH 15/20] hotfix - fail submitted txs whos nonce is out of bound --- app/scripts/migrations/019.js | 60 +++++++++++++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + 2 files changed, 61 insertions(+) create mode 100644 app/scripts/migrations/019.js diff --git a/app/scripts/migrations/019.js b/app/scripts/migrations/019.js new file mode 100644 index 000000000..e6e8f1f01 --- /dev/null +++ b/app/scripts/migrations/019.js @@ -0,0 +1,60 @@ + +const version = 19 + +/* + +This migration sets transactions with the 'Gave up submitting tx.' err message +to a 'failed' stated + +*/ + +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) + + if (parseInt(txMeta.txParams.nonce, 16) > highestConfirmedNonce + 1) { + txMeta.status = 'failed' + txMeta.err = { + message: 'nonce too high', + note: 'migration 019 custom error', + } + } + return txMeta + }) + return newState +} + +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 +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 6bfc622f7..e9cbd7b98 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -29,4 +29,5 @@ module.exports = [ require('./016'), require('./017'), require('./018'), + require('./019'), ] From f42687d25f844efd3be915e077cddcb4ccd458cf Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 23 Aug 2017 22:53:29 -0700 Subject: [PATCH 16/20] fix description --- app/scripts/migrations/019.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/migrations/019.js b/app/scripts/migrations/019.js index e6e8f1f01..9dfe96050 100644 --- a/app/scripts/migrations/019.js +++ b/app/scripts/migrations/019.js @@ -3,8 +3,8 @@ const version = 19 /* -This migration sets transactions with the 'Gave up submitting tx.' err message -to a 'failed' stated +This migration sets transactions as failed +whos nonce is too high */ From 17a71a9b4cb5cc23cbedfc52ba35c562dffdc02e Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 23:09:57 -0700 Subject: [PATCH 17/20] Only cancel pending txs with non continuously high nonces --- app/scripts/migrations/019.js | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/019.js b/app/scripts/migrations/019.js index 9dfe96050..d41b39d1c 100644 --- a/app/scripts/migrations/019.js +++ b/app/scripts/migrations/019.js @@ -30,6 +30,7 @@ module.exports = { function transformState (state) { const newState = state const transactions = newState.TransactionController.transactions + newState.TransactionController.transactions = transactions.map((txMeta, _, txList) => { if (txMeta.status !== 'submitted') return txMeta @@ -38,7 +39,14 @@ function transformState (state) { .filter((tx) => tx.metamaskNetworkId.from === txMeta.metamaskNetworkId.from) const highestConfirmedNonce = getHighestNonce(confirmedTxs) - if (parseInt(txMeta.txParams.nonce, 16) > highestConfirmedNonce + 1) { + 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', @@ -50,6 +58,20 @@ function transformState (state) { 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 { name: 'local', nonce: highest, details: { startPoint, highest } } +} + function getHighestNonce (txList) { const nonces = txList.map((txMeta) => { const nonce = txMeta.txParams.nonce @@ -58,3 +80,4 @@ function getHighestNonce (txList) { const highestNonce = Math.max.apply(null, nonces) return highestNonce } + From 803e696cdc2a2f60d6097f1ff1efa8202d8e4320 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 23 Aug 2017 23:24:01 -0700 Subject: [PATCH 18/20] Make method return a number --- app/scripts/migrations/019.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/migrations/019.js b/app/scripts/migrations/019.js index d41b39d1c..072c96370 100644 --- a/app/scripts/migrations/019.js +++ b/app/scripts/migrations/019.js @@ -69,7 +69,7 @@ function getHighestContinuousFrom (txList, startPoint) { highest++ } - return { name: 'local', nonce: highest, details: { startPoint, highest } } + return highest } function getHighestNonce (txList) { From c2624dd1a071ea4388df23400f4319e863d061c6 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 24 Aug 2017 00:02:06 -0700 Subject: [PATCH 19/20] fall back to `latest` if blockNumber is null --- app/scripts/lib/nonce-tracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 08f1e1e86..0029ac953 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -88,7 +88,7 @@ class NonceTracker { // and pending count are from the same block const currentBlock = await this._getCurrentBlock() const blockNumber = currentBlock.blockNumber - const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber) + 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, baseCount } From 72bccec440179d8126391d9e249e9f3acbbb5a88 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 24 Aug 2017 00:16:19 -0700 Subject: [PATCH 20/20] Version 3.9.11 --- CHANGELOG.md | 2 ++ app/manifest.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca49048c..cab45914e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +## 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. diff --git a/app/manifest.json b/app/manifest.json index 60eeb2a35..7ec32ea78 100644 --- a/app/manifest.json +++ b/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",