add disabled validators to unwanted mask

Author: ordian

polkadot/node/network/protocol/src/request_response/mod.rs

@@ -248,8 +248,8 @@ impl Protocol {
 				name,
 				fallback_names,
 				max_request_size: 1_000,
-				/// Responses are just confirmation, in essence not even a bit. So 100 seems
-				/// plenty.
+				// Responses are just confirmation, in essence not even a bit. So 100 seems
+				// plenty.
 				max_response_size: 100,
 				request_timeout: DISPUTE_REQUEST_TIMEOUT,
 				inbound_queue: tx,

polkadot/node/network/statement-distribution/src/v2/mod.rs

@@ -596,9 +596,9 @@ pub(crate) async fn handle_active_leaves_update<Context>(
 				?disabled_validators,
 				"Disabled validators detected"
 			);
-		}
 
-		per_session.extend_disabled_validators(disabled_validators);
+			per_session.extend_disabled_validators(disabled_validators);
+		}
 
 		let local_validator = per_session.local_validator.and_then(|v| {
 			find_local_validator_state(
@@ -2778,17 +2778,24 @@ pub(crate) async fn dispatch_requests<Context>(ctx: &mut Context, state: &mut St
 			}
 		}
 
+		// Add disabled validators to the unwanted mask.
+		let disabled_mask = per_session
+			.disabled_bitmask(group_index)
+			.expect("group existence checked above; qed");
+		unwanted_mask.seconded_in_group |= &disabled_mask;
+		unwanted_mask.validated_in_group |= &disabled_mask;
+
 		// don't require a backing threshold for cluster candidates.
 		let require_backing = relay_parent_state.local_validator.as_ref()?.group != group_index;
 
-		Some(RequestProperties {
-			unwanted_mask,
-			backing_threshold: if require_backing {
-				Some(per_session.groups.get_size_and_backing_threshold(group_index)?.1)
-			} else {
-				None
-			},
-		})
+		let backing_threshold = if require_backing {
+			let threshold = per_session.groups.get_size_and_backing_threshold(group_index)?.1;
+			Some(threshold)
+		} else {
+			None
+		};
+
+		Some(RequestProperties { unwanted_mask, backing_threshold })
 	};
 
 	while let Some(request) = state.request_manager.next_request(
@@ -2861,10 +2868,6 @@ pub(crate) async fn handle_response<Context>(
 			Some(g) => g,
 		};
 
-		let disabled_mask = per_session
-			.disabled_bitmask(group_index)
-			.expect("group_index checked above; qed");
-
 		let res = response.validate_response(
 			&mut state.request_manager,
 			group,
@@ -2879,7 +2882,6 @@ pub(crate) async fn handle_response<Context>(
 
 				Some(g_index) == expected_group
 			},
-			disabled_mask,
 		);
 
 		for (peer, rep) in res.reputation_changes {
@@ -2977,6 +2979,14 @@ pub(crate) async fn handle_response<Context>(
 	//    includable.
 }
 
+/// Returns true if the statement filter meets the backing threshold for grid requests.
+pub(crate) fn seconded_and_sufficient(
+	filter: &StatementFilter,
+	backing_threshold: Option<usize>,
+) -> bool {
+	backing_threshold.map_or(true, |t| filter.has_seconded() && filter.backing_validators() >= t)
+}
+
 /// Answer an incoming request for a candidate.
 pub(crate) fn answer_request(state: &mut State, message: ResponderMessage) {
 	let ResponderMessage { request, sent_feedback } = message;
@@ -3017,11 +3027,13 @@ pub(crate) fn answer_request(state: &mut State, message: ResponderMessage) {
 		Some(d) => d,
 	};
 
-	let group_size = per_session
+	let group_index = confirmed.group_index();
+	let group = per_session
 		.groups
-		.get(confirmed.group_index())
-		.expect("group from session's candidate always known; qed")
-		.len();
+		.get(group_index)
+		.expect("group from session's candidate always known; qed");
+
+	let group_size = group.len();
 
 	// check request bitfields are right size.
 	if mask.seconded_in_group.len() != group_size || mask.validated_in_group.len() != group_size {
@@ -3075,17 +3087,56 @@ pub(crate) fn answer_request(state: &mut State, message: ResponderMessage) {
 		validated_in_group: !mask.validated_in_group.clone(),
 	};
 
-	// Ignore disabled validators when sending the response.
-	let disabled_mask = per_session.disabled_bitmask(confirmed.group_index()).unwrap_or_default();
+	// Ignore disabled validators from the latest state when sending the response.
+	let disabled_mask = per_session
+		.disabled_bitmask(confirmed.group_index())
+		.expect("group existence checked; qed");
 	and_mask.mask_seconded(&disabled_mask);
 	and_mask.mask_valid(&disabled_mask);
 
+	let mut sent_filter = StatementFilter::blank(group_size);
 	let statements: Vec<_> = relay_parent_state
 		.statement_store
 		.group_statements(&per_session.groups, confirmed.group_index(), *candidate_hash, &and_mask)
 		.map(|s| s.as_unchecked().clone())
+		.inspect(|s| {
+			let index_in_group = |v: ValidatorIndex| group.iter().position(|x| &v == x);
+			let Some(i) = index_in_group(s.unchecked_validator_index()) else {
+				return
+			};
+
+			match s.unchecked_payload() {
+				CompactStatement::Seconded(_) => {
+					sent_filter.seconded_in_group.set(i, true);
+				},
+				CompactStatement::Valid(_) => {
+					sent_filter.validated_in_group.set(i, true);
+				},
+			}
+		})
 		.collect();
 
+	// There should be no response at all for grid requests when the
+	// backing threshold is no longer met as a result of disabled validators.
+	if !is_cluster {
+		let threshold = per_session
+			.groups
+			.get_size_and_backing_threshold(group_index)
+			.expect("group existence checked above; qed")
+			.1;
+
+		if !seconded_and_sufficient(&sent_filter, Some(threshold)) {
+			gum::info!(
+				target: LOG_TARGET,
+				?candidate_hash,
+				relay_parent = ?confirmed.relay_parent(),
+				?group_index,
+				"Dropping a request from a grid peer because the backing threshold is no longer met."
+			);
+			return
+		}
+	}
+
 	// Update bookkeeping about which statements peers have received.
 	for statement in &statements {
 		if is_cluster {

polkadot/node/network/statement-distribution/src/v2/requests.rs

@@ -30,13 +30,12 @@
 //! (which requires state not owned by the request manager).
 
 use super::{
-	BENEFIT_VALID_RESPONSE, BENEFIT_VALID_STATEMENT, COST_DISABLED_VALIDATOR,
+	seconded_and_sufficient, BENEFIT_VALID_RESPONSE, BENEFIT_VALID_STATEMENT,
 	COST_IMPROPERLY_DECODED_RESPONSE, COST_INVALID_RESPONSE, COST_INVALID_SIGNATURE,
 	COST_UNREQUESTED_RESPONSE_STATEMENT, REQUEST_RETRY_DELAY,
 };
 use crate::LOG_TARGET;
 
-use bitvec::prelude::{BitVec, Lsb0};
 use polkadot_node_network_protocol::{
 	request_response::{
 		outgoing::{Recipient as RequestRecipient, RequestError},
@@ -496,10 +495,6 @@ fn find_request_target_with_update(
 	}
 }
 
-fn seconded_and_sufficient(filter: &StatementFilter, backing_threshold: Option<usize>) -> bool {
-	backing_threshold.map_or(true, |t| filter.has_seconded() && filter.backing_validators() >= t)
-}
-
 /// A response to a request, which has not yet been handled.
 pub struct UnhandledResponse {
 	response: TaggedResponse,
@@ -529,7 +524,6 @@ impl UnhandledResponse {
 	///   * If the response is partially valid, misbehavior by the responding peer.
 	///   * If there are other peers which have advertised the same candidate for different
 	///     relay-parents or para-ids, misbehavior reports for those peers will also be generated.
-	///   * Statements from disabled validators are filtered out and reported as misbehavior.
 	///
 	/// Finally, in the case that the response is either valid or partially valid,
 	/// this will clean up all remaining requests for the candidate in the manager.
@@ -544,7 +538,6 @@ impl UnhandledResponse {
 		session: SessionIndex,
 		validator_key_lookup: impl Fn(ValidatorIndex) -> Option<ValidatorId>,
 		allowed_para_lookup: impl Fn(ParaId, GroupIndex) -> bool,
-		disabled_mask: BitVec<u8, Lsb0>,
 	) -> ResponseValidationOutput {
 		let UnhandledResponse {
 			response: TaggedResponse { identifier, requested_peer, props, response },
@@ -628,7 +621,6 @@ impl UnhandledResponse {
 			session,
 			validator_key_lookup,
 			allowed_para_lookup,
-			disabled_mask,
 		);
 
 		if let CandidateRequestStatus::Complete { .. } = output.request_status {
@@ -648,11 +640,10 @@ fn validate_complete_response(
 	session: SessionIndex,
 	validator_key_lookup: impl Fn(ValidatorIndex) -> Option<ValidatorId>,
 	allowed_para_lookup: impl Fn(ParaId, GroupIndex) -> bool,
-	mut disabled_mask: BitVec<u8, Lsb0>,
 ) -> ResponseValidationOutput {
 	let RequestProperties { backing_threshold, mut unwanted_mask } = props;
 
-	// sanity check bitmasks size. this is based entirely on
+	// sanity check bitmask size. this is based entirely on
 	// local logic here.
 	if !unwanted_mask.has_len(group.len()) {
 		gum::error!(
@@ -665,16 +656,6 @@ fn validate_complete_response(
 		unwanted_mask.seconded_in_group.resize(group.len(), true);
 		unwanted_mask.validated_in_group.resize(group.len(), true);
 	}
-	if disabled_mask.len() != group.len() {
-		gum::error!(
-			target: LOG_TARGET,
-			group_len = group.len(),
-			"Logic bug: group size != disabled bitmask len"
-		);
-
-		// resize and attempt to continue.
-		disabled_mask.resize(group.len(), false);
-	}
 
 	let invalid_candidate_output = || ResponseValidationOutput {
 		request_status: CandidateRequestStatus::Incomplete,
@@ -752,11 +733,6 @@ fn validate_complete_response(
 						rep_changes.push((requested_peer, COST_UNREQUESTED_RESPONSE_STATEMENT));
 						continue
 					}
-
-					if disabled_mask[i] {
-						rep_changes.push((requested_peer, COST_DISABLED_VALIDATOR));
-						continue
-					}
 				},
 				CompactStatement::Valid(_) => {
 					if unwanted_mask.validated_in_group[i] {
@@ -768,11 +744,6 @@ fn validate_complete_response(
 						rep_changes.push((requested_peer, COST_UNREQUESTED_RESPONSE_STATEMENT));
 						continue
 					}
-
-					if disabled_mask[i] {
-						rep_changes.push((requested_peer, COST_DISABLED_VALIDATOR));
-						continue
-					}
 				},
 			}
 
@@ -1038,7 +1009,6 @@ mod tests {
 		let group = &[ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)];
 
 		let unwanted_mask = StatementFilter::blank(group_size);
-		let disabled_mask = unwanted_mask.seconded_in_group.clone();
 		let request_properties = RequestProperties { unwanted_mask, backing_threshold: None };
 
 		// Get requests.
@@ -1082,7 +1052,6 @@ mod tests {
 				0,
 				validator_key_lookup,
 				allowed_para_lookup,
-				disabled_mask.clone(),
 			);
 			assert_eq!(
 				output,
@@ -1121,7 +1090,6 @@ mod tests {
 				0,
 				validator_key_lookup,
 				allowed_para_lookup,
-				disabled_mask,
 			);
 			assert_eq!(
 				output,
@@ -1161,7 +1129,6 @@ mod tests {
 		let group = &[ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)];
 
 		let unwanted_mask = StatementFilter::blank(group_size);
-		let disabled_mask = unwanted_mask.seconded_in_group.clone();
 		let request_properties = RequestProperties { unwanted_mask, backing_threshold: None };
 		let peer_advertised =
 			|_identifier: &CandidateIdentifier, _peer: &_| Some(StatementFilter::full(group_size));
@@ -1202,7 +1169,6 @@ mod tests {
 				0,
 				validator_key_lookup,
 				allowed_para_lookup,
-				disabled_mask,
 			);
 			assert_eq!(
 				output,
@@ -1243,7 +1209,6 @@ mod tests {
 		let group = &[ValidatorIndex(0), ValidatorIndex(1), ValidatorIndex(2)];
 
 		let unwanted_mask = StatementFilter::blank(group_size);
-		let disabled_mask = unwanted_mask.seconded_in_group.clone();
 		let request_properties = RequestProperties { unwanted_mask, backing_threshold: None };
 		let peer_advertised =
 			|_identifier: &CandidateIdentifier, _peer: &_| Some(StatementFilter::full(group_size));
@@ -1282,7 +1247,6 @@ mod tests {
 				0,
 				validator_key_lookup,
 				allowed_para_lookup,
-				disabled_mask,
 			);
 			assert_eq!(
 				output,