Do not wrap transactions into Box in pool (#1069)

It makes code more complex and may require more memory allocations than needed.

pool/src/pool.rs

@@ -14,11 +14,11 @@
 
 //! Top-level Pool type, methods, and tests
 
+use rand;
+use rand::Rng;
 use std::collections::{HashMap, HashSet};
 use std::sync::Arc;
 use time;
-use rand;
-use rand::Rng;
 
 use core::core::hash::Hash;
 use core::core::hash::Hashed;
@@ -29,8 +29,8 @@ use core::core::{block, hash};
 use util::LOGGER;
 use util::secp::pedersen::Commitment;
 
-use types::*;
 pub use graph;
+use types::*;
 
 /// The pool itself.
 /// The transactions HashMap holds ownership of all transactions in the pool,
@@ -42,9 +42,9 @@ pub struct TransactionPool<T> {
 	/// propagation
 	pub time_stem_transactions: HashMap<hash::Hash, i64>,
 	/// All transactions in the stempool
-	pub stem_transactions: HashMap<hash::Hash, Box<transaction::Transaction>>,
+	pub stem_transactions: HashMap<hash::Hash, transaction::Transaction>,
 	/// All transactions in the pool
-	pub transactions: HashMap<hash::Hash, Box<transaction::Transaction>>,
+	pub transactions: HashMap<hash::Hash, transaction::Transaction>,
 	/// The stem pool
 	pub stempool: Pool,
 	/// The pool itself
@@ -99,7 +99,7 @@ where
 
 				// if any kernel matches then keep the tx for later
 				if cb.kern_ids.contains(&short_id) {
-					txs.push(*tx.clone());
+					txs.push(tx.clone());
 					break;
 				}
 			}
@@ -208,10 +208,10 @@ where
 
 	/// Attempts to add a transaction to the stempool or the memory pool.
 	///
-	/// Adds a transaction to the stem memory pool, deferring to the orphans pool
+	/// Adds a transaction to the stem memory pool, deferring to the orphans
-	/// if necessary, and performing any connection-related validity checks.
+	/// pool if necessary, and performing any connection-related validity
-	/// Happens under an exclusive mutable reference gated by the write portion
+	/// checks. Happens under an exclusive mutable reference gated by the
-	/// of a RWLock.
+	/// write portion of a RWLock.
 	pub fn add_to_memory_pool(
 		&mut self,
 		_: TxSource,
@@ -349,7 +349,7 @@ where
 				);
 
 				self.adapter.stem_tx_accepted(&tx);
-				self.stem_transactions.insert(tx_hash, Box::new(tx));
+				self.stem_transactions.insert(tx_hash, tx);
 				// Track this transaction
 				self.time_stem_transactions.insert(tx_hash, timer);
 			} else {
@@ -361,7 +361,7 @@ where
 					new_unspents,
 				);
 				self.adapter.tx_accepted(&tx);
-				self.transactions.insert(tx_hash, Box::new(tx));
+				self.transactions.insert(tx_hash, tx);
 			}
 			self.reconcile_orphans().unwrap();
 			Ok(())
@@ -429,8 +429,8 @@ where
 		}
 	}
 
-	/// Attempt to deaggregate multi-kernel transaction as much as possible based on the content
+	/// Attempt to deaggregate multi-kernel transaction as much as possible
-	/// of the mempool
+	/// based on the content of the mempool
 	pub fn deaggregate_transaction(
 		&self,
 		tx: transaction::Transaction,
@@ -477,7 +477,7 @@ where
 				&& candidates_kernels_set.is_subset(&kernels_set)
 			{
 				debug!(LOGGER, "Found a transaction with the same kernel");
-				found_txs.push(*tx.clone());
+				found_txs.push(tx.clone());
 			}
 		}
 
@@ -636,7 +636,7 @@ where
 	pub fn reconcile_block(
 		&mut self,
 		block: &block::Block,
-	) -> Result<Vec<Box<transaction::Transaction>>, PoolError> {
+	) -> Result<Vec<transaction::Transaction>, PoolError> {
 		// If this pool has been kept in sync correctly, serializing all
 		// updates, then the inputs must consume only members of the blockchain
 		// output set.
@@ -796,7 +796,7 @@ where
 		&mut self,
 		marked_transactions: HashSet<hash::Hash>,
 		stem: bool,
-	) -> Vec<Box<transaction::Transaction>> {
+	) -> Vec<transaction::Transaction> {
 		let mut removed_txs = Vec::new();
 
 		if stem {
@@ -839,7 +839,7 @@ where
 	pub fn prepare_mineable_transactions(
 		&self,
 		num_to_fetch: u32,
-	) -> Vec<Box<transaction::Transaction>> {
+	) -> Vec<transaction::Transaction> {
 		self.pool
 			.get_mineable_transactions(num_to_fetch)
 			.iter()

pool/tests/pool.rs

@@ -26,19 +26,19 @@ extern crate time;
 
 use std::collections::HashMap;
 
+use core::core::block;
 use core::core::transaction::{self, ProofMessageElements};
 use core::core::{OutputIdentifier, Transaction};
-use core::core::block;
 
-use pool::*;
-use core::global;
 use blockchain::{DummyChain, DummyChainImpl, DummyOutputSet};
-use std::sync::{Arc, RwLock};
-use core::global::ChainTypes;
 use core::core::Proof;
 use core::core::hash::{Hash, Hashed};
 use core::core::pmmr::MerkleProof;
 use core::core::target::Difficulty;
+use core::global;
+use core::global::ChainTypes;
+use pool::*;
+use std::sync::{Arc, RwLock};
 use types::PoolError::InvalidTx;
 
 use keychain::Keychain;
@@ -182,8 +182,9 @@ fn test_multikernel_pool_add() {
 
 #[test]
 /// Attempt to deaggregate a multi_kernel transaction
-/// Push the parent transaction in the mempool then send a multikernel tx containing it and a
+/// Push the parent transaction in the mempool then send a multikernel tx
-/// child transaction In the end, the pool should contain both transactions.
+/// containing it and a child transaction In the end, the pool should contain
+/// both transactions.
 fn test_multikernel_deaggregate() {
 	let mut dummy_chain = DummyChainImpl::new();
 	let head_header = block::BlockHeader {
@@ -366,8 +367,8 @@ fn test_pool_stempool_add() {
 }
 
 #[test]
-/// A basic test; add a transaction to the stempool and one the regular transaction pool
+/// A basic test; add a transaction to the stempool and one the regular
-/// Child transaction should be added to the stempool.
+/// transaction pool Child transaction should be added to the stempool.
 fn test_stempool_pool_add() {
 	let mut dummy_chain = DummyChainImpl::new();
 	let head_header = block::BlockHeader {
@@ -645,7 +646,7 @@ fn test_zero_confirmation_reconciliation() {
 	{
 		let read_pool = pool.read().unwrap();
 		let mut mineable_txs = read_pool.prepare_mineable_transactions(3);
-		txs = mineable_txs.drain(..).map(|x| *x).collect();
+		txs = mineable_txs.drain(..).collect();
 
 		// confirm we can preparing both txs for mining here
 		// one root tx in the pool, and one non-root vertex in the pool
@@ -908,7 +909,7 @@ fn test_block_building() {
 
 	// Request blocks
 	let block: block::Block;
-	let mut txs: Vec<Box<transaction::Transaction>>;
+	let mut txs: Vec<transaction::Transaction>;
 	{
 		let read_pool = pool.read().unwrap();
 		txs = read_pool.prepare_mineable_transactions(3);
@@ -916,7 +917,7 @@ fn test_block_building() {
 		// TODO: This is ugly, either make block::new take owned
 		// txs instead of mut refs, or change
 		// prepare_mineable_transactions to return mut refs
-		let block_txs: Vec<transaction::Transaction> = txs.drain(..).map(|x| *x).collect();
+		let block_txs: Vec<transaction::Transaction> = txs.drain(..).collect();
 		let tx_refs: Vec<&transaction::Transaction> = block_txs.iter().collect();
 
 		let keychain = Keychain::from_random_seed().unwrap();

servers/src/grin/dandelion_monitor.rs

@@ -12,24 +12,25 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::{Arc, RwLock};
 use std::thread;
 use std::time::Duration;
-use std::sync::{Arc, RwLock};
-use std::sync::atomic::{AtomicBool, Ordering};
 use time::now_utc;
 use util::LOGGER;
 
-use pool::TransactionPool;
-use pool::PoolConfig;
-use pool::TxSource;
 use pool::BlockChain;
+use pool::PoolConfig;
+use pool::TransactionPool;
+use pool::TxSource;
 
 /// A process to monitor transactions in the stempool.
 /// With Dandelion, transaction can be broadcasted in stem or fluff phase.
-/// When sent in stem phase, the transaction is relayed to only node: the dandelion relay. In
+/// When sent in stem phase, the transaction is relayed to only node: the
-/// order to maintain reliability a timer is started for each transaction sent in stem phase.
+/// dandelion relay. In order to maintain reliability a timer is started for
-/// This function will monitor the stempool and test if the timer is expired for each transaction.
+/// each transaction sent in stem phase. This function will monitor the
-/// In that case the transaction will be sent in fluff phase (to multiple peers) instead of
+/// stempool and test if the timer is expired for each transaction. In that case
+/// the transaction will be sent in fluff phase (to multiple peers) instead of
 /// sending only to the peer relay.
 pub fn monitor_transactions<T>(
 	config: PoolConfig,
@@ -59,7 +60,7 @@ pub fn monitor_transactions<T>(
 						let stem_transaction = stem_transactions.get(tx_hash).unwrap();
 						let res = tx_pool.write().unwrap().add_to_memory_pool(
 							source,
-							*stem_transaction.clone(),
+							stem_transaction.clone(),
 							false,
 						);
 

servers/src/mining/mine_block.rs

@@ -15,29 +15,29 @@
 //! Build a block to mine: gathers transactions from the pool, assembles
 //! them into a block and returns it.
 
-use std::thread;
-use std::sync::{Arc, RwLock};
-use time;
-use std::time::Duration;
-use rand::{self, Rng};
 use itertools::Itertools;
+use rand::{self, Rng};
+use std::sync::{Arc, RwLock};
+use std::thread;
+use std::time::Duration;
+use time;
 
-use core::ser::AsFixedBytes;
 use chain;
 use chain::types::BlockSums;
-use pool;
+use common::adapters::PoolToChainAdapter;
+use common::types::Error;
 use core::consensus;
 use core::core;
 use core::core::Transaction;
 use core::core::hash::Hashed;
 use core::ser;
+use core::ser::AsFixedBytes;
 use keychain::{Identifier, Keychain};
-use wallet;
+use pool;
-use wallet::BlockFees;
 use util;
 use util::LOGGER;
-use common::types::Error;
+use wallet;
-use common::adapters::PoolToChainAdapter;
+use wallet::BlockFees;
 
 /// Serializer that outputs the pre-pow part of the header,
 /// including the nonce (last 8 bytes) that can be sent off
@@ -158,11 +158,12 @@ fn build_block(
 	let difficulty = consensus::next_difficulty(diff_iter).unwrap();
 
 	// extract current transaction from the pool
-	let txs_box = tx_pool
+	let txs = tx_pool
 		.read()
 		.unwrap()
 		.prepare_mineable_transactions(max_tx);
-	let txs: Vec<&Transaction> = txs_box.iter().map(|tx| tx.as_ref()).collect();
+
+	let txs: Vec<&Transaction> = txs.iter().collect();
 
 	// build the coinbase and the block itself
 	let fees = txs.iter().map(|tx| tx.fee()).sum();