Skip to content

Commit

Permalink
Merge #2074 #2084
Browse files Browse the repository at this point in the history
2074: fix: orphan block pool deadlock r=doitian,zhangsoledad a=quake

resolved potential deadlock in `OrphanBlockPool`  and a unit test which simulates deadlocking was added also.

2084: feat: Expose methods so we can use CKB as a library r=doitian,yangby-cryptape a=xxuejie



Co-authored-by: quake <[email protected]>
Co-authored-by: Xuejie Xiao <[email protected]>
  • Loading branch information
3 people authored May 18, 2020
3 parents 522d3ba + 9a66805 + d556997 commit 5ae1b8c
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 4 deletions.
4 changes: 4 additions & 0 deletions chain/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ impl ChainService {
}
}

pub fn external_process_block(&mut self, block: Arc<BlockView>) -> Result<bool, Error> {
self.process_block(block, Switch::NONE)
}

// process_block will do block verify
// but invoker should guarantee block header be verified
pub(crate) fn process_block(
Expand Down
36 changes: 33 additions & 3 deletions sync/src/orphan_block_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ impl OrphanBlockPool {
}

pub fn get_block(&self, hash: &packed::Byte32) -> Option<core::BlockView> {
self.parents.write().get(hash).and_then(|parent_hash| {
self.blocks
.write()
// acquire the `blocks` read lock first, guarantee ordering of acquisition is same as `remove_blocks_by_parent`, avoids deadlocking
let guard = self.blocks.read();
self.parents.read().get(hash).and_then(|parent_hash| {
guard
.get(parent_hash)
.and_then(|value| value.get(hash).map(|v| v.1.clone()))
})
Expand All @@ -76,6 +77,8 @@ mod tests {
use faketime::unix_time_as_millis;
use std::collections::HashSet;
use std::iter::FromIterator;
use std::sync::Arc;
use std::thread;

fn gen_block(parent_header: &HeaderView) -> BlockView {
BlockBuilder::default()
Expand Down Expand Up @@ -106,4 +109,31 @@ mod tests {
let block: HashSet<BlockView> = HashSet::from_iter(blocks.into_iter());
assert_eq!(orphan, block)
}

#[test]
fn test_remove_blocks_by_parent_and_get_block_should_not_deadlock() {
let consensus = ConsensusBuilder::default().build();
let pool = OrphanBlockPool::with_capacity(1024);
let mut header = consensus.genesis_block().header();
let mut hashes = Vec::new();
for _ in 1..1024 {
let new_block = gen_block(&header);
pool.insert(PeerIndex::new(0usize), new_block.clone());
header = new_block.header();
hashes.push(header.hash());
}

let pool_arc1 = Arc::new(pool);
let pool_arc2 = Arc::clone(&pool_arc1);

let thread1 = thread::spawn(move || {
pool_arc1.remove_blocks_by_parent(&consensus.genesis_block().hash());
});

for hash in hashes.iter().rev() {
pool_arc2.get_block(hash);
}

thread1.join().unwrap();
}
}
2 changes: 1 addition & 1 deletion util/app-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ impl Setup {
result
}

fn consensus(&self) -> Result<Consensus, ExitCode> {
pub fn consensus(&self) -> Result<Consensus, ExitCode> {
let result = consensus_from_spec(&self.chain_spec()?);

if let Ok(consensus) = &result {
Expand Down

0 comments on commit 5ae1b8c

Please sign in to comment.