Dispute vote filtering for block authors (#3498)

* guide: filter_multi_dispute_data

* guide: elaborate

* Implementation of dispute data filtering

* tests for filtering

* don't use std, you fool!

* use swap_remove

* Update runtime/parachains/src/disputes.rs

Co-authored-by: Andronik Ordian <write@reusable.software>

* use btreeste

* address API nit

Co-authored-by: Andronik Ordian <write@reusable.software>

primitives/src/v1/mod.rs

@@ -1124,6 +1124,22 @@ impl DisputeStatement {
 			Err(())
 		}
 	}
+
+	/// Whether the statement indicates validity.
+	pub fn indicates_validity(&self) -> bool {
+		match *self {
+			DisputeStatement::Valid(_) => true,
+			DisputeStatement::Invalid(_) => false,
+		}
+	}
+
+	/// Whether the statement indicates invalidity.
+	pub fn indicates_invalidity(&self) -> bool {
+		match *self {
+			DisputeStatement::Valid(_) => false,
+			DisputeStatement::Invalid(_) => true,
+		}
+	}
 }
 
 /// Different kinds of statements of validity on  a candidate.

roadmap/implementers-guide/src/runtime/disputes.md

@@ -66,6 +66,13 @@ Frozen: Option<BlockNumber>,
 
 ## Routines
 
+* `filter_multi_dispute_data(MultiDisputeStatementSet) -> MultiDisputeStatementSet`:
+  1. Takes a `MultiDisputeStatementSet` and filters it down to a `MultiDisputeStatementSet`
+    that satisfies all the criteria of `provide_multi_dispute_data`. That is, eliminating
+    ancient votes, votes which overwhelm the maximum amount of spam slots, and duplicates. 
+    This can be used by block authors to create the final submission in a block which is 
+    guaranteed to pass the `provide_multi_dispute_data` checks.
+
 * `provide_multi_dispute_data(MultiDisputeStatementSet) -> Vec<(SessionIndex, Hash)>`:
   1. Pass on each dispute statement set to `provide_dispute_data`, propagating failure.
   1. Return a list of all candidates who just had disputes initiated.

runtime/parachains/src/disputes.rs

@@ -17,6 +17,7 @@
 //! Runtime component for handling disputes of parachain candidates.
 
 use sp_std::prelude::*;
+use sp_std::collections::btree_set::BTreeSet;
 use primitives::v1::{
 	byzantine_threshold, supermajority_threshold, ApprovalVote, CandidateHash, CompactStatement,
 	ConsensusLog, DisputeState, DisputeStatement, DisputeStatementSet, ExplicitDisputeStatement,
@@ -176,11 +177,7 @@ impl<T: Config> DisputesHandler<T::BlockNumber> for pallet::Pallet<T> {
 	}
 
 	fn filter_multi_dispute_data(statement_sets: &mut MultiDisputeStatementSet) {
-		// TODO: filter duplicate and ancient dispute statements. For now, don't import anything
-		// because there will be redundancies.
-		//
-		// https://github.com/paritytech/polkadot/issues/3472
-		statement_sets.clear();
+		pallet::Pallet::<T>::filter_multi_dispute_data(statement_sets)
 	}
 
 	fn provide_multi_dispute_data(
@@ -516,6 +513,48 @@ impl<BlockNumber: Clone> DisputeStateImporter<BlockNumber> {
 	}
 }
 
+// A filter on a dispute statement set.
+#[derive(PartialEq)]
+#[cfg_attr(test, derive(Debug))]
+enum StatementSetFilter {
+	// Remove the entire dispute statement set.
+	RemoveAll,
+	// Remove the votes with given index from the statement set.
+	RemoveIndices(Vec<usize>),
+}
+
+impl StatementSetFilter {
+	fn filter_statement_set(self, mut statement_set: DisputeStatementSet)
+		-> Option<DisputeStatementSet>
+	{
+		match self {
+			StatementSetFilter::RemoveAll => None,
+			StatementSetFilter::RemoveIndices(mut indices) => {
+				indices.sort();
+				indices.dedup();
+
+				// reverse order ensures correctness
+				for index in indices.into_iter().rev() {
+					// swap_remove guarantees linear complexity.
+					statement_set.statements.swap_remove(index);
+				}
+
+				if statement_set.statements.is_empty() {
+					None
+				} else {
+					Some(statement_set)
+				}
+			}
+		}
+	}
+
+	fn remove_index(&mut self, i: usize) {
+		if let StatementSetFilter::RemoveIndices(ref mut indices) = *self {
+			indices.push(i)
+		}
+	}
+}
+
 impl<T: Config> Pallet<T> {
 	/// Called by the initializer to initialize the disputes module.
 	pub(crate) fn initializer_initialize(now: T::BlockNumber) -> Weight {
@@ -644,6 +683,173 @@ impl<T: Config> Pallet<T> {
 		Ok(fresh)
 	}
 
+	fn filter_multi_dispute_data(statement_sets: &mut MultiDisputeStatementSet) {
+		let config = <configuration::Pallet<T>>::config();
+
+		let old_statement_sets = sp_std::mem::take(statement_sets);
+
+		// Deduplicate.
+		let dedup_iter = {
+			let mut targets = BTreeSet::new();
+			old_statement_sets.into_iter().filter(move |set| {
+				let target = (set.candidate_hash, set.session);
+				targets.insert(target)
+			})
+		};
+
+		*statement_sets = dedup_iter.filter_map(|set| {
+			let filter = Self::filter_dispute_data(&config, &set);
+
+			filter.filter_statement_set(set)
+		}).collect();
+	}
+
+	// Given a statement set, this produces a filter to be applied to the statement set.
+	// It either removes the entire dispute statement set or some specific votes from it.
+	//
+	// Votes which are duplicate or already known by the chain are filtered out.
+	// The entire set is removed if the dispute is ancient or concluded.
+	fn filter_dispute_data(config: &HostConfiguration<T::BlockNumber>, set: &DisputeStatementSet)
+		-> StatementSetFilter
+	{
+		let mut filter = StatementSetFilter::RemoveIndices(Vec::new());
+
+		// Dispute statement sets on any dispute which concluded
+		// before this point are to be rejected.
+		let now = <frame_system::Pallet<T>>::block_number();
+		let oldest_accepted = now.saturating_sub(config.dispute_post_conclusion_acceptance_period);
+
+		// Load session info to access validators
+		let session_info = match <session_info::Pallet<T>>::session_info(set.session) {
+			Some(s) => s,
+			None => return StatementSetFilter::RemoveAll,
+		};
+
+		let n_validators = session_info.validators.len();
+
+		// Check for ancient.
+		let dispute_state = {
+			if let Some(dispute_state) = <Disputes<T>>::get(&set.session, &set.candidate_hash) {
+				if dispute_state.concluded_at.as_ref().map_or(false, |c| c < &oldest_accepted) {
+					return StatementSetFilter::RemoveAll;
+				}
+
+				dispute_state
+			} else {
+				DisputeState {
+					validators_for: bitvec![BitOrderLsb0, u8; 0; n_validators],
+					validators_against: bitvec![BitOrderLsb0, u8; 0; n_validators],
+					start: now,
+					concluded_at: None,
+				}
+			}
+		};
+
+		// Check and import all votes.
+		let summary = {
+			let mut importer = DisputeStateImporter::new(dispute_state, now);
+			for (i, (statement, validator_index, signature))
+				in set.statements.iter().enumerate()
+			{
+				let validator_public = match session_info
+					.validators.get(validator_index.0 as usize)
+				{
+					None => {
+						filter.remove_index(i);
+						continue
+					}
+					Some(v) => v,
+				};
+
+				let valid = statement.indicates_validity();
+
+				if let Err(_) = importer.import(*validator_index, valid) {
+					filter.remove_index(i);
+					continue
+				}
+
+				// Check signature after attempting import.
+				//
+				// Since we expect that this filter will be applied to
+				// disputes long after they're concluded, 99% of the time,
+				// the duplicate filter above will catch them before needing
+				// to do a heavy signature check.
+				//
+				// This is only really important until the post-conclusion acceptance threshold
+				// is reached, and then no part of this loop will be hit.
+				if let Err(()) = check_signature(
+					&validator_public,
+					set.candidate_hash,
+					set.session,
+					statement,
+					signature,
+				) {
+					filter.remove_index(i);
+					continue
+				}
+			}
+
+			importer.finish()
+		};
+
+		// Apply spam slot changes. Bail early if too many occupied.
+		let is_local = <Included<T>>::contains_key(&set.session, &set.candidate_hash);
+		if !is_local {
+			let mut spam_slots: Vec<u32> = SpamSlots::<T>::get(&set.session)
+				.unwrap_or_else(|| vec![0; n_validators]);
+
+			for (validator_index, spam_slot_change) in summary.spam_slot_changes {
+				let spam_slot = spam_slots.get_mut(validator_index.0 as usize)
+					.expect("index is in-bounds, as checked above; qed");
+
+				if let SpamSlotChange::Inc = spam_slot_change {
+					if *spam_slot >= config.dispute_max_spam_slots {
+						// Find the vote by this validator and filter it out.
+						let first_index_in_set = set.statements
+							.iter()
+							.position(|(_, v_i, _)| &validator_index == v_i)
+							.expect("spam slots are only incremented when a new statement \
+									from a validator is included; qed");
+
+						// Note that there may be many votes by the validator in the statement
+						// set. There are not supposed to be, but the purpose of this function
+						// is to filter out invalid submissions, after all.
+						//
+						// This is fine - we only need to handle the first one, because all
+						// subsequent votes' indices have been added to the filter already
+						// by the duplicate checks above. It's only the first one which
+						// may not already have been filtered out.
+						filter.remove_index(first_index_in_set);
+					}
+
+					// It's also worth noting that the `DisputeStateImporter`
+					// which produces these spam slot updates only produces
+					// one spam slot update per validator because it rejects
+					// duplicate votes.
+					//
+					// So we don't need to worry about spam slots being
+					// updated incorrectly after receiving duplicates.
+					*spam_slot += 1;
+				} else {
+					*spam_slot = spam_slot.saturating_sub(1);
+				}
+			}
+
+			// We write the spam slots here because sequential calls to
+			// `filter_dispute_data` have a dependency on each other.
+			//
+			// For example, if a validator V occupies 1 spam slot and
+			// max is 2, then 2 sequential calls incrementing spam slot
+			// cannot be allowed.
+			//
+			// However, 3 sequential calls, where the first increments,
+			// the second decrements, and the third increments would be allowed.
+			SpamSlots::<T>::insert(&set.session, spam_slots);
+		}
+
+		filter
+	}
+
 	/// Handle a set of dispute statements corresponding to a single candidate.
 	///
 	/// Fails if the dispute data is invalid. Returns a boolean indicating whether the
@@ -702,10 +908,7 @@ impl<T: Config> Pallet<T> {
 					signature,
 				).map_err(|()| Error::<T>::InvalidSignature)?;
 
-				let valid = match statement {
-					DisputeStatement::Valid(_) => true,
-					DisputeStatement::Invalid(_) => false,
-				};
+				let valid = statement.indicates_validity();
 
 				importer.import(*validator_index, valid).map_err(Error::<T>::from)?;
 			}
@@ -2078,4 +2281,429 @@ mod tests {
 		assert!(check_signature(&validator_id.public(), candidate_hash, session, &statement_3, &signed_5).is_err());
 		assert!(check_signature(&validator_id.public(), candidate_hash, session, &statement_4, &signed_5).is_err());
 	}
+
+	#[test]
+	fn filter_removes_duplicates_within_set() {
+		new_test_ext(Default::default()).execute_with(|| {
+			let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
+
+			run_to_block(
+				3,
+				|b| {
+					// a new session at each block
+					Some((
+						true,
+						b,
+						vec![(&0, v0.public())],
+						Some(vec![(&0, v0.public())]),
+					))
+				}
+			);
+
+			let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1));
+
+			let payload = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let sig_a = v0.sign(&payload);
+			let sig_b = v0.sign(&payload);
+			let sig_c = v0.sign(&payload);
+
+			let mut statements = vec![
+				DisputeStatementSet {
+					candidate_hash: candidate_hash.clone(),
+					session: 1,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_a.clone(),
+						),
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_b,
+						),
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_c,
+						),
+					]
+				}
+			];
+
+			Pallet::<Test>::filter_multi_dispute_data(&mut statements);
+
+			assert_eq!(
+				statements,
+				vec![
+					DisputeStatementSet {
+						candidate_hash: candidate_hash.clone(),
+						session: 1,
+						statements: vec![
+							(
+								DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+								ValidatorIndex(0),
+								sig_a,
+							),
+						]
+					}
+				]
+			)
+		})
+	}
+
+	#[test]
+	fn filter_correctly_accounts_spam_slots() {
+		let dispute_max_spam_slots = 2;
+
+		let mock_genesis_config = MockGenesisConfig {
+			configuration: crate::configuration::GenesisConfig {
+				config: HostConfiguration {
+					dispute_max_spam_slots,
+					.. Default::default()
+				},
+				.. Default::default()
+			},
+			.. Default::default()
+		};
+
+		new_test_ext(mock_genesis_config).execute_with(|| {
+			let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
+			let v1 = <ValidatorId as CryptoType>::Pair::generate().0;
+			let v2 = <ValidatorId as CryptoType>::Pair::generate().0;
+			let v3 = <ValidatorId as CryptoType>::Pair::generate().0;
+
+			run_to_block(
+				3,
+				|b| {
+					// a new session at each block
+					Some((
+						true,
+						b,
+						vec![(&0, v0.public()), (&1, v1.public()), (&2, v2.public()), (&3, v3.public())],
+						Some(vec![(&0, v0.public()), (&1, v1.public()), (&2, v2.public()), (&3, v3.public())]),
+					))
+				}
+			);
+
+			let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1));
+			let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(2));
+			let candidate_hash_c = CandidateHash(sp_core::H256::repeat_byte(3));
+
+			let payload_a = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash_a.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let payload_b = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash_b.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let payload_c = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash_c.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let sig_0a = v0.sign(&payload_a);
+			let sig_0b = v0.sign(&payload_b);
+			let sig_0c = v0.sign(&payload_c);
+
+			let sig_1b = v1.sign(&payload_b);
+
+			let mut statements = vec![
+				DisputeStatementSet {
+					candidate_hash: candidate_hash_a.clone(),
+					session: 1,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_0a.clone(),
+						),
+					]
+				},
+				DisputeStatementSet {
+					candidate_hash: candidate_hash_b.clone(),
+					session: 1,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_0b.clone(),
+						),
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(3),
+							sig_1b.clone(),
+						),
+					]
+				},
+				DisputeStatementSet {
+					candidate_hash: candidate_hash_c.clone(),
+					session: 1,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_0c.clone(),
+						),
+					]
+				},
+			];
+
+			let old_statements = statements.clone();
+			Pallet::<Test>::filter_multi_dispute_data(&mut statements);
+
+			assert_eq!(statements, old_statements);
+		})
+	}
+
+	#[test]
+	fn filter_removes_session_out_of_bounds() {
+		new_test_ext(Default::default()).execute_with(|| {
+			let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
+
+			run_to_block(
+				3,
+				|b| {
+					// a new session at each block
+					Some((
+						true,
+						b,
+						vec![(&0, v0.public())],
+						Some(vec![(&0, v0.public())]),
+					))
+				}
+			);
+
+			let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1));
+
+			let payload = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let sig_a = v0.sign(&payload);
+
+			let mut statements = vec![
+				DisputeStatementSet {
+					candidate_hash: candidate_hash.clone(),
+					session: 100,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),
+							ValidatorIndex(0),
+							sig_a,
+						),
+					]
+				}
+			];
+
+			Pallet::<Test>::filter_multi_dispute_data(&mut statements);
+
+			assert!(statements.is_empty());
+		})
+	}
+
+	#[test]
+	fn filter_removes_concluded_ancient() {
+		let dispute_post_conclusion_acceptance_period = 2;
+
+		let mock_genesis_config = MockGenesisConfig {
+			configuration: crate::configuration::GenesisConfig {
+				config: HostConfiguration {
+					dispute_post_conclusion_acceptance_period,
+					.. Default::default()
+				},
+				.. Default::default()
+			},
+			.. Default::default()
+		};
+
+		new_test_ext(mock_genesis_config).execute_with(|| {
+			let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
+
+			run_to_block(
+				3,
+				|b| {
+					// a new session at each block
+					Some((
+						true,
+						b,
+						vec![(&0, v0.public())],
+						Some(vec![(&0, v0.public())]),
+					))
+				}
+			);
+
+			let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(1));
+			let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(2));
+
+			<Disputes<Test>>::insert(
+				&1,
+				&candidate_hash_a,
+				DisputeState {
+					validators_for: bitvec![BitOrderLsb0, u8; 0; 4],
+					validators_against: bitvec![BitOrderLsb0, u8; 0; 4],
+					start: 0,
+					concluded_at: Some(0),
+				},
+			);
+
+			<Disputes<Test>>::insert(
+				&1,
+				&candidate_hash_b,
+				DisputeState {
+					validators_for: bitvec![BitOrderLsb0, u8; 0; 4],
+					validators_against: bitvec![BitOrderLsb0, u8; 0; 4],
+					start: 0,
+					concluded_at: Some(1),
+				},
+			);
+
+			let payload_a = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash_a.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let payload_b = ExplicitDisputeStatement {
+				valid: true,
+				candidate_hash: candidate_hash_b.clone(),
+				session: 1,
+			}.signing_payload();
+
+			let sig_a = v0.sign(&payload_a);
+			let sig_b = v0.sign(&payload_b);
+
+			let mut statements = vec![
+				DisputeStatementSet {
+					candidate_hash: candidate_hash_a.clone(),
+					session: 1,
+					statements: vec![
+						(
+							DisputeStatement::Valid(ValidDisputeStatementKind::Explicit),