From 89dc70f9cdeedf4312c2ea67c871d043a92e0148 Mon Sep 17 00:00:00 2001 From: eskimor Date: Mon, 14 Nov 2022 12:41:45 +0100 Subject: [PATCH] Only report concluded if there is an actual dispute. (#6270) * Only report concluded if there is an actual dispute. Hence no "non"-disputes will be added to disputes anymore. * Fix redundant check. * Test for no onesided disputes. Co-authored-by: eskimor --- node/core/dispute-coordinator/src/import.rs | 116 ++++++++---------- .../dispute-coordinator/src/initialized.rs | 65 +++++----- node/core/dispute-coordinator/src/tests.rs | 62 ++++++++++ node/primitives/src/disputes/message.rs | 3 + node/primitives/src/disputes/status.rs | 27 +++- 5 files changed, 166 insertions(+), 107 deletions(-) diff --git a/node/core/dispute-coordinator/src/import.rs b/node/core/dispute-coordinator/src/import.rs index 8eb3d173dcf7..c0f0d3d9e009 100644 --- a/node/core/dispute-coordinator/src/import.rs +++ b/node/core/dispute-coordinator/src/import.rs @@ -28,7 +28,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; -use polkadot_node_primitives::{CandidateVotes, SignedDisputeStatement}; +use polkadot_node_primitives::{CandidateVotes, DisputeStatus, SignedDisputeStatement, Timestamp}; use polkadot_node_subsystem_util::rolling_session_window::RollingSessionWindow; use polkadot_primitives::v2::{ CandidateReceipt, DisputeStatement, IndexedVec, SessionIndex, SessionInfo, @@ -154,22 +154,8 @@ pub struct CandidateVoteState { /// Information about own votes: own_vote: OwnVoteState, - /// Whether or not the dispute concluded invalid. - concluded_invalid: bool, - - /// Whether or not the dispute concluded valid. - /// - /// Note: Due to equivocations it is technically possible for a dispute to conclude both valid - /// and invalid. In that case the invalid result takes precedence. - concluded_valid: bool, - - /// There is an ongoing dispute and we reached f+1 votes -> the dispute is confirmed - /// - /// as at least one honest validator cast a vote for the candidate. - is_confirmed: bool, - - /// Whether or not we have an ongoing dispute. - is_disputed: bool, + /// Current dispute status, if there is any. + dispute_status: Option, } impl CandidateVoteState { @@ -179,18 +165,11 @@ impl CandidateVoteState { pub fn new_from_receipt(candidate_receipt: CandidateReceipt) -> Self { let votes = CandidateVotes { candidate_receipt, valid: BTreeMap::new(), invalid: BTreeMap::new() }; - Self { - votes, - own_vote: OwnVoteState::NoVote, - concluded_invalid: false, - concluded_valid: false, - is_confirmed: false, - is_disputed: false, - } + Self { votes, own_vote: OwnVoteState::NoVote, dispute_status: None } } /// Create a new `CandidateVoteState` from already existing votes. - pub fn new<'a>(votes: CandidateVotes, env: &CandidateEnvironment<'a>) -> Self { + pub fn new<'a>(votes: CandidateVotes, env: &CandidateEnvironment<'a>, now: Timestamp) -> Self { let own_vote = OwnVoteState::new(&votes, env); let n_validators = env.validators().len(); @@ -198,16 +177,31 @@ impl CandidateVoteState { let supermajority_threshold = polkadot_primitives::v2::supermajority_threshold(n_validators); - let concluded_invalid = votes.invalid.len() >= supermajority_threshold; - let concluded_valid = votes.valid.len() >= supermajority_threshold; - // We have a dispute, if we have votes on both sides: let is_disputed = !votes.invalid.is_empty() && !votes.valid.is_empty(); - let byzantine_threshold = polkadot_primitives::v2::byzantine_threshold(n_validators); - let is_confirmed = votes.voted_indices().len() > byzantine_threshold && is_disputed; + let dispute_status = if is_disputed { + let mut status = DisputeStatus::active(); + let byzantine_threshold = polkadot_primitives::v2::byzantine_threshold(n_validators); + let is_confirmed = votes.voted_indices().len() > byzantine_threshold; + if is_confirmed { + status = status.confirm(); + }; + let concluded_for = votes.valid.len() >= supermajority_threshold; + if concluded_for { + status = status.conclude_for(now); + }; + + let concluded_against = votes.invalid.len() >= supermajority_threshold; + if concluded_against { + status = status.conclude_against(now); + }; + Some(status) + } else { + None + }; - Self { votes, own_vote, concluded_invalid, concluded_valid, is_confirmed, is_disputed } + Self { votes, own_vote, dispute_status } } /// Import fresh statements. @@ -217,6 +211,7 @@ impl CandidateVoteState { self, env: &CandidateEnvironment, statements: Vec<(SignedDisputeStatement, ValidatorIndex)>, + now: Timestamp, ) -> ImportResult { let (mut votes, old_state) = self.into_old_state(); @@ -294,7 +289,7 @@ impl CandidateVoteState { } } - let new_state = Self::new(votes, env); + let new_state = Self::new(votes, env, now); ImportResult { old_state, @@ -313,32 +308,15 @@ impl CandidateVoteState { /// Extract `CandidateVotes` for handling import of new statements. fn into_old_state(self) -> (CandidateVotes, CandidateVoteState<()>) { - let CandidateVoteState { - votes, - own_vote, - concluded_invalid, - concluded_valid, - is_confirmed, - is_disputed, - } = self; - ( - votes, - CandidateVoteState { - votes: (), - own_vote, - concluded_invalid, - concluded_valid, - is_confirmed, - is_disputed, - }, - ) + let CandidateVoteState { votes, own_vote, dispute_status } = self; + (votes, CandidateVoteState { votes: (), own_vote, dispute_status }) } } impl CandidateVoteState { /// Whether or not we have an ongoing dispute. pub fn is_disputed(&self) -> bool { - self.is_disputed + self.dispute_status.is_some() } /// Whether there is an ongoing confirmed dispute. @@ -346,7 +324,7 @@ impl CandidateVoteState { /// This checks whether there is a dispute ongoing and we have more than byzantine threshold /// votes. pub fn is_confirmed(&self) -> bool { - self.is_confirmed + self.dispute_status.map_or(false, |s| s.is_confirmed_concluded()) } /// This machine already cast some vote in that dispute/for that candidate. @@ -359,14 +337,19 @@ impl CandidateVoteState { self.own_vote.approval_votes() } - /// Whether or not this dispute has already enough valid votes to conclude. - pub fn is_concluded_valid(&self) -> bool { - self.concluded_valid + /// Whether or not there is a dispute and it has already enough valid votes to conclude. + pub fn has_concluded_for(&self) -> bool { + self.dispute_status.map_or(false, |s| s.has_concluded_for()) + } + + /// Whether or not there is a dispute and it has already enough invalid votes to conclude. + pub fn has_concluded_against(&self) -> bool { + self.dispute_status.map_or(false, |s| s.has_concluded_against()) } - /// Whether or not this dispute has already enough invalid votes to conclude. - pub fn is_concluded_invalid(&self) -> bool { - self.concluded_invalid + /// Get access to the dispute status, in case there is one. + pub fn dispute_status(&self) -> &Option { + &self.dispute_status } /// Access to underlying votes. @@ -451,18 +434,18 @@ impl ImportResult { } /// Whether or not any dispute just concluded valid due to the import. - pub fn is_freshly_concluded_valid(&self) -> bool { - !self.old_state().is_concluded_valid() && self.new_state().is_concluded_valid() + pub fn is_freshly_concluded_for(&self) -> bool { + !self.old_state().has_concluded_for() && self.new_state().has_concluded_for() } /// Whether or not any dispute just concluded invalid due to the import. - pub fn is_freshly_concluded_invalid(&self) -> bool { - !self.old_state().is_concluded_invalid() && self.new_state().is_concluded_invalid() + pub fn is_freshly_concluded_against(&self) -> bool { + !self.old_state().has_concluded_against() && self.new_state().has_concluded_against() } /// Whether or not any dispute just concluded either invalid or valid due to the import. pub fn is_freshly_concluded(&self) -> bool { - self.is_freshly_concluded_invalid() || self.is_freshly_concluded_valid() + self.is_freshly_concluded_against() || self.is_freshly_concluded_for() } /// Modify this `ImportResult`s, by importing additional approval votes. @@ -473,6 +456,7 @@ impl ImportResult { self, env: &CandidateEnvironment, approval_votes: HashMap, + now: Timestamp, ) -> Self { let Self { old_state, @@ -508,7 +492,7 @@ impl ImportResult { } } - let new_state = CandidateVoteState::new(votes, env); + let new_state = CandidateVoteState::new(votes, env, now); Self { old_state, diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs index 901a6d863ed2..0df1a620826c 100644 --- a/node/core/dispute-coordinator/src/initialized.rs +++ b/node/core/dispute-coordinator/src/initialized.rs @@ -748,7 +748,7 @@ impl Initialized { .load_candidate_votes(session, &candidate_hash)? .map(CandidateVotes::from) { - Some(votes) => CandidateVoteState::new(votes, &env), + Some(votes) => CandidateVoteState::new(votes, &env, now), None => if let MaybeCandidateReceipt::Provides(candidate_receipt) = candidate_receipt { CandidateVoteState::new_from_receipt(candidate_receipt) @@ -766,7 +766,7 @@ impl Initialized { gum::trace!(target: LOG_TARGET, ?candidate_hash, ?session, "Loaded votes"); let import_result = { - let intermediate_result = old_state.import_statements(&env, statements); + let intermediate_result = old_state.import_statements(&env, statements, now); // Handle approval vote import: // @@ -803,7 +803,7 @@ impl Initialized { ); intermediate_result }, - Ok(votes) => intermediate_result.import_approval_votes(&env, votes), + Ok(votes) => intermediate_result.import_approval_votes(&env, votes, now), } } else { gum::trace!( @@ -977,43 +977,34 @@ impl Initialized { } // All good, update recent disputes if state has changed: - if import_result.dispute_state_changed() { - let mut recent_disputes = overlay_db.load_recent_disputes()?.unwrap_or_default(); + if let Some(new_status) = new_state.dispute_status() { + // Only bother with db access, if there was an actual change. + if import_result.dispute_state_changed() { + let mut recent_disputes = overlay_db.load_recent_disputes()?.unwrap_or_default(); + + let status = + recent_disputes.entry((session, candidate_hash)).or_insert_with(|| { + gum::info!( + target: LOG_TARGET, + ?candidate_hash, + session, + "New dispute initiated for candidate.", + ); + DisputeStatus::active() + }); + + *status = *new_status; - let status = recent_disputes.entry((session, candidate_hash)).or_insert_with(|| { - gum::info!( + gum::trace!( target: LOG_TARGET, ?candidate_hash, - session, - "New dispute initiated for candidate.", + ?status, + has_concluded_for = ?new_state.has_concluded_for(), + has_concluded_against = ?new_state.has_concluded_against(), + "Writing recent disputes with updates for candidate" ); - DisputeStatus::active() - }); - - if new_state.is_confirmed() { - *status = status.confirm(); + overlay_db.write_recent_disputes(recent_disputes); } - - // Note: concluded-invalid overwrites concluded-valid, - // so we do this check first. Dispute state machine is - // non-commutative. - if new_state.is_concluded_valid() { - *status = status.concluded_for(now); - } - - if new_state.is_concluded_invalid() { - *status = status.concluded_against(now); - } - - gum::trace!( - target: LOG_TARGET, - ?candidate_hash, - ?status, - is_concluded_valid = ?new_state.is_concluded_valid(), - is_concluded_invalid = ?new_state.is_concluded_invalid(), - "Writing recent disputes with updates for candidate" - ); - overlay_db.write_recent_disputes(recent_disputes); } // Update metrics: @@ -1036,7 +1027,7 @@ impl Initialized { ); self.metrics.on_approval_votes(import_result.imported_approval_votes()); - if import_result.is_freshly_concluded_valid() { + if import_result.is_freshly_concluded_for() { gum::info!( target: LOG_TARGET, ?candidate_hash, @@ -1045,7 +1036,7 @@ impl Initialized { ); self.metrics.on_concluded_valid(); } - if import_result.is_freshly_concluded_invalid() { + if import_result.is_freshly_concluded_against() { gum::info!( target: LOG_TARGET, ?candidate_hash, diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs index d7208c1a59eb..b2f779041a4c 100644 --- a/node/core/dispute-coordinator/src/tests.rs +++ b/node/core/dispute-coordinator/src/tests.rs @@ -2917,6 +2917,68 @@ fn redundant_votes_ignored() { }); } +#[test] +/// Make sure no disputes are recorded when there are no opposing votes, even if we reached supermajority. +fn no_onesided_disputes() { + 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 = make_valid_candidate_receipt(); + let candidate_hash = candidate_receipt.hash(); + test_state + .activate_leaf_at_session(&mut virtual_overseer, session, 1, Vec::new()) + .await; + + 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, + ValidatorIndex(index), + )); + } + + let (pending_confirmation, confirmation_rx) = oneshot::channel(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ImportStatements { + candidate_receipt: candidate_receipt.clone(), + session, + statements, + pending_confirmation: Some(pending_confirmation), + }, + }) + .await; + assert_matches!(confirmation_rx.await, Ok(ImportStatementsResult::ValidImport)); + + // We should not have any active disputes now. + let (tx, rx) = oneshot::channel(); + virtual_overseer + .send(FromOrchestra::Communication { + msg: DisputeCoordinatorMessage::ActiveDisputes(tx), + }) + .await; + + assert!(rx.await.unwrap().is_empty()); + + virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await; + + // No more messages expected: + assert!(virtual_overseer.try_recv().await.is_none()); + + test_state + }) + }); +} + #[test] fn refrain_from_participation() { test_harness(|mut test_state, mut virtual_overseer| { diff --git a/node/primitives/src/disputes/message.rs b/node/primitives/src/disputes/message.rs index 93d4e804f906..1a943f8dcee6 100644 --- a/node/primitives/src/disputes/message.rs +++ b/node/primitives/src/disputes/message.rs @@ -33,6 +33,9 @@ use polkadot_primitives::v2::{ /// /// And most likely has been constructed correctly. This is used with /// `DisputeDistributionMessage::SendDispute` for sending out votes. +/// +/// NOTE: This is sent over the wire, any changes are a change in protocol and need to be +/// versioned. #[derive(Debug, Clone)] pub struct DisputeMessage(UncheckedDisputeMessage); diff --git a/node/primitives/src/disputes/status.rs b/node/primitives/src/disputes/status.rs index c0ffb907423b..7cc26c1a1f7a 100644 --- a/node/primitives/src/disputes/status.rs +++ b/node/primitives/src/disputes/status.rs @@ -19,8 +19,12 @@ use parity_scale_codec::{Decode, Encode}; /// Timestamp based on the 1 Jan 1970 UNIX base, which is persistent across node restarts and OS reboots. pub type Timestamp = u64; -/// The status of dispute. This is a state machine which can be altered by the -/// helper methods. +/// The status of dispute. +/// +/// As managed by the dispute coordinator. +/// +/// NOTE: This status is persisted to the database, any changes have to be versioned and a db +/// migration will be needed. #[derive(Debug, Clone, Copy, Encode, Decode, PartialEq)] pub enum DisputeStatus { /// The dispute is active and unconcluded. @@ -69,9 +73,24 @@ impl DisputeStatus { } } + /// Concluded valid? + pub fn has_concluded_for(&self) -> bool { + match self { + &DisputeStatus::ConcludedFor(_) => true, + _ => false, + } + } + /// Concluded invalid? + pub fn has_concluded_against(&self) -> bool { + match self { + &DisputeStatus::ConcludedAgainst(_) => true, + _ => false, + } + } + /// Transition the status to a new status after observing the dispute has concluded for the candidate. /// This may be a no-op if the status was already concluded. - pub fn concluded_for(self, now: Timestamp) -> DisputeStatus { + pub fn conclude_for(self, now: Timestamp) -> DisputeStatus { match self { DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedFor(now), DisputeStatus::ConcludedFor(at) => DisputeStatus::ConcludedFor(std::cmp::min(at, now)), @@ -81,7 +100,7 @@ impl DisputeStatus { /// Transition the status to a new status after observing the dispute has concluded against the candidate. /// This may be a no-op if the status was already concluded. - pub fn concluded_against(self, now: Timestamp) -> DisputeStatus { + pub fn conclude_against(self, now: Timestamp) -> DisputeStatus { match self { DisputeStatus::Active | DisputeStatus::Confirmed => DisputeStatus::ConcludedAgainst(now),