From 1c041f6b9fd291fb8186620f07b5a2f871c1c1ea Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 27 Jan 2022 13:44:11 +0100 Subject: [PATCH 1/5] Fix incomplete sorting. --- runtime/parachains/src/disputes.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 7b2d4054e6ba..34d17cf0ea98 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -130,7 +130,14 @@ 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)) => { + let height_ord = a_height.cmp(&b_height); + if height_ord == Ordering::Equal { + a.candidate_hash.cmp(&b.candidate_hash) + } else { + height_ord + } + }, // Prioritize earlier remote disputes using session as rough proxy. (None, None) => { let session_ord = a.session.cmp(&b.session); @@ -163,7 +170,15 @@ pub trait DisputesHandler { ) { return Err(()) } - if statement_sets.as_slice().windows(2).any(|sub| sub.get(0) == sub.get(1)) { + let compare_statement_sets_window_same_dispute = |sub: &[DisputeStatementSet]| { + match (sub.get(0), sub.get(1)) { + // should not be possible: + (None, None) | (None, Some(_)) | (Some(_), None) => false, + (Some(set1), Some(set2)) => + set1.session == set2.session && set1.candidate_hash == set2.candidate_hash + } + }; + if statement_sets.as_slice().windows(2).any(compare_statement_sets_window_same_dispute) { return Err(()) } Ok(()) From 6813383d58bd81d1351c73330828453816d488d2 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 27 Jan 2022 13:48:46 +0100 Subject: [PATCH 2/5] fmt. --- runtime/parachains/src/disputes.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 34d17cf0ea98..8a5979939c2b 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -170,15 +170,19 @@ pub trait DisputesHandler { ) { return Err(()) } - let compare_statement_sets_window_same_dispute = |sub: &[DisputeStatementSet]| { - match (sub.get(0), sub.get(1)) { - // should not be possible: - (None, None) | (None, Some(_)) | (Some(_), None) => false, - (Some(set1), Some(set2)) => - set1.session == set2.session && set1.candidate_hash == set2.candidate_hash - } - }; - if statement_sets.as_slice().windows(2).any(compare_statement_sets_window_same_dispute) { + let compare_statement_sets_window_same_dispute = |sub: &[DisputeStatementSet]| { + match (sub.get(0), sub.get(1)) { + // should not be possible: + (None, None) | (None, Some(_)) | (Some(_), None) => false, + (Some(set1), Some(set2)) => + set1.session == set2.session && set1.candidate_hash == set2.candidate_hash, + } + }; + if statement_sets + .as_slice() + .windows(2) + .any(compare_statement_sets_window_same_dispute) + { return Err(()) } Ok(()) From 429c6e7c7645f867ed120f25c8201f191b41387a Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 27 Jan 2022 14:37:36 +0100 Subject: [PATCH 3/5] Better test. --- runtime/parachains/src/disputes.rs | 59 +++++++++--------------------- 1 file changed, 18 insertions(+), 41 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 8a5979939c2b..43360a5afc5f 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -3509,67 +3509,44 @@ 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!( as DisputesHandler< ::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, }] ); }) From 931f57d5bb5d8414abe1e30f932f77a1fea27ae4 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 27 Jan 2022 15:03:59 +0100 Subject: [PATCH 4/5] Update runtime/parachains/src/disputes.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- runtime/parachains/src/disputes.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 43360a5afc5f..748a6dab89b1 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -131,12 +131,7 @@ where (Some(_), None) => Ordering::Less, // For local disputes, prioritize those that occur at an earlier height. (Some(a_height), Some(b_height)) => { - let height_ord = a_height.cmp(&b_height); - if height_ord == Ordering::Equal { - a.candidate_hash.cmp(&b.candidate_hash) - } else { - height_ord - } + 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) => { From 990b8b4e9d32ee376f5ba82d87b51a0ed45e93c5 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 27 Jan 2022 16:58:11 +0100 Subject: [PATCH 5/5] cargo fmt --- runtime/parachains/src/disputes.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/runtime/parachains/src/disputes.rs b/runtime/parachains/src/disputes.rs index 748a6dab89b1..65b97c3b1d83 100644 --- a/runtime/parachains/src/disputes.rs +++ b/runtime/parachains/src/disputes.rs @@ -130,9 +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).then_with(|| a.candidate_hash.cmp(&b.candidate_hash)) - }, + (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);