Unverified Commit 4764ec7c authored by Bernhard Schuster's avatar Bernhard Schuster Committed by GitHub
Browse files

disputes: fix relay chain selection sanity check (#3750)



* Fix off-by-one bug in approval voting and adjust tests

* Address feedback

* Remove default disputes

* Remove spaces

* cargo +nightly fmt
Co-authored-by: Lldenaurois's avatarLldenaurois <Ljdenaurois@gmail.com>
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>
parent d275db9a
Pipeline #155107 passed with stages
in 41 minutes and 52 seconds
......@@ -1239,6 +1239,15 @@ async fn handle_approved_ancestor(
// ancestry is moving backwards.
all_approved_max = Some((block_hash, target_number - i as BlockNumber));
}
block_descriptions.push(BlockDescription {
block_hash,
session: entry.session(),
candidates: entry
.candidates()
.iter()
.map(|(_idx, candidate_hash)| *candidate_hash)
.collect(),
});
} else if bits.len() <= ABNORMAL_DEPTH_THRESHOLD {
all_approved_max = None;
block_descriptions.clear();
......@@ -1321,15 +1330,6 @@ async fn handle_approved_ancestor(
}
}
}
block_descriptions.push(BlockDescription {
block_hash,
session: entry.session(),
candidates: entry
.candidates()
.iter()
.map(|(_idx, candidate_hash)| *candidate_hash)
.collect(),
});
}
tracing::trace!(
......
......@@ -157,9 +157,6 @@ impl TestChainStorage {
minimum_block_number: BlockNumber,
leaf: Hash,
) -> Option<HighestApprovedAncestorBlock> {
let hash = leaf;
let number = self.blocks_by_hash.get(&leaf)?.number;
let mut descriptions = Vec::new();
let mut block_hash = leaf;
let mut highest_approved_ancestor = None;
......@@ -168,31 +165,23 @@ impl TestChainStorage {
if minimum_block_number >= block.number {
break
}
descriptions.push(BlockDescription {
session: 1 as _, // dummy, not checked
block_hash,
candidates: vec![], // not relevant for any test cases
});
if !self.approved_blocks.contains(&block_hash) {
highest_approved_ancestor = None;
descriptions.clear();
} else if highest_approved_ancestor.is_none() {
descriptions.clear();
highest_approved_ancestor = Some(block_hash);
}
let next = block.parent_hash();
if &leaf != next {
block_hash = *next;
} else {
break
if highest_approved_ancestor.is_none() {
highest_approved_ancestor = Some((block_hash, block.number));
}
descriptions.push(BlockDescription {
session: 1 as _, // dummy, not checked
block_hash,
candidates: vec![], // not relevant for any test cases
});
}
block_hash = *block.parent_hash();
}
if highest_approved_ancestor.is_none() {
return None
}
Some(HighestApprovedAncestorBlock {
highest_approved_ancestor.map(|(hash, number)| HighestApprovedAncestorBlock {
hash,
number,
descriptions: descriptions.into_iter().rev().collect(),
......@@ -212,16 +201,12 @@ impl TestChainStorage {
let mut undisputed_chain = Some(highest_approved_block_hash);
let mut block_hash = highest_approved_block_hash;
while let Some(block) = self.blocks_by_hash.get(&block_hash) {
block_hash = block.hash();
if ChainBuilder::GENESIS_HASH == block_hash {
return None
}
let next = block.parent_hash();
if self.disputed_blocks.contains(&block_hash) {
undisputed_chain = Some(*next);
}
if block.number() == &base_blocknumber {
return None
break
}
block_hash = *next;
}
......@@ -434,36 +419,38 @@ fn run_specialized_test_w_harness<F: FnOnce() -> CaseVars>(case_var_provider: F)
// Verify test integrity: the provided highest approved
// ancestor must match the chain derived one.
let highest_approved_ancestor_w_desc =
best_chain_containing_block.and_then(|best_chain_containing_block| {
let min_blocknumber = chain
.blocks_by_hash
.get(&best_chain_containing_block)
.map(|x| x.number)
.unwrap_or_default();
let highest_approved_ancestor_w_desc =
chain.highest_approved_ancestors(min_blocknumber, best_chain_containing_block);
if let (
Some(highest_approved_ancestor_w_desc),
Some(highest_approved_ancestor_block),
) = (&highest_approved_ancestor_w_desc, highest_approved_ancestor_block)
{
assert_eq!(
highest_approved_ancestor_block, highest_approved_ancestor_w_desc.hash,
"TestCaseIntegrity: Provided and expected approved ancestor hash mismatch: {:?} vs {:?}",
highest_approved_ancestor_block, highest_approved_ancestor_w_desc.hash,
);
}
highest_approved_ancestor_w_desc
});
if let Some(haacwd) = &highest_approved_ancestor_w_desc {
let expected = chain.undisputed_chain(haacwd.number, haacwd.hash);
assert_eq!(
expected, undisputed_chain,
"TestCaseIntegrity: Provided and anticipated undisputed chain mismatch: {:?} vs {:?}",
undisputed_chain, expected,
)
}
let highest_approved_ancestor_w_desc = best_chain_containing_block
.and_then(|best_chain_containing_block| {
chain.blocks_by_hash.get(&target_block).map(|target_block_header| {
let target_blocknumber = target_block_header.number;
let highest_approved_ancestor_w_desc = chain.highest_approved_ancestors(
target_blocknumber,
best_chain_containing_block,
);
if let (
Some(highest_approved_ancestor_w_desc),
Some(highest_approved_ancestor_block),
) = (&highest_approved_ancestor_w_desc, highest_approved_ancestor_block)
{
assert_eq!(
highest_approved_ancestor_block, highest_approved_ancestor_w_desc.hash,
"TestCaseIntegrity: Provided and expected approved ancestor hash mismatch: {:?} vs {:?}",
highest_approved_ancestor_block, highest_approved_ancestor_w_desc.hash,
);
let expected = chain
.undisputed_chain(target_blocknumber, highest_approved_ancestor_block);
assert_eq!(
expected, undisputed_chain,
"TestCaseIntegrity: Provided and anticipated undisputed chain mismatch: {:?} vs {:?}",
undisputed_chain, expected,
);
}
highest_approved_ancestor_w_desc
})
})
.flatten();
test_skeleton(
&chain,
......@@ -512,10 +499,10 @@ struct CaseVars {
/// ```raw
/// genesis -- 0xA1 --- 0xA2 --- 0xA3 --- 0xA4(!avail) --- 0xA5(!avail)
/// \
/// `- 0xB2
/// \
/// `- 0xB2
/// ```
fn chain_0() -> CaseVars {
fn chain_undisputed() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
let mut builder = ChainBuilder::new();
......@@ -531,6 +518,37 @@ fn chain_0() -> CaseVars {
builder.set_heads(vec![a5, b3]);
CaseVars {
chain: builder.init(),
target_block: a1,
best_chain_containing_block: Some(a5),
highest_approved_ancestor_block: Some(a3),
undisputed_chain: Some(a3),
expected_finality_target_result: Some(a3),
}
}
/// ```raw
/// genesis -- 0xA1 --- 0xA2 --- 0xA3(disputed) --- 0xA4(!avail) --- 0xA5(!avail)
/// \
/// `- 0xB2
/// ```
fn chain_0() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
let mut builder = ChainBuilder::new();
let a1 = builder.fast_forward_approved(0xA0, head, 1);
let a2 = builder.fast_forward_approved(0xA0, a1, 2);
let a3 = builder.fast_forward_disputed(0xA0, a2, 3);
let a4 = builder.fast_forward(0xA0, a3, 4);
let a5 = builder.fast_forward(0xA0, a4, 5);
let b1 = builder.fast_forward_approved(0xB0, a1, 2);
let b2 = builder.fast_forward_approved(0xB0, b1, 3);
let b3 = builder.fast_forward_approved(0xB0, b2, 4);
builder.set_heads(vec![a5, b3]);
CaseVars {
chain: builder.init(),
target_block: a1,
......@@ -543,8 +561,8 @@ fn chain_0() -> CaseVars {
/// ```raw
/// genesis -- 0xA1 --- 0xA2(disputed) --- 0xA3
/// \
/// `- 0xB2 --- 0xB3(!available)
/// \
/// `- 0xB2 --- 0xB3(!available)
/// ```
fn chain_1() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
......@@ -555,7 +573,7 @@ fn chain_1() -> CaseVars {
let a3 = builder.fast_forward_approved(0xA0, a2, 3);
let b2 = builder.fast_forward_approved(0xB0, a1, 2);
let b3 = builder.fast_forward_approved(0xB0, b2, 3);
let b3 = builder.fast_forward(0xB0, b2, 3);
builder.set_heads(vec![a3, b3]);
......@@ -571,8 +589,8 @@ fn chain_1() -> CaseVars {
/// ```raw
/// genesis -- 0xA1 --- 0xA2(disputed) --- 0xA3
/// \
/// `- 0xB2 --- 0xB3
/// \
/// `- 0xB2 --- 0xB3
/// ```
fn chain_2() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
......@@ -599,8 +617,8 @@ fn chain_2() -> CaseVars {
/// ```raw
/// genesis -- 0xA1 --- 0xA2 --- 0xA3(disputed)
/// \
/// `- 0xB2 --- 0xB3
/// \
/// `- 0xB2 --- 0xB3
/// ```
fn chain_3() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
......@@ -627,10 +645,10 @@ fn chain_3() -> CaseVars {
/// ```raw
/// genesis -- 0xA1 --- 0xA2 --- 0xA3(disputed)
/// \
/// `- 0xB2 --- 0xB3
/// \
/// `- 0xB2 --- 0xB3
///
/// ? --- NEX(does_not_exist)
/// ? --- NEX(does_not_exist)
/// ```
fn chain_4() -> CaseVars {
let head: Hash = ChainBuilder::GENESIS_HASH;
......@@ -715,6 +733,11 @@ fn chain_6() -> CaseVars {
}
}
#[test]
fn chain_sel_undisputed() {
run_specialized_test_w_harness(chain_undisputed);
}
#[test]
fn chain_sel_0() {
run_specialized_test_w_harness(chain_0);
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment