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. diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 30c59fa46..08f1e1e86 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/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) + 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 () { diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 11f99751c..3312a3bd0 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() }) }) @@ -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() + }) + }) }) })