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

disputes: fix relay chain selection sanity check #3750

Merged
5 commits merged into from
Sep 1, 2021
Merged

disputes: fix relay chain selection sanity check #3750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77b3527
Fix off-by-one bug in approval voting and adjust tests
Lldenaurois Sep 1, 2021
c31dc3f
Address feedback
Lldenaurois Sep 1, 2021
fd134a1
Remove default disputes
Lldenaurois Sep 1, 2021
f7d891e
Remove spaces
Lldenaurois Sep 1, 2021
e4471f7
cargo +nightly fmt
ordian Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions node/core/approval-voting/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -1321,15 +1330,6 @@ async fn handle_approved_ancestor(
}
}
}
block_descriptions.push(BlockDescription {
drahnr marked this conversation as resolved.
Show resolved Hide resolved
block_hash,
session: entry.session(),
candidates: entry
.candidates()
.iter()
.map(|(_idx, candidate_hash)| *candidate_hash)
.collect(),
});
}

tracing::trace!(
Expand Down
161 changes: 92 additions & 69 deletions node/service/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,6 @@ impl TestChainStorage {
minimum_block_number: BlockNumber,
leaf: Hash,
) -> Option<HighestApprovedAncestorBlock> {
let hash = leaf;
drahnr marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand All @@ -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 {
drahnr marked this conversation as resolved.
Show resolved Hide resolved
hash,
number,
descriptions: descriptions.into_iter().rev().collect(),
Expand All @@ -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
}
drahnr marked this conversation as resolved.
Show resolved Hide resolved
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;
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();

Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

If 0xA3 is not disputed, then it is automatically undisputed_chain, since chain.disputes.is_empty() == true. However, this is contrary to the expectation that 0xA2 is the undisputed_chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before, I'd prefer to keep the test case input the same as before, if there is a need for additional ones, C&P :)

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,
Expand All @@ -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;
Expand All @@ -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);
drahnr marked this conversation as resolved.
Show resolved Hide resolved

builder.set_heads(vec![a3, b3]);

Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down