Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop transactions who's nonce is lower then the known network nonce but were not included in a block #6388

Merged
merged 8 commits into from
May 16, 2019
1 change: 1 addition & 0 deletions app/scripts/controllers/transactions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ class TransactionController extends EventEmitter {
})
this.pendingTxTracker.on('tx:failed', this.txStateManager.setTxStatusFailed.bind(this.txStateManager))
this.pendingTxTracker.on('tx:confirmed', (txId) => this.confirmTransaction(txId))
this.pendingTxTracker.on('tx:dropped', this.txStateManager.setTxStatusDropped.bind(this.txStateManager))
this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => {
if (!txMeta.firstRetryBlockNumber) {
txMeta.firstRetryBlockNumber = latestBlockNumber
Expand Down
53 changes: 45 additions & 8 deletions app/scripts/controllers/transactions/pending-tx-tracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const EthQuery = require('ethjs-query')
class PendingTransactionTracker extends EventEmitter {
constructor (config) {
super()
this.droppedBuffer = {}
this.query = new EthQuery(config.provider)
this.nonceTracker = config.nonceTracker
this.getPendingTransactions = config.getPendingTransactions
Expand Down Expand Up @@ -139,22 +140,42 @@ class PendingTransactionTracker extends EventEmitter {
const noTxHashErr = new Error('We had an error while submitting this transaction, please try again.')
noTxHashErr.name = 'NoTxHashError'
this.emit('tx:failed', txId, noTxHashErr)

return
}

// If another tx with the same nonce is mined, set as failed.
// If another tx with the same nonce is mined, set as dropped.
const taken = await this._checkIfNonceIsTaken(txMeta)
if (taken) {
const nonceTakenErr = new Error('Another transaction with this nonce has been mined.')
nonceTakenErr.name = 'NonceTakenErr'
return this.emit('tx:failed', txId, nonceTakenErr)
let dropped
try {
// check the network if the nonce is ahead the tx
// and the tx has not been mined into a block

dropped = await this._checkIftxWasDropped(txMeta)
// the dropped buffer is in case we ask a node for the tx
// that is behind the node we asked for tx count
// IS A SECURITY FOR HITTING NODES IN INFURA THAT COULD GO OUT
// OF SYNC.
// on the next block event it will return fire as dropped
if (dropped && !this.droppedBuffer[txHash]) {
this.droppedBuffer[txHash] = true
dropped = false
} else if (dropped && this.droppedBuffer[txHash]) {
// clean up
delete this.droppedBuffer[txHash]
}

} catch (e) {
log.error(e)
}
if (taken || dropped) {
return this.emit('tx:dropped', txId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emitting "dropped" here and _checkIftxWasDropped,
suggest to move all of that logic into one function or break the nonceIsTaken into a different status?

jennypollack marked this conversation as resolved.
Show resolved Hide resolved
}

// get latest transaction status
try {
const txParams = await this.query.getTransactionByHash(txHash)
if (!txParams) return
if (txParams.blockNumber) {
const { blockNumber } = await this.query.getTransactionByHash(txHash) || {}
if (blockNumber) {
this.emit('tx:confirmed', txId)
}
} catch (err) {
Expand All @@ -165,6 +186,22 @@ class PendingTransactionTracker extends EventEmitter {
this.emit('tx:warning', txMeta, err)
}
}
/**
checks to see if if the tx's nonce has been used by another transaction
@param txMeta {Object} - txMeta object
@emits tx:dropped
@returns {boolean}
*/

async _checkIftxWasDropped (txMeta) {
const { txParams: { nonce, from }, hash } = txMeta
const nextNonce = await this.query.getTransactionCount(from)
const { blockNumber } = await this.query.getTransactionByHash(hash) || {}
if (!blockNumber && parseInt(nextNonce) > parseInt(nonce)) {
return true
}
return false
}

/**
checks to see if a confirmed txMeta has the same nonce
Expand Down
72 changes: 67 additions & 5 deletions test/unit/app/controllers/transactions/pending-tx-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('PendingTransactionTracker', function () {
}
})

it('should become failed if another tx with the same nonce succeeds', async function () {
it('should emit dropped if another tx with the same nonce succeeds', async function () {

// SETUP
const txGen = new MockTxGen()
Expand All @@ -84,17 +84,16 @@ describe('PendingTransactionTracker', function () {

// THE EXPECTATION
const spy = sinon.spy()
pendingTxTracker.on('tx:failed', (txId, err) => {
pendingTxTracker.on('tx:dropped', (txId) => {
assert.equal(txId, pending.id, 'should fail the pending tx')
assert.equal(err.name, 'NonceTakenErr', 'should emit a nonce taken error.')
spy(txId, err)
spy(txId)
})

// THE METHOD
await pendingTxTracker._checkPendingTx(pending)

// THE ASSERTION
assert.ok(spy.calledWith(pending.id), 'tx failed should be emitted')
assert.ok(spy.calledWith(pending.id), 'tx dropped should be emitted')
})
})

Expand All @@ -107,6 +106,38 @@ describe('PendingTransactionTracker', function () {
pendingTxTracker._checkPendingTx(txMetaNoHash)
})

it('should emit tx:dropped with the txMetas id only after the second call', function (done) {
txMeta = {
id: 1,
hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
status: 'submitted',
txParams: {
from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
nonce: '0x1',
value: '0xfffff',
},
history: [{}],
rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
}

providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker.once('tx:dropped', (id) => {
if (id === txMeta.id) {
delete providerResultStub['eth_getTransactionCount']
delete providerResultStub['eth_getTransactionByHash']
return done()
} else {
done(new Error('wrong tx Id'))
}
})

pendingTxTracker._checkPendingTx(txMeta).then(() => {
pendingTxTracker._checkPendingTx(txMeta).catch(done)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we ignoring the returned promise? Should we be returning it to the chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test: should emit tx:dropped with the txMetas id only after the second call

so we are testing the emit's logic here

}).catch(done)
})


it('should should return if query does not return txParams', function () {
providerResultStub.eth_getTransactionByHash = null
pendingTxTracker._checkPendingTx(txMeta)
Expand Down Expand Up @@ -283,6 +314,37 @@ describe('PendingTransactionTracker', function () {
})
})

describe('#_checkIftxWasDropped', () => {
const txMeta = {
id: 1,
hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
status: 'submitted',
txParams: {
from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
nonce: '0x1',
value: '0xfffff',
},
rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
}
it('should return false', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this test title to include the condition when it returns false, e.g.,

should return false when ...

providerResultStub['eth_getTransactionCount'] = '0x01'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
assert(!dropped, 'should be false')
done()
}).catch(done)
})

it('should return true', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this test title to include the condition when it returns true, e.g.,

should return true when ...

providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
assert(dropped, 'should be true')
done()
}).catch(done)
})
})

describe('#_checkIfNonceIsTaken', function () {
beforeEach(function () {
const confirmedTxList = [{
Expand Down