From 9f6736dff6ecda272bc6c58cfde69951b1f2839a Mon Sep 17 00:00:00 2001 From: ordian Date: Thu, 12 Oct 2023 10:46:14 +0200 Subject: [PATCH] ignore disabled statements on responses --- .../statement-distribution/src/v2/mod.rs | 3 + .../statement-distribution/src/v2/requests.rs | 40 +++++- .../src/v2/tests/requests.rs | 121 +++++++++++++++--- 3 files changed, 141 insertions(+), 23 deletions(-) diff --git a/polkadot/node/network/statement-distribution/src/v2/mod.rs b/polkadot/node/network/statement-distribution/src/v2/mod.rs index 2d88169bfe32..7990f0a5ad2f 100644 --- a/polkadot/node/network/statement-distribution/src/v2/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/mod.rs @@ -2622,6 +2622,8 @@ pub(crate) async fn handle_response( Some(g) => g, }; + let disabled_mask = relay_parent_state.statement_store.disabled_bitmask(group); + let res = response.validate_response( &mut state.request_manager, group, @@ -2636,6 +2638,7 @@ pub(crate) async fn handle_response( Some(g_index) == expected_group }, + disabled_mask, ); for (peer, rep) in res.reputation_changes { diff --git a/polkadot/node/network/statement-distribution/src/v2/requests.rs b/polkadot/node/network/statement-distribution/src/v2/requests.rs index f13496024fcf..4218fae7b385 100644 --- a/polkadot/node/network/statement-distribution/src/v2/requests.rs +++ b/polkadot/node/network/statement-distribution/src/v2/requests.rs @@ -30,12 +30,13 @@ //! (which requires state not owned by the request manager). use super::{ - BENEFIT_VALID_RESPONSE, BENEFIT_VALID_STATEMENT, COST_IMPROPERLY_DECODED_RESPONSE, - COST_INVALID_RESPONSE, COST_INVALID_SIGNATURE, COST_UNREQUESTED_RESPONSE_STATEMENT, - REQUEST_RETRY_DELAY, + BENEFIT_VALID_RESPONSE, BENEFIT_VALID_STATEMENT, COST_DISABLED_VALIDATOR, + 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}, @@ -510,6 +511,7 @@ 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. @@ -524,6 +526,7 @@ impl UnhandledResponse { session: SessionIndex, validator_key_lookup: impl Fn(ValidatorIndex) -> Option, allowed_para_lookup: impl Fn(ParaId, GroupIndex) -> bool, + disabled_mask: BitVec, ) -> ResponseValidationOutput { let UnhandledResponse { response: TaggedResponse { identifier, requested_peer, props, response }, @@ -600,6 +603,7 @@ impl UnhandledResponse { session, validator_key_lookup, allowed_para_lookup, + disabled_mask, ); if let CandidateRequestStatus::Complete { .. } = output.request_status { @@ -619,10 +623,11 @@ fn validate_complete_response( session: SessionIndex, validator_key_lookup: impl Fn(ValidatorIndex) -> Option, allowed_para_lookup: impl Fn(ParaId, GroupIndex) -> bool, + mut disabled_mask: BitVec, ) -> ResponseValidationOutput { let RequestProperties { backing_threshold, mut unwanted_mask } = props; - // sanity check bitmask size. this is based entirely on + // sanity check bitmasks size. this is based entirely on // local logic here. if !unwanted_mask.has_len(group.len()) { gum::error!( @@ -635,6 +640,16 @@ 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, @@ -712,6 +727,11 @@ 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] { @@ -723,6 +743,11 @@ 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 + } }, } @@ -988,6 +1013,7 @@ 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. @@ -1031,6 +1057,7 @@ mod tests { 0, validator_key_lookup, allowed_para_lookup, + disabled_mask.clone(), ); assert_eq!( output, @@ -1069,6 +1096,7 @@ mod tests { 0, validator_key_lookup, allowed_para_lookup, + disabled_mask, ); assert_eq!( output, @@ -1108,6 +1136,7 @@ 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)); @@ -1148,6 +1177,7 @@ mod tests { 0, validator_key_lookup, allowed_para_lookup, + disabled_mask, ); assert_eq!( output, @@ -1188,6 +1218,7 @@ 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)); @@ -1226,6 +1257,7 @@ mod tests { 0, validator_key_lookup, allowed_para_lookup, + disabled_mask, ); assert_eq!( output, diff --git a/polkadot/node/network/statement-distribution/src/v2/tests/requests.rs b/polkadot/node/network/statement-distribution/src/v2/tests/requests.rs index fef1819e9416..4ae68930ee13 100644 --- a/polkadot/node/network/statement-distribution/src/v2/tests/requests.rs +++ b/polkadot/node/network/statement-distribution/src/v2/tests/requests.rs @@ -1111,20 +1111,22 @@ fn peer_reported_for_providing_statement_from_disabled_validator() { }; let relay_parent = Hash::repeat_byte(1); - let peer_a = PeerId::random(); + let peer_disabled = PeerId::random(); + let peer_b = PeerId::random(); test_harness(config, |state, mut overseer| async move { let local_validator = state.local.clone().unwrap(); let local_para = ParaId::from(local_validator.group_index.0); let other_group_validators = state.group_validators(local_validator.group_index, true); - let v_a = other_group_validators[0]; + let index_disabled = other_group_validators[0]; + let index_b = other_group_validators[1]; - let disabled_validators = vec![v_a]; + let disabled_validators = vec![index_disabled]; let test_leaf = state.make_dummy_leaf_with_disabled_validators(relay_parent, disabled_validators); - let (candidate, _) = make_candidate( + let (candidate, pvd) = make_candidate( relay_parent, 1, local_para, @@ -1134,15 +1136,23 @@ fn peer_reported_for_providing_statement_from_disabled_validator() { ); let candidate_hash = candidate.hash(); - // peer A is in group, has relay parent in view. + // peer A is in group, has relay parent in view and disabled. + // peer B is in group, has relay parent in view. { connect_peer( &mut overseer, - peer_a.clone(), - Some(vec![state.discovery_id(v_a)].into_iter().collect()), + peer_disabled.clone(), + Some(vec![state.discovery_id(index_disabled)].into_iter().collect()), ) .await; - send_peer_view_change(&mut overseer, peer_a.clone(), view![relay_parent]).await; + connect_peer( + &mut overseer, + peer_b.clone(), + Some(vec![state.discovery_id(index_b)].into_iter().collect()), + ) + .await; + send_peer_view_change(&mut overseer, peer_disabled.clone(), view![relay_parent]).await; + send_peer_view_change(&mut overseer, peer_b.clone(), view![relay_parent]).await; } activate_leaf(&mut overseer, &test_leaf, &state, true).await; @@ -1155,22 +1165,48 @@ fn peer_reported_for_providing_statement_from_disabled_validator() { ) .await; + let seconded_disabled = state + .sign_statement( + index_disabled, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(); + + let seconded_b = state + .sign_statement( + index_b, + CompactStatement::Seconded(candidate_hash), + &SigningContext { parent_hash: relay_parent, session_index: 1 }, + ) + .as_unchecked() + .clone(); { - let a_seconded_disabled = state - .sign_statement( - v_a, - CompactStatement::Seconded(candidate_hash), - &SigningContext { parent_hash: relay_parent, session_index: 1 }, - ) - .as_unchecked() - .clone(); + send_peer_message( + &mut overseer, + peer_disabled.clone(), + protocol_v2::StatementDistributionMessage::Statement( + relay_parent, + seconded_disabled.clone(), + ), + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_disabled && r == COST_DISABLED_VALIDATOR.into() => { } + ); + } + { send_peer_message( &mut overseer, - peer_a.clone(), + peer_b.clone(), protocol_v2::StatementDistributionMessage::Statement( relay_parent, - a_seconded_disabled, + seconded_b.clone(), ), ) .await; @@ -1178,8 +1214,55 @@ fn peer_reported_for_providing_statement_from_disabled_validator() { assert_matches!( overseer.recv().await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) - if p == peer_a && r == COST_DISABLED_VALIDATOR.into() => { } + if p == peer_b && r == BENEFIT_VALID_STATEMENT_FIRST.into() => { } + ); + } + + // Send a request to peer and mock its response with a statement from disabled validator. + { + let statements = vec![seconded_disabled]; + + handle_sent_request( + &mut overseer, + peer_b, + candidate_hash, + StatementFilter::blank(group_size), + candidate.clone(), + pvd.clone(), + statements, + ) + .await; + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_b && r == COST_DISABLED_VALIDATOR.into() => { } ); + + assert_matches!( + overseer.recv().await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(p, r))) + if p == peer_b && r == BENEFIT_VALID_RESPONSE.into() => { } + ); + + assert_matches!( + overseer.recv().await, + AllMessages:: NetworkBridgeTx( + NetworkBridgeTxMessage::SendValidationMessage( + peers, + Versioned::V2( + protocol_v2::ValidationProtocol::StatementDistribution( + protocol_v2::StatementDistributionMessage::Statement(hash, statement), + ), + ), + ) + ) => { + assert_eq!(peers, vec![peer_disabled]); + assert_eq!(hash, relay_parent); + assert_eq!(statement, seconded_b); + } + ); + answer_expected_hypothetical_depth_request(&mut overseer, vec![], None, false).await; } overseer