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

Fix incomplete sorting. #4795

Merged
merged 8 commits into from
Jan 27, 2022
Merged

Fix incomplete sorting. #4795

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
479deff
Fix incomplete sorting.
eskimor Jan 27, 2022
e1188d2
fmt.
eskimor Jan 27, 2022
53b8280
Better test.
eskimor Jan 27, 2022
1398ae9
Update runtime/parachains/src/disputes.rs
eskimor Jan 27, 2022
5f92eb2
chore fmt
drahnr Jan 27, 2022
5bad807
simplify the sorting for two items
drahnr Jan 27, 2022
8c3d373
add test for assure duplicates are detected
drahnr Jan 27, 2022
6f5cdfa
fixup tests
drahnr Jan 27, 2022
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
265 changes: 209 additions & 56 deletions runtime/parachains/src/disputes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ where
(None, Some(_)) => Ordering::Greater,
(Some(_), None) => Ordering::Less,
// For local disputes, prioritize those that occur at an earlier height.
(Some(a_height), Some(b_height)) => a_height.cmp(&b_height),
(Some(a_height), Some(b_height)) =>
a_height.cmp(&b_height).then_with(|| a.candidate_hash.cmp(&b.candidate_hash)),
// Prioritize earlier remote disputes using session as rough proxy.
(None, None) => {
let session_ord = a.session.cmp(&b.session);
Expand All @@ -146,6 +147,34 @@ where

use super::paras_inherent::IsSortedBy;

/// Returns `true` if duplicate items were found, otherwise `false`.
///
/// `check_equal(a: &T, b: &T)` _must_ return `true`, iff `a` and `b` are equal, otherwise `false.
/// The definition of _equal_ is to be defined by the user.
///
/// Attention: Requires the input `iter` to be sorted, such that _equals_
/// would be adjacent in respect whatever `check_equal` defines as equality!
fn contains_duplicates_in_sorted_iter<
'a,
T: 'a,
I: 'a + IntoIterator<Item = &'a T>,
C: 'static + FnMut(&T, &T) -> bool,
>(
iter: I,
mut check_equal: C,
) -> bool {
let mut iter = iter.into_iter();
if let Some(mut previous) = iter.next() {
while let Some(current) = iter.next() {
if check_equal(previous, current) {
return true
}
previous = current;
Copy link
Member

Choose a reason for hiding this comment

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

Personally preferred the previous functional-style impl with windows(2). But this one is also readable, so no complaints.

Copy link
Contributor

@drahnr drahnr Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I actually prefer this one, it has no unreachable! or implicit unreachable, it's imho cleaner, yet a tad less idiomatic.

}
}
return false
}

/// Hook into disputes handling.
///
/// Allows decoupling parachains handling from disputes so that it can
Expand All @@ -163,7 +192,10 @@ pub trait DisputesHandler<BlockNumber: Ord> {
) {
return Err(())
}
if statement_sets.as_slice().windows(2).any(|sub| sub.get(0) == sub.get(1)) {
// Sorted, so according to session and candidate hash, this will detect duplicates.
if contains_duplicates_in_sorted_iter(statement_sets, |previous, current| {
current.session == previous.session && current.candidate_hash == previous.candidate_hash
}) {
return Err(())
}
Ok(())
Expand Down Expand Up @@ -1360,6 +1392,16 @@ mod tests {
}
}

#[test]
fn test_contains_duplicates_in_sorted_iter() {
// We here use the implicit ascending sorting and builtin equality of integers
let v = vec![1, 2, 3, 5, 5, 8];
assert_eq!(true, contains_duplicates_in_sorted_iter(&v, |a, b| a == b));

let v = vec![1, 2, 3, 4];
assert_eq!(false, contains_duplicates_in_sorted_iter(&v, |a, b| a == b));
}

#[test]
fn test_dispute_state_flag_from_state() {
assert_eq!(
Expand Down Expand Up @@ -2966,24 +3008,20 @@ mod tests {
<Test as frame_system::Config>::BlockNumber,
>>::deduplicate_and_sort_dispute_data(&mut disputes).unwrap_err();

use core::cmp::Ordering;

// assert ordering of local only disputes, and at the same time, and being free of duplicates
assert_eq!(disputes_orig.len(), disputes.len() + 1);

// assert ordering of local only disputes
disputes.windows(2).for_each(|window| {
let a = window[0].clone();
let b = window[1].clone();
let are_these_equal = |a: &DisputeStatementSet, b: &DisputeStatementSet| {
use core::cmp::Ordering;
// we only have local disputes here, so sorting of those adheres to the
// simplified sorting logic
let session_cmp = a.session.cmp(&b.session);
let cmp = if session_cmp == Ordering::Equal {
b.candidate_hash.cmp(&b.candidate_hash)
} else {
session_cmp
};
let cmp =
a.session.cmp(&b.session).then_with(|| a.candidate_hash.cmp(&b.candidate_hash));
assert_ne!(cmp, Ordering::Greater);
});
cmp == Ordering::Equal
};

assert_eq!(false, contains_duplicates_in_sorted_iter(&disputes, are_these_equal));
})
}

Expand Down Expand Up @@ -3490,72 +3528,187 @@ mod tests {
let sig_a = v0.sign(&payload);
let sig_a_against = v1.sign(&payload_against);

let sig_b = v0.sign(&payload);
let sig_b_against = v1.sign(&payload_against);
let statements = vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_a.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_a_against.clone(),
),
];

let mut statements = vec![
let mut sets = vec![
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_a.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_a_against.clone(),
),
],
statements: statements.clone(),
},
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_b.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_b_against.clone(),
),
],
statements: statements.clone(),
},
];

// `Err(())` indicates presence of duplicates
assert!(<Pallet::<Test> as DisputesHandler<
<Test as frame_system::Config>::BlockNumber,
>>::deduplicate_and_sort_dispute_data(&mut statements)
>>::deduplicate_and_sort_dispute_data(&mut sets)
.is_err());

assert_eq!(
statements,
sets,
vec![DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_a.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_a_against.clone(),
),
],
statements,
}]
);
})
}

#[test]
fn assure_no_duplicate_statements_sets_are_fine() {
new_test_ext(Default::default()).execute_with(|| {
let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
let v1 = <ValidatorId as CryptoType>::Pair::generate().0;

run_to_block(3, |b| {
// a new session at each block
Some((
true,
b,
vec![(&0, v0.public()), (&1, v1.public())],
Some(vec![(&0, v0.public()), (&1, v1.public())]),
))
});

let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1));

let payload = ExplicitDisputeStatement {
valid: true,
candidate_hash: candidate_hash_a.clone(),
session: 1,
}
.signing_payload();

let payload_against = ExplicitDisputeStatement {
valid: false,
candidate_hash: candidate_hash_a.clone(),
session: 1,
}
.signing_payload();

let sig_a = v0.sign(&payload);
let sig_a_against = v1.sign(&payload_against);

let statements = vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_a.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_a_against.clone(),
),
];

let sets = vec![
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: statements.clone(),
},
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 2,
statements: statements.clone(),
},
];

// `Err(())` indicates presence of duplicates
assert!(<Pallet::<Test> as DisputesHandler<
<Test as frame_system::Config>::BlockNumber,
>>::assure_deduplicated_and_sorted(&sets)
.is_ok());
})
}

#[test]
fn assure_detects_duplicate_statements_sets() {
new_test_ext(Default::default()).execute_with(|| {
let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
let v1 = <ValidatorId as CryptoType>::Pair::generate().0;

run_to_block(3, |b| {
// a new session at each block
Some((
true,
b,
vec![(&0, v0.public()), (&1, v1.public())],
Some(vec![(&0, v0.public()), (&1, v1.public())]),
))
});

let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1));

let payload = ExplicitDisputeStatement {
valid: true,
candidate_hash: candidate_hash_a.clone(),
session: 1,
}
.signing_payload();

let payload_against = ExplicitDisputeStatement {
valid: false,
candidate_hash: candidate_hash_a.clone(),
session: 1,
}
.signing_payload();

let sig_a = v0.sign(&payload);
let sig_a_against = v1.sign(&payload_against);

let statements = vec![
(
DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
ValidatorIndex(0),
sig_a.clone(),
),
(
DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
ValidatorIndex(1),
sig_a_against.clone(),
),
];

let sets = vec![
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: statements.clone(),
},
DisputeStatementSet {
candidate_hash: candidate_hash_a.clone(),
session: 1,
statements: statements.clone(),
},
];

// `Err(())` indicates presence of duplicates
assert!(<Pallet::<Test> as DisputesHandler<
<Test as frame_system::Config>::BlockNumber,
>>::assure_deduplicated_and_sorted(&sets)
.is_err());
})
}

#[test]
fn filter_ignores_single_sided() {
new_test_ext(Default::default()).execute_with(|| {
Expand Down