From ae2a4d78e8c7733da1963965e38e154351d54a20 Mon Sep 17 00:00:00 2001 From: Dan Date: Tue, 5 Dec 2017 17:21:14 -0330 Subject: Exponentional backoff on transaction retry in pending-tx-tracker --- app/scripts/controllers/transactions.js | 6 ++++++ app/scripts/lib/pending-tx-tracker.js | 16 ++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index a861c0342..063ba8f65 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -72,6 +72,12 @@ module.exports = class TransactionController extends EventEmitter { }) this.pendingTxTracker.on('tx:failed', this.txStateManager.setTxStatusFailed.bind(this.txStateManager)) this.pendingTxTracker.on('tx:confirmed', this.txStateManager.setTxStatusConfirmed.bind(this.txStateManager)) + this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => { + if (!txMeta.firstRetryBlockNumber) { + txMeta.firstRetryBlockNumber = latestBlockNumber + this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:retry') + } + }) this.pendingTxTracker.on('tx:retry', (txMeta) => { if (!('retryCount' in txMeta)) txMeta.retryCount = 0 txMeta.retryCount++ diff --git a/app/scripts/lib/pending-tx-tracker.js b/app/scripts/lib/pending-tx-tracker.js index 0d7c6a92c..60c837040 100644 --- a/app/scripts/lib/pending-tx-tracker.js +++ b/app/scripts/lib/pending-tx-tracker.js @@ -65,7 +65,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter { } - resubmitPendingTxs () { + resubmitPendingTxs (block) { const pending = this.getPendingTransactions() // only try resubmitting if their are transactions to resubmit if (!pending.length) return @@ -101,13 +101,25 @@ module.exports = class PendingTransactionTracker extends EventEmitter { })) } - async _resubmitTx (txMeta) { + async _resubmitTx (txMeta, latestBlockNumber) { + if (!txMeta.firstRetryBlockNumber) { + this.emit('tx:block-update', txMeta, latestBlockNumber) + } + if (Date.now() > txMeta.time + this.retryTimePeriod) { const hours = (this.retryTimePeriod / 3.6e+6).toFixed(1) const err = new Error(`Gave up submitting after ${hours} hours.`) return this.emit('tx:failed', txMeta.id, err) } + const firstRetryBlockNumber = txMeta.firstRetryBlockNumber + const txBlockDistance = Number.parseInt(latestBlockNumber, 16) - Number.parseInt(firstRetryBlockNumber, 16) + + const retryCount = txMeta.retryCount || 0 + + // Exponential backoff to limit retries at publishing + if (txBlockDistance <= Math.pow(2, retryCount) - 1) return + // Only auto-submit already-signed txs: if (!('rawTx' in txMeta)) return -- cgit From 871d9fd9fb8417a2dd47abafe68ae07e4903ba6e Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 6 Dec 2017 13:02:38 -0330 Subject: Fix undefined latestBlockNumber in _resubmitTx --- app/scripts/lib/pending-tx-tracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/lib/pending-tx-tracker.js b/app/scripts/lib/pending-tx-tracker.js index 60c837040..62d621ac1 100644 --- a/app/scripts/lib/pending-tx-tracker.js +++ b/app/scripts/lib/pending-tx-tracker.js @@ -69,7 +69,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter { const pending = this.getPendingTransactions() // only try resubmitting if their are transactions to resubmit if (!pending.length) return - pending.forEach((txMeta) => this._resubmitTx(txMeta).catch((err) => { + pending.forEach((txMeta) => this._resubmitTx(txMeta, block.number).catch((err) => { /* Dont marked as failed if the error is a "known" transaction warning "there is already a transaction with the same sender-nonce -- cgit From ea23da9e75c92cbf82d0daa19847d2035361a656 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 6 Dec 2017 13:07:31 -0330 Subject: Correct note for updateTx after block-update event in transaction.js --- 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 063ba8f65..ce709bd28 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -75,7 +75,7 @@ module.exports = class TransactionController extends EventEmitter { this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => { if (!txMeta.firstRetryBlockNumber) { txMeta.firstRetryBlockNumber = latestBlockNumber - this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:retry') + this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:block-update') } }) this.pendingTxTracker.on('tx:retry', (txMeta) => { -- cgit From f58aae3f2ba3d8a129b2c09dc3b45369c488fd04 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 6 Dec 2017 13:21:09 -0330 Subject: firstRetryBlockNumber defaults to latestBlockNumber if undefined on txMeta in _resubmitTx --- app/scripts/lib/pending-tx-tracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/lib/pending-tx-tracker.js b/app/scripts/lib/pending-tx-tracker.js index 62d621ac1..dc6e526fd 100644 --- a/app/scripts/lib/pending-tx-tracker.js +++ b/app/scripts/lib/pending-tx-tracker.js @@ -112,7 +112,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter { return this.emit('tx:failed', txMeta.id, err) } - const firstRetryBlockNumber = txMeta.firstRetryBlockNumber + const firstRetryBlockNumber = txMeta.firstRetryBlockNumber || latestBlockNumber const txBlockDistance = Number.parseInt(latestBlockNumber, 16) - Number.parseInt(firstRetryBlockNumber, 16) const retryCount = txMeta.retryCount || 0 -- cgit From af8fcb67778fadd7056c0930eeb9d22eb28b0e69 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 6 Dec 2017 14:10:11 -0330 Subject: Update resubmitPendingTxs tests. --- test/unit/pending-tx-test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/pending-tx-test.js b/test/unit/pending-tx-test.js index 4b5170dfe..7069554f7 100644 --- a/test/unit/pending-tx-test.js +++ b/test/unit/pending-tx-test.js @@ -206,6 +206,7 @@ describe('PendingTransactionTracker', function () { }) describe('#resubmitPendingTxs', function () { + const blockStub = { number: '0x0' }; beforeEach(function () { const txMeta2 = txMeta3 = txMeta txList = [txMeta, txMeta2, txMeta3].map((tx) => { @@ -223,7 +224,7 @@ describe('PendingTransactionTracker', function () { Promise.all(txList.map((tx) => tx.processed)) .then((txCompletedList) => done()) .catch(done) - pendingTxTracker.resubmitPendingTxs() + pendingTxTracker.resubmitPendingTxs(blockStub) }) it('should not emit \'tx:failed\' if the txMeta throws a known txError', function (done) { knownErrors =[ @@ -250,7 +251,7 @@ describe('PendingTransactionTracker', function () { .then((txCompletedList) => done()) .catch(done) - pendingTxTracker.resubmitPendingTxs() + pendingTxTracker.resubmitPendingTxs(blockStub) }) it('should emit \'tx:warning\' if it encountered a real error', function (done) { pendingTxTracker.once('tx:warning', (txMeta, err) => { @@ -268,7 +269,7 @@ describe('PendingTransactionTracker', function () { .then((txCompletedList) => done()) .catch(done) - pendingTxTracker.resubmitPendingTxs() + pendingTxTracker.resubmitPendingTxs(blockStub) }) }) describe('#_resubmitTx', function () { -- cgit From 3356c15d04d947c92a2c19690384396651c352e5 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 6 Dec 2017 14:11:14 -0330 Subject: Add tests for exponential backoff code in _resubmitTx --- test/unit/pending-tx-test.js | 80 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 17 deletions(-) diff --git a/test/unit/pending-tx-test.js b/test/unit/pending-tx-test.js index 7069554f7..393601a57 100644 --- a/test/unit/pending-tx-test.js +++ b/test/unit/pending-tx-test.js @@ -273,24 +273,70 @@ describe('PendingTransactionTracker', function () { }) }) describe('#_resubmitTx', function () { - it('should publishing the transaction', function (done) { - const enoughBalance = '0x100000' - pendingTxTracker.getBalance = (address) => { - assert.equal(address, txMeta.txParams.from, 'Should pass the address') - return enoughBalance - } - pendingTxTracker.publishTransaction = async (rawTx) => { - assert.equal(rawTx, txMeta.rawTx, 'Should pass the rawTx') - } + const mockFirstRetryBlockNumber = '0x1' + let txMetaToTestExponentialBackoff + + beforeEach(() => { + pendingTxTracker.getBalance = (address) => { + assert.equal(address, txMeta.txParams.from, 'Should pass the address') + return enoughBalance + } + pendingTxTracker.publishTransaction = async (rawTx) => { + assert.equal(rawTx, txMeta.rawTx, 'Should pass the rawTx') + } + sinon.spy(pendingTxTracker, 'publishTransaction') + + txMetaToTestExponentialBackoff = Object.assign({}, txMeta, { + retryCount: 4, + firstRetryBlockNumber: mockFirstRetryBlockNumber, + }) + }) - // Stubbing out current account state: - // Adding the fake tx: - pendingTxTracker._resubmitTx(txMeta) - .then(() => done()) - .catch((err) => { - assert.ifError(err, 'should not throw an error') - done(err) + afterEach(() => { + pendingTxTracker.publishTransaction.reset() + }) + + it('should publish the transaction', function (done) { + const enoughBalance = '0x100000' + + // Stubbing out current account state: + // Adding the fake tx: + pendingTxTracker._resubmitTx(txMeta) + .then(() => done()) + .catch((err) => { + assert.ifError(err, 'should not throw an error') + done(err) + }) + + assert.equal(pendingTxTracker.publishTransaction.callCount, 1, 'Should call publish transaction') + }) + + it('should not publish the transaction if the limit of retries has been exceeded', function (done) { + const enoughBalance = '0x100000' + const mockLatestBlockNumber = '0x5' + + pendingTxTracker._resubmitTx(txMetaToTestExponentialBackoff, mockLatestBlockNumber) + .then(() => done()) + .catch((err) => { + assert.ifError(err, 'should not throw an error') + done(err) + }) + + assert.equal(pendingTxTracker.publishTransaction.callCount, 0, 'Should NOT call publish transaction') + }) + + it('should publish the transaction if the number of blocks since last retry exceeds the last set limit', function (done) { + const enoughBalance = '0x100000' + const mockLatestBlockNumber = '0x11' + + pendingTxTracker._resubmitTx(txMetaToTestExponentialBackoff, mockLatestBlockNumber) + .then(() => done()) + .catch((err) => { + assert.ifError(err, 'should not throw an error') + done(err) + }) + + assert.equal(pendingTxTracker.publishTransaction.callCount, 1, 'Should call publish transaction') }) - }) }) }) -- cgit