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>
paritytech · ordian · Feb 1, 2023 · Oct 24, 2022 · Nov 7, 2022 · Nov 7, 2022
commit 8d3ff430f656ee9fbcf599c3e6985315939e71ce
diff --git 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<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,35 +165,43 @@ 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();
 
 		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<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,40 +308,23 @@ 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.
 	///
 	/// 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 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<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,

diff --git 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,