Ensure backing votes don't get overridden.

paritytech · paritytech-processbot · Dec 7, 2022 · Dec 2, 2022 · Dec 2, 2022 · Dec 2, 2022
commit c3c074d5aa31526ed0430a4b28d618c75bebc3f3
diff --git a/node/core/dispute-coordinator/src/import.rs b/node/core/dispute-coordinator/src/import.rs
@@ -28,7 +28,9 @@
 
 use std::collections::{BTreeMap, HashMap, HashSet};
 
-use polkadot_node_primitives::{CandidateVotes, DisputeStatus, SignedDisputeStatement, Timestamp};
+use polkadot_node_primitives::{
+	disputes::ValidCandidateVotes, CandidateVotes, DisputeStatus, SignedDisputeStatement, Timestamp,
+};
 use polkadot_node_subsystem_util::rolling_session_window::RollingSessionWindow;
 use polkadot_primitives::v2::{
 	CandidateReceipt, DisputeStatement, IndexedVec, SessionIndex, SessionInfo,
@@ -101,7 +103,7 @@ impl OwnVoteState {
 		let mut our_valid_votes = env
 			.controlled_indices()
 			.iter()
-			.filter_map(|i| votes.valid.get_key_value(i))
+			.filter_map(|i| votes.valid.raw().get_key_value(i))
 			.peekable();
 		let mut our_invalid_votes =
 			env.controlled_indices.iter().filter_map(|i| votes.invalid.get_key_value(i));
@@ -163,8 +165,11 @@ impl CandidateVoteState<CandidateVotes> {
 	///
 	/// in case there have not been any previous votes.
 	pub fn new_from_receipt(candidate_receipt: CandidateReceipt) -> Self {
-		let votes =
-			CandidateVotes { candidate_receipt, valid: BTreeMap::new(), invalid: BTreeMap::new() };
+		let votes = CandidateVotes {
+			candidate_receipt,
+			valid: ValidCandidateVotes::new(),
+			invalid: BTreeMap::new(),
+		};
 		Self { votes, own_vote: OwnVoteState::NoVote, dispute_status: None }
 	}
 
@@ -178,7 +183,7 @@ impl CandidateVoteState<CandidateVotes> {
 			polkadot_primitives::v2::supermajority_threshold(n_validators);
 
 		// We have a dispute, if we have votes on both sides:
-		let is_disputed = !votes.invalid.is_empty() && !votes.valid.is_empty();
+		let is_disputed = !votes.invalid.is_empty() && !votes.valid.raw().is_empty();
 
 		let dispute_status = if is_disputed {
 			let mut status = DisputeStatus::active();
@@ -187,7 +192,7 @@ impl CandidateVoteState<CandidateVotes> {
 			if is_confirmed {
 				status = status.confirm();
 			};
-			let concluded_for = votes.valid.len() >= supermajority_threshold;
+			let concluded_for = votes.valid.raw().len() >= supermajority_threshold;
 			if concluded_for {
 				status = status.conclude_for(now);
 			};
@@ -262,25 +267,20 @@ impl CandidateVoteState<CandidateVotes> {
 
 			match statement.statement() {
 				DisputeStatement::Valid(valid_kind) => {
-					let fresh = insert_into_statements(
-						&mut votes.valid,
-						*valid_kind,
+					let fresh = votes.valid.insert_vote(
 						val_index,
+						*valid_kind,
 						statement.into_validator_signature(),
 					);
-
 					if fresh {
 						imported_valid_votes += 1;
 					}
 				},
 				DisputeStatement::Invalid(invalid_kind) => {
-					let fresh = insert_into_statements(
-						&mut votes.invalid,
-						*invalid_kind,
-						val_index,
-						statement.into_validator_signature(),
-					);
-
+					let fresh = votes
+						.invalid
+						.insert(val_index, (*invalid_kind, statement.into_validator_signature()))
+						.is_none();
 					if fresh {
 						new_invalid_voters.push(val_index);
 						imported_invalid_votes += 1;
@@ -481,12 +481,7 @@ impl ImportResult {
 				},
 				"Signature check for imported approval votes failed! This is a serious bug. Session: {:?}, candidate hash: {:?}, validator index: {:?}", env.session_index(), votes.candidate_receipt.hash(), index
 			);
-			if insert_into_statements(
-				&mut votes.valid,
-				ValidDisputeStatementKind::ApprovalChecking,
-				index,
-				sig,
-			) {
+			if votes.valid.insert_vote(index, ValidDisputeStatementKind::ApprovalChecking, sig) {
 				imported_valid_votes += 1;
 				imported_approval_votes += 1;
 			}
@@ -535,14 +530,3 @@ fn find_controlled_validator_indices(
 
 	controlled
 }
-
-// Returns 'true' if no other vote by that validator was already
-// present and 'false' otherwise. Same semantics as `HashSet`.
-fn insert_into_statements<T>(
-	m: &mut BTreeMap<ValidatorIndex, (T, ValidatorSignature)>,
-	tag: T,
-	val_index: ValidatorIndex,
-	val_signature: ValidatorSignature,
-) -> bool {
-	m.insert(val_index, (tag, val_signature)).is_none()
-}
diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs
@@ -26,8 +26,8 @@ use futures::{
 use sc_keystore::LocalKeystore;
 
 use polkadot_node_primitives::{
-	CandidateVotes, DisputeMessage, DisputeMessageCheckError, DisputeStatus,
-	SignedDisputeStatement, Timestamp,
+	disputes::ValidCandidateVotes, CandidateVotes, DisputeMessage, DisputeMessageCheckError,
+	DisputeStatus, SignedDisputeStatement, Timestamp,
 };
 use polkadot_node_subsystem::{
 	messages::{
@@ -1024,7 +1024,7 @@ impl Initialized {
 			imported_approval_votes = ?import_result.imported_approval_votes(),
 			imported_valid_votes = ?import_result.imported_valid_votes(),
 			imported_invalid_votes = ?import_result.imported_invalid_votes(),
-			total_valid_votes = ?import_result.new_state().votes().valid.len(),
+			total_valid_votes = ?import_result.new_state().votes().valid.raw().len(),
 			total_invalid_votes = ?import_result.new_state().votes().invalid.len(),
 			confirmed = ?import_result.new_state().is_confirmed(),
 			"Import summary"
@@ -1099,7 +1099,7 @@ impl Initialized {
 			.map(CandidateVotes::from)
 			.unwrap_or_else(|| CandidateVotes {
 				candidate_receipt: candidate_receipt.clone(),
-				valid: BTreeMap::new(),
+				valid: ValidCandidateVotes::new(),
 				invalid: BTreeMap::new(),
 			});
 
@@ -1272,8 +1272,12 @@ fn make_dispute_message(
 			.map_err(|()| DisputeMessageCreationError::InvalidStoredStatement)?;
 			(our_vote, our_index, other_vote, *validator_index)
 		} else {
-			let (validator_index, (statement_kind, validator_signature)) =
-				votes.valid.iter().next().ok_or(DisputeMessageCreationError::NoOppositeVote)?;
+			let (validator_index, (statement_kind, validator_signature)) = votes
+				.valid
+				.raw()
+				.iter()
+				.next()
+				.ok_or(DisputeMessageCreationError::NoOppositeVote)?;
 			let other_vote = SignedDisputeStatement::new_checked(
 				DisputeStatement::Valid(*statement_kind),
 				*our_vote.candidate_hash(),

diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs
@@ -706,7 +706,7 @@ fn too_many_unconfirmed_statements_are_considered_spam() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 1);
+				assert_eq!(votes.valid.raw().len(), 1);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -839,8 +839,11 @@ fn approval_vote_import_works() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
-				assert!(votes.valid.get(&ValidatorIndex(4)).is_some(), "Approval vote is missing!");
+				assert_eq!(votes.valid.raw().len(), 2);
+				assert!(
+					votes.valid.raw().get(&ValidatorIndex(4)).is_some(),
+					"Approval vote is missing!"
+				);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -964,7 +967,7 @@ fn dispute_gets_confirmed_via_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -999,7 +1002,7 @@ fn dispute_gets_confirmed_via_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 1);
+				assert_eq!(votes.valid.raw().len(), 1);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -1132,7 +1135,7 @@ fn dispute_gets_confirmed_at_byzantine_threshold() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert_eq!(votes.invalid.len(), 2);
 			}
 
@@ -1167,7 +1170,7 @@ fn dispute_gets_confirmed_at_byzantine_threshold() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 1);
+				assert_eq!(votes.valid.raw().len(), 1);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -1246,7 +1249,7 @@ fn backing_statements_import_works_and_no_spam() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert_eq!(votes.invalid.len(), 0);
 			}
 
@@ -1394,7 +1397,7 @@ fn conflicting_votes_lead_to_dispute_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -1421,7 +1424,7 @@ fn conflicting_votes_lead_to_dispute_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert_eq!(votes.invalid.len(), 2);
 			}
 
@@ -1505,7 +1508,7 @@ fn positive_votes_dont_trigger_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 1);
+				assert_eq!(votes.valid.raw().len(), 1);
 				assert!(votes.invalid.is_empty());
 			}
 
@@ -1541,7 +1544,7 @@ fn positive_votes_dont_trigger_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2);
+				assert_eq!(votes.valid.raw().len(), 2);
 				assert!(votes.invalid.is_empty());
 			}
 
@@ -3075,7 +3078,7 @@ fn refrain_from_participation() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 1);
+				assert_eq!(votes.valid.raw().len(), 1);
 				assert_eq!(votes.invalid.len(), 1);
 			}
 
@@ -3183,7 +3186,7 @@ fn participation_for_included_candidates() {
 					.await;
 
 				let (_, _, votes) = rx.await.unwrap().get(0).unwrap().clone();
-				assert_eq!(votes.valid.len(), 2); // 2 => we have participated
+				assert_eq!(votes.valid.raw().len(), 2); // 2 => we have participated
 				assert_eq!(votes.invalid.len(), 1);
 			}
 

diff --git a/node/core/provisioner/src/disputes/prioritized_selection/mod.rs b/node/core/provisioner/src/disputes/prioritized_selection/mod.rs
@@ -216,7 +216,7 @@ where
 
 		// Check if votes are within the limit
 		for (session_index, candidate_hash, selected_votes) in votes {
-			let votes_len = selected_votes.valid.len() + selected_votes.invalid.len();
+			let votes_len = selected_votes.valid.raw().len() + selected_votes.invalid.len();
 			if votes_len + total_votes_len > MAX_DISPUTE_VOTES_FORWARDED_TO_RUNTIME {
 				// we are done - no more votes can be added
 				return result

diff --git a/node/core/provisioner/src/disputes/prioritized_selection/tests.rs b/node/core/provisioner/src/disputes/prioritized_selection/tests.rs
@@ -393,7 +393,9 @@ impl TestDisputes {
 					ValidDisputeStatementKind::Explicit,
 					0,
 					local_votes_count,
-				),
+				)
+				.into_iter()
+				.collect(),
 				invalid: BTreeMap::new(),
 			},
 		);

diff --git a/node/network/dispute-distribution/src/sender/mod.rs b/node/network/dispute-distribution/src/sender/mod.rs
@@ -279,7 +279,7 @@ impl DisputeSender {
 			Some(votes) => votes,
 		};
 
-		let our_valid_vote = votes.valid.get(&our_index);
+		let our_valid_vote = votes.valid.raw().get(&our_index);
 
 		let our_invalid_vote = votes.invalid.get(&our_index);
 
@@ -291,7 +291,7 @@ impl DisputeSender {
 		} else if let Some(our_invalid_vote) = our_invalid_vote {
 			// Get some valid vote as well:
 			let valid_vote =
-				votes.valid.iter().next().ok_or(JfyiError::MissingVotesFromCoordinator)?;
+				votes.valid.raw().iter().next().ok_or(JfyiError::MissingVotesFromCoordinator)?;
 			(valid_vote, (&our_index, our_invalid_vote))
 		} else {
 			// There is no vote from us yet - nothing to do.