Make sure to preserve backing votes (#6382)

* Guide updates

* Consider more dead forks.

* Ensure backing votes don't get overridden.

* Fix spelling.

* Fix comments.

* Update node/primitives/src/lib.rs

Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>

Co-authored-by: eskimor <eskimor@no-such-url.com>
Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>

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()
-}

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(),

node/core/dispute-coordinator/src/scraping/mod.rs

@@ -19,7 +19,7 @@ use std::num::NonZeroUsize;
 use futures::channel::oneshot;
 use lru::LruCache;
 
-use polkadot_node_primitives::MAX_FINALITY_LAG;
+use polkadot_node_primitives::{DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION, MAX_FINALITY_LAG};
 use polkadot_node_subsystem::{
 	messages::ChainApiMessage, overseer, ActivatedLeaf, ActiveLeavesUpdate, ChainApiError,
 	SubsystemSender,
@@ -65,7 +65,7 @@ const LRU_OBSERVED_BLOCKS_CAPACITY: NonZeroUsize = match NonZeroUsize::new(20) {
 /// With this information it provides a `CandidateComparator` and as a return value of
 /// `process_active_leaves_update` any scraped votes.
 ///
-/// Scraped candidates are available `CANDIDATE_LIFETIME_AFTER_FINALIZATION` more blocks
+/// Scraped candidates are available `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` more blocks
 /// after finalization as a precaution not to prune them prematurely.
 pub struct ChainScraper {
 	/// All candidates we have seen included, which not yet have been finalized.
@@ -87,16 +87,6 @@ impl ChainScraper {
 	/// As long as we have `MAX_FINALITY_LAG` this makes sense as a value.
 	pub(crate) const ANCESTRY_SIZE_LIMIT: u32 = MAX_FINALITY_LAG;
 
-	/// How many blocks after finalization a backed/included candidate should be kept.
-	/// We don't want to remove scraped candidates on finalization because we want to
-	/// be sure that disputes will conclude on abandoned forks.
-	/// Removing the candidate on finalization creates a possibility for an attacker to
-	/// avoid slashing. If a bad fork is abandoned too quickly because in the same another
-	/// better one gets finalized the entries for the bad fork will be pruned and we
-	/// will never participate in a dispute for it. We want such disputes to conclude
-	/// in a timely manner so that the offenders are slashed.
-	pub(crate) const CANDIDATE_LIFETIME_AFTER_FINALIZATION: BlockNumber = 2;
-
 	/// Create a properly initialized `OrderingProvider`.
 	///
 	/// Returns: `Self` and any scraped votes.
@@ -175,12 +165,13 @@ impl ChainScraper {
 
 	/// Prune finalized candidates.
 	///
-	/// We keep each candidate for `CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks after finalization.
+	/// We keep each candidate for `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks after finalization.
 	/// After that we treat it as low priority.
 	pub fn process_finalized_block(&mut self, finalized_block_number: &BlockNumber) {
-		// `CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1` because `finalized_block_number`counts to the
+		// `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1` because `finalized_block_number`counts to the
 		// candidate lifetime.
-		match finalized_block_number.checked_sub(Self::CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1) {
+		match finalized_block_number.checked_sub(DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION - 1)
+		{
 			Some(key_to_prune) => {
 				self.backed_candidates.remove_up_to_height(&key_to_prune);
 				self.included_candidates.remove_up_to_height(&key_to_prune);

node/core/dispute-coordinator/src/scraping/tests.rs

@@ -23,6 +23,7 @@ use parity_scale_codec::Encode;
 use sp_core::testing::TaskExecutor;
 
 use ::test_helpers::{dummy_collator, dummy_collator_signature, dummy_hash};
+use polkadot_node_primitives::DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 use polkadot_node_subsystem::{
 	jaeger,
 	messages::{
@@ -452,9 +453,9 @@ fn scraper_prunes_finalized_candidates() {
 
 		let candidate = make_candidate_receipt(get_block_number_hash(TEST_TARGET_BLOCK_NUMBER));
 
-		// After `CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the candidate should be removed
+		// After `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` blocks the candidate should be removed
 		finalized_block_number =
-			TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		assert!(!scraper.is_candidate_backed(&candidate.hash()));
@@ -512,10 +513,10 @@ fn scraper_handles_backed_but_not_included_candidate() {
 		// The candidate should be removed.
 		assert!(
 			finalized_block_number <
-				TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION
+				TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION
 		);
 		finalized_block_number +=
-			TEST_TARGET_BLOCK_NUMBER + ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			TEST_TARGET_BLOCK_NUMBER + DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		assert!(!scraper.is_candidate_included(&candidate.hash()));
@@ -562,7 +563,7 @@ fn scraper_handles_the_same_candidate_incuded_in_two_different_block_heights() {
 		// Finalize blocks to enforce pruning of scraped events.
 		// The magic candidate was added twice, so it shouldn't be removed if we finalize two more blocks.
 		finalized_block_number = test_targets.first().expect("there are two block nums") +
-			ChainScraper::CANDIDATE_LIFETIME_AFTER_FINALIZATION;
+			DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION;
 		process_finalized_block(&mut scraper, &finalized_block_number);
 
 		let magic_candidate = make_candidate_receipt(get_magic_candidate_hash());

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);
 			}
 

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

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(),
 			},
 		);