From c912bb8c171d3e5fa0414d73fdd98c15fe208327 Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 20 Jun 2016 23:48:47 +0200 Subject: [PATCH 1/3] Fix lock order --- ethcore/src/miner/miner.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 735ad5cf4..f548d8a74 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -106,7 +106,8 @@ impl Miner { #[cfg_attr(feature="dev", allow(cyclomatic_complexity))] fn prepare_sealing(&self, chain: &MiningBlockChainClient) { trace!(target: "miner", "prepare_sealing: entering"); - let transactions = self.transaction_queue.lock().unwrap().top_transactions(); + let mut queue = self.transaction_queue.lock().unwrap(); + let transactions = queue.top_transactions(); let mut sealing_work = self.sealing_work.lock().unwrap(); let best_hash = chain.best_block_header().sha3(); @@ -163,7 +164,6 @@ impl Miner { let block = open_block.close(); - let mut queue = self.transaction_queue.lock().unwrap(); let fetch_account = |a: &Address| AccountDetails { nonce: chain.latest_nonce(a), balance: chain.latest_balance(a), From b6673788202123b47ef6a4fc42e15c1a1070fc97 Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 21 Jun 2016 11:26:43 +0200 Subject: [PATCH 2/3] Release lock while pushing transactions --- ethcore/src/miner/miner.rs | 64 +++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index f548d8a74..e559306c0 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -106,35 +106,38 @@ impl Miner { #[cfg_attr(feature="dev", allow(cyclomatic_complexity))] fn prepare_sealing(&self, chain: &MiningBlockChainClient) { trace!(target: "miner", "prepare_sealing: entering"); - let mut queue = self.transaction_queue.lock().unwrap(); - let transactions = queue.top_transactions(); - let mut sealing_work = self.sealing_work.lock().unwrap(); - let best_hash = chain.best_block_header().sha3(); + let (transactions, mut open_block) = { + let queue = self.transaction_queue.lock().unwrap(); + let transactions = queue.top_transactions(); + let mut sealing_work = self.sealing_work.lock().unwrap(); + let best_hash = chain.best_block_header().sha3(); /* - // check to see if last ClosedBlock in would_seals is actually same parent block. - // if so - // duplicate, re-open and push any new transactions. - // if at least one was pushed successfully, close and enqueue new ClosedBlock; - // otherwise, leave everything alone. - // otherwise, author a fresh block. + // check to see if last ClosedBlock in would_seals is actually same parent block. + // if so + // duplicate, re-open and push any new transactions. + // if at least one was pushed successfully, close and enqueue new ClosedBlock; + // otherwise, leave everything alone. + // otherwise, author a fresh block. */ - let mut open_block = match sealing_work.pop_if(|b| b.block().fields().header.parent_hash() == &best_hash) { - Some(old_block) => { - trace!(target: "miner", "Already have previous work; updating and returning"); - // add transactions to old_block - let e = self.engine(); - old_block.reopen(e, chain.vm_factory()) - } - None => { - // block not found - create it. - trace!(target: "miner", "No existing work - making new block"); - chain.prepare_open_block( - self.author(), - self.gas_floor_target(), - self.extra_data() - ) - } + let open_block = match sealing_work.pop_if(|b| b.block().fields().header.parent_hash() == &best_hash) { + Some(old_block) => { + trace!(target: "miner", "Already have previous work; updating and returning"); + // add transactions to old_block + let e = self.engine(); + old_block.reopen(e, chain.vm_factory()) + } + None => { + // block not found - create it. + trace!(target: "miner", "No existing work - making new block"); + chain.prepare_open_block( + self.author(), + self.gas_floor_target(), + self.extra_data() + ) + } + }; + (transactions, open_block) }; let mut invalid_transactions = HashSet::new(); @@ -169,8 +172,11 @@ impl Miner { balance: chain.latest_balance(a), }; - for hash in invalid_transactions.into_iter() { - queue.remove_invalid(&hash, &fetch_account); + { + let mut queue = self.transaction_queue.lock().unwrap(); + for hash in invalid_transactions.into_iter() { + queue.remove_invalid(&hash, &fetch_account); + } } if !block.transactions().is_empty() { @@ -196,6 +202,8 @@ impl Miner { trace!(target: "miner", "prepare_sealing: unable to generate seal internally"); } } + + let mut sealing_work = self.sealing_work.lock().unwrap(); if sealing_work.peek_last_ref().map_or(true, |pb| pb.block().fields().header.hash() != block.block().fields().header.hash()) { trace!(target: "miner", "Pushing a new, refreshed or borrowed pending {}...", block.block().fields().header.hash()); sealing_work.push(block); From 840f961dc29d0642d21394576a2e21006c6c8c2e Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 21 Jun 2016 14:34:22 +0200 Subject: [PATCH 3/3] don't bother assigning queue. --- ethcore/src/miner/miner.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index e559306c0..563eb0eed 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -108,8 +108,7 @@ impl Miner { trace!(target: "miner", "prepare_sealing: entering"); let (transactions, mut open_block) = { - let queue = self.transaction_queue.lock().unwrap(); - let transactions = queue.top_transactions(); + let transactions = {self.transaction_queue.lock().unwrap().top_transactions()}; let mut sealing_work = self.sealing_work.lock().unwrap(); let best_hash = chain.best_block_header().sha3(); /*