Skip to content

Commit

Permalink
fix: fix potential race condition between add_block and sync (#4677)
Browse files Browse the repository at this point in the history
Description
---
This fixes a potential race condition. 

It is possible for `add_block` to pass the `is_add_block_disabled()`, start doing orphan validation (which can take quite long). 
While this is happening, `sync` sets the `add_block_disabled` flag and acquires a read_lock to then do pre-processing to determine sync mode, etc. While this is busy, `add_block` asks for a write_lock. 
`Add_block` gets its write_lock before `sync` gets its write_block because of the prioritization of RWLock. 

Also moved the `if db.contains(&DbKey::BlockHash(block_hash))` before the orphan validation as this is a much cheaper operation.
  • Loading branch information
SWvheerden authored Sep 14, 2022
1 parent 996a88a commit 55f2b9c
Showing 1 changed file with 20 additions and 15 deletions.
35 changes: 20 additions & 15 deletions base_layer/core/src/chain_storage/blockchain_database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -875,17 +875,11 @@ where B: BlockchainBackend
}

let new_height = block.header.height;
// Perform orphan block validation.
if let Err(e) = self.validators.orphan.validate(&block) {
warn!(
target: LOG_TARGET,
"Block #{} ({}) failed validation - {}",
&new_height,
block.hash().to_hex(),
e.to_string()
);
return Err(e.into());
}
// This is important, we ask for a write lock to disable all read access to the db. The sync process sets the
// add_block disable flag, but we can have a race condition between the two especially since the orphan
// validation can take some time during big blocks as it does Rangeproof and metadata signature validation.
// Because the sync process first acquires a read_lock then a write_lock, and the RWLock will be prioritised,
// the add_block write lock will be given out before the sync write_lock.
trace!(
target: LOG_TARGET,
"[add_block] waiting for write access to add block block #{}",
Expand All @@ -900,6 +894,21 @@ where B: BlockchainBackend
new_height,
timer.elapsed()
);
let block_hash = block.hash();
if db.contains(&DbKey::BlockHash(block_hash))? {
return Ok(BlockAddResult::BlockExists);
}
// Perform orphan block validation.
if let Err(e) = self.validators.orphan.validate(&block) {
warn!(
target: LOG_TARGET,
"Block #{} ({}) failed validation - {}",
&new_height,
block.hash().to_hex(),
e.to_string()
);
return Err(e.into());
}
let block_add_result = add_block(
&mut *db,
&self.config,
Expand Down Expand Up @@ -1390,10 +1399,6 @@ fn add_block<T: BlockchainBackend>(
difficulty_calculator: &DifficultyCalculator,
block: Arc<Block>,
) -> Result<BlockAddResult, ChainStorageError> {
let block_hash = block.hash();
if db.contains(&DbKey::BlockHash(block_hash))? {
return Ok(BlockAddResult::BlockExists);
}
handle_possible_reorg(
db,
config,
Expand Down

0 comments on commit 55f2b9c

Please sign in to comment.