secondary filtering (#4783)

* filter again if it's the first statement and spam slots were applied

* Update runtime/parachains/src/disputes.rs

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

* fixins

* add a proper test case, simplify some code

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

polkadot/runtime/parachains/src/disputes.rs

@@ -516,14 +516,19 @@ pub mod pallet {
 bitflags::bitflags! {
 	#[derive(Default)]
 	struct DisputeStateFlags: u8 {
+		/// The byzantine threshold of `f + 1` votes (and hence participating validators) was reached.
 		const CONFIRMED = 0b0001;
+		/// Is the supermajority for validity of the dispute reached.
 		const FOR_SUPERMAJORITY = 0b0010;
+		/// Is the supermajority against the validity of the block reached.
 		const AGAINST_SUPERMAJORITY = 0b0100;
 	}
 }
 
 impl DisputeStateFlags {
 	fn from_state<BlockNumber>(state: &DisputeState<BlockNumber>) -> Self {
+		// Only correct since `DisputeState` is _always_ initialized
+		// with the validator set based on the session.
 		let n = state.validators_for.len();
 
 		let byzantine_threshold = byzantine_threshold(n);
@@ -549,18 +554,20 @@ impl DisputeStateFlags {
 
 #[derive(PartialEq, RuntimeDebug)]
 enum SpamSlotChange {
+	/// Add a `+1` to the spam slot for a particular validator index in this session.
 	Inc,
+	/// Subtract `-1` ...
 	Dec,
 }
 
 struct ImportSummary<BlockNumber> {
-	// The new state, with all votes imported.
+	/// The new state, with all votes imported.
 	state: DisputeState<BlockNumber>,
-	// Changes to spam slots. Validator index paired with directional change.
+	/// Changes to spam slots. Validator index paired with directional change.
 	spam_slot_changes: Vec<(ValidatorIndex, SpamSlotChange)>,
-	// Validators to slash for being (wrongly) on the AGAINST side.
+	/// Validators to slash for being (wrongly) on the AGAINST side.
 	slash_against: Vec<ValidatorIndex>,
-	// Validators to slash for being (wrongly) on the FOR side.
+	/// Validators to slash for being (wrongly) on the FOR side.
 	slash_for: Vec<ValidatorIndex>,
 	// New participants in the dispute.
 	new_participants: bitvec::vec::BitVec<u8, BitOrderLsb0>,
@@ -570,7 +577,9 @@ struct ImportSummary<BlockNumber> {
 
 #[derive(RuntimeDebug, PartialEq, Eq)]
 enum VoteImportError {
+	/// Validator index was outside the range of valid validator indices in the given session.
 	ValidatorIndexOutOfBounds,
+	/// Found a duplicate statement in the dispute statement set.
 	DuplicateStatement,
 }
 
@@ -583,10 +592,15 @@ impl<T: Config> From<VoteImportError> for Error<T> {
 	}
 }
 
+/// A transport statement bit change for a single validator.
 #[derive(RuntimeDebug, PartialEq, Eq)]
 struct ImportUndo {
+	/// The validator index to which to associate the statement import.
 	validator_index: ValidatorIndex,
+	/// The direction of the vote, for block validity (`true`) or invalidity (`false`).
 	valid: bool,
+	/// Has the validator participated before, i.e. in backing or
+	/// with an oposing vote.
 	new_participant: bool,
 }
 
@@ -632,9 +646,9 @@ impl<BlockNumber: Clone> DisputeStateImporter<BlockNumber> {
 
 		bits.set(validator.0 as usize, true);
 
-		// New participants tracks those which didn't appear on either
-		// side of the dispute until now. So we check the other side
-		// and checked the first side before.
+		// New participants tracks those validators by index, which didn't appear on either
+		// side of the dispute until now (so they make a first appearance).
+		// To verify this we need to assure they also were not part of the opposing side before.
 		if other_bits.get(validator.0 as usize).map_or(false, |b| !*b) {
 			undo.new_participant = true;
 			self.new_participants.set(validator.0 as usize, true);
@@ -643,6 +657,7 @@ impl<BlockNumber: Clone> DisputeStateImporter<BlockNumber> {
 		Ok(undo)
 	}
 
+	/// Revert a done transaction.
 	fn undo(&mut self, undo: ImportUndo) {
 		if undo.valid {
 			self.state.validators_for.set(undo.validator_index.0 as usize, false);
@@ -655,6 +670,7 @@ impl<BlockNumber: Clone> DisputeStateImporter<BlockNumber> {
 		}
 	}
 
+	/// Collect all dispute votes.
 	fn finish(mut self) -> ImportSummary<BlockNumber> {
 		let pre_flags = self.pre_flags;
 		let post_flags = DisputeStateFlags::from_state(&self.state);
@@ -681,8 +697,12 @@ impl<BlockNumber: Clone> DisputeStateImporter<BlockNumber> {
 					.map(|i| (ValidatorIndex(i as _), SpamSlotChange::Dec))
 					.collect()
 			},
-			(true, true) | (true, false) => {
-				// nothing to do. (true, false) is also impossible.
+			(true, false) => {
+				log::error!("Dispute statements are never removed. This is a bug");
+				Vec::new()
+			},
+			(true, true) => {
+				// No change, nothing to do.
 				Vec::new()
 			},
 		};
@@ -925,27 +945,32 @@ impl<T: Config> Pallet<T> {
 		let n_validators = session_info.validators.len();
 
 		// Check for ancient.
-		let dispute_state = {
+		let (first_votes, 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
+				(false, dispute_state)
 			} else {
-				DisputeState {
-					validators_for: bitvec![u8, BitOrderLsb0; 0; n_validators],
-					validators_against: bitvec![u8, BitOrderLsb0; 0; n_validators],
-					start: now,
-					concluded_at: None,
-				}
+				// No state in storage, this indicates it's the first dispute statement set as well.
+				(
+					true,
+					DisputeState {
+						validators_for: bitvec![u8, BitOrderLsb0; 0; n_validators],
+						validators_against: bitvec![u8, BitOrderLsb0; 0; n_validators],
+						start: now,
+						concluded_at: None,
+					},
+				)
 			}
 		};
 
 		// Check and import all votes.
-		let summary = {
+		let mut summary = {
 			let mut importer = DisputeStateImporter::new(dispute_state, now);
 			for (i, (statement, validator_index, signature)) in set.statements.iter().enumerate() {
+				// assure the validator index and is present in the session info
 				let validator_public = match session_info.validators.get(validator_index.0 as usize)
 				{
 					None => {
@@ -1005,7 +1030,7 @@ impl<T: Config> Pallet<T> {
 		if !is_local {
 			let mut spam_slots: Vec<u32> =
 				SpamSlots::<T>::get(&set.session).unwrap_or_else(|| vec![0; n_validators]);
-
+			let mut spam_filter_struck = false;
 			for (validator_index, spam_slot_change) in summary.spam_slot_changes {
 				let spam_slot = spam_slots
 					.get_mut(validator_index.0 as usize)
@@ -1013,11 +1038,13 @@ impl<T: Config> Pallet<T> {
 
 				if let SpamSlotChange::Inc = spam_slot_change {
 					if *spam_slot >= max_spam_slots {
+						spam_filter_struck = true;
+
 						// 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)
+							.position(|(_statement, v_i, _signature)| &validator_index == v_i)
 							.expect(
 								"spam slots are only incremented when a new statement \
 									from a validator is included; qed",
@@ -1032,6 +1059,10 @@ impl<T: Config> Pallet<T> {
 						// 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);
+
+						// Removing individual statments can cause the dispute to become onesided.
+						// Checking that (again) is done after the loop. Remove the bit indices.
+						summary.new_participants.set(validator_index.0 as _, false);
 					}
 
 					// It's also worth noting that the `DisputeStateImporter`
@@ -1057,6 +1088,36 @@ impl<T: Config> Pallet<T> {
 			// 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);
+
+			// This is only relevant in cases where it's the first vote and the state
+			// would hence hold a onesided dispute. If a onesided dispute can never be
+			// started, by induction, we can never enter a state of a one sided dispute.
+			if spam_filter_struck && first_votes {
+				let mut vote_for_count = 0_u64;
+				let mut vote_against_count = 0_u64;
+				// Since this is the first set of statements for the dispute,
+				// it's sufficient to count the votes in the statement set after they
+				set.statements.iter().for_each(|(statement, v_i, _signature)| {
+					if Some(true) ==
+						summary.new_participants.get(v_i.0 as usize).map(|b| b.as_ref().clone())
+					{
+						match statement {
+							// `summary.new_flags` contains the spam free votes.
+							// Note that this does not distinguish between pro or con votes,
+							// since allowing both of them, even if the spam threshold would be reached
+							// is a good thing.
+							// Overflow of the counters is no concern, disputes are limited by weight.
+							DisputeStatement::Valid(_) => vote_for_count += 1,
+							DisputeStatement::Invalid(_) => vote_against_count += 1,
+						}
+					}
+				});
+				if vote_for_count.is_zero() || vote_against_count.is_zero() {
+					// It wasn't one-sided before the spam filters, but now it is,
+					// so we need to be thorough and not import that dispute.
+					return StatementSetFilter::RemoveAll
+				}
+			}
 		}
 
 		filter

polkadot/runtime/parachains/src/disputes/tests.rs

@@ -24,6 +24,7 @@ use crate::{
 		REWARD_VALIDATORS,
 	},
 };
+use assert_matches::assert_matches;
 use frame_support::{
 	assert_err, assert_noop, assert_ok,
 	traits::{OnFinalize, OnInitialize},
@@ -288,6 +289,217 @@ fn test_import_slash_against() {
 	assert_eq!(summary.new_flags, DisputeStateFlags::FOR_SUPERMAJORITY);
 }
 
+fn generate_dispute_statement_set_entry(
+	session: u32,
+	candidate_hash: CandidateHash,
+	statement: DisputeStatement,
+	validator: &<ValidatorId as CryptoType>::Pair,
+) -> (DisputeStatement, ValidatorSignature) {
+	let valid = match &statement {
+		DisputeStatement::Valid(_) => true,
+		_ => false,
+	};
+	let signature_bytes = validator
+		.sign(&ExplicitDisputeStatement { valid, candidate_hash, session }.signing_payload());
+	let signature = ValidatorSignature::try_from(signature_bytes).unwrap();
+	(statement, signature)
+}
+
+fn generate_dispute_statement_set(
+	session: SessionIndex,
+	candidate_hash: CandidateHash,
+	validators: &[<ValidatorId as CryptoType>::Pair],
+	vidxs: Vec<(usize, DisputeStatement)>,
+) -> DisputeStatementSet {
+	let statements = vidxs
+		.into_iter()
+		.map(|(v_i, statement)| {
+			let validator_index = ValidatorIndex(v_i as u32);
+			let (statement, signature) = generate_dispute_statement_set_entry(
+				session,
+				candidate_hash.clone(),
+				statement,
+				&validators[v_i],
+			);
+			(statement, validator_index, signature)
+		})
+		.collect::<Vec<(DisputeStatement, ValidatorIndex, ValidatorSignature)>>();
+	DisputeStatementSet { candidate_hash: candidate_hash.clone(), session, statements }
+}
+
+#[test]
+fn dispute_statement_becoming_onesided_due_to_spamslots_is_accepted() {
+	let dispute_conclusion_by_time_out_period = 3;
+	let start = 10;
+	let session = start - 1;
+	let dispute_max_spam_slots = 2;
+	let post_conclusion_acceptance_period = 3;
+
+	let mock_genesis_config = MockGenesisConfig {
+		configuration: crate::configuration::GenesisConfig {
+			config: HostConfiguration {
+				dispute_conclusion_by_time_out_period,
+				dispute_max_spam_slots,
+				..Default::default()
+			},
+			..Default::default()
+		},
+		..Default::default()
+	};
+
+	new_test_ext(mock_genesis_config).execute_with(|| {
+		// We need 6 validators for the byzantine threshold to be 2
+		static ACCOUNT_IDS: &[AccountId] = &[0, 1, 2, 3, 4, 5, 6, 7];
+		let validators = std::iter::repeat(())
+			.take(7)
+			.map(|_| <ValidatorId as CryptoType>::Pair::generate().0)
+			.collect::<Vec<_>>();
+		let validators = &validators;
+
+		// a new session at each block, but always the same validators
+		let session_change_callback = |block_number: u32| -> Option<NewSession<'_>> {
+			let session_validators =
+				Vec::from_iter(ACCOUNT_IDS.iter().zip(validators.iter().map(|pair| pair.public())));
+			Some((true, block_number, session_validators.clone(), Some(session_validators)))
+		};
+
+		run_to_block(start, session_change_callback);
+
+		// Must be _foreign_ parachain candidate
+		// otherwise slots do not trigger.
+		let candidate_hash_a = CandidateHash(sp_core::H256::repeat_byte(0xA));
+		let candidate_hash_b = CandidateHash(sp_core::H256::repeat_byte(0xB));
+		let candidate_hash_c = CandidateHash(sp_core::H256::repeat_byte(0xC));
+		let candidate_hash_d = CandidateHash(sp_core::H256::repeat_byte(0xD));
+
+		let stmts = vec![
+			// a
+			generate_dispute_statement_set(
+				session,
+				candidate_hash_a,
+				validators,
+				vec![
+					(3, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)),
+					(6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)),
+				],
+			),
+			// b
+			generate_dispute_statement_set(
+				session,
+				candidate_hash_b,
+				validators,
+				vec![
+					(1, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)),
+					(6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)),
+				],
+			),
+			// c
+			generate_dispute_statement_set(
+				session,
+				candidate_hash_c,
+				validators,
+				vec![
+					(2, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)),
+					(6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)),
+				],
+			),
+			// d
+			generate_dispute_statement_set(
+				session,
+				candidate_hash_d,
+				validators,
+				vec![
+					(4, DisputeStatement::Valid(ValidDisputeStatementKind::Explicit)),
+					(5, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)),
+				],
+			),
+			generate_dispute_statement_set(
+				session,
+				candidate_hash_d,
+				validators,
+				vec![(6, DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit))],
+			),
+		];
+
+		// no filtering happens, host config for `dispute_max_spam_slots: 2` is the default
+		// updates spam slots implicitly
+		let set = stmts[0].clone();
+		let filter = Pallet::<Test>::filter_dispute_data(
+			&set,
+			post_conclusion_acceptance_period,
+			dispute_max_spam_slots,
+			VerifyDisputeSignatures::Skip,
+		);
+		assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty());
+		assert_matches!(filter.filter_statement_set(set.clone()), Some(modified) => {
+			assert_eq!(&set, modified.as_ref());
+		});
+		assert_eq!(SpamSlots::<Test>::get(session), Some(vec![0, 0, 0, 1, 0, 0, 1]));
+
+		// <----->
+
+		// 2nd, still ok? Should be
+		let set = stmts[1].clone();
+		let filter = Pallet::<Test>::filter_dispute_data(
+			&set,
+			post_conclusion_acceptance_period,
+			dispute_max_spam_slots,
+			VerifyDisputeSignatures::Skip,
+		);
+		assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty());
+		assert_matches!(filter.filter_statement_set(set.clone()), Some(modified) => {
+			assert_eq!(&set, modified.as_ref());
+		});
+		assert_eq!(SpamSlots::<Test>::get(session), Some(vec![0, 1, 0, 1, 0, 0, 2]));
+
+		// <----->
+
+		// now this is the third spammy participation of validator 6 and hence
+		let set = stmts[2].clone();
+		let filter = Pallet::<Test>::filter_dispute_data(
+			&set,
+			post_conclusion_acceptance_period,
+			dispute_max_spam_slots,
+			VerifyDisputeSignatures::Skip,
+		);
+		assert_matches!(&filter, StatementSetFilter::RemoveAll);
+		// no need to apply the filter,
+		// we don't do anything with the result, and spam slots were updated already
+
+		// <----->
+
+		// now there is no pariticipation in this dispute being initiated
+		// only validator 4 and 5 are part of it
+		// with 3 validators it's not a an unconfirmed dispute anymore
+		// so validator 6, while being considered spammy should work again
+		let set = stmts[3].clone();
+		let filter = Pallet::<Test>::filter_dispute_data(
+			&set,
+			post_conclusion_acceptance_period,
+			dispute_max_spam_slots,
+			VerifyDisputeSignatures::Skip,
+		);
+		assert_matches!(&filter, StatementSetFilter::RemoveIndices(v) if v.is_empty());
+		// no need to apply the filter,
+		// we don't do anything with the result, and spam slots were updated already
+
+		// <----->
+
+		// it's a spammy participant, so a new dispute will not be accepted being initiated by a spammer
+		let set = stmts[4].clone();
+		let filter = Pallet::<Test>::filter_dispute_data(
+			&set,
+			post_conclusion_acceptance_period,
+			dispute_max_spam_slots,
+			VerifyDisputeSignatures::Skip,
+		);
+		assert_matches!(&filter, StatementSetFilter::RemoveAll);
+		assert_matches!(filter.filter_statement_set(set.clone()), None);
+
+		assert_eq!(SpamSlots::<Test>::get(session), Some(vec![0, 1, 1, 1, 1, 1, 3]));
+	});
+}
+
 // Test that punish_inconclusive is correctly called.
 #[test]
 fn test_initializer_initialize() {