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 <eskimor@no-such-url.com>

polkadot/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<Votes> {
 	/// 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<DisputeStatus>,
 }
 
 impl CandidateVoteState<CandidateVotes> {
@@ -179,18 +165,11 @@ impl CandidateVoteState<CandidateVotes> {
 	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<CandidateVotes> {
 		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);
+			};
 
-		Self { votes, own_vote, concluded_invalid, concluded_valid, is_confirmed, is_disputed }
+			let concluded_against = votes.invalid.len() >= supermajority_threshold;
+			if concluded_against {
+				status = status.conclude_against(now);
+			};
+			Some(status)
+		} else {
+			None
+		};
+
+		Self { votes, own_vote, dispute_status }
 	}
 
 	/// Import fresh statements.
@@ -217,6 +211,7 @@ impl CandidateVoteState<CandidateVotes> {
 		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<CandidateVotes> {
 			}
 		}
 
-		let new_state = Self::new(votes, env);
+		let new_state = Self::new(votes, env, now);
 
 		ImportResult {
 			old_state,
@@ -313,32 +308,15 @@ impl CandidateVoteState<CandidateVotes> {
 
 	/// 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<V> CandidateVoteState<V> {
 	/// 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<V> CandidateVoteState<V> {
 	/// 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<V> CandidateVoteState<V> {
 		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 this dispute has already enough invalid votes to conclude.
-	pub fn is_concluded_invalid(&self) -> bool {
-		self.concluded_invalid
+	/// 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())
+	}
+
+	/// Get access to the dispute status, in case there is one.
+	pub fn dispute_status(&self) -> &Option<DisputeStatus> {
+		&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<ValidatorIndex, ValidatorSignature>,
+		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,

polkadot/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!(
+				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;
+
+				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,

polkadot/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| {

polkadot/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);
 

polkadot/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),