diff --git a/polkadot/node/core/dispute-coordinator/src/import.rs b/polkadot/node/core/dispute-coordinator/src/import.rs index 0c53211b358f84b93306dc72747fbd9110ded130..912521834075689683919657ae904cfd237efb17 100644 --- a/polkadot/node/core/dispute-coordinator/src/import.rs +++ b/polkadot/node/core/dispute-coordinator/src/import.rs @@ -179,6 +179,9 @@ pub struct CandidateVoteState<Votes> { /// Current dispute status, if there is any. dispute_status: Option<DisputeStatus>, + + /// Are there `byzantine threshold + 1` invalid votes + byzantine_threshold_against: bool, } impl CandidateVoteState<CandidateVotes> { @@ -191,7 +194,12 @@ impl CandidateVoteState<CandidateVotes> { valid: ValidCandidateVotes::new(), invalid: BTreeMap::new(), }; - Self { votes, own_vote: OwnVoteState::CannotVote, dispute_status: None } + Self { + votes, + own_vote: OwnVoteState::CannotVote, + dispute_status: None, + byzantine_threshold_against: false, + } } /// Create a new `CandidateVoteState` from already existing votes. @@ -205,7 +213,7 @@ impl CandidateVoteState<CandidateVotes> { // We have a dispute, if we have votes on both sides: let is_disputed = !votes.invalid.is_empty() && !votes.valid.raw().is_empty(); - let dispute_status = if is_disputed { + let (dispute_status, byzantine_threshold_against) = if is_disputed { let mut status = DisputeStatus::active(); let byzantine_threshold = polkadot_primitives::byzantine_threshold(n_validators); let is_confirmed = votes.voted_indices().len() > byzantine_threshold; @@ -221,12 +229,12 @@ impl CandidateVoteState<CandidateVotes> { if concluded_against { status = status.conclude_against(now); }; - Some(status) + (Some(status), votes.invalid.len() > byzantine_threshold) } else { - None + (None, false) }; - Self { votes, own_vote, dispute_status } + Self { votes, own_vote, dispute_status, byzantine_threshold_against } } /// Import fresh statements. @@ -328,8 +336,12 @@ impl CandidateVoteState<CandidateVotes> { /// Extract `CandidateVotes` for handling import of new statements. fn into_old_state(self) -> (CandidateVotes, CandidateVoteState<()>) { - let CandidateVoteState { votes, own_vote, dispute_status } = self; - (votes, CandidateVoteState { votes: (), own_vote, dispute_status }) + let CandidateVoteState { votes, own_vote, dispute_status, byzantine_threshold_against } = + self; + ( + votes, + CandidateVoteState { votes: (), own_vote, dispute_status, byzantine_threshold_against }, + ) } } @@ -477,6 +489,13 @@ impl ImportResult { self.is_freshly_concluded_against() || self.is_freshly_concluded_for() } + /// Whether or not the invalid vote count for the dispute went beyond the byzantine threshold + /// after the last import + pub fn has_fresh_byzantine_threshold_against(&self) -> bool { + !self.old_state().byzantine_threshold_against && + self.new_state().byzantine_threshold_against + } + /// Modify this `ImportResult`s, by importing additional approval votes. /// /// Both results and `new_state` will be changed as if those approval votes had been in the diff --git a/polkadot/node/core/dispute-coordinator/src/initialized.rs b/polkadot/node/core/dispute-coordinator/src/initialized.rs index c0eb029f4d0f534862ca35a45c390cecf1ede94b..b5c548bf1ffd77f3ce815a5556e1f00730b761f2 100644 --- a/polkadot/node/core/dispute-coordinator/src/initialized.rs +++ b/polkadot/node/core/dispute-coordinator/src/initialized.rs @@ -1094,7 +1094,7 @@ impl Initialized { // Notify ChainSelection if a dispute has concluded against a candidate. ChainSelection // will need to mark the candidate's relay parent as reverted. - if import_result.is_freshly_concluded_against() { + if import_result.has_fresh_byzantine_threshold_against() { let blocks_including = self.scraper.get_blocks_including_candidate(&candidate_hash); for (parent_block_number, parent_block_hash) in &blocks_including { gum::trace!( diff --git a/polkadot/node/core/dispute-coordinator/src/tests.rs b/polkadot/node/core/dispute-coordinator/src/tests.rs index b685660d62b711444c77f3db437949ebda14308e..7d3b87f3c22849ebb0bc8de475c0e493421c196a 100644 --- a/polkadot/node/core/dispute-coordinator/src/tests.rs +++ b/polkadot/node/core/dispute-coordinator/src/tests.rs @@ -121,17 +121,20 @@ async fn generate_opposing_votes_pair( ) -> (SignedDisputeStatement, SignedDisputeStatement) { let valid_vote = match valid_vote_type { VoteType::Backing => - test_state - .issue_backing_statement_with_index(valid_voter_idx, candidate_hash, session) - .await, - VoteType::Explicit => - test_state - .issue_explicit_statement_with_index(valid_voter_idx, candidate_hash, session, true) - .await, + test_state.issue_backing_statement_with_index(valid_voter_idx, candidate_hash, session), + VoteType::Explicit => test_state.issue_explicit_statement_with_index( + valid_voter_idx, + candidate_hash, + session, + true, + ), }; - let invalid_vote = test_state - .issue_explicit_statement_with_index(invalid_voter_idx, candidate_hash, session, false) - .await; + let invalid_vote = test_state.issue_explicit_statement_with_index( + invalid_voter_idx, + candidate_hash, + session, + false, + ); (valid_vote, invalid_vote) } @@ -466,7 +469,7 @@ impl TestState { } } - async fn issue_explicit_statement_with_index( + fn issue_explicit_statement_with_index( &self, index: ValidatorIndex, candidate_hash: CandidateHash, @@ -482,7 +485,7 @@ impl TestState { .unwrap() } - async fn issue_backing_statement_with_index( + fn issue_backing_statement_with_index( &self, index: ValidatorIndex, candidate_hash: CandidateHash, @@ -1203,13 +1206,17 @@ fn backing_statements_import_works_and_no_spam() { .activate_leaf_at_session(&mut virtual_overseer, session, 1, Vec::new()) .await; - let valid_vote1 = test_state - .issue_backing_statement_with_index(ValidatorIndex(3), candidate_hash, session) - .await; + let valid_vote1 = test_state.issue_backing_statement_with_index( + ValidatorIndex(3), + candidate_hash, + session, + ); - let valid_vote2 = test_state - .issue_backing_statement_with_index(ValidatorIndex(4), candidate_hash, session) - .await; + let valid_vote2 = test_state.issue_backing_statement_with_index( + ValidatorIndex(4), + candidate_hash, + session, + ); let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -1256,13 +1263,17 @@ fn backing_statements_import_works_and_no_spam() { let candidate_receipt = make_invalid_candidate_receipt(); let candidate_hash = candidate_receipt.hash(); - let valid_vote1 = test_state - .issue_backing_statement_with_index(ValidatorIndex(3), candidate_hash, session) - .await; + let valid_vote1 = test_state.issue_backing_statement_with_index( + ValidatorIndex(3), + candidate_hash, + session, + ); - let valid_vote2 = test_state - .issue_backing_statement_with_index(ValidatorIndex(4), candidate_hash, session) - .await; + let valid_vote2 = test_state.issue_backing_statement_with_index( + ValidatorIndex(4), + candidate_hash, + session, + ); test_state .activate_leaf_at_session( @@ -1333,14 +1344,12 @@ fn conflicting_votes_lead_to_dispute_participation() { ) .await; - let invalid_vote_2 = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(2), - candidate_hash, - session, - false, - ) - .await; + let invalid_vote_2 = test_state.issue_explicit_statement_with_index( + ValidatorIndex(2), + candidate_hash, + session, + false, + ); virtual_overseer .send(FromOrchestra::Communication { @@ -1450,23 +1459,19 @@ fn positive_votes_dont_trigger_participation() { ) .await; - let valid_vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(2), - candidate_hash, - session, - true, - ) - .await; + let valid_vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(2), + candidate_hash, + session, + true, + ); - let valid_vote_2 = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(1), - candidate_hash, - session, - true, - ) - .await; + let valid_vote_2 = test_state.issue_explicit_statement_with_index( + ValidatorIndex(1), + candidate_hash, + session, + true, + ); virtual_overseer .send(FromOrchestra::Communication { @@ -1789,14 +1794,12 @@ fn supermajority_valid_dispute_may_be_finalized() { let mut statements = Vec::new(); for i in (0_u32..supermajority_threshold as u32 - 1).map(|i| i + 3) { - let vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(i), - candidate_hash, - session, - true, - ) - .await; + let vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + true, + ); statements.push((vote, ValidatorIndex(i as _))); } @@ -1929,14 +1932,12 @@ fn concluded_supermajority_for_non_active_after_time() { let mut statements = Vec::new(); // -2: 1 for already imported vote and one for local vote (which is valid). for i in (0_u32..supermajority_threshold as u32 - 2).map(|i| i + 3) { - let vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(i), - candidate_hash, - session, - true, - ) - .await; + let vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + true, + ); statements.push((vote, ValidatorIndex(i as _))); } @@ -2051,14 +2052,12 @@ fn concluded_supermajority_against_non_active_after_time() { let mut statements = Vec::new(); // minus 2, because of local vote and one previously imported invalid vote. for i in (0_u32..supermajority_threshold as u32 - 2).map(|i| i + 3) { - let vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(i), - candidate_hash, - session, - false, - ) - .await; + let vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + false, + ); statements.push((vote, ValidatorIndex(i as _))); } @@ -2204,14 +2203,12 @@ fn resume_dispute_without_local_statement() { let mut statements = Vec::new(); // Getting votes for supermajority. Should already have two valid votes. for i in vec![3, 4, 5, 6, 7] { - let vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(i), - candidate_hash, - session, - true, - ) - .await; + let vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + true, + ); statements.push((vote, ValidatorIndex(i as _))); } @@ -2274,14 +2271,12 @@ fn resume_dispute_with_local_statement() { ) .await; - let local_valid_vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(0), - candidate_hash, - session, - true, - ) - .await; + let local_valid_vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(0), + candidate_hash, + session, + true, + ); let (valid_vote, invalid_vote) = generate_opposing_votes_pair( &test_state, @@ -2476,14 +2471,12 @@ fn issue_local_statement_does_cause_distribution_but_not_duplicate_participation .activate_leaf_at_session(&mut virtual_overseer, session, 1, Vec::new()) .await; - let other_vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(1), - candidate_hash, - session, - !validity, - ) - .await; + let other_vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(1), + candidate_hash, + session, + !validity, + ); let (pending_confirmation, confirmation_rx) = oneshot::channel(); virtual_overseer @@ -2687,13 +2680,17 @@ fn redundant_votes_ignored() { .activate_leaf_at_session(&mut virtual_overseer, session, 1, Vec::new()) .await; - let valid_vote = test_state - .issue_backing_statement_with_index(ValidatorIndex(1), candidate_hash, session) - .await; + let valid_vote = test_state.issue_backing_statement_with_index( + ValidatorIndex(1), + candidate_hash, + session, + ); - let valid_vote_2 = test_state - .issue_backing_statement_with_index(ValidatorIndex(1), candidate_hash, session) - .await; + let valid_vote_2 = test_state.issue_backing_statement_with_index( + ValidatorIndex(1), + candidate_hash, + session, + ); assert!(valid_vote.validator_signature() != valid_vote_2.validator_signature()); @@ -2762,13 +2759,11 @@ fn no_onesided_disputes() { let mut statements = Vec::new(); for index in 1..10 { statements.push(( - test_state - .issue_backing_statement_with_index( - ValidatorIndex(index), - candidate_hash, - session, - ) - .await, + test_state.issue_backing_statement_with_index( + ValidatorIndex(index), + candidate_hash, + session, + ), ValidatorIndex(index), )); } @@ -3044,9 +3039,11 @@ fn local_participation_in_dispute_for_backed_candidate() { ) .await; - let backing_valid = test_state - .issue_backing_statement_with_index(ValidatorIndex(3), candidate_hash, session) - .await; + let backing_valid = test_state.issue_backing_statement_with_index( + ValidatorIndex(3), + candidate_hash, + session, + ); virtual_overseer .send(FromOrchestra::Communication { @@ -3287,8 +3284,8 @@ fn informs_chain_selection_when_dispute_concluded_against() { ) .await; - let supermajority_threshold = - polkadot_primitives::supermajority_threshold(test_state.validators.len()); + let byzantine_threshold = + polkadot_primitives::byzantine_threshold(test_state.validators.len()); let (valid_vote, invalid_vote) = generate_opposing_votes_pair( &test_state, @@ -3329,18 +3326,16 @@ fn informs_chain_selection_when_dispute_concluded_against() { .await; let mut statements = Vec::new(); - // minus 2, because of local vote and one previously imported invalid vote. - for i in (0_u32..supermajority_threshold as u32 - 2).map(|i| i + 3) { - let vote = test_state - .issue_explicit_statement_with_index( - ValidatorIndex(i), - candidate_hash, - session, - false, - ) - .await; + // own vote + `byzantine_threshold` more votes should be enough to issue `RevertBlocks` + for i in 3_u32..byzantine_threshold as u32 + 3 { + let vote = test_state.issue_explicit_statement_with_index( + ValidatorIndex(i), + candidate_hash, + session, + false, + ); - statements.push((vote, ValidatorIndex(i as _))); + statements.push((vote, ValidatorIndex(i))); } virtual_overseer @@ -3353,8 +3348,6 @@ fn informs_chain_selection_when_dispute_concluded_against() { }, }) .await; - handle_approval_vote_request(&mut virtual_overseer, &candidate_hash, HashMap::new()) - .await; // Checking that concluded dispute has signaled the reversion of all parent blocks. assert_matches!( @@ -3368,6 +3361,27 @@ fn informs_chain_selection_when_dispute_concluded_against() { "Overseer did not receive `ChainSelectionMessage::RevertBlocks` message" ); + // One more import which should not trigger reversion + // Validator index is `byzantine_threshold + 4` + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_receipt: candidate_receipt.clone(), + session, + statements: vec![( + test_state.issue_explicit_statement_with_index( + ValidatorIndex(byzantine_threshold as u32 + 4), + candidate_hash, + session, + false, + ), + ValidatorIndex(byzantine_threshold as u32 + 4), + )], + pending_confirmation: None, + }, + }) + .await; + // Wrap up virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await; assert_matches!(