From fbfbbed0770814866986be793103028555d50288 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 6 Sep 2021 13:55:13 -0500 Subject: [PATCH 1/5] participate in disputes only if haven't voted already --- node/core/dispute-coordinator/src/real/mod.rs | 41 +++++++++++++++---- .../dispute-coordinator/src/real/tests.rs | 3 ++ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/mod.rs b/node/core/dispute-coordinator/src/real/mod.rs index 53f71697798c..9fd38d59330b 100644 --- a/node/core/dispute-coordinator/src/real/mod.rs +++ b/node/core/dispute-coordinator/src/real/mod.rs @@ -48,7 +48,7 @@ use polkadot_node_subsystem_util::rolling_session_window::{ }; use polkadot_primitives::v1::{ BlockNumber, CandidateHash, CandidateReceipt, DisputeStatement, Hash, SessionIndex, - SessionInfo, ValidatorIndex, ValidatorPair, ValidatorSignature, + SessionInfo, ValidatorIndex, ValidatorPair, ValidatorSignature, ValidatorId, }; use futures::{channel::oneshot, prelude::*}; @@ -745,8 +745,17 @@ async fn handle_import_statements( if status != prev_status { // This branch is only hit when the candidate is freshly disputed - // status was previously `None`, and now is not. - if prev_status.is_none() { - // No matter what, if the dispute is new, we participate. + if prev_status.is_none() && { + let controlled_indices = find_controlled_validator_indices( + &state.keystore, + &validators, + ); + let voted_indices = votes.voted_indices(); + + !controlled_indices.iter().all(|val_index| voted_indices.contains(&val_index)) + } { + // If the dispute is new, we participate UNLESS all our controlled + // keys have already participated. // // We also block the coordinator while awaiting our determination // of whether the vote is available. @@ -792,6 +801,22 @@ async fn handle_import_statements( Ok(()) } +fn find_controlled_validator_indices( + keystore: &LocalKeystore, + validators: &[ValidatorId], +) -> HashSet { + let mut controlled = HashSet::new(); + for (index, validator) in validators.iter().enumerate() { + if keystore.key_pair::(validator).ok().flatten().is_none() { + continue + } + + controlled.insert(ValidatorIndex(index as _)); + } + + controlled +} + async fn issue_local_statement( ctx: &mut impl SubsystemContext, overlay_db: &mut OverlayedBackend<'_, impl Backend>, @@ -833,14 +858,12 @@ async fn issue_local_statement( let mut statements = Vec::new(); let voted_indices: HashSet<_> = voted_indices.into_iter().collect(); - for (index, validator) in validators.iter().enumerate() { - let index = ValidatorIndex(index as _); + let controlled_indices = find_controlled_validator_indices(&state.keystore, &validators[..]); + + for index in controlled_indices { if voted_indices.contains(&index) { continue } - if state.keystore.key_pair::(validator).ok().flatten().is_none() { - continue - } let keystore = state.keystore.clone() as Arc<_>; let res = SignedDisputeStatement::sign_explicit( @@ -848,7 +871,7 @@ async fn issue_local_statement( valid, candidate_hash, session, - validator.clone(), + validators[index.0 as usize].clone(), ) .await; diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 60d7ade2fd79..e6730422ff8f 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1523,3 +1523,6 @@ fn resume_dispute_with_local_statement_without_local_key() { }) }); } + + +// TODO [now]: issue_local_statements does not cause duplicate participation. From 174476630cdfd1920c61c97de34185225cf3ba4c Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 7 Sep 2021 17:13:30 +0200 Subject: [PATCH 2/5] Add tests for dispute participation on local import. --- Cargo.lock | 1 + node/core/dispute-coordinator/Cargo.toml | 1 + .../dispute-coordinator/src/real/tests.rs | 120 +++++++++++++++++- 3 files changed, 121 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a9e18164d46a..17d9e54dd643 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6139,6 +6139,7 @@ dependencies = [ "sp-core", "sp-keyring", "sp-keystore", + "sp-tracing", "thiserror", "tracing", ] diff --git a/node/core/dispute-coordinator/Cargo.toml b/node/core/dispute-coordinator/Cargo.toml index 5c1284a5f1da..9b4566b7d5f3 100644 --- a/node/core/dispute-coordinator/Cargo.toml +++ b/node/core/dispute-coordinator/Cargo.toml @@ -26,6 +26,7 @@ polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" } +sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" } assert_matches = "1.4.0" [features] diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index e6730422ff8f..335c84c2e795 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1525,4 +1525,122 @@ fn resume_dispute_with_local_statement_without_local_key() { } -// TODO [now]: issue_local_statements does not cause duplicate participation. +#[test] +fn issue_valid_local_statement_does_cause_distribution_but_not_duplicate_participation() { + issue_local_statement_does_cause_distribution_but_not_duplicate_participation(true); +} + +#[test] +fn issue_invalid_local_statement_does_cause_distribution_but_not_duplicate_participation() { + issue_local_statement_does_cause_distribution_but_not_duplicate_participation(false); +} + +fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation(validity: bool) { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt = CandidateReceipt::default(); + let candidate_hash = candidate_receipt.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + + let other_vote = + test_state.issue_statement_with_index(1, candidate_hash, session, !validity).await; + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (other_vote, ValidatorIndex(1)), + ], + pending_confirmation, + }, + }) + .await; + + assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + // Initiate dispute locally: + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::IssueLocalStatement( + session, + candidate_hash, + candidate_receipt.clone(), + validity, + ), + }) + .await; + + // Dispute distribution should get notified now: + assert_matches!( + virtual_overseer.recv().await, + AllMessages::DisputeDistribution( + DisputeDistributionMessage::SendDispute(msg) + ) => { + assert_eq!(msg.session_index(), session); + assert_eq!(msg.candidate_receipt(), &candidate_receipt); + } + ); + + // Make sure we don't get a `DisputeParticiationMessage`. + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + +#[test] +fn negative_issue_local_statement_only_triggers_import() { + test_harness(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + let session = 1; + + test_state.handle_resume_sync(&mut virtual_overseer, session).await; + + let candidate_receipt = CandidateReceipt::default(); + let candidate_hash = candidate_receipt.hash(); + + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + tracing::debug!("AFTER activbate_leaf"); + + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::IssueLocalStatement( + session, + candidate_hash, + candidate_receipt.clone(), + false, + ), + }) + .await; + tracing::debug!("AFTER Issue local statement"); + + let backend = DbBackend::new(test_state.db.clone(), test_state.config.column_config()); + + let votes = backend.load_candidate_votes(session, &candidate_hash).unwrap().unwrap(); + assert_eq!(votes.invalid.len(), 1); + assert_eq!(votes.valid.len(), 0); + + let disputes = backend.load_recent_disputes().unwrap(); + tracing::debug!("AFTER load recent disputes"); + assert_eq!(disputes, None); + + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + // Make sure we don't get a `DisputeParticiationMessage`. + assert!(virtual_overseer.try_recv().await.is_none()); + tracing::debug!("AFTER try_recv"); + + test_state + }) + }); +} From 09696bace9c710a636a14ea893db4bbe0f479812 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 7 Sep 2021 21:14:48 +0200 Subject: [PATCH 3/5] Fixes. --- .../dispute-coordinator/src/real/tests.rs | 111 ++++++------------ 1 file changed, 39 insertions(+), 72 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 335c84c2e795..9db820036b8b 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -107,6 +107,7 @@ impl Default for TestState { Sr25519Keyring::Dave, Sr25519Keyring::Eve, Sr25519Keyring::One, + Sr25519Keyring::Ferdie, ]; let validator_public = validators.iter().map(|k| ValidatorId::from(k.public())).collect(); @@ -114,7 +115,7 @@ impl Default for TestState { let validator_groups = vec![ vec![ValidatorIndex(0), ValidatorIndex(1)], vec![ValidatorIndex(2), ValidatorIndex(3)], - vec![ValidatorIndex(4), ValidatorIndex(5)], + vec![ValidatorIndex(4), ValidatorIndex(5), ValidatorIndex(6)], ]; let master_keystore = make_keystore(&validators).into(); @@ -316,7 +317,7 @@ fn conflicting_votes_lead_to_dispute_participation() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(3, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -332,7 +333,7 @@ fn conflicting_votes_lead_to_dispute_participation() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(3)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -434,7 +435,7 @@ fn positive_votes_dont_trigger_participation() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let valid_vote_2 = test_state.issue_statement_with_index(1, candidate_hash, session, true).await; @@ -446,7 +447,7 @@ fn positive_votes_dont_trigger_participation() { candidate_hash, candidate_receipt: candidate_receipt.clone(), session, - statements: vec![(valid_vote, ValidatorIndex(0))], + statements: vec![(valid_vote, ValidatorIndex(2))], pending_confirmation, }, }) @@ -539,7 +540,7 @@ fn wrong_validator_index_is_ignored() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -553,7 +554,7 @@ fn wrong_validator_index_is_ignored() { session, statements: vec![ (valid_vote, ValidatorIndex(1)), - (invalid_vote, ValidatorIndex(0)), + (invalid_vote, ValidatorIndex(2)), ], pending_confirmation, }, @@ -609,7 +610,7 @@ fn finality_votes_ignore_disputed_candidates() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -622,7 +623,7 @@ fn finality_votes_ignore_disputed_candidates() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(2)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -715,7 +716,7 @@ fn supermajority_valid_dispute_may_be_finalized() { polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -728,7 +729,7 @@ fn supermajority_valid_dispute_may_be_finalized() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(2)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -755,7 +756,7 @@ fn supermajority_valid_dispute_may_be_finalized() { ); let mut statements = Vec::new(); - for i in (0..supermajority_threshold - 1).map(|i| i + 2) { + for i in (0..supermajority_threshold - 1).map(|i| i + 3) { let vote = test_state.issue_statement_with_index(i, candidate_hash, session, true).await; @@ -848,7 +849,7 @@ fn concluded_supermajority_for_non_active_after_time() { polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -861,7 +862,7 @@ fn concluded_supermajority_for_non_active_after_time() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(2)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -882,7 +883,7 @@ fn concluded_supermajority_for_non_active_after_time() { ); let mut statements = Vec::new(); - for i in (0..supermajority_threshold - 1).map(|i| i + 2) { + for i in (0..supermajority_threshold - 1).map(|i| i + 3) { let vote = test_state.issue_statement_with_index(i, candidate_hash, session, true).await; @@ -951,7 +952,7 @@ fn concluded_supermajority_against_non_active_after_time() { polkadot_primitives::v1::supermajority_threshold(test_state.validators.len()); let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -964,7 +965,7 @@ fn concluded_supermajority_against_non_active_after_time() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(2)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -985,7 +986,7 @@ fn concluded_supermajority_against_non_active_after_time() { ); let mut statements = Vec::new(); - for i in (0..supermajority_threshold - 1).map(|i| i + 2) { + for i in (0..supermajority_threshold - 1).map(|i| i + 3) { let vote = test_state.issue_statement_with_index(i, candidate_hash, session, false).await; @@ -1051,7 +1052,7 @@ fn fresh_dispute_ignored_if_unavailable() { test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; let valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + test_state.issue_statement_with_index(2, candidate_hash, session, true).await; let invalid_vote = test_state.issue_statement_with_index(1, candidate_hash, session, false).await; @@ -1064,7 +1065,7 @@ fn fresh_dispute_ignored_if_unavailable() { candidate_receipt: candidate_receipt.clone(), session, statements: vec![ - (valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(2)), (invalid_vote, ValidatorIndex(1)), ], pending_confirmation, @@ -1298,18 +1299,6 @@ fn resume_dispute_with_local_statement() { }) .await; - assert_matches!( - virtual_overseer.recv().await, - AllMessages::DisputeParticipation( - DisputeParticipationMessage::Participate { - report_availability, - .. - } - ) => { - report_availability.send(true).unwrap(); - } - ); - assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); { @@ -1330,7 +1319,7 @@ fn resume_dispute_with_local_statement() { test_state }) }) - // Alice should send a DisputeParticiationMessage::Participate on restart since she has no + // Alice should not send a DisputeParticiationMessage::Participate on restart since she has a // local statement for the active dispute. .resume(|test_state, mut virtual_overseer| { Box::pin(async move { @@ -1384,18 +1373,6 @@ fn resume_dispute_without_local_statement_or_local_key() { }) .await; - assert_matches!( - virtual_overseer.recv().await, - AllMessages::DisputeParticipation( - DisputeParticipationMessage::Participate { - report_availability, - .. - } - ) => { - report_availability.send(true).unwrap(); - } - ); - assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); { @@ -1416,8 +1393,8 @@ fn resume_dispute_without_local_statement_or_local_key() { test_state }) }) - // Alice should send a DisputeParticiationMessage::Participate on restart since she has no - // local statement for the active dispute. + // Two should not send a DisputeParticiationMessage::Participate on restart since she is no + // validator in that dispute. .resume(|test_state, mut virtual_overseer| { Box::pin(async move { test_state.handle_resume_sync(&mut virtual_overseer, session).await; @@ -1437,9 +1414,8 @@ fn resume_dispute_without_local_statement_or_local_key() { fn resume_dispute_with_local_statement_without_local_key() { let session = 1; - let mut test_state = TestState::default(); - test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); - test_state + let test_state = TestState::default(); + let mut test_state = test_state .resume(|mut test_state, mut virtual_overseer| { Box::pin(async move { test_state.handle_resume_sync(&mut virtual_overseer, session).await; @@ -1475,18 +1451,6 @@ fn resume_dispute_with_local_statement_without_local_key() { }) .await; - assert_matches!( - virtual_overseer.recv().await, - AllMessages::DisputeParticipation( - DisputeParticipationMessage::Participate { - report_availability, - .. - } - ) => { - report_availability.send(true).unwrap(); - } - ); - assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); { @@ -1506,14 +1470,17 @@ fn resume_dispute_with_local_statement_without_local_key() { test_state }) - }) - // Alice should send a DisputeParticiationMessage::Participate on restart since she has no - // local statement for the active dispute. - .resume(|test_state, mut virtual_overseer| { + }); + // No keys: + test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); + // Two should not send a DisputeParticiationMessage::Participate on restart since we gave + // her a non existing key. + test_state.resume(|test_state, mut virtual_overseer| { Box::pin(async move { test_state.handle_resume_sync(&mut virtual_overseer, session).await; - // Assert that subsystem is not sending Participation messages because we issued a local statement + // Assert that subsystem is not sending Participation messages because we don't + // have a key. assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; @@ -1591,6 +1558,8 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation ); // Make sure we don't get a `DisputeParticiationMessage`. + assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; assert!(virtual_overseer.try_recv().await.is_none()); @@ -1611,7 +1580,6 @@ fn negative_issue_local_statement_only_triggers_import() { let candidate_hash = candidate_receipt.hash(); test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - tracing::debug!("AFTER activbate_leaf"); virtual_overseer .send(FromOverseer::Communication { @@ -1623,7 +1591,6 @@ fn negative_issue_local_statement_only_triggers_import() { ), }) .await; - tracing::debug!("AFTER Issue local statement"); let backend = DbBackend::new(test_state.db.clone(), test_state.config.column_config()); @@ -1632,13 +1599,13 @@ fn negative_issue_local_statement_only_triggers_import() { assert_eq!(votes.valid.len(), 0); let disputes = backend.load_recent_disputes().unwrap(); - tracing::debug!("AFTER load recent disputes"); assert_eq!(disputes, None); + // Assert that subsystem is not sending Participation messages: + assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; - // Make sure we don't get a `DisputeParticiationMessage`. assert!(virtual_overseer.try_recv().await.is_none()); - tracing::debug!("AFTER try_recv"); test_state }) From 191f34bbb622156563513283c6c53a52fc14cd18 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 7 Sep 2021 21:15:24 +0200 Subject: [PATCH 4/5] cargo fmt --- node/core/dispute-coordinator/src/real/mod.rs | 8 +- .../dispute-coordinator/src/real/tests.rs | 127 +++++++++--------- 2 files changed, 65 insertions(+), 70 deletions(-) diff --git a/node/core/dispute-coordinator/src/real/mod.rs b/node/core/dispute-coordinator/src/real/mod.rs index 9fd38d59330b..791a2fdcbf95 100644 --- a/node/core/dispute-coordinator/src/real/mod.rs +++ b/node/core/dispute-coordinator/src/real/mod.rs @@ -48,7 +48,7 @@ use polkadot_node_subsystem_util::rolling_session_window::{ }; use polkadot_primitives::v1::{ BlockNumber, CandidateHash, CandidateReceipt, DisputeStatement, Hash, SessionIndex, - SessionInfo, ValidatorIndex, ValidatorPair, ValidatorSignature, ValidatorId, + SessionInfo, ValidatorId, ValidatorIndex, ValidatorPair, ValidatorSignature, }; use futures::{channel::oneshot, prelude::*}; @@ -746,10 +746,8 @@ async fn handle_import_statements( // This branch is only hit when the candidate is freshly disputed - // status was previously `None`, and now is not. if prev_status.is_none() && { - let controlled_indices = find_controlled_validator_indices( - &state.keystore, - &validators, - ); + let controlled_indices = + find_controlled_validator_indices(&state.keystore, &validators); let voted_indices = votes.voted_indices(); !controlled_indices.iter().all(|val_index| voted_indices.contains(&val_index)) diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 9db820036b8b..4445f62f1133 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -1394,7 +1394,7 @@ fn resume_dispute_without_local_statement_or_local_key() { }) }) // Two should not send a DisputeParticiationMessage::Participate on restart since she is no - // validator in that dispute. + // validator in that dispute. .resume(|test_state, mut virtual_overseer| { Box::pin(async move { test_state.handle_resume_sync(&mut virtual_overseer, session).await; @@ -1415,83 +1415,81 @@ fn resume_dispute_with_local_statement_without_local_key() { let session = 1; let test_state = TestState::default(); - let mut test_state = test_state - .resume(|mut test_state, mut virtual_overseer| { - Box::pin(async move { - test_state.handle_resume_sync(&mut virtual_overseer, session).await; + let mut test_state = test_state.resume(|mut test_state, mut virtual_overseer| { + Box::pin(async move { + test_state.handle_resume_sync(&mut virtual_overseer, session).await; - let candidate_receipt = CandidateReceipt::default(); - let candidate_hash = candidate_receipt.hash(); + let candidate_receipt = CandidateReceipt::default(); + let candidate_hash = candidate_receipt.hash(); - test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; + test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let local_valid_vote = - test_state.issue_statement_with_index(0, candidate_hash, session, true).await; + let local_valid_vote = + test_state.issue_statement_with_index(0, candidate_hash, session, true).await; - let valid_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, true).await; + let valid_vote = + test_state.issue_statement_with_index(1, candidate_hash, session, true).await; - let invalid_vote = - test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + let invalid_vote = + test_state.issue_statement_with_index(2, candidate_hash, session, false).await; + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOverseer::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_hash, + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![ + (local_valid_vote, ValidatorIndex(0)), + (valid_vote, ValidatorIndex(1)), + (invalid_vote, ValidatorIndex(2)), + ], + pending_confirmation, + }, + }) + .await; + + assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + { + let (tx, rx) = oneshot::channel(); - let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer .send(FromOverseer::Communication { - msg: DisputeCoordinatorMessage::ImportStatements { - candidate_hash, - candidate_receipt: candidate_receipt.clone(), - session, - statements: vec![ - (local_valid_vote, ValidatorIndex(0)), - (valid_vote, ValidatorIndex(1)), - (invalid_vote, ValidatorIndex(2)), - ], - pending_confirmation, - }, + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), }) .await; - assert_eq!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); - - { - let (tx, rx) = oneshot::channel(); - - virtual_overseer - .send(FromOverseer::Communication { - msg: DisputeCoordinatorMessage::ActiveDisputes(tx), - }) - .await; - - assert_eq!(rx.await.unwrap().len(), 1); - } + assert_eq!(rx.await.unwrap().len(), 1); + } - virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; - assert!(virtual_overseer.try_recv().await.is_none()); + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + assert!(virtual_overseer.try_recv().await.is_none()); - test_state - }) - }); - // No keys: - test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); - // Two should not send a DisputeParticiationMessage::Participate on restart since we gave - // her a non existing key. - test_state.resume(|test_state, mut virtual_overseer| { - Box::pin(async move { - test_state.handle_resume_sync(&mut virtual_overseer, session).await; + test_state + }) + }); + // No keys: + test_state.subsystem_keystore = make_keystore(&[Sr25519Keyring::Two]).into(); + // Two should not send a DisputeParticiationMessage::Participate on restart since we gave + // her a non existing key. + test_state.resume(|test_state, mut virtual_overseer| { + Box::pin(async move { + test_state.handle_resume_sync(&mut virtual_overseer, session).await; - // Assert that subsystem is not sending Participation messages because we don't - // have a key. - assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); + // Assert that subsystem is not sending Participation messages because we don't + // have a key. + assert!(virtual_overseer.recv().timeout(TEST_TIMEOUT).await.is_none()); - virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; - assert!(virtual_overseer.try_recv().await.is_none()); + virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; + assert!(virtual_overseer.try_recv().await.is_none()); - test_state - }) - }); + test_state + }) + }); } - #[test] fn issue_valid_local_statement_does_cause_distribution_but_not_duplicate_participation() { issue_local_statement_does_cause_distribution_but_not_duplicate_participation(true); @@ -1514,8 +1512,9 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation test_state.activate_leaf_at_session(&mut virtual_overseer, session, 1).await; - let other_vote = - test_state.issue_statement_with_index(1, candidate_hash, session, !validity).await; + let other_vote = test_state + .issue_statement_with_index(1, candidate_hash, session, !validity) + .await; let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1524,9 +1523,7 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation candidate_hash, candidate_receipt: candidate_receipt.clone(), session, - statements: vec![ - (other_vote, ValidatorIndex(1)), - ], + statements: vec![(other_vote, ValidatorIndex(1))], pending_confirmation, }, }) From 2c09bf675e88c83134543aa5c5efe83502ae592c Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 7 Sep 2021 21:17:14 +0200 Subject: [PATCH 5/5] Get rid of redundant dependency. --- Cargo.lock | 1 - node/core/dispute-coordinator/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 17d9e54dd643..a9e18164d46a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6139,7 +6139,6 @@ dependencies = [ "sp-core", "sp-keyring", "sp-keystore", - "sp-tracing", "thiserror", "tracing", ] diff --git a/node/core/dispute-coordinator/Cargo.toml b/node/core/dispute-coordinator/Cargo.toml index 9b4566b7d5f3..5c1284a5f1da 100644 --- a/node/core/dispute-coordinator/Cargo.toml +++ b/node/core/dispute-coordinator/Cargo.toml @@ -26,7 +26,6 @@ polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" } -sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" } assert_matches = "1.4.0" [features]