From 620a33b7bd6013f20d613be279b301ddada4ab89 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Sun, 15 Dec 2013 10:01:29 +0700 Subject: [PATCH 1/2] Keep track of all transactionIDs for a given Transaction that go on the network --- src/js/ripple/transaction.js | 12 ++++++++ src/js/ripple/transactionmanager.js | 35 +++++++++++++++++++--- src/js/ripple/transactionqueue.js | 45 +++++++++++++++++++++++------ 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/src/js/ripple/transaction.js b/src/js/ripple/transaction.js index 75cb15b4..5a63c8b5 100644 --- a/src/js/ripple/transaction.js +++ b/src/js/ripple/transaction.js @@ -81,6 +81,12 @@ function Transaction(remote) { this.finalized = false; this._previous_signing_hash = void(0); + + // We aren't clever enough to eschew preventative measures so we keep an array + // of all submitted transactionIDs (which can change due to load_factor + // effecting the Fee amount). This should be populated with a transactionID + // any time it goes on the network + this.submittedTxnIDs = [] }; util.inherits(Transaction, EventEmitter); @@ -210,6 +216,12 @@ Transaction.prototype.signingHash = function() { return this.hash(config.testnet ? 'HASH_TX_SIGN_TESTNET' : 'HASH_TX_SIGN'); }; +Transaction.prototype.addSubmittedTxnID = function(hash) { + if (-1 == this.submittedTxnIDs.indexOf(hash)) { + this.submittedTxnIDs.push(hash); + } +}; + Transaction.prototype.hash = function(prefix, as_uint256) { if (typeof prefix === 'string') { if (typeof hashprefixes[prefix] === 'undefined') { diff --git a/src/js/ripple/transactionmanager.js b/src/js/ripple/transactionmanager.js index 00eb9b99..d7eeb5e0 100644 --- a/src/js/ripple/transactionmanager.js +++ b/src/js/ripple/transactionmanager.js @@ -34,11 +34,14 @@ function TransactionManager(account) { self._sequenceCache[sequence] = transaction; - var pending = self._pending.get('_hash', hash); + // ND: we need to check against all submissions ids + var pending = self._pending.getBySubmissions(hash); + // var pending = self._pending.get('_hash', hash); self._remote._trace('transactionmanager: transaction_received: %s', transaction.transaction); if (pending) { + // ND: A `success` handler will `finalize` this later pending.emit('success', transaction); } else { self._cache[hash] = transaction; @@ -48,6 +51,7 @@ function TransactionManager(account) { this._account.on('transaction-outbound', transactionReceived); function adjustFees() { + // ND: note, that `Fee` is a component of a transactionID self._pending.forEach(function(pending) { if (self._remote.local_fee && pending.tx_json.Fee) { var oldFee = pending.tx_json.Fee; @@ -248,7 +252,11 @@ TransactionManager.prototype._request = function(tx) { submitRequest.tx_json(tx.tx_json); } - tx._hash = tx.hash(); + // ND: We could consider sharing the work with tx_blob when doing + // local_signing + + tx.addSubmittedTxnID(tx.hash()); + // tx._hash = tx.hash(); remote._trace('transactionmanager: submit: %s', tx.tx_json); @@ -302,8 +310,11 @@ TransactionManager.prototype._request = function(tx) { // Finalized (e.g. aborted) transactions must stop all activity if (tx.finalized) return; + + // ND: If for some unknown reason our hash wasn't computed correctly this is + // an extra measure. if (message.tx_json && message.tx_json.hash) { - tx._hash = message.tx_json.hash; + tx.addSubmittedTxnID(message.tx_json.hash); } message.result = message.engine_result || ''; @@ -331,6 +342,16 @@ TransactionManager.prototype._request = function(tx) { }; submitRequest.timeout(this._submissionTimeout, function() { + // ND: What if the response is just slow and we get a response that + // `submitted` above will cause to have concurrent resubmit logic streams? + // It's simpler to just mute handlers and look out for finalized + // `transaction` messages. + + // ND: We should audit the code for other potential multiple resubmit + // streams. Connection/reconnection could be one? That's why it's imperative + // that ALL transactionIDs sent over network are tracked. + submitRequest.removeAllListeners(); + // Finalized (e.g. aborted) transactions must stop all activity if (tx.finalized) return; @@ -397,7 +418,9 @@ TransactionManager.prototype.submit = function(tx) { function finalize(message) { if (!tx.finalized) { - self._pending.removeHash(tx._hash); + // ND: We can just remove this `tx` by identity + self._pending.remove(tx); + // self._pending.removeHash(tx._hash); remote._trace('transactionmanager: finalize_transaction: %s', tx.tx_json); tx.finalized = true; tx.emit('final', message); @@ -421,6 +444,10 @@ TransactionManager.prototype.submit = function(tx) { } else if (fee && fee > this._maxFee) { tx.emit('error', new RippleError('tejMaxFeeExceeded', 'Max fee exceeded')); } else { + // ND: this is the ONLY place we put the tx into the queue. The + // TransactionQueue queue is merely a list, so any mutations to tx._hash + // will cause subsequent look ups (eg. inside 'transaction-outbound' + // validated transaction clearing) to fail. this._pending.push(tx); this._request(tx); } diff --git a/src/js/ripple/transactionqueue.js b/src/js/ripple/transactionqueue.js index 6790a7b2..23ab001f 100644 --- a/src/js/ripple/transactionqueue.js +++ b/src/js/ripple/transactionqueue.js @@ -13,7 +13,18 @@ TransactionQueue.prototype.length = function() { return this._queue.length; }; -TransactionQueue.prototype.indexOf = function(prop, val) { +TransactionQueue.prototype.getBySubmissions = function(hash) { + for (var i=0, tx; tx=this._queue[i]; i++) { + for (var j=0, id; id=tx.submittedTxnIDs[j]; j++) { + if (hash === id) { + return tx; + }; + } + } + return false; +}; + +/*TransactionQueue.prototype.indexOf = function(prop, val) { var index = -1; for (var i=0, tx; tx=this._queue[i]; i++) { if (tx[prop] === val) { @@ -23,17 +34,22 @@ TransactionQueue.prototype.indexOf = function(prop, val) { } return index; }; - -TransactionQueue.prototype.hasHash = function(hash) { - return this.indexOf('hash', hash) !== -1; +*/ + // ND: fixed `hash` vs _`hash` +/*TransactionQueue.prototype.hasHash = function(hash) { + return this.indexOf('_hash', hash) !== -1; }; +*/ -TransactionQueue.prototype.get = function(prop, val) { + +/*TransactionQueue.prototype.get = function(prop, val) { var index = this.indexOf(prop, val); return index > -1 ? this._queue[index] : false; }; +*/ -TransactionQueue.prototype.removeSequence = function(sequence) { +// ND: These are unused +/*TransactionQueue.prototype.removeSequence = function(sequence) { var result = [ ]; for (var i=0, tx; tx=this._queue[i]; i++) { if (!tx.tx_json) continue; @@ -41,15 +57,26 @@ TransactionQueue.prototype.removeSequence = function(sequence) { } this._queue = result; }; - -TransactionQueue.prototype.removeHash = function(hash) { +*/ +/*TransactionQueue.prototype.removeHash = function(hash) { var result = [ ]; for (var i=0, tx; tx=this._queue[i]; i++) { if (!tx.tx_json) continue; - if (tx._hash !== hash) result.push(tx); + if (tx.hash !== hash) result.push(tx); } this._queue = result; }; +*/ + +// ND: We are just removing the Transaction by identity +TransactionQueue.prototype.remove = function(removedTx) { + for (var i = this._queue.length - 1; i >= 0; i--) { + if (this._queue[i] === removedTx) { + this._queue.splice(i, 1); + break; + }; + }; +}; TransactionQueue.prototype.forEach = function() { Array.prototype.forEach.apply(this._queue, arguments); From e34cf6ebcfea4674e141a5b386f1e1f73a6166f8 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Sun, 15 Dec 2013 10:09:54 +0700 Subject: [PATCH 2/2] Remove unused methods from TransactionQueue --- src/js/ripple/transactionmanager.js | 1 + src/js/ripple/transactionqueue.js | 53 +---------------------------- 2 files changed, 2 insertions(+), 52 deletions(-) diff --git a/src/js/ripple/transactionmanager.js b/src/js/ripple/transactionmanager.js index d7eeb5e0..60111fa3 100644 --- a/src/js/ripple/transactionmanager.js +++ b/src/js/ripple/transactionmanager.js @@ -19,6 +19,7 @@ function TransactionManager(account) { this._pending = new PendingQueue; this._nextSequence = void(0); this._cache = { }; + // ND: Do we ever clean this up? this._sequenceCache = { }; this._maxFee = this._remote.max_fee; this._submissionTimeout = this._remote._submission_timeout; diff --git a/src/js/ripple/transactionqueue.js b/src/js/ripple/transactionqueue.js index 23ab001f..4fc10b45 100644 --- a/src/js/ripple/transactionqueue.js +++ b/src/js/ripple/transactionqueue.js @@ -1,12 +1,5 @@ - function TransactionQueue() { - var self = this; - this._queue = [ ]; - - Object.defineProperty(this, '_length', { - get: function() { return self._queue.length } - }); } TransactionQueue.prototype.length = function() { @@ -24,50 +17,6 @@ TransactionQueue.prototype.getBySubmissions = function(hash) { return false; }; -/*TransactionQueue.prototype.indexOf = function(prop, val) { - var index = -1; - for (var i=0, tx; tx=this._queue[i]; i++) { - if (tx[prop] === val) { - index = i; - break; - } - } - return index; -}; -*/ - // ND: fixed `hash` vs _`hash` -/*TransactionQueue.prototype.hasHash = function(hash) { - return this.indexOf('_hash', hash) !== -1; -}; -*/ - - -/*TransactionQueue.prototype.get = function(prop, val) { - var index = this.indexOf(prop, val); - return index > -1 ? this._queue[index] : false; -}; -*/ - -// ND: These are unused -/*TransactionQueue.prototype.removeSequence = function(sequence) { - var result = [ ]; - for (var i=0, tx; tx=this._queue[i]; i++) { - if (!tx.tx_json) continue; - if (tx.tx_json.Sequence !== sequence) result.push(tx); - } - this._queue = result; -}; -*/ -/*TransactionQueue.prototype.removeHash = function(hash) { - var result = [ ]; - for (var i=0, tx; tx=this._queue[i]; i++) { - if (!tx.tx_json) continue; - if (tx.hash !== hash) result.push(tx); - } - this._queue = result; -}; -*/ - // ND: We are just removing the Transaction by identity TransactionQueue.prototype.remove = function(removedTx) { for (var i = this._queue.length - 1; i >= 0; i--) { @@ -86,4 +35,4 @@ TransactionQueue.prototype.push = function() { Array.prototype.push.apply(this._queue, arguments); }; -exports.TransactionQueue = TransactionQueue; +exports.TransactionQueue = TransactionQueue; \ No newline at end of file