Merge pull request #2670 from danjm/MM-2669-tx-retry-exponential-backoff

Exponentional backoff on transaction retry in pending-tx-tracker

3 files changed, 88 insertions, 23 deletions

diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js
index a861c0342..ce709bd28 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:block-update')
+      }
+    })
     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..dc6e526fd 100644
--- a/app/scripts/lib/pending-tx-tracker.js
+++ b/app/scripts/lib/pending-tx-tracker.js
@@ -65,11 +65,11 @@ 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
-    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
@@ -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 || latestBlockNumber
+    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
 
diff --git a/test/unit/pending-tx-test.js b/test/unit/pending-tx-test.js
index 4b5170dfe..393601a57 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,28 +269,74 @@ describe('PendingTransactionTracker', function () {
       .then((txCompletedList) => done())
       .catch(done)
 
-      pendingTxTracker.resubmitPendingTxs()
+      pendingTxTracker.resubmitPendingTxs(blockStub)
     })
   })
   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
 
-    // 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)
+    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,
+      })
+    })
+
+    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')
     })
-  })
  })
 })