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

Fixing marking blocks as bad & SyncMessage bugs + small client refactoring. #503

Merged
merged 9 commits into from
Feb 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 20 additions & 20 deletions ethcore/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,20 +144,20 @@ impl IsBlock for ExecutedBlock {

/// Block that is ready for transactions to be added.
///
/// It's a bit like a Vec<Transaction>, eccept that whenever a transaction is pushed, we execute it and
/// It's a bit like a Vec<Transaction>, except that whenever a transaction is pushed, we execute it and
/// maintain the system `state()`. We also archive execution receipts in preparation for later block creation.
pub struct OpenBlock<'x, 'y> {
pub struct OpenBlock<'x> {
block: ExecutedBlock,
engine: &'x Engine,
last_hashes: &'y LastHashes,
last_hashes: LastHashes,
}

/// Just like OpenBlock, except that we've applied `Engine::on_close_block`, finished up the non-seal header fields,
/// and collected the uncles.
///
/// There is no function available to push a transaction. If you want that you'll need to `reopen()` it.
pub struct ClosedBlock<'x, 'y> {
open_block: OpenBlock<'x, 'y>,
pub struct ClosedBlock<'x> {
open_block: OpenBlock<'x>,
uncle_bytes: Bytes,
}

Expand All @@ -169,9 +169,9 @@ pub struct SealedBlock {
uncle_bytes: Bytes,
}

impl<'x, 'y> OpenBlock<'x, 'y> {
impl<'x> OpenBlock<'x> {
/// Create a new OpenBlock ready for transaction pushing.
pub fn new(engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: &'y LastHashes, author: Address, extra_data: Bytes) -> Self {
pub fn new(engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: LastHashes, author: Address, extra_data: Bytes) -> Self {
let mut r = OpenBlock {
block: ExecutedBlock::new(State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce())),
engine: engine,
Expand Down Expand Up @@ -259,7 +259,7 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
}

/// Turn this into a `ClosedBlock`. A BlockChain must be provided in order to figure out the uncles.
pub fn close(self) -> ClosedBlock<'x, 'y> {
pub fn close(self) -> ClosedBlock<'x> {
let mut s = self;
s.engine.on_close_block(&mut s.block);
s.block.base.header.transactions_root = ordered_trie_root(s.block.base.transactions.iter().map(|ref e| e.rlp_bytes().to_vec()).collect());
Expand All @@ -275,16 +275,16 @@ impl<'x, 'y> OpenBlock<'x, 'y> {
}
}

impl<'x, 'y> IsBlock for OpenBlock<'x, 'y> {
impl<'x> IsBlock for OpenBlock<'x> {
fn block(&self) -> &ExecutedBlock { &self.block }
}

impl<'x, 'y> IsBlock for ClosedBlock<'x, 'y> {
impl<'x> IsBlock for ClosedBlock<'x> {
fn block(&self) -> &ExecutedBlock { &self.open_block.block }
}

impl<'x, 'y> ClosedBlock<'x, 'y> {
fn new(open_block: OpenBlock<'x, 'y>, uncle_bytes: Bytes) -> Self {
impl<'x> ClosedBlock<'x> {
fn new(open_block: OpenBlock<'x>, uncle_bytes: Bytes) -> Self {
ClosedBlock {
open_block: open_block,
uncle_bytes: uncle_bytes,
Expand All @@ -307,7 +307,7 @@ impl<'x, 'y> ClosedBlock<'x, 'y> {
}

/// Turn this back into an `OpenBlock`.
pub fn reopen(self) -> OpenBlock<'x, 'y> { self.open_block }
pub fn reopen(self) -> OpenBlock<'x> { self.open_block }

/// Drop this object and return the underlieing database.
pub fn drain(self) -> JournalDB { self.open_block.block.state.drop().1 }
Expand All @@ -332,7 +332,7 @@ impl IsBlock for SealedBlock {
}

/// Enact the block given by block header, transactions and uncles
pub fn enact<'x, 'y>(header: &Header, transactions: &[SignedTransaction], uncles: &[Header], engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: &'y LastHashes) -> Result<ClosedBlock<'x, 'y>, Error> {
pub fn enact<'x>(header: &Header, transactions: &[SignedTransaction], uncles: &[Header], engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: LastHashes) -> Result<ClosedBlock<'x>, Error> {
{
if ::log::max_log_level() >= ::log::LogLevel::Trace {
let s = State::from_existing(db.clone(), parent.state_root().clone(), engine.account_start_nonce());
Expand All @@ -350,20 +350,20 @@ pub fn enact<'x, 'y>(header: &Header, transactions: &[SignedTransaction], uncles
}

/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header
pub fn enact_bytes<'x, 'y>(block_bytes: &[u8], engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: &'y LastHashes) -> Result<ClosedBlock<'x, 'y>, Error> {
pub fn enact_bytes<'x>(block_bytes: &[u8], engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: LastHashes) -> Result<ClosedBlock<'x>, Error> {
let block = BlockView::new(block_bytes);
let header = block.header();
enact(&header, &block.transactions(), &block.uncles(), engine, db, parent, last_hashes)
}

/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header
pub fn enact_verified<'x, 'y>(block: &PreVerifiedBlock, engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: &'y LastHashes) -> Result<ClosedBlock<'x, 'y>, Error> {
pub fn enact_verified<'x>(block: &PreVerifiedBlock, engine: &'x Engine, db: JournalDB, parent: &Header, last_hashes: LastHashes) -> Result<ClosedBlock<'x>, Error> {
let view = BlockView::new(&block.bytes);
enact(&block.header, &block.transactions, &view.uncles(), engine, db, parent, last_hashes)
}

/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header. Seal the block aferwards
pub fn enact_and_seal(block_bytes: &[u8], engine: &Engine, db: JournalDB, parent: &Header, last_hashes: &LastHashes) -> Result<SealedBlock, Error> {
pub fn enact_and_seal(block_bytes: &[u8], engine: &Engine, db: JournalDB, parent: &Header, last_hashes: LastHashes) -> Result<SealedBlock, Error> {
let header = BlockView::new(block_bytes).header_view();
Ok(try!(try!(enact_bytes(block_bytes, engine, db, parent, last_hashes)).seal(header.seal())))
}
Expand All @@ -384,7 +384,7 @@ mod tests {
let mut db = db_result.take();
engine.spec().ensure_db_good(&mut db);
let last_hashes = vec![genesis_header.hash()];
let b = OpenBlock::new(engine.deref(), db, &genesis_header, &last_hashes, Address::zero(), vec![]);
let b = OpenBlock::new(engine.deref(), db, &genesis_header, last_hashes, Address::zero(), vec![]);
let b = b.close();
let _ = b.seal(vec![]);
}
Expand All @@ -398,14 +398,14 @@ mod tests {
let mut db_result = get_temp_journal_db();
let mut db = db_result.take();
engine.spec().ensure_db_good(&mut db);
let b = OpenBlock::new(engine.deref(), db, &genesis_header, &vec![genesis_header.hash()], Address::zero(), vec![]).close().seal(vec![]).unwrap();
let b = OpenBlock::new(engine.deref(), db, &genesis_header, vec![genesis_header.hash()], Address::zero(), vec![]).close().seal(vec![]).unwrap();
let orig_bytes = b.rlp_bytes();
let orig_db = b.drain();

let mut db_result = get_temp_journal_db();
let mut db = db_result.take();
engine.spec().ensure_db_good(&mut db);
let e = enact_and_seal(&orig_bytes, engine.deref(), db, &genesis_header, &vec![genesis_header.hash()]).unwrap();
let e = enact_and_seal(&orig_bytes, engine.deref(), db, &genesis_header, vec![genesis_header.hash()]).unwrap();

assert_eq!(e.rlp_bytes(), orig_bytes);

Expand Down
24 changes: 15 additions & 9 deletions ethcore/src/block_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,29 +285,35 @@ impl BlockQueue {
}

/// Mark given block and all its children as bad. Stops verification.
pub fn mark_as_bad(&mut self, hash: &H256) {
pub fn mark_as_bad(&mut self, block_hashes: &[H256]) {
let mut verification_lock = self.verification.lock().unwrap();
let mut processing = self.processing.write().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

does this introduce a possible deadlock? if so, is it clear and well-documented that this is the canonical order in which to lock these resources?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before verification lock was obtained a the beginning, but processing write-lock was obtained inside the loop - I've just extracted that above the loop, maintaining the order.

Had a quick look on all functions and it's not clear in which order to lock verification and processing - I will look into this.


let mut verification = verification_lock.deref_mut();
verification.bad.insert(hash.clone());
self.processing.write().unwrap().remove(&hash);

verification.bad.reserve(block_hashes.len());
for hash in block_hashes {
verification.bad.insert(hash.clone());
processing.remove(&hash);
}

let mut new_verified = VecDeque::new();
for block in verification.verified.drain(..) {
if verification.bad.contains(&block.header.parent_hash) {
verification.bad.insert(block.header.hash());
self.processing.write().unwrap().remove(&block.header.hash());
}
else {
processing.remove(&block.header.hash());
} else {
new_verified.push_back(block);
}
}
verification.verified = new_verified;
}

/// Mark given block as processed
pub fn mark_as_good(&mut self, hashes: &[H256]) {
pub fn mark_as_good(&mut self, block_hashes: &[H256]) {
let mut processing = self.processing.write().unwrap();
for h in hashes {
processing.remove(&h);
for hash in block_hashes {
processing.remove(&hash);
}
}

Expand Down