filter backing votes for disputes concluded in the past

polkadot/runtime/parachains/src/builder.rs

@@ -53,7 +53,7 @@ fn mock_validation_code() -> ValidationCode {
 ///
 /// This is directly from frame-benchmarking. Copy/pasted so we can use it when not compiling with
 /// "features = runtime-benchmarks".
-fn account<AccountId: Decode>(name: &'static str, index: u32, seed: u32) -> AccountId {
+pub fn account<AccountId: Decode>(name: &'static str, index: u32, seed: u32) -> AccountId {
 	let entropy = (name, index, seed).using_encoded(sp_io::hashing::blake2_256);
 	AccountId::decode(&mut TrailingZeroInput::new(&entropy[..]))
 		.expect("infinite input; no invalid input; qed")

polkadot/runtime/parachains/src/disputes.rs

@@ -41,7 +41,7 @@ use sp_runtime::{
 
 #[cfg(test)]
 #[allow(unused_imports)]
-pub(crate) use self::tests::run_to_block;
+pub(crate) use self::tests::{make_dispute_concluding_against, run_to_block};
 
 pub mod slashing;
 #[cfg(test)]
@@ -230,6 +230,9 @@ pub trait DisputesHandler<BlockNumber: Ord> {
 		included_in: BlockNumber,
 	);
 
+	/// Get a list of disputes in a session that concluded invalid.
+	fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet<CandidateHash>;
+
 	/// Retrieve the included state of a given candidate in a particular session. If it
 	/// returns `Some`, then we have a local dispute for the given `candidate_hash`.
 	fn included_state(session: SessionIndex, candidate_hash: CandidateHash) -> Option<BlockNumber>;
@@ -272,6 +275,10 @@ impl<BlockNumber: Ord> DisputesHandler<BlockNumber> for () {
 		Ok(Vec::new())
 	}
 
+	fn disputes_concluded_invalid(_session: SessionIndex) -> BTreeSet<CandidateHash> {
+		BTreeSet::new()
+	}
+
 	fn note_included(
 		_session: SessionIndex,
 		_candidate_hash: CandidateHash,
@@ -321,6 +328,10 @@ where
 		pallet::Pallet::<T>::process_checked_multi_dispute_data(statement_sets)
 	}
 
+	fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet<CandidateHash> {
+		pallet::Pallet::<T>::disputes_concluded_invalid(session)
+	}
+
 	fn note_included(
 		session: SessionIndex,
 		candidate_hash: CandidateHash,
@@ -556,7 +567,7 @@ enum VoteImportError {
 	MaliciousBacker,
 }
 
-#[derive(RuntimeDebug, Copy, Clone, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 enum VoteKind {
 	/// A backing vote that is counted as "for" vote in dispute resolution.
 	Backing,
@@ -949,7 +960,10 @@ impl<T: Config> Pallet<T> {
 		// Load session info to access validators
 		let session_info = match session_info::Sessions::<T>::get(set.session) {
 			Some(s) => s,
-			None => return StatementSetFilter::RemoveAll,
+			None => {
+				log::debug!(target: LOG_TARGET, "SessionInfo not found: {}", set.session);
+				return StatementSetFilter::RemoveAll
+			},
 		};
 
 		let config = configuration::ActiveConfig::<T>::get();
@@ -960,6 +974,7 @@ impl<T: Config> Pallet<T> {
 		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) {
+					log::debug!(target: LOG_TARGET, "Dispute is ancient: {:?}", set.candidate_hash);
 					return StatementSetFilter::RemoveAll
 				}
 
@@ -987,6 +1002,7 @@ impl<T: Config> Pallet<T> {
 				let validator_public = match session_info.validators.get(*validator_index) {
 					None => {
 						filter.remove_index(i);
+						log::debug!(target: LOG_TARGET, "Invalid validator index in a dispute: {}", validator_index.0);
 						continue
 					},
 					Some(v) => v,
@@ -998,6 +1014,7 @@ impl<T: Config> Pallet<T> {
 					Ok(u) => u,
 					Err(_) => {
 						filter.remove_index(i);
+						log::debug!(target: LOG_TARGET, "Import of a dispute failed for validator: {}", validator_index.0);
 						continue
 					},
 				};
@@ -1023,7 +1040,7 @@ impl<T: Config> Pallet<T> {
 					// config.
 					config.approval_voting_params.max_approval_coalesce_count > 1,
 				) {
-					log::warn!("Failed to check dispute signature");
+					log::warn!(target: LOG_TARGET, "Failed to check dispute signature");
 
 					importer.undo(undo);
 					filter.remove_index(i);
@@ -1038,13 +1055,15 @@ impl<T: Config> Pallet<T> {
 		if summary.state.validators_for.count_ones() == 0 ||
 			summary.state.validators_against.count_ones() == 0
 		{
+			log::debug!(target: LOG_TARGET, "Rejected one-sided dispute");
 			return StatementSetFilter::RemoveAll
 		}
 
 		// Reject disputes containing less votes than needed for confirmation.
 		if (summary.state.validators_for.clone() | &summary.state.validators_against).count_ones() <=
 			byzantine_threshold(summary.state.validators_for.len())
 		{
+			log::debug!(target: LOG_TARGET, "Rejected unconfirmed dispute");
 			return StatementSetFilter::RemoveAll
 		}
 
@@ -1216,12 +1235,13 @@ impl<T: Config> Pallet<T> {
 		let revert_to = included_in - One::one();
 
 		Included::<T>::insert(&session, &candidate_hash, revert_to);
+	}
 
-		if let Some(state) = Disputes::<T>::get(&session, candidate_hash) {
-			if has_supermajority_against(&state) {
-				Self::revert_and_freeze(revert_to);
-			}
-		}
+	pub(crate) fn disputes_concluded_invalid(session: SessionIndex) -> BTreeSet<CandidateHash> {
+		Disputes::<T>::iter_prefix(session)
+			.filter(|(_hash, state)| has_supermajority_against(state))
+			.map(|(hash, _state)| hash)
+			.collect()
 	}
 
 	pub(crate) fn included_state(

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

@@ -31,6 +31,7 @@ use frame_support::{
 use frame_system::pallet_prelude::BlockNumberFor;
 use polkadot_primitives::BlockNumber;
 use sp_core::{crypto::CryptoType, Pair};
+use sp_keyring::Sr25519Keyring;
 
 const VOTE_FOR: VoteKind = VoteKind::ExplicitValid;
 const VOTE_AGAINST: VoteKind = VoteKind::Invalid;
@@ -86,6 +87,27 @@ type NewSession<'a> = (
 	Option<Vec<(&'a AccountId, ValidatorId)>>,
 );
 
+pub(crate) fn make_dispute_concluding_against(
+	candidate_hash: CandidateHash,
+	session: SessionIndex,
+	validators: &[(ValidatorIndex, Sr25519Keyring)],
+) -> DisputeStatementSet {
+	let mut statements = Vec::new();
+	for (j, (i, key)) in validators.iter().enumerate() {
+		// make the first statement backing
+		// and the rest against it
+		let statement = if j == 0 {
+			DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded(candidate_hash.0))
+		} else {
+			DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit)
+		};
+		let payload_data = statement.payload_data(candidate_hash, session).unwrap();
+		let signature = key.sign(&payload_data);
+		statements.push((statement, *i, signature.into()));
+	}
+	DisputeStatementSet { candidate_hash, session, statements }
+}
+
 // Run to specific block, while calling disputes pallet hooks manually, because disputes is not
 // integrated in initializer yet.
 pub(crate) fn run_to_block<'a>(
@@ -555,71 +577,6 @@ fn test_disputes_with_missing_backing_votes_are_rejected() {
 	})
 }
 
-#[test]
-fn test_freeze_on_note_included() {
-	new_test_ext(Default::default()).execute_with(|| {
-		let v0 = <ValidatorId as CryptoType>::Pair::generate().0;
-		let v1 = <ValidatorId as CryptoType>::Pair::generate().0;
-
-		run_to_block(6, |b| {
-			// a new session at each block
-			Some((
-				true,
-				b,
-				vec![(&0, v0.public()), (&1, v1.public())],
-				Some(vec![(&0, v0.public()), (&1, v1.public())]),
-			))
-		});
-
-		let candidate_hash = CandidateHash(sp_core::H256::repeat_byte(1));
-		let inclusion_parent = sp_core::H256::repeat_byte(0xff);
-		let session = 3;
-
-		// v0 votes for 3
-		let stmts = vec![DisputeStatementSet {
-			candidate_hash,
-			session: 3,
-			statements: vec![
-				(
-					DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
-					ValidatorIndex(0),
-					v0.sign(
-						&ExplicitDisputeStatement { valid: false, candidate_hash, session: 3 }
-							.signing_payload(),
-					),
-				),
-				(
-					DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit),
-					ValidatorIndex(1),
-					v1.sign(
-						&ExplicitDisputeStatement { valid: false, candidate_hash, session: 3 }
-							.signing_payload(),
-					),
-				),
-				(
-					DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(
-						inclusion_parent,
-					)),
-					ValidatorIndex(1),
-					v0.sign(&CompactStatement::Valid(candidate_hash).signing_payload(
-						&SigningContext { session_index: session, parent_hash: inclusion_parent },
-					)),
-				),
-			],
-		}];
-		assert!(Pallet::<Test>::process_checked_multi_dispute_data(
-			&stmts
-				.into_iter()
-				.map(CheckedDisputeStatementSet::unchecked_from_unchecked)
-				.collect()
-		)
-		.is_ok());
-
-		Pallet::<Test>::note_included(3, candidate_hash, 3);
-		assert_eq!(Frozen::<Test>::get(), Some(2));
-	});
-}
-
 #[test]
 fn test_freeze_provided_against_supermajority_for_included() {
 	new_test_ext(Default::default()).execute_with(|| {

polkadot/runtime/parachains/src/paras_inherent/mod.rs

@@ -424,11 +424,11 @@ impl<T: Config> Pallet<T> {
 
 			METRICS.on_after_filter(all_weight_after.ref_time());
 			log::debug!(
-			target: LOG_TARGET,
-			"[process_inherent_data] after filter: bitfields.len(): {}, backed_candidates.len(): {}, checked_disputes_sets.len() {}",
-			bitfields.len(),
-			backed_candidates.len(),
-			checked_disputes_sets.len()
+				target: LOG_TARGET,
+				"[process_inherent_data] after filter: bitfields.len(): {}, backed_candidates.len(): {}, checked_disputes_sets.len() {}",
+				bitfields.len(),
+				backed_candidates.len(),
+				checked_disputes_sets.len()
 			);
 			log::debug!(target: LOG_TARGET, "Size after filter: {}, candidates + bitfields: {}, disputes: {}", all_weight_after.proof_size(), non_disputes_weight.proof_size(), checked_disputes_sets_consumed_weight.proof_size());
 			log::debug!(target: LOG_TARGET, "Time weight after filter: {}, candidates + bitfields: {}, disputes: {}", all_weight_after.ref_time(), non_disputes_weight.ref_time(), checked_disputes_sets_consumed_weight.ref_time());
@@ -490,26 +490,16 @@ impl<T: Config> Pallet<T> {
 		// Contains the disputes that are concluded in the current session only,
 		// since these are the only ones that are relevant for the occupied cores
 		// and lightens the load on `free_disputed` significantly.
-		// Cores can't be occupied with candidates of the previous sessions, and only
-		// things with new votes can have just concluded. We only need to collect
-		// cores with disputes that conclude just now, because disputes that
-		// concluded longer ago have already had any corresponding cores cleaned up.
-		let current_concluded_invalid_disputes = checked_disputes_sets
-			.iter()
-			.map(AsRef::as_ref)
-			.filter(|dss| dss.session == current_session)
-			.map(|dss| (dss.session, dss.candidate_hash))
-			.filter(|(session, candidate)| {
-				<T>::DisputesHandler::concluded_invalid(*session, *candidate)
-			})
-			.map(|(_session, candidate)| candidate)
-			.collect::<BTreeSet<CandidateHash>>();
+		let mut current_concluded_invalid_disputes =
+			<T>::DisputesHandler::disputes_concluded_invalid(current_session);
 
 		// Get the cores freed as a result of concluded invalid candidates.
 		let (freed_disputed, concluded_invalid_hashes): (Vec<CoreIndex>, BTreeSet<CandidateHash>) =
 			inclusion::Pallet::<T>::free_disputed(&current_concluded_invalid_disputes)
 				.into_iter()
 				.unzip();
+		// Also include descendants of the concluded invalid candidates.
+		current_concluded_invalid_disputes.extend(concluded_invalid_hashes);
 
 		// Create a bit index from the set of core indices where each index corresponds to
 		// a core index that was freed due to a dispute.
@@ -583,7 +573,7 @@ impl<T: Config> Pallet<T> {
 		let backed_candidates_with_core = sanitize_backed_candidates::<T>(
 			backed_candidates,
 			&allowed_relay_parents,
-			concluded_invalid_hashes,
+			current_concluded_invalid_disputes,
 			eligible,
 			core_index_enabled,
 		);
@@ -1080,8 +1070,11 @@ fn limit_and_sanitize_disputes<
 			if max_consumable_weight.all_gte(updated) {
 				// Always apply the weight. Invalid data cost processing time too:
 				weight_acc = updated;
+				let candidate_hash = dss.candidate_hash;
 				if let Some(checked) = dispute_statement_set_valid(dss) {
 					checked_acc.push(checked);
+				} else {
+					log::debug!(target: LOG_TARGET, "Filtered dispute: {:?}", candidate_hash);
 				}
 			}
 		});