Transaction queue improvements

ethcore/src/miner/miner.rs

@@ -564,7 +564,7 @@ impl Miner {
 		prepare_new
 	}
 
-	fn add_transactions_to_queue(&self, chain: &MiningBlockChainClient, transactions: Vec<SignedTransaction>, origin: TransactionOrigin, transaction_queue: &mut BanningTransactionQueue) ->
+	fn add_transactions_to_queue(&self, chain: &MiningBlockChainClient, transactions: Vec<SignedTransaction>, default_origin: TransactionOrigin, transaction_queue: &mut BanningTransactionQueue) ->
 		Vec<Result<TransactionImportResult, Error>> {
 
 		let fetch_account = |a: &Address| AccountDetails {
@@ -572,15 +572,28 @@ impl Miner {
 			balance: chain.latest_balance(a),
 		};
 
+		let accounts = self.accounts.as_ref()
+			.and_then(|provider| provider.accounts().ok())
+			.map(|accounts| accounts.into_iter().collect::<HashSet<_>>());
+
 		let schedule = chain.latest_schedule();
 		let gas_required = |tx: &SignedTransaction| tx.gas_required(&schedule).into();
 		transactions.into_iter()
-			.map(|tx| match origin {
+			.map(|tx| {
-				TransactionOrigin::Local | TransactionOrigin::RetractedBlock => {
+				let origin = accounts.as_ref().and_then(|accounts| {
-					transaction_queue.add(tx, origin, &fetch_account, &gas_required)
+					tx.sender().ok().and_then(|sender| match accounts.contains(&sender) {
-				},
+						true => Some(TransactionOrigin::Local),
-				TransactionOrigin::External => {
+						false => None,
-					transaction_queue.add_with_banlist(tx, &fetch_account, &gas_required)
+					})
+				}).unwrap_or(default_origin);
+
+				match origin {
+					TransactionOrigin::Local | TransactionOrigin::RetractedBlock => {
+						transaction_queue.add(tx, origin, &fetch_account, &gas_required)
+					},
+					TransactionOrigin::External => {
+						transaction_queue.add_with_banlist(tx, &fetch_account, &gas_required)
+					}
 				}
 			})
 			.collect()

ethcore/src/miner/transaction_queue.rs

@@ -250,6 +250,7 @@ impl Ord for TransactionOrder {
 }
 
 /// Verified transaction (with sender)
+#[derive(Debug)]
 struct VerifiedTransaction {
 	/// Transaction
 	transaction: SignedTransaction,
@@ -637,7 +638,11 @@ impl TransactionQueue {
 					self.local_transactions.mark_future(hash);
 				},
 				Err(Error::Transaction(ref err)) => {
-					self.local_transactions.mark_rejected(cloned_tx, err.clone());
+					// Sometimes transactions are re-imported, so
+					// don't overwrite transactions if they are already on the list
+					if !self.local_transactions.contains(&cloned_tx.hash()) {
+						self.local_transactions.mark_rejected(cloned_tx, err.clone());
+					}
 				},
 				Err(_) => {
 					self.local_transactions.mark_invalid(cloned_tx);
@@ -772,6 +777,12 @@ impl TransactionQueue {
 			None => return,
 			Some(t) => t,
 		};
+
+		// Never penalize local transactions
+		if transaction.origin.is_local() {
+			return;
+		}
+
 		let sender = transaction.sender();
 
 		// Penalize all transactions from this sender
@@ -843,6 +854,33 @@ impl TransactionQueue {
 		}
 	}
 
+	/// Marks all transactions from particular sender as local transactions
+	fn mark_transactions_local(&mut self, sender: &Address) {
+		fn mark_local<F: FnMut(H256)>(sender: &Address, set: &mut TransactionSet, mut mark: F) {
+			// Mark all transactions from this sender as local
+			let nonces_from_sender = set.by_address.row(sender)
+				.map(|row_map| {
+					row_map.iter().filter_map(|(nonce, order)| if order.origin.is_local() {
+						None
+					} else {
+						Some(*nonce)
+					}).collect::<Vec<U256>>()
+				})
+				.unwrap_or_else(Vec::new);
+
+			for k in nonces_from_sender {
+				let mut order = set.drop(sender, &k).expect("transaction known to be in self.current/self.future; qed");
+				order.origin = TransactionOrigin::Local;
+				mark(order.hash);
+				set.insert(*sender, k, order);
+			}
+		}
+
+		let local = &mut self.local_transactions;
+		mark_local(sender, &mut self.current, |hash| local.mark_pending(hash));
+		mark_local(sender, &mut self.future, |hash| local.mark_future(hash));
+	}
+
 	/// Update height of all transactions in future transactions set.
 	fn update_future(&mut self, sender: &Address, current_nonce: U256) {
 		// We need to drain all transactions for current sender from future and reinsert them with updated height
@@ -1022,6 +1060,10 @@ impl TransactionQueue {
 			.cloned()
 			.map_or(state_nonce, |n| n + U256::one());
 
+		if tx.origin.is_local() {
+			self.mark_transactions_local(&address);
+		}
+
 		// Future transaction
 		if nonce > next_nonce {
 			// We have a gap - put to future.
@@ -1099,6 +1141,7 @@ impl TransactionQueue {
 		let old_hash = by_hash.insert(hash, tx);
 		assert!(old_hash.is_none(), "Each hash has to be inserted exactly once.");
 
+		trace!(target: "txqueue", "Inserting: {:?}", order);
 
 		if let Some(old) = set.insert(address, nonce, order.clone()) {
 			Self::replace_orders(address, nonce, old, order, set, by_hash, local)
@@ -1726,6 +1769,31 @@ mod test {
 		assert_eq!(top.len(), 2);
 	}
 
+	#[test]
+	fn when_importing_local_should_mark_others_from_the_same_sender_as_local() {
+		// given
+		let mut txq = TransactionQueue::default();
+		let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into());
+		// the second one has same nonce but higher `gas_price`
+		let (_, tx0) = new_similar_tx_pair();
+
+		txq.add(tx0.clone(), TransactionOrigin::External, &default_account_details, &gas_estimator).unwrap();
+		txq.add(tx1.clone(), TransactionOrigin::External, &default_account_details, &gas_estimator).unwrap();
+		// the one with higher gas price is first
+		assert_eq!(txq.top_transactions()[0], tx0);
+		assert_eq!(txq.top_transactions()[1], tx1);
+
+		// when
+		// insert second as local
+		txq.add(tx2.clone(), TransactionOrigin::Local, &default_account_details, &gas_estimator).unwrap();
+
+		// then
+		// the order should be updated
+		assert_eq!(txq.top_transactions()[0], tx1);
+		assert_eq!(txq.top_transactions()[1], tx0);
+		assert_eq!(txq.top_transactions()[2], tx2);
+	}
+
 	#[test]
 	fn should_prioritize_reimported_transactions_within_same_nonce_height() {
 		// given
@@ -1793,6 +1861,38 @@ mod test {
 		assert_eq!(top.len(), 4);
 	}
 
+	#[test]
+	fn should_not_penalize_local_transactions() {
+		// given
+		let mut txq = TransactionQueue::default();
+		// txa, txb - slightly bigger gas price to have consistent ordering
+		let (txa, txb) = new_tx_pair_default(1.into(), 0.into());
+		let (tx1, tx2) = new_tx_pair_with_gas_price_increment(3.into());
+
+		// insert everything
+		txq.add(txa.clone(), TransactionOrigin::Local, &default_account_details, &gas_estimator).unwrap();
+		txq.add(txb.clone(), TransactionOrigin::Local, &default_account_details, &gas_estimator).unwrap();
+		txq.add(tx1.clone(), TransactionOrigin::Local, &default_account_details, &gas_estimator).unwrap();
+		txq.add(tx2.clone(), TransactionOrigin::Local, &default_account_details, &gas_estimator).unwrap();
+
+		let top = txq.top_transactions();
+		assert_eq!(top[0], tx1);
+		assert_eq!(top[1], txa);
+		assert_eq!(top[2], tx2);
+		assert_eq!(top[3], txb);
+		assert_eq!(top.len(), 4);
+
+		// when
+		txq.penalize(&tx1.hash());
+
+		// then (order is the same)
+		let top = txq.top_transactions();
+		assert_eq!(top[0], tx1);
+		assert_eq!(top[1], txa);
+		assert_eq!(top[2], tx2);
+		assert_eq!(top[3], txb);
+		assert_eq!(top.len(), 4);
+	}
 
 	#[test]
 	fn should_penalize_transactions_from_sender() {

js/src/dapps/localtx/Application/application.css

@@ -0,0 +1,19 @@
+.container {
+  padding: 1rem 2rem;
+  text-align: center;
+
+  h1 {
+    margin-top: 3rem;
+    margin-bottom: 1rem;
+  }
+
+  table {
+    text-align: left;
+    margin: auto;
+    max-width: 90vw;
+
+    th {
+      text-align: center;
+    }
+  }
+}

js/src/dapps/localtx/Application/application.js

@@ -19,17 +19,20 @@ import React, { Component } from 'react';
 
 import { api } from '../parity';
 
+import styles from './application.css';
+
 import { Transaction, LocalTransaction } from '../Transaction';
 
 export default class Application extends Component {
   state = {
     loading: true,
     transactions: [],
-    localTransactions: {}
+    localTransactions: {},
+    blockNumber: 0
   }
 
   componentDidMount () {
-    const poll = () => this.fetchTransactionData().then(poll);
+    const poll = () => this.fetchTransactionData().then(poll).catch(poll);
     this._timeout = setTimeout(poll, 2000);
   }
 
@@ -42,7 +45,8 @@ export default class Application extends Component {
       api.parity.pendingTransactions(),
       api.parity.pendingTransactionsStats(),
       api.parity.localTransactions(),
-    ]).then(([pending, stats, local]) => {
+      api.eth.blockNumber()
+    ]).then(([pending, stats, local, blockNumber]) => {
       // Combine results together
       const transactions = pending.map(tx => {
         return {
@@ -87,7 +91,8 @@ export default class Application extends Component {
       this.setState({
         loading: false,
         transactions,
-        localTransactions
+        localTransactions,
+        blockNumber
       });
     });
   }
@@ -97,13 +102,13 @@ export default class Application extends Component {
 
     if (loading) {
       return (
-        <div>Loading...</div>
+        <div className={ styles.container }>Loading...</div>
       );
     }
 
     return (
-      <div style={ { padding: '1rem 2rem'}}>
+      <div className={ styles.container }>
-        <h1>Your past local transactions</h1>
+        <h1>Your local transactions</h1>
         { this.renderLocals() }
         <h1>Transactions in the queue</h1>
         { this.renderQueueSummary() }
@@ -135,7 +140,7 @@ export default class Application extends Component {
   }
 
   renderQueue () {
-    const { transactions } = this.state;
+    const { blockNumber, transactions } = this.state;
     if (!transactions.length) {
       return (
         <h3>The queue seems is empty.</h3>
@@ -156,6 +161,7 @@ export default class Application extends Component {
               isLocal={ tx.isLocal }
               transaction={ tx.transaction }
               stats={ tx.stats }
+              blockNumber={ blockNumber }
               />
           ))
         }

js/src/dapps/localtx/Transaction/transaction.js

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
+import BigNumber from 'bignumber.js';
 import React, { Component, PropTypes } from 'react';
 import classnames from 'classnames';
 
@@ -26,7 +27,7 @@ import IdentityIcon from '../../githubhint/IdentityIcon';
 class BaseTransaction extends Component {
 
   shortHash (hash) {
-    return `${hash.substr(0, 6)}..${hash.substr(hash.length - 4)}`;
+    return `${hash.substr(0, 5)}..${hash.substr(hash.length - 3)}`;
   }
 
   renderHash (hash) {
@@ -93,6 +94,7 @@ export class Transaction extends BaseTransaction {
   static propTypes = {
     idx: PropTypes.number.isRequired,
     transaction: PropTypes.object.isRequired,
+    blockNumber: PropTypes.object.isRequired,
     isLocal: PropTypes.bool,
     stats: PropTypes.object
   };
@@ -122,7 +124,7 @@ export class Transaction extends BaseTransaction {
           Gas
         </th>
         <th>
-          First seen
+          First propagation
         </th>
         <th>
           # Propagated
@@ -141,6 +143,7 @@ export class Transaction extends BaseTransaction {
     });
     const noOfPeers = Object.keys(stats.propagatedTo).length;
     const noOfPropagations = Object.values(stats.propagatedTo).reduce((sum, val) => sum + val, 0);
+    const blockNo = new BigNumber(stats.firstSeen);
 
     return (
       <tr className={ clazz }>
@@ -159,8 +162,8 @@ export class Transaction extends BaseTransaction {
         <td>
           { this.renderGas(transaction) }
         </td>
-        <td>
+        <td title={ blockNo.toFormat(0) }>
-          { stats.firstSeen }
+          { this.renderTime(stats.firstSeen) }
         </td>
         <td>
           { this.renderPropagation(stats) }
@@ -168,6 +171,16 @@ export class Transaction extends BaseTransaction {
       </tr>
     );
   }
+
+  renderTime (firstSeen) {
+    const { blockNumber } = this.props;
+    if (!firstSeen) {
+      return 'never';
+    }
+
+    const timeInMinutes = blockNumber.sub(firstSeen).mul(14).div(60).toFormat(1);
+    return `${timeInMinutes} minutes ago`;
+  }
 }
 
 export class LocalTransaction extends BaseTransaction {
@@ -200,9 +213,6 @@ export class LocalTransaction extends BaseTransaction {
         <th>
           Gas Price / Gas
         </th>
-        <th>
-          Propagated
-        </th>
         <th>
           Status
         </th>
@@ -252,7 +262,9 @@ export class LocalTransaction extends BaseTransaction {
     });
 
     const closeSending = () => this.setState({
-      isSending: false
+      isSending: false,
+      gasPrice: null,
+      gas: null
     });
 
     api.eth.sendTransaction(newTransaction)
@@ -292,11 +304,10 @@ export class LocalTransaction extends BaseTransaction {
           <br />
           { this.renderGas(transaction) }
         </td>
-        <td>
-          { status === 'pending' ? this.renderPropagation(stats) : '-' }
-        </td>
         <td>
           { this.renderStatus() }
+          <br />
+          { status === 'pending' ? this.renderPropagation(stats) : null }
         </td>
       </tr>
     );