Skip to content

Commit

Permalink
fix: ensure that accumulated orphan chain data is committed before he…
Browse files Browse the repository at this point in the history
…ader validation (#3462)

Description
---
- commit orphan chain accumulated data before orphan header validation so that
  accumulated target difficulty is available for chain comparison
- use hash for reorg chain rewind because the height value of the candidate block is not yet 
   checked against the fork height.  
- improve test infra to allow blockchain tests to be easily be constructed
- add a failing test for the fix in the PR 

Motivation and Context
---
Observed errors:
```
2021-10-10 09:17:19.987800805 [c::cs::database] WARN  Discarding orphan 0291ba64d777e46016ca7b055bdf6979c1fe11bf31b78a7c20d507cb69c3f293 because it has an invalid header: FatalStorageError("The requested chain_header_in_all_chains was not found via hash:9c2c5734a783d891b617905e27e861ac1595760d5c22335c8d31feb7dc38a2a5 in the database")
```

The above error occurs because of orphan accumulated data was set in the `DbTransaction` but is not 
actually committed to lmdb. This manifests as a validation error and was not picked up by other tests because
the validator was mocked out. 

The solution in this PR is to write the transactions as needed. This means that in a rollback situation, the 
accumulated orphan chain data will remain. That _should_ be ok since that data can be overwritten/replaced
on the next reorg evaluation if needed.
 
This demonstrates the shortcomings of the current approach and the need for the calling code in 
`BlockchainDatabase<B>` to have access to ACID DB transactions.
I have played with the idea of abstracting and generifying atomic transactions but found the need for [GAT](https://blog.rust-lang.org/2021/08/03/GATs-stabilization-push.html) to get it to work. We no longer
have or use any other BlockchainBackend impl other than LMDB so leaking LMDB `Read,WriteTransaction`
may be acceptable. Passing the transaction in to every function in BlockchainBackend may be a deal breaker.

How Has This Been Tested?
---
- Issue reproduced in `handle_possible_reorg::it_links_many_orphan_branches_to_main_chain`
- Manually (not necessarily tested, the base node runs for a day and is still at the tip)

Consolidated some of the existing "blockchain builder" test infra that was duplicated and added
a declarative macro for building chains `let specs = block_spec!(["A->GB"], ["B->A", difficulty: 100]);`
  • Loading branch information
sdbondi committed Oct 18, 2021
1 parent 53989f4 commit 80f7c78
Show file tree
Hide file tree
Showing 16 changed files with 642 additions and 269 deletions.
Expand Up @@ -604,10 +604,10 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {
.sync_validators
.final_horizon_state
.validate(
&*self.db().clone().into_inner().db_read_access()?,
header.height(),
&pruned_utxo_sum,
&pruned_kernel_sum,
&*self.db().clone().into_inner().db_read_access()?,
)
.map_err(HorizonSyncError::FinalStateValidationFailed)?;

Expand Down
2 changes: 1 addition & 1 deletion base_layer/core/src/chain_storage/accumulated_data.rs
Expand Up @@ -273,7 +273,7 @@ impl BlockHeaderAccumulatedDataBuilder<'_> {
.hash
.ok_or_else(|| ChainStorageError::InvalidOperation("hash not provided".to_string()))?;

if hash == self.previous_accum.hash {
if hash == previous_accum.hash {
return Err(ChainStorageError::InvalidOperation(
"Hash was set to the same hash that is contained in previous accumulated data".to_string(),
));
Expand Down
13 changes: 9 additions & 4 deletions base_layer/core/src/chain_storage/block_add_result.rs
Expand Up @@ -46,6 +46,14 @@ impl BlockAddResult {
matches!(self, BlockAddResult::Ok(_))
}

pub fn is_chain_reorg(&self) -> bool {
matches!(self, BlockAddResult::ChainReorg { .. })
}

pub fn is_orphaned(&self) -> bool {
matches!(self, BlockAddResult::OrphanBlock)
}

pub fn assert_added(&self) -> ChainBlock {
match self {
BlockAddResult::ChainReorg { added, removed } => panic!(
Expand All @@ -60,10 +68,7 @@ impl BlockAddResult {
}

pub fn assert_orphaned(&self) {
match self {
BlockAddResult::OrphanBlock => (),
_ => panic!("Result was not orphaned"),
}
assert!(self.is_orphaned(), "Result was not orphaned");
}

pub fn assert_reorg(&self, num_added: usize, num_removed: usize) {
Expand Down

0 comments on commit 80f7c78

Please sign in to comment.