From cee07fef747cb5146a170e066aeb2ca2b5cc129e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 9 Dec 2016 15:54:13 +0100 Subject: [PATCH] Trigger remove_old on new block --- ethcore/src/miner/miner.rs | 60 +++++++++----------------- ethcore/src/miner/transaction_queue.rs | 42 ++++++++++++------ 2 files changed, 48 insertions(+), 54 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 8d1f55567..cebd35b09 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -1083,20 +1083,6 @@ impl MinerService for Miner { fn chain_new_blocks(&self, chain: &MiningBlockChainClient, _imported: &[H256], _invalid: &[H256], enacted: &[H256], retracted: &[H256]) { trace!(target: "miner", "chain_new_blocks"); - fn fetch_transactions(chain: &MiningBlockChainClient, hash: &H256) -> Vec { - let block = chain - .block(BlockID::Hash(*hash)) - // Client should send message after commit to db and inserting to chain. - .expect("Expected in-chain blocks."); - let block = BlockView::new(&block); - let txs = block.transactions(); - // populate sender - for tx in &txs { - let _sender = tx.sender(); - } - txs - } - // 1. We ignore blocks that were `imported` (because it means that they are not in canon-chain, and transactions // should be still available in the queue. // 2. We ignore blocks that are `invalid` because it doesn't have any meaning in terms of the transactions that @@ -1107,35 +1093,29 @@ impl MinerService for Miner { // Then import all transactions... { - let out_of_chain = retracted - .par_iter() - .map(|h| fetch_transactions(chain, h)); - out_of_chain.for_each(|txs| { - let mut transaction_queue = self.transaction_queue.lock(); - let _ = self.add_transactions_to_queue( - chain, txs, TransactionOrigin::RetractedBlock, &mut transaction_queue - ); - }); + retracted.par_iter() + .map(|hash| { + let block = chain.block(BlockID::Hash(*hash)) + .expect("Client is sending message after commit to db and inserting to chain; the block is available; qed"); + let block = BlockView::new(&block); + let txs = block.transactions(); + // populate sender + for tx in &txs { + let _sender = tx.sender(); + } + txs + }).for_each(|txs| { + let mut transaction_queue = self.transaction_queue.lock(); + let _ = self.add_transactions_to_queue( + chain, txs, TransactionOrigin::RetractedBlock, &mut transaction_queue + ); + }); } - // ...and at the end remove old ones + // ...and at the end remove the old ones { - let in_chain = enacted - .par_iter() - .map(|h: &H256| fetch_transactions(chain, h)); - - in_chain.for_each(|mut txs| { - let mut transaction_queue = self.transaction_queue.lock(); - - let to_remove = txs.drain(..) - .map(|tx| { - tx.sender().expect("Transaction is in block, so sender has to be defined.") - }) - .collect::>(); - for sender in to_remove { - transaction_queue.remove_all(sender, chain.latest_nonce(&sender)); - } - }); + let mut transaction_queue = self.transaction_queue.lock(); + transaction_queue.remove_old(|sender| chain.latest_nonce(sender)); } if enacted.len() > 0 { diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index 84981b893..03a9da8e3 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -79,10 +79,10 @@ //! we check if the transactions should go to `current` (comparing state nonce) //! - When it's removed from `current` - all transactions from this sender (`current` & `future`) are recalculated. //! 3. `remove_all` is used to inform the queue about client (state) nonce changes. -//! - It removes all transactions (either from `current` or `future`) with nonce < client nonce -//! - It moves matching `future` transactions to `current` -//! 4. `remove_old` is used periodically to clear the transactions if node goes out of sync -//! (in such case `remove_all` is not invoked because we are syncing the chain) +//! - It removes all transactions (either from `current` or `future`) with nonce < client nonce +//! - It moves matching `future` transactions to `current` +//! 4. `remove_old` is used as convenient method to update the state nonce for all senders in the queue. +//! - Invokes `remove_all` with latest state nonce for all senders. use std::ops::Deref; use std::cmp::Ordering; @@ -754,6 +754,21 @@ impl TransactionQueue { /// Removes all transactions from particular sender up to (excluding) given client (state) nonce. /// Client (State) Nonce = next valid nonce for this sender. pub fn remove_all(&mut self, sender: Address, client_nonce: U256) { + // Check if there is anything in current... + let should_check_in_current = self.current.by_address.row(&sender) + // If nonce == client_nonce nothing is changed + .and_then(|by_nonce| by_nonce.keys().find(|nonce| *nonce < &client_nonce)) + .map(|_| ()); + // ... or future + let should_check_in_future = self.future.by_address.row(&sender) + // if nonce == client_nonce we need to promote to current + .and_then(|by_nonce| by_nonce.keys().find(|nonce| *nonce <= &client_nonce)) + .map(|_| ()); + + if should_check_in_current.or(should_check_in_future).is_none() { + return; + } + // We will either move transaction to future or remove it completely // so there will be no transactions from this sender in current self.last_nonces.remove(&sender); @@ -768,16 +783,16 @@ impl TransactionQueue { } /// Checks the current nonce for all transactions' senders in the queue and removes the old transactions. - pub fn remove_old(&mut self, fetch_account: &F) where - F: Fn(&Address) -> AccountDetails, + pub fn remove_old(&mut self, fetch_nonce: F) where + F: Fn(&Address) -> U256, { - let senders = self.current.by_address - .keys() - .map(|key| (*key, fetch_account(key).nonce)) - .collect::>(); + let senders = self.current.by_address.keys() + .chain(self.future.by_address.keys()) + .cloned() + .collect::>(); - for (sender, nonce) in senders { - self.remove_all(sender, nonce); + for sender in senders { + self.remove_all(sender, fetch_nonce(&sender)); } } @@ -2480,7 +2495,6 @@ mod test { let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); let (tx3, tx4) = new_tx_pair_default(1.into(), 0.into()); let nonce1 = tx1.nonce; - let new_details = |_a: &Address| AccountDetails { nonce: nonce1 + U256::one(), balance: !U256::zero() }; // Insert all transactions txq.add(tx1, TransactionOrigin::External, &default_account_details, &gas_estimator).unwrap(); @@ -2490,7 +2504,7 @@ mod test { assert_eq!(txq.top_transactions().len(), 4); // when - txq.remove_old(&new_details); + txq.remove_old(|_| nonce1 + U256::one()); // then assert_eq!(txq.top_transactions().len(), 2);