From 3026429b05fead27d83779eeec0ced682bb964d3 Mon Sep 17 00:00:00 2001 From: hashmap Date: Wed, 16 May 2018 01:21:33 +0200 Subject: [PATCH] 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 | 36 +++++++++++++-------------- pool/tests/pool.rs | 25 ++++++++++--------- servers/src/grin/dandelion_monitor.rs | 21 ++++++++-------- servers/src/mining/mine_block.rs | 27 ++++++++++---------- 4 files changed, 56 insertions(+), 53 deletions(-) diff --git a/pool/src/pool.rs b/pool/src/pool.rs index 70bcd32f5..cdf01f225 100644 --- a/pool/src/pool.rs +++ b/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 { /// propagation pub time_stem_transactions: HashMap, /// All transactions in the stempool - pub stem_transactions: HashMap>, + pub stem_transactions: HashMap, /// All transactions in the pool - pub transactions: HashMap>, + pub transactions: HashMap, /// 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 - /// if necessary, and performing any connection-related validity checks. - /// Happens under an exclusive mutable reference gated by the write portion - /// of a RWLock. + /// Adds a transaction to the stem memory pool, deferring to the orphans + /// pool if necessary, and performing any connection-related validity + /// checks. Happens under an exclusive mutable reference gated by the + /// 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 - /// of the mempool + /// Attempt to deaggregate multi-kernel transaction as much as possible + /// 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>, PoolError> { + ) -> Result, 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, stem: bool, - ) -> Vec> { + ) -> Vec { let mut removed_txs = Vec::new(); if stem { @@ -839,7 +839,7 @@ where pub fn prepare_mineable_transactions( &self, num_to_fetch: u32, - ) -> Vec> { + ) -> Vec { self.pool .get_mineable_transactions(num_to_fetch) .iter() diff --git a/pool/tests/pool.rs b/pool/tests/pool.rs index f8de28d9a..58b3ef83f 100644 --- a/pool/tests/pool.rs +++ b/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 -/// child transaction In the end, the pool should contain both transactions. +/// Push the parent transaction in the mempool then send a multikernel tx +/// 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 -/// Child transaction should be added to the stempool. +/// A basic test; add a transaction to the stempool and one the regular +/// 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>; + let mut txs: Vec; { 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 = txs.drain(..).map(|x| *x).collect(); + let block_txs: Vec = txs.drain(..).collect(); let tx_refs: Vec<&transaction::Transaction> = block_txs.iter().collect(); let keychain = Keychain::from_random_seed().unwrap(); diff --git a/servers/src/grin/dandelion_monitor.rs b/servers/src/grin/dandelion_monitor.rs index 3fc578b6b..05116acd2 100644 --- a/servers/src/grin/dandelion_monitor.rs +++ b/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 -/// order to maintain reliability a timer is started for each transaction sent in stem phase. -/// This function will monitor the 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 +/// When sent in stem phase, the transaction is relayed to only node: the +/// dandelion relay. In order to maintain reliability a timer is started for +/// each transaction sent in stem phase. This function will monitor the +/// 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( config: PoolConfig, @@ -59,7 +60,7 @@ pub fn monitor_transactions( 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, ); diff --git a/servers/src/mining/mine_block.rs b/servers/src/mining/mine_block.rs index 988652fef..3396cf0bc 100644 --- a/servers/src/mining/mine_block.rs +++ b/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 wallet::BlockFees; +use pool; use util; use util::LOGGER; -use common::types::Error; -use common::adapters::PoolToChainAdapter; +use wallet; +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();