Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix lock order when updating sealing #1364

Merged
merged 3 commits into from
Jun 21, 2016
Merged

Fix lock order when updating sealing #1364

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c912bb8
Fix lock order
arkpar Jun 20, 2016
b667378
Release lock while pushing transactions
arkpar Jun 21, 2016
840f961
don't bother assigning queue.
gavofyork Jun 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 35 additions & 28 deletions ethcore/src/miner/miner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,34 +106,37 @@ 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 sealing_work = self.sealing_work.lock().unwrap();
let best_hash = chain.best_block_header().sha3();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually working as expected - the lock should be dropped cause top_transactions is doing a copy.
I didn't wanted to keep transaction queue locked when transactions are executed.

But if you believe it might be dangerous I'm ok with it.

let (transactions, mut open_block) = {
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();
/*
// 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();
Expand Down Expand Up @@ -163,14 +166,16 @@ 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),
};

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() {
Expand All @@ -196,6 +201,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);
Expand Down