From 0fd32e67d4c8e911cd5cd88b81f04d11b2202609 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 14 Jun 2017 12:01:45 -0700 Subject: [PATCH 01/14] Do not mark slowly mined txs as failed. Fixes #1567 Also seems to fix #1556 Also improves resubmit performance by only resubmitting on `latest`. --- CHANGELOG.md | 1 + app/scripts/controllers/transactions.js | 12 ++++-------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f99e680e..0d3e86342 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - Add a warning to JSON file import. +- Fix bug where slowly mined txs would sometimes be incorrectly marked as failed. ## 3.7.8 2017-6-12 diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2db8041eb..931f01855 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -25,7 +25,7 @@ module.exports = class TransactionController extends EventEmitter { this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) - this.blockTracker.on('block', this.resubmitPendingTxs.bind(this)) + this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) @@ -339,7 +339,7 @@ module.exports = class TransactionController extends EventEmitter { // checks if a signed tx is in a block and // if included sets the tx status as 'confirmed' checkForTxInBlock () { - var signedTxList = this.getFilteredTxList({status: 'submitted'}) + var signedTxList = this.getFilteredTxList({ status: 'submitted' }) if (!signedTxList.length) return signedTxList.forEach((txMeta) => { var txHash = txMeta.hash @@ -430,12 +430,8 @@ module.exports = class TransactionController extends EventEmitter { } if (txMeta.retryCount > RETRY_LIMIT) { - txMeta.err = { - isWarning: true, - message: 'Gave up submitting tx.', - } - this.updateTx(txMeta) - return log.error(txMeta.err.message) + const message = 'Gave up submitting tx ' + txMeta.hash + return log.warning(message) } txMeta.retryCount++ From a025cd178d8741dc41c31d3823004d5c5628a15a Mon Sep 17 00:00:00 2001 From: Kevin Serrano Date: Wed, 14 Jun 2017 15:23:13 -0700 Subject: [PATCH 02/14] Add issue template. --- ISSUE_TEMPLATE | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 ISSUE_TEMPLATE diff --git a/ISSUE_TEMPLATE b/ISSUE_TEMPLATE new file mode 100644 index 000000000..9c17ec3cd --- /dev/null +++ b/ISSUE_TEMPLATE @@ -0,0 +1,17 @@ + From 56490c6468bb83dccf04941ded5fec1017e5fe2c Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 14 Jun 2017 16:14:15 -0700 Subject: [PATCH 03/14] Bump provider-engine --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 2c23d9e10..127211374 100644 --- a/package.json +++ b/package.json @@ -123,7 +123,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.18.2", - "web3-provider-engine": "^12.2.4", + "web3-provider-engine": "^13.0.0", "web3-stream-provider": "^2.0.6", "xtend": "^4.0.1" }, From 6ae97290f0e744479a41e31507f79309137d94c0 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 14 Jun 2017 16:14:55 -0700 Subject: [PATCH 04/14] check for the tx in the block that provider engine gives us --- app/scripts/controllers/transactions.js | 20 +++++--------------- package.json | 2 +- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2db8041eb..71f90c2cd 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -338,12 +338,13 @@ module.exports = class TransactionController extends EventEmitter { // checks if a signed tx is in a block and // if included sets the tx status as 'confirmed' - checkForTxInBlock () { + checkForTxInBlock (block) { 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', @@ -351,20 +352,9 @@ module.exports = class TransactionController extends EventEmitter { } return this.setTxStatusFailed(txId, errReason) } - 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) - } + + block.transactions.forEach((tx) => { + if (tx.hash === txHash) this.setTxStatusConfirmed(txId) }) }) } diff --git a/package.json b/package.json index 127211374..7ee5dc5be 100644 --- a/package.json +++ b/package.json @@ -123,7 +123,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.18.2", - "web3-provider-engine": "^13.0.0", + "web3-provider-engine": "^13.0.1", "web3-stream-provider": "^2.0.6", "xtend": "^4.0.1" }, From a10740af7e35aa60e0445598403e6bda22382c2f Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 14 Jun 2017 20:17:59 -0700 Subject: [PATCH 05/14] add a check for weather a tx is included in a block when jumping blocks --- app/scripts/controllers/transactions.js | 41 +++++++++++++++++++++++++ test/unit/tx-controller-test.js | 1 + 2 files changed, 42 insertions(+) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2db8041eb..41f651458 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -26,6 +26,7 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.blockTracker.on('block', this.resubmitPendingTxs.bind(this)) + this.provider._blockTracker.on('sync', this.queryPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) @@ -369,6 +370,15 @@ module.exports = class TransactionController extends EventEmitter { }) } + queryPendingTxs ({oldBlock, newBlock}) { + if (!oldBlock) { + this._checkPendingTxs() + return + } + const diff = Number.parseInt(newBlock.number) - Number.parseInt(oldBlock.number) + if (diff > 1) this._checkPendingTxs() + } + // PRIVATE METHODS // Should find the tx in the tx list and @@ -443,6 +453,37 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils.publishTransaction(rawTx, cb) } + _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) + } + 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) + } + }) + }) + } + } diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index f0d8a706e..0d35cd62c 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -19,6 +19,7 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, + provider: { _blockTracker: new EventEmitter()}, blockTracker: new EventEmitter(), ethQuery: new EthQuery(new EventEmitter()), signTransaction: (ethTx) => new Promise((resolve) => { From da33efe77515bbfff7100f3205b4d0d907e9296b Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 14 Jun 2017 21:42:29 -0700 Subject: [PATCH 06/14] bump eth-query for quiter logs --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 2c23d9e10..c60aff821 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,7 @@ "eth-bin-to-ops": "^1.0.1", "eth-contract-metadata": "^1.0.0", "eth-hd-keyring": "^1.1.1", - "eth-query": "^2.1.1", + "eth-query": "^2.1.2", "eth-sig-util": "^1.1.1", "eth-simple-keyring": "^1.1.1", "ethereumjs-tx": "^1.3.0", From 07539a63e4378153778b5bb264aaffad7e46cf34 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 14 Jun 2017 21:52:49 -0700 Subject: [PATCH 07/14] remove unnecessary log --- app/scripts/controllers/transactions.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 931f01855..2769a8d59 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -429,10 +429,7 @@ module.exports = class TransactionController extends EventEmitter { return cb() } - if (txMeta.retryCount > RETRY_LIMIT) { - const message = 'Gave up submitting tx ' + txMeta.hash - return log.warning(message) - } + if (txMeta.retryCount > RETRY_LIMIT) return txMeta.retryCount++ const rawTx = txMeta.rawTx From 2e5deef2b0bdaaa67ee1584da52b7001cc9e849b Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 15 Jun 2017 13:48:48 -0700 Subject: [PATCH 08/14] check nonce and balance when resubmiting tx --- app/scripts/controllers/transactions.js | 27 +++++++++++++++---------- app/scripts/metamask-controller.js | 1 + test/unit/tx-controller-test.js | 1 + 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2769a8d59..58dc8a6ab 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -25,10 +25,10 @@ module.exports = class TransactionController extends EventEmitter { this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) - this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) + this.provider._blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) - + this.ethStore = opts.ethStore // memstore is computed from a few different stores this._updateMemstore() this.store.subscribe(() => this._updateMemstore()) @@ -411,26 +411,31 @@ module.exports = class TransactionController extends EventEmitter { const pending = this.getTxsByMetaData('status', 'submitted') // only try resubmitting if their are transactions to resubmit if (!pending.length) return - const resubmit = denodeify(this.resubmitTx.bind(this)) + const resubmit = denodeify(this._resubmitTx.bind(this)) Promise.all(pending.map(txMeta => resubmit(txMeta))) .catch((reason) => { log.info('Problem resubmitting tx', reason) }) } - resubmitTx (txMeta, cb) { - // Increment a try counter. - if (!('retryCount' in txMeta)) { - txMeta.retryCount = 0 - } + _resubmitTx (txMeta, cb) { + const address = txMeta.txParams.from + const balance = this.ethStore.getState().accounts[address].balance + const nonce = Number.parseInt(this.ethStore.getState().accounts[address].nonce) + const txNonce = Number.parseInt(txMeta.txParams.nonce) + const gtBalance = Number.parseInt(txMeta.txParams.value) > Number.parseInt(balance) + if (!('retryCount' in txMeta)) txMeta.retryCount = 0 + // if the value of the transaction is greater then the balance + // or the nonce of the transaction is lower then the accounts nonce + // dont resubmit the tx + if (gtBalance || txNonce < nonce) return cb() // Only auto-submit already-signed txs: - if (!('rawTx' in txMeta)) { - return cb() - } + if (!('rawTx' in txMeta)) return cb() if (txMeta.retryCount > RETRY_LIMIT) return + // Increment a try counter. txMeta.retryCount++ const rawTx = txMeta.rawTx this.txProviderUtils.publishTransaction(rawTx, cb) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a7eb3d056..727c19fb7 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -98,6 +98,7 @@ module.exports = class MetamaskController extends EventEmitter { provider: this.provider, blockTracker: this.provider, ethQuery: this.ethQuery, + ethStore: this.ethStore, }) // notices diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index f0d8a706e..702bdd03d 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -19,6 +19,7 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, + provider: { _blockTracker: new EventEmitter() }, blockTracker: new EventEmitter(), ethQuery: new EthQuery(new EventEmitter()), signTransaction: (ethTx) => new Promise((resolve) => { From 27b874f2c48fd1cb9dc0984646cb739173ddaf2c Mon Sep 17 00:00:00 2001 From: kumavis Date: Thu, 15 Jun 2017 14:08:07 -0700 Subject: [PATCH 09/14] transactions controller - add comments --- app/scripts/controllers/transactions.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 41f651458..aa168b736 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -26,6 +26,7 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.blockTracker.on('block', this.resubmitPendingTxs.bind(this)) + // provider-engine only exploses the 'block' event, not 'latest' for 'sync' this.provider._blockTracker.on('sync', this.queryPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) @@ -371,10 +372,12 @@ module.exports = class TransactionController extends EventEmitter { } queryPendingTxs ({oldBlock, newBlock}) { + // check pending transactions on start if (!oldBlock) { this._checkPendingTxs() return } + // if we synced by more than one block, check for missed pending transactions const diff = Number.parseInt(newBlock.number) - Number.parseInt(oldBlock.number) if (diff > 1) this._checkPendingTxs() } @@ -453,6 +456,8 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils.publishTransaction(rawTx, cb) } + // 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 From 3062254c5660cc4f3fa887c58f0bba6a6b533ca7 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 15 Jun 2017 16:20:19 -0700 Subject: [PATCH 10/14] Do not recommend posting logs publicly on github. This exposes all user accounts, and not all users will want to do this, so we should not recommend this. --- ISSUE_TEMPLATE | 2 -- 1 file changed, 2 deletions(-) diff --git a/ISSUE_TEMPLATE b/ISSUE_TEMPLATE index 9c17ec3cd..d0ff3c08e 100644 --- a/ISSUE_TEMPLATE +++ b/ISSUE_TEMPLATE @@ -4,8 +4,6 @@ FAQ: Common questions such as "Where is my ether?" or "Where did my tokens go?" are answered in the FAQ. Bug Reports: - In order to quickly expedite your issue, please follow the directions here and paste the results into your issue. - https://github.com/MetaMask/faq/blob/master/LOGS.md Briefly describe the issue you've encountered * Expected Behavior From 06f6aa7a00f57004746d1e21759ac56396d9b855 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 15 Jun 2017 18:00:24 -0700 Subject: [PATCH 11/14] Debounce background updates Our background sometimes emits absurd quantities of updates very quickly. This PR reduces the amount of inter-process traffic by ensuring the `sendUpdate` method does not fire more than every 200 ms. Fixes #1621 --- app/scripts/metamask-controller.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a7eb3d056..d745d29dc 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -23,6 +23,7 @@ const autoFaucet = require('./lib/auto-faucet') const nodeify = require('./lib/nodeify') const accountImporter = require('./account-import-strategies') const getBuyEthUrl = require('./lib/buy-eth-url') +const debounce = require('debounce') const version = require('../manifest.json').version @@ -30,6 +31,9 @@ module.exports = class MetamaskController extends EventEmitter { constructor (opts) { super() + + this.sendUpdate = debounce(this.privateSendUpdate.bind(this), 200) + this.opts = opts const initState = opts.initState || {} @@ -354,7 +358,7 @@ module.exports = class MetamaskController extends EventEmitter { ) } - sendUpdate () { + privateSendUpdate () { this.emit('update', this.getState()) } From 3e4f2cf3d3c13c876f7ba1d77e8075ae973b1755 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Fri, 16 Jun 2017 16:34:38 -0700 Subject: [PATCH 12/14] bump provider engine --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 7ee5dc5be..3cace3250 100644 --- a/package.json +++ b/package.json @@ -123,7 +123,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.18.2", - "web3-provider-engine": "^13.0.1", + "web3-provider-engine": "^13.0.3", "web3-stream-provider": "^2.0.6", "xtend": "^4.0.1" }, From 5f8e74e0aa7eed37fc86a84f1495cc65030e9136 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Fri, 16 Jun 2017 16:36:32 -0700 Subject: [PATCH 13/14] put the block listeners back on the provider --- app/scripts/controllers/transactions.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 9c20a7f1a..d56a81150 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -24,10 +24,9 @@ module.exports = class TransactionController extends EventEmitter { this.blockTracker = opts.blockTracker this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) - this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + this.blockTracker.on('rawBlock', this.checkForTxInBlock.bind(this)) this.blockTracker.on('block', this.resubmitPendingTxs.bind(this)) - // provider-engine only exploses the 'block' event, not 'latest' for 'sync' - this.provider._blockTracker.on('sync', this.queryPendingTxs.bind(this)) + this.blockTracker.on('sync', this.queryPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) From 9c2ead3d528046e4a6eed5bc45e82c972000354d Mon Sep 17 00:00:00 2001 From: frankiebee Date: Fri, 16 Jun 2017 16:43:38 -0700 Subject: [PATCH 14/14] put event back on the "blockTracker:/provider" --- app/scripts/controllers/transactions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index f6eb4e4d9..042d8e66d 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -27,7 +27,7 @@ module.exports = class TransactionController extends EventEmitter { this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) // provider-engine only exploses the 'block' event, not 'latest' for 'sync' this.provider._blockTracker.on('sync', this.queryPendingTxs.bind(this)) - this.provider._blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) + this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) this.ethStore = opts.ethStore