diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | app/scripts/controllers/transactions.js | 71 | ||||
| -rw-r--r-- | app/scripts/lib/nonce-tracker.js | 59 | ||||
| -rw-r--r-- | package.json | 1 |
4 files changed, 88 insertions, 44 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d15b3512..4d265d318 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 61e96ca13..5f3d84ebe 100644 --- a/app/scripts/controllers/transactions.js +++ b/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) + } + } } } diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index ab2893b10..b76dac4e8 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/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 diff --git a/package.json b/package.json index e0bb303bf..d40ad068b 100644 --- a/package.json +++ b/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", |