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>

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

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

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

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

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

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

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

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

polkadot/node/primitives/src/disputes/mod.rs

@@ -14,7 +14,10 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::{
+	btree_map::{Entry as Bentry, Keys as Bkeys},
+	BTreeMap, BTreeSet,
+};
 
 use parity_scale_codec::{Decode, Encode};
 
@@ -49,7 +52,7 @@ pub struct CandidateVotes {
 	/// The receipt of the candidate itself.
 	pub candidate_receipt: CandidateReceipt,
 	/// Votes of validity, sorted by validator index.
-	pub valid: BTreeMap<ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)>,
+	pub valid: ValidCandidateVotes,
 	/// Votes of invalidity, sorted by validator index.
 	pub invalid: BTreeMap<ValidatorIndex, (InvalidDisputeStatementKind, ValidatorSignature)>,
 }
@@ -69,6 +72,99 @@ impl CandidateVotes {
 	}
 }
 
+#[derive(Debug, Clone)]
+/// Valid candidate votes.
+///
+/// Prefere backing votes over other votes.
+pub struct ValidCandidateVotes {
+	votes: BTreeMap<ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)>,
+}
+
+impl ValidCandidateVotes {
+	/// Create new empty `ValidCandidateVotes`
+	pub fn new() -> Self {
+		Self { votes: BTreeMap::new() }
+	}
+	/// Insert a vote, replacing any already existing vote.
+	///
+	/// Except, for backing votes: Backing votes are always kept, and will never get overridden.
+	/// Import of other king of `valid` votes, will be ignored if a backing vote is already
+	/// present. Any already existing `valid` vote, will be overridden by any given backing vote.
+	///
+	/// Returns: true, if the insert had any effect.
+	pub fn insert_vote(
+		&mut self,
+		validator_index: ValidatorIndex,
+		kind: ValidDisputeStatementKind,
+		sig: ValidatorSignature,
+	) -> bool {
+		match self.votes.entry(validator_index) {
+			Bentry::Vacant(vacant) => {
+				vacant.insert((kind, sig));
+				true
+			},
+			Bentry::Occupied(mut occupied) => match occupied.get().0 {
+				ValidDisputeStatementKind::BackingValid(_) |
+				ValidDisputeStatementKind::BackingSeconded(_) => false,
+				ValidDisputeStatementKind::Explicit |
+				ValidDisputeStatementKind::ApprovalChecking => {
+					occupied.insert((kind, sig));
+					kind != occupied.get().0
+				},
+			},
+		}
+	}
+
+	/// Retain any votes that match the given criteria.
+	pub fn retain<F>(&mut self, f: F)
+	where
+		F: FnMut(&ValidatorIndex, &mut (ValidDisputeStatementKind, ValidatorSignature)) -> bool,
+	{
+		self.votes.retain(f)
+	}
+
+	/// Get all the validator indeces we have votes for.
+	pub fn keys(
+		&self,
+	) -> Bkeys<'_, ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)> {
+		self.votes.keys()
+	}
+
+	/// Get read only direct access to underlying map.
+	pub fn raw(
+		&self,
+	) -> &BTreeMap<ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)> {
+		&self.votes
+	}
+}
+
+impl FromIterator<(ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature))>
+	for ValidCandidateVotes
+{
+	fn from_iter<T>(iter: T) -> Self
+	where
+		T: IntoIterator<Item = (ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature))>,
+	{
+		Self { votes: BTreeMap::from_iter(iter) }
+	}
+}
+
+impl From<ValidCandidateVotes>
+	for BTreeMap<ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)>
+{
+	fn from(wrapped: ValidCandidateVotes) -> Self {
+		wrapped.votes
+	}
+}
+impl IntoIterator for ValidCandidateVotes {
+	type Item = (ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature));
+	type IntoIter = <BTreeMap<ValidatorIndex, (ValidDisputeStatementKind, ValidatorSignature)> as IntoIterator>::IntoIter;
+
+	fn into_iter(self) -> Self::IntoIter {
+		self.votes.into_iter()
+	}
+}
+
 impl SignedDisputeStatement {
 	/// Create a new `SignedDisputeStatement` from information
 	/// that is available on-chain, and hence already can be trusted.

polkadot/node/primitives/src/lib.rs

@@ -30,10 +30,10 @@ use parity_scale_codec::{Decode, Encode, Error as CodecError, Input};
 use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
 
 use polkadot_primitives::v2::{
-	BlakeTwo256, CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt,
-	CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId, OutboundHrmpMessage,
-	PersistedValidationData, SessionIndex, Signed, UncheckedSigned, UpwardMessage, ValidationCode,
-	ValidatorIndex, MAX_CODE_SIZE, MAX_POV_SIZE,
+	BlakeTwo256, BlockNumber, CandidateCommitments, CandidateHash, CollatorPair,
+	CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId,
+	OutboundHrmpMessage, PersistedValidationData, SessionIndex, Signed, UncheckedSigned,
+	UpwardMessage, ValidationCode, ValidatorIndex, MAX_CODE_SIZE, MAX_POV_SIZE,
 };
 pub use sp_consensus_babe::{
 	AllowedSlots as BabeAllowedSlots, BabeEpochConfiguration, Epoch as BabeEpoch,
@@ -73,8 +73,28 @@ pub const BACKING_EXECUTION_TIMEOUT: Duration = Duration::from_secs(2);
 /// ensure that in the absence of extremely large disparities between hardware,
 /// blocks that pass backing are considered executable by approval checkers or
 /// dispute participants.
+///
+/// NOTE: If this value is increased significantly, also check the dispute coordinator to consider
+/// candidates longer into finalization: `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION`.
 pub const APPROVAL_EXECUTION_TIMEOUT: Duration = Duration::from_secs(12);
 
+/// How many blocks after finalization an information about 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 another
+/// better one gets finalized the entries for the bad fork will be pruned and we
+/// might never participate in a dispute for it.
+///
+/// This value should consider the timeout we allow for participation in approval-voting. In
+/// particular, the following condition should hold:
+///
+/// slot time * `DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION` > `APPROVAL_EXECUTION_TIMEOUT`
+/// + slot time
+pub const DISPUTE_CANDIDATE_LIFETIME_AFTER_FINALIZATION: BlockNumber = 10;
+
 /// Linked to `MAX_FINALITY_LAG` in relay chain selection,
 /// `MAX_HEADS_LOOK_BACK` in `approval-voting` and
 /// `MAX_BATCH_SCRAPE_ANCESTORS` in `dispute-coordinator`

polkadot/roadmap/implementers-guide/src/node/disputes/dispute-coordinator.md

@@ -9,12 +9,14 @@ In particular the dispute-coordinator is responsible for:
 
 - Ensuring that the node is able to raise a dispute in case an invalid candidate
   is found during approval checking.
-- Ensuring lazy approval votes (votes given without running the parachain
-  validation function) will be recorded, so lazy nodes can get slashed properly.
+- Ensuring approval votes will be recorded.
 - Coordinating actual participation in a dispute, ensuring that the node
   participates in any justified dispute in a way that ensures resolution of
   disputes on the network even in the case of many disputes raised (flood/DoS
   scenario).
+- Ensuring disputes resolve, even for candidates on abandoned forks as much as
+  reasonably possible, to rule out "free tries" and thus guarantee our gambler's
+  ruin property.
 - Provide an API for chain selection, so we can prevent finalization of any
   chain which has included candidates for which a dispute is either ongoing or
   concluded invalid and avoid building on chains with an included invalid
@@ -22,6 +24,8 @@ In particular the dispute-coordinator is responsible for:
 - Provide an API for retrieving (resolved) disputes, including all votes, both
   implicit (approval, backing) and explicit dispute votes. So validators can get
   rewarded/slashed accordingly.
+- Ensure backing votes are recorded and will never get overridden by explicit
+  votes.
 
 ## Ensuring That Disputes Can Be Raised
 
@@ -76,32 +80,38 @@ inefficient process. (Quadratic complexity adds up, with 35 votes in total per c
 Approval votes are very relevant nonetheless as we are going to see in the next
 section.
 
-## Ensuring Lazy Approval Votes Will Be Recorded
+## Ensuring approval votes will be recorded
 
 ### Ensuring Recording
 
-While there is no need to record approval votes in the dispute coordinator
-preemptively, we do need to make sure they are recorded when a dispute
-actually happens. This is because only votes recorded by the dispute
-coordinator will be considered for slashing. It is sufficient for our
-threat model that malicious backers are slashed as opposed to both backers and
-approval checkers. However, we still must import approval votes from the approvals
-process into the disputes process to ensure that lazy approval checkers
-actually run the parachain validation function. Slashing lazy approval checkers is necessary, else we risk a useless approvals process where every approval
-checker blindly votes valid for every candidate. If we did not import approval
-votes, lazy nodes would likely cast a properly checked explicit vote as part
-of the dispute in addition to their blind approval vote and thus avoid a slash.
-With the 2/3rd honest assumption it seems unrealistic that lazy approval voters
-will keep sending unchecked approval votes once they became aware of a raised
-dispute. Hence the most crucial approval votes to import are the early ones
-(tranche 0), to take into account network latencies and such we still want to
-import approval votes at a later point in time as well (in particular we need
-to make sure the dispute can conclude, but more on that later).
+Only votes recorded by the dispute coordinator will be considered for slashing.
 
-As mentioned already previously, importing votes is most efficient when batched.
-At the same time approval voting and disputes are running concurrently so
-approval votes are expected to trickle in still, when a dispute is already
-ongoing.
+While there is no need to record approval votes in the dispute coordinator
+preemptively, we make some effort to have any in approval-voting received
+approval votes recorded when a dispute actually happens:
+
+This is not required for concluding the dispute, as nodes send their own vote
+anyway (either explicit valid or their existing approval-vote). What nodes can
+do though, is participating in approval-voting, casting a vote, but later when a
+dispute is raised reconsider their vote and send an explicit invalid vote. If
+they managed to only have that one recorded, then they could avoid a slash.
+
+This is not a problem for our basic security assumptions: The backers are the
+ones to be supposed to have skin in the game, so we are not too woried about
+colluding approval voters getting away slash free as the gambler's ruin property
+is maintained anyway. There is however a separate problem, from colluding
+approval-voters, that is "lazy" approval voters. If it were easy and reliable
+for approval-voters to reconsider their vote, in case of an actual dispute, then
+they don't have a direct incentive (apart from playing a part in securing the
+network) to properly run the validation function at all - they could just always
+vote "valid" totally risk free. (While they would alwasy risk a slash by voting
+invalid.)
+
+
+So we do want to fetch approval votes from approval-voting. Importing votes is
+most efficient when batched. At the same time approval voting and disputes are
+running concurrently so approval votes are expected to trickle in still, when a
+dispute is already ongoing.
 
 Hence, we have the following requirements for importing approval votes:
 
@@ -112,12 +122,12 @@ Hence, we have the following requirements for importing approval votes:
    already running.
 
 With a design where approval voting sends votes to the dispute-coordinator by
-itself, we would need to make approval voting aware of ongoing disputes and
-once it is aware it could start sending all already existing votes batched and
+itself, we would need to make approval voting aware of ongoing disputes and once
+it is aware it could start sending all already existing votes batched and
 trickling in votes as they come. The problem with this is, that it adds some
 unnecessary complexity to approval-voting and also we might still import most of
-the votes unbatched, but one-by-one, depending on what point in time the dispute
-was raised.
+the votes unbatched one-by-one, depending on what point in time the dispute was
+raised.
 
 Instead of the dispute coordinator informing approval-voting of an ongoing
 dispute for it to begin forwarding votes to the dispute coordinator, it makes
@@ -126,14 +136,11 @@ candidates in dispute. This way, the dispute coordinator can also pick the best
 time for maximizing the number of votes in the batch.
 
 Now the question remains, when should the dispute coordinator ask
-approval-voting for votes? As argued above already, querying approval votes at
-the beginning of the dispute, will likely already take care of most malicious
-votes. Still we would like to have a record of all, if possible. So what are
-other points in time we might query approval votes?
+approval-voting for votes?
 
 In fact for slashing it is only relevant to have them once the dispute
 concluded, so we can query approval voting the moment the dispute concludes!
-There are two potential caveats with this though:
+Two concerns that come to mind, are easily addressed:
 
 1. Timing: We would like to rely as little as possible on implementation details
    of approval voting. In particular, if the dispute is ongoing for a long time,
@@ -153,31 +160,12 @@ There are two potential caveats with this though:
    chain can no longer be finalized (some other/better fork already has been).
    This second problem can somehow be mitigated by also importing votes as soon
    as a dispute is detected, but not fully resolved. It is still inherently
-   racy. The problem can be solved in at least two ways: Either go back to full
-   eager import of approval votes into the dispute-coordinator in some more
-   efficient manner or by changing requirements on approval-voting, making it
-   hold on votes longer than necessary for approval-voting itself. Conceptually
-   both solutions are equivalent, as we make sure votes are available even
-   without an ongoing dispute. For now, in the interest of time we punt on this
-   issue: If nodes import votes as soon as a dispute is raised in addition to
-   when it concludes, we have a good chance of getting relevant votes and even
-   if not, the fundamental security properties will still hold: Backers are
-   getting slashed, therefore gambler's ruin is maintained. We would still like
-   to fix this at [some
-   point](https://github.com/paritytech/polkadot/issues/5864).
-2. There could be a chicken and egg problem: If we wait for approval vote import
-   for the dispute to conclude, we would run into a problem if we needed those
-   approval votes to get enough votes to conclude the dispute. Luckily it turns
-   out that this is not quite true or at least can be made not true easily: As
-   already mentioned, approval voting and disputes are running concurrently, but
-   not only that, they race with each other! A node might simultaneously start
-   participating in a dispute via the dispute coordinator, due to learning about
-   a dispute via dispute-distribution, while also participating in approval
-   voting. By distributing our own approval vote we make sure the dispute can
-   conclude regardless how the race ended (we either participate explicitly
-   anyway or we sent our already present approval vote). By importing all
-   approval votes we make it possible to slash lazy approval voters, even
-   if they also cast an invalid explicit vote.
+   racy. The good thing is, this should be good enough: We are worried about
+   lazy approval checkers, the system does not need to be perfect. It should be
+   enough if there is some risk of getting caught.
+2. We are not worried about the dispute not concluding, as nodes will always
+   send their own vote, regardless of it being an explict or an already existing
+   approval-vote.
 
 Conclusion: As long as we make sure, if our own approval vote gets imported
 (which would prevent dispute participation) to also distribute it via
@@ -186,28 +174,27 @@ approval-voting deleting votes we will import approval votes twice during a
 dispute: Once when it is raised, to make as sure as possible to see approval
 votes also for abandoned forks and second when the dispute concludes, to
 maximize the amount of potentially malicious approval votes to be recorded. The
-raciness obviously is not fully resolved by this, [a
-ticket](https://github.com/paritytech/polkadot/issues/5864) exists.
+raciness obviously is not fully resolved by this, but this is fine as argued
+above.
 
 Ensuring vote import on chain is covered in the next section.
 
-As already touched: Honest nodes
-will likely validate twice, once in approval voting and once via
-dispute-participation. Avoiding that does not really seem worthwhile though, as
-disputes are for one exceptional, so a little wasted effort won't affect
-everyday performance - second, even with eager importing of approval votes,
-those doubled work is still present as disputes and approvals are racing. Every
-time participation is faster than approval, a node would do double work.
+What we don't care about is that honest approval-voters will likely validate
+twice, once in approval voting and once via dispute-participation. Avoiding that
+does not really seem worthwhile though, as disputes are for one exceptional, so
+a little wasted effort won't affect everyday performance - second, even with
+eager importing of approval votes, those doubled work is still present as
+disputes and approvals are racing. Every time participation is faster than
+approval, a node would do double work.
 
 ### Ensuring Chain Import
 
 While in the previous section we discussed means for nodes to ensure relevant
 votes are recorded so lazy approval checkers get slashed properly, it is crucial
 to also discuss the actual chain import. Only if we guarantee that recorded votes
-will also get imported on chain (on all potential chains really) we will succeed
+will get imported on chain (on all potential chains really) we will succeed
 in executing slashes. Particularly we need to make sure backing votes end up on
-chain consistantly. In contrast recording and slashing lazy approval voters only
-needs to be likely, not certain.
+chain consistently.
 
 Dispute distribution will make sure all explicit dispute votes get distributed
 among nodes which includes current block producers (current authority set) which
@@ -309,7 +296,7 @@ to send out a thousand tiny network messages by just sending out a single
 garbage message, is still a significant amplification and is nothing to ignore -
 this could absolutely be used to cause harm!
 
-#### Participation
+### Participation
 
 As explained, just blindly participating in any "dispute" that comes along is
 not a good idea. First we would like to make sure the dispute is actually
@@ -336,7 +323,7 @@ participation at all on any _vote import_ if any of the following holds true:
   honest node that already voted, so the dispute must be genuine.
 
 Note: A node might be out of sync with the chain and we might only learn about a
-block including a candidate, after we learned about the dispute. This means, we
+block, including a candidate, after we learned about the dispute. This means, we
 have to re-evaluate participation decisions on block import!
 
 With this, nodes won't waste significant resources on completely made up
@@ -348,7 +335,7 @@ obviously only need to worry about order if participations start queuing up.
 
 We treat participation for candidates that we have seen included with priority
 and put them on a priority queue which sorts participation based on the block
-number of the relay parent of that candidate and for candidates with the same
+number of the relay parent of the candidate and for candidates with the same
 relay parent height further by the `CandidateHash`. This ordering is globally
 unique and also prioritizes older candidates.
 
@@ -359,38 +346,57 @@ times instead of just once to the oldest offender. This is obviously a good
 idea, in particular it makes it impossible for an attacker to prevent rolling
 back a very old candidate, by keeping raising disputes for newer candidates.
 
-For candidates we have not seen included, but we know are backed (thanks to chain
-scraping) or we have seen a dispute with 1/3+1 participation (confirmed dispute)
-on them - we put participation on a best-effort queue. It has got the same
-ordering as the priority one - by block heights of the relay parent, older blocks
-are with priority. There is a possibility not to be able to obtain the block number
-of the parent when we are inserting the dispute in the queue. The reason for this
-is either the dispute is completely made up or we are out of sync with the other
-nodes in terms of last finalized block. The former is very unlikely. If we are
-adding a dispute in best-effort it should already be either confirmed or the
-candidate is backed. In the latter case we will promote the dispute to the
-priority queue once we learn about the new block. NOTE: this is still work in
-progress and is tracked by [this issue]
-(https://github.com/paritytech/polkadot/issues/5875).
+For candidates we have not seen included, but we know are backed (thanks to
+chain scraping) or we have seen a dispute with 1/3+1 participation (confirmed
+dispute) on them - we put participation on a best-effort queue. It has got the
+same ordering as the priority one - by block heights of the relay parent, older
+blocks are with priority. There is a possibility not to be able to obtain the
+block number of the parent when we are inserting the dispute in the queue. To
+account for races, we will promote any existing participation request to the
+priority queue once we learn about an including block. NOTE: this is still work
+in progress and is tracked by [this
+issue](https://github.com/paritytech/polkadot/issues/5875).
 
-#### Import
+### Abandoned Forks
 
-In the last section we looked at how to treat queuing participations to handle
-heavy dispute load well. This already ensures, that honest nodes won't amplify
-cheap DoS attacks. There is one minor issue remaining: Even if we delay
+Finalization: As mentioned we care about included and backed candidates on any
+non-finalized chain, given that any disputed chain will not get finalized, we
+don't need to care about finalized blocks, but what about forks that fall behind
+the finalized chain in terms of block number? For those we would still like to
+be able to participate in any raised disputes, otherwise attackers might be able
+to avoid a slash if they manage to create a better fork after they learned about
+the approval checkers. Therefore we do care about those forks even after they
+have fallen behind the finalized chain.
+
+For simplicity we also care about the actual finalized chain (not just forks) up
+to a certain depth. We do have to limit the depth, because otherwise we open a
+DoS vector again. The depth (into the finalized chain) should be oriented on the
+approval-voting execution timeout, in particular it should be significantly
+larger. Otherwise by the time the execution is allowed to finish, we already
+dropped information about those candidates and the dispute could not conclude.
+
+## Import
+
+### Spam Considerations
+
+In the last section we looked at how to treat queuing participations to
+handle heavy dispute load well. This already ensures, that honest nodes won't
+amplify cheap DoS attacks. There is one minor issue remaining: Even if we delay
 participation until we have some confirmation of the authenticity of the
-dispute, we should also not blindly import all votes arriving into the
-database as this might be used to just slowly fill up disk space, until the node
-is no longer functional. This leads to our last protection mechanism at the
-dispute coordinator level (dispute-distribution also has its own), which is spam
-slots. For each import, where we don't know whether it might be spam or not we
-increment a counter for each signing participant of explicit `invalid` votes.
+dispute, we should also not blindly import all votes arriving into the database
+as this might be used to just slowly fill up disk space, until the node is no
+longer functional. This leads to our last protection mechanism at the dispute
+coordinator level (dispute-distribution also has its own), which is spam slots.
+For each import containing an invalid vote, where we don't know whether it might
+be spam or not we increment a counter for each signing participant of explicit
+`invalid` votes.
 
 What votes do we treat as a potential spam? A vote will increase a spam slot if
-and only if all of the following condidions are satisfied:
-* the candidate under dispute is not included on any chain
+and only if all of the following conditions are satisfied:
+
+* the candidate under dispute was not seen included nor backed on any chain
 * the dispute is not confirmed
-* we haven't casted a vote for the dispute
+* we haven't cast a vote for the dispute
 
 The reason this works is because we only need to worry about actual dispute
 votes. Import of backing votes are already rate limited and concern only real
@@ -401,6 +407,17 @@ an explicit `invalid` vote in the import. Only a third of the validators can be
 malicious, so spam disk usage is limited to `2*vote_size*n/3*NUM_SPAM_SLOTS`, with
 `n` being the number of validators.
 
+### Backing Votes
+
+Backing votes are in some way special. For starters they are the only valid
+votes that are guaranteed to exist for any valid dispute to be raised. Second
+they are the only votes that commit to a shorter execution timeout
+`BACKING_EXECUTION_TIMEOUT`, compared to a more lenient timeout used in approval
+voting. To account properly for execution time variance across machines,
+slashing might treat backing votes differently (more aggressively) than other
+voting `valid` votes. Hence in import we shall never override a backing vote
+with another valid vote. They can not be assumed to be interchangeable.
+
 ## Attacks & Considerations
 
 The following attacks on the priority queue and best-effort queues are