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

Commit

Permalink
Fix incomplete sorting. (#4795)
Browse files Browse the repository at this point in the history 
* Fix incomplete sorting.

* fmt.

* Better test.

* Update runtime/parachains/src/disputes.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* chore fmt

* simplify the sorting for two items

* add test for assure duplicates are detected

* fixup tests

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
  • Loading branch information
@eskimor @bkchr @drahnr
3 people authored Jan 27, 2022
1 parent f8afb49 commit f0041c4
Showing 1 changed file with 209 additions and 56 deletions.
265 changes: 209 additions & 56 deletions runtime/parachains/src/disputes.rs
View file
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;
}
}
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

0 comments on commit f0041c4

Please sign in to comment.