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.

Sign up for GitHub

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

transactions/pending - buffer 3 blocks before dropping a tx #7483

Merged
merged 2 commits into from
Dec 5, 2019
Merged

transactions/pending - buffer 3 blocks before dropping a tx #7483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f31cfb
transactions/pending - buffer 3 blocks before dropping a tx
frankiebee Nov 20, 2019
2682b07
transactions/pending - only increment for dropped txs
frankiebee Dec 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions app/scripts/controllers/transactions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ class TransactionController extends EventEmitter {
* @param {number} txId - The tx's ID
* @returns {Promise<void>}
*/
async confirmTransaction (txId) {
async confirmTransaction (txId, txReceipt) {
// get the txReceipt before marking the transaction confirmed
// to ensure the receipt is gotten before the ui revives the tx
const txMeta = this.txStateManager.getTx(txId)
Expand All @@ -480,7 +480,6 @@ class TransactionController extends EventEmitter {
}

try {
const txReceipt = await this.query.getTransactionReceipt(txMeta.hash)

// It seems that sometimes the numerical values being returned from
// this.query.getTransactionReceipt are BN instances and not strings.
Expand Down Expand Up @@ -594,7 +593,7 @@ class TransactionController extends EventEmitter {
this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:warning')
})
this.pendingTxTracker.on('tx:failed', this.txStateManager.setTxStatusFailed.bind(this.txStateManager))
this.pendingTxTracker.on('tx:confirmed', (txId) => this.confirmTransaction(txId))
this.pendingTxTracker.on('tx:confirmed', (txId, transactionReceipt) => this.confirmTransaction(txId, transactionReceipt))
this.pendingTxTracker.on('tx:dropped', this.txStateManager.setTxStatusDropped.bind(this.txStateManager))
this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => {
if (!txMeta.firstRetryBlockNumber) {
Expand Down
34 changes: 24 additions & 10 deletions app/scripts/controllers/transactions/pending-tx-tracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ class PendingTransactionTracker extends EventEmitter {
Ask the network for the transaction to see if it has been include in a block
@param txMeta {Object} - the txMeta object
@emits tx:failed
@emits tx:dropped
@emits tx:confirmed
@emits tx:warning
*/
Expand All @@ -143,6 +144,9 @@ class PendingTransactionTracker extends EventEmitter {

return
}
// *note to self* hard failure point
const transactionReceipt = await this.query.getTransactionReceipt(txHash)


// If another tx with the same nonce is mined, set as dropped.
const taken = await this._checkIfNonceIsTaken(txMeta)
Expand All @@ -151,16 +155,23 @@ class PendingTransactionTracker extends EventEmitter {
// 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)
dropped = await this._checkIftxWasDropped(txMeta, transactionReceipt)
// 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
if (typeof this.droppedBuffer[txHash] !== 'number') {
this.droppedBuffer[txHash] = 0
}

// 3 block count buffer
if (dropped && this.droppedBuffer[txHash] < 3) {
dropped = false
} else if (dropped && this.droppedBuffer[txHash]) {
++this.droppedBuffer[txHash]
}

if (dropped && this.droppedBuffer[txHash] === 3) {
// clean up
delete this.droppedBuffer[txHash]
}
Expand All @@ -174,9 +185,9 @@ class PendingTransactionTracker extends EventEmitter {

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

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

/**
checks to see if a confirmed txMeta has the same nonce
checks local txs to see if a confirmed txMeta has the same nonce
@param txMeta {Object} - txMeta object
@returns {boolean}
*/
Expand All @@ -214,6 +225,9 @@ class PendingTransactionTracker extends EventEmitter {
const address = txMeta.txParams.from
const completed = this.getCompletedTransactions(address)
const sameNonce = completed.filter((otherMeta) => {
if (otherMeta.id === txMeta.id) {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
return false
}
return otherMeta.txParams.nonce === txMeta.txParams.nonce
})
return sameNonce.length > 0
Expand Down
38 changes: 24 additions & 14 deletions test/unit/app/controllers/transactions/pending-tx-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,14 @@ describe('PendingTransactionTracker', function () {
value: '0x01',
hash: '0xbad',
status: 'confirmed',
nonce: '0x01',
}, { count: 1 })
}, { count: 1, fromNonce: '0x01' })

const pending = txGen.generate({
id: '123',
value: '0x02',
hash: '0xfad',
hash: '0x2a919d2512ec963f524bfd9730fb66b6d5a2e399d1dd957abb5e2b544a12644b',
status: 'submitted',
nonce: '0x01',
}, { count: 1 })[0]
}, { count: 1, fromNonce: '0x01' })[0]

stub = sinon.stub(pendingTxTracker, 'getCompletedTransactions')
.returns(txGen.txs)
Expand Down Expand Up @@ -106,7 +104,7 @@ describe('PendingTransactionTracker', function () {
pendingTxTracker._checkPendingTx(txMetaNoHash)
})

it('should emit tx:dropped with the txMetas id only after the second call', function (done) {
it('should emit tx:dropped with the txMetas id only after the fourth call', function (done) {
txMeta = {
id: 1,
hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
Expand All @@ -120,20 +118,32 @@ describe('PendingTransactionTracker', function () {
rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
}

let counter = 0
providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
providerResultStub['eth_getTransactionReceipt'] = {}
pendingTxTracker.once('tx:dropped', (id) => {
if (id === txMeta.id) {
delete providerResultStub['eth_getTransactionCount']
delete providerResultStub['eth_getTransactionByHash']
return done()
delete providerResultStub['eth_getTransactionReceipt']
if (counter === 3) {
return done()
} else {
return done(new Error(`Counter does not equal 3 got ${counter} instead`))
}
} else {
done(new Error('wrong tx Id'))
}
})

pendingTxTracker._checkPendingTx(txMeta).then(() => {
pendingTxTracker._checkPendingTx(txMeta).catch(done)
++counter
pendingTxTracker._checkPendingTx(txMeta).then(() => {
++counter
pendingTxTracker._checkPendingTx(txMeta).then(() => {
++counter
pendingTxTracker._checkPendingTx(txMeta)
})
})
}).catch(done)
})

Expand Down Expand Up @@ -328,17 +338,17 @@ describe('PendingTransactionTracker', function () {
}
it('should return false when the nonce is the suggested network nonce', (done) => {
providerResultStub['eth_getTransactionCount'] = '0x01'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
providerResultStub['eth_getTransactionReceipt'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta, {}).then((dropped) => {
assert(!dropped, 'should be false')
done()
}).catch(done)
})

it('should return true when the network nonce is higher then the txMeta nonce', function (done) {
providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
providerResultStub['eth_getTransactionReceipt'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta, {}).then((dropped) => {
assert(dropped, 'should be true')
done()
}).catch(done)
Expand Down