From bbcf35daa28fd2fda65af143685f3f0fccd8ad34 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 10 Jul 2023 10:34:10 +0300 Subject: [PATCH 01/15] node: approval-distribution/voting: lazy approvals signature checks Signed-off-by: Alexandru Gheorghe --- node/core/approval-voting/src/lib.rs | 30 +- node/core/approval-voting/src/tests.rs | 1 + node/core/dispute-coordinator/src/import.rs | 31 +- node/network/approval-distribution/src/lib.rs | 390 ++++++++++++------ .../approval-distribution/src/metrics.rs | 28 ++ .../approval-distribution/src/tests.rs | 75 ++-- node/network/protocol/src/grid_topology.rs | 27 ++ node/subsystem-types/src/messages.rs | 7 +- 8 files changed, 399 insertions(+), 190 deletions(-) diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs index a6a74da50480..608a948e9cd1 100644 --- a/node/core/approval-voting/src/lib.rs +++ b/node/core/approval-voting/src/lib.rs @@ -1269,7 +1269,7 @@ async fn handle_from_overseer( actions }, - ApprovalVotingMessage::CheckAndImportApproval(a, res) => + ApprovalVotingMessage::CheckAndImportApproval(a, res, sender_is_originator) => check_and_import_approval( ctx.sender(), state, @@ -1277,6 +1277,7 @@ async fn handle_from_overseer( session_info_provider, metrics, a, + sender_is_originator, |r| { let _ = res.send(r); }, @@ -1930,6 +1931,7 @@ async fn check_and_import_approval( session_info_provider: &mut RuntimeInfo, metrics: &Metrics, approval: IndirectSignedApprovalVote, + sender_is_originator: bool, with_response: impl FnOnce(ApprovalCheckResult) -> T, ) -> SubsystemResult<(Vec, T)> where @@ -1997,17 +1999,21 @@ where }; // Signature check: - match DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking).check_signature( - &pubkey, - approved_candidate_hash, - block_entry.session(), - &approval.signature, - ) { - Err(_) => respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::InvalidSignature( - approval.validator - ),)), - Ok(()) => {}, - }; + // We perform signatures checks just for the cases where the approvals weren't received directly from + // peer and we received them via gossipping. + if !sender_is_originator { + match DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking).check_signature( + &pubkey, + approved_candidate_hash, + block_entry.session(), + &approval.signature, + ) { + Err(_) => respond_early!(ApprovalCheckResult::Bad( + ApprovalCheckError::InvalidSignature(approval.validator), + )), + Ok(()) => {}, + }; + } let candidate_entry = match db.load_candidate_entry(&approved_candidate_hash)? { Some(c) => c, diff --git a/node/core/approval-voting/src/tests.rs b/node/core/approval-voting/src/tests.rs index f58e60c6a487..4ee3f193f6b1 100644 --- a/node/core/approval-voting/src/tests.rs +++ b/node/core/approval-voting/src/tests.rs @@ -590,6 +590,7 @@ async fn check_and_import_approval( msg: ApprovalVotingMessage::CheckAndImportApproval( IndirectSignedApprovalVote { block_hash, candidate_index, validator, signature }, tx, + false, ), }, ) diff --git a/node/core/dispute-coordinator/src/import.rs b/node/core/dispute-coordinator/src/import.rs index 912521834075..1e4e4dd19577 100644 --- a/node/core/dispute-coordinator/src/import.rs +++ b/node/core/dispute-coordinator/src/import.rs @@ -518,17 +518,26 @@ impl ImportResult { let (mut votes, _) = new_state.into_old_state(); for (index, sig) in approval_votes.into_iter() { - debug_assert!( - { - let pub_key = &env.session_info().validators.get(index).expect("indices are validated by approval-voting subsystem; qed"); - let candidate_hash = votes.candidate_receipt.hash(); - let session_index = env.session_index(); - DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) - .check_signature(pub_key, candidate_hash, session_index, &sig) - .is_ok() - }, - "Signature check for imported approval votes failed! This is a serious bug. Session: {:?}, candidate hash: {:?}, validator index: {:?}", env.session_index(), votes.candidate_receipt.hash(), index - ); + let pub_key = &env + .session_info() + .validators + .get(index) + .expect("indices are validated by approval-voting subsystem; qed"); + let candidate_hash = votes.candidate_receipt.hash(); + let session_index = env.session_index(); + // The candidate sent us an invalid signature, so don't import it. + // This might happen because in approval-voting we do not checks signatures for votes received from the originator. + if !DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) + .check_signature(pub_key, candidate_hash, session_index, &sig) + .is_ok() + { + gum::trace!( + target: LOG_TARGET, + "Approval checking signature was invalid, so just ignore it" + ); + continue + } + if votes.valid.insert_vote(index, ValidDisputeStatementKind::ApprovalChecking, sig) { imported_valid_votes += 1; imported_approval_votes += 1; diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 79aa090a140f..d4bc7cc8634e 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -210,34 +210,30 @@ struct Knowledge { // When there is no entry, this means the message is unknown // When there is an entry with `MessageKind::Assignment`, the assignment is known. // When there is an entry with `MessageKind::Approval`, the assignment and approval are known. - known_messages: HashMap, + known_messages: HashMap>, } impl Knowledge { fn contains(&self, message: &MessageSubject, kind: MessageKind) -> bool { - match (kind, self.known_messages.get(message)) { - (_, None) => false, - (MessageKind::Assignment, Some(_)) => true, - (MessageKind::Approval, Some(MessageKind::Assignment)) => false, - (MessageKind::Approval, Some(MessageKind::Approval)) => true, - } + self.known_messages + .get(message) + .map(|messages| messages.contains(&kind)) + .unwrap_or(false) } fn insert(&mut self, message: MessageSubject, kind: MessageKind) -> bool { match self.known_messages.entry(message) { hash_map::Entry::Vacant(vacant) => { - vacant.insert(kind); + vacant.insert(vec![kind]); true }, - hash_map::Entry::Occupied(mut occupied) => match (*occupied.get(), kind) { - (MessageKind::Assignment, MessageKind::Assignment) => false, - (MessageKind::Approval, MessageKind::Approval) => false, - (MessageKind::Approval, MessageKind::Assignment) => false, - (MessageKind::Assignment, MessageKind::Approval) => { - *occupied.get_mut() = MessageKind::Approval; + hash_map::Entry::Occupied(mut occupied) => + if !occupied.get().contains(&kind) { + occupied.get_mut().push(kind); true + } else { + false }, - }, } } } @@ -278,13 +274,15 @@ struct BlockEntry { enum ApprovalState { Assigned(AssignmentCert), Approved(AssignmentCert, ValidatorSignature), + UnassignedApproval(IndirectSignedApprovalVote, MessageSource), } impl ApprovalState { - fn assignment_cert(&self) -> &AssignmentCert { + fn assignment_cert(&self) -> Option<&AssignmentCert> { match *self { - ApprovalState::Assigned(ref cert) => cert, - ApprovalState::Approved(ref cert, _) => cert, + ApprovalState::Assigned(ref cert) => Some(cert), + ApprovalState::Approved(ref cert, _) => Some(cert), + ApprovalState::UnassignedApproval(_, _) => None, } } @@ -292,6 +290,7 @@ impl ApprovalState { match *self { ApprovalState::Assigned(_) => None, ApprovalState::Approved(_, ref sig) => Some(sig.clone()), + ApprovalState::UnassignedApproval(_, _) => None, } } } @@ -301,7 +300,10 @@ impl ApprovalState { // assignments preceding approvals in all cases. #[derive(Debug)] struct MessageState { - required_routing: RequiredRouting, + // Required routing for an approval + required_routing_assignment: RequiredRouting, + // Require routing for an assignment + required_routing_approval: RequiredRouting, local: bool, random_routing: RandomRouting, approval_state: ApprovalState, @@ -451,6 +453,8 @@ impl State { for (peer_id, view) in self.peer_views.iter() { let intersection = view.iter().filter(|h| new_hashes.contains(h)); let view_intersection = View::new(intersection.cloned(), view.finalized_number); + let should_trigger_aggression = + self.aggression_config.should_trigger_aggression(self.approval_checking_lag); Self::unify_with_peer( sender, metrics, @@ -460,6 +464,7 @@ impl State { *peer_id, view_intersection, rng, + should_trigger_aggression, ) .await; } @@ -678,7 +683,8 @@ impl State { } }); } - + let should_trigger_aggression = + self.aggression_config.should_trigger_aggression(self.approval_checking_lag); Self::unify_with_peer( ctx.sender(), metrics, @@ -688,6 +694,7 @@ impl State { peer_id, view, rng, + should_trigger_aggression, ) .await; } @@ -774,7 +781,6 @@ impl State { return }, }; - // compute metadata on the assignment. let message_subject = MessageSubject(block_hash, claimed_candidate_index, validator_index); let message_kind = MessageKind::Assignment; @@ -872,7 +878,7 @@ impl State { BENEFIT_VALID_MESSAGE_FIRST, ) .await; - entry.knowledge.known_messages.insert(message_subject.clone(), message_kind); + entry.knowledge.insert(message_subject.clone(), message_kind); if let Some(peer_knowledge) = entry.known_by.get_mut(&peer_id) { peer_knowledge.received.insert(message_subject.clone(), message_kind); } @@ -950,32 +956,44 @@ impl State { let topology = self.topologies.get_topology(entry.session); let local = source == MessageSource::Local; - let required_routing = topology.map_or(RequiredRouting::PendingTopology, |t| { + let required_routing_assignment = topology.map_or(RequiredRouting::PendingTopology, |t| { t.local_grid_neighbors().required_routing_by_index(validator_index, local) }); - let message_state = match entry.candidates.get_mut(claimed_candidate_index as usize) { - Some(candidate_entry) => { - // set the approval state for validator_index to Assigned - // unless the approval state is set already - candidate_entry.messages.entry(validator_index).or_insert_with(|| MessageState { - required_routing, - local, - random_routing: Default::default(), - approval_state: ApprovalState::Assigned(assignment.cert.clone()), - }) - }, - None => { - gum::warn!( - target: LOG_TARGET, - hash = ?block_hash, - ?claimed_candidate_index, - "Expected a candidate entry on import_and_circulate_assignment", - ); + let (message_state, cached_approval_vote) = + match entry.candidates.get_mut(claimed_candidate_index as usize) { + Some(candidate_entry) => { + // We might have received the approval before the assignment, so save it for processing after the assignment + let cached_approval_vote = candidate_entry.messages.remove(&validator_index); + metrics.on_delayed_approval_processed(); + ( + candidate_entry.messages.entry(validator_index).or_insert_with(|| { + MessageState { + required_routing_assignment, + required_routing_approval: if local { + RequiredRouting::All + } else { + RequiredRouting::None + }, + local, + random_routing: Default::default(), + approval_state: ApprovalState::Assigned(assignment.cert.clone()), + } + }), + cached_approval_vote, + ) + }, + None => { + gum::warn!( + target: LOG_TARGET, + hash = ?block_hash, + ?claimed_candidate_index, + "Expected a candidate entry on import_and_circulate_assignment", + ); - return - }, - }; + return + }, + }; // Dispatch the message to all peers in the routing set which // know the block. @@ -994,7 +1012,7 @@ impl State { if let Some(true) = topology .as_ref() - .map(|t| t.local_grid_neighbors().route_to_peer(required_routing, peer)) + .map(|t| t.local_grid_neighbors().route_to_peer(required_routing_assignment, peer)) { return true } @@ -1039,6 +1057,19 @@ impl State { )) .await; } + + if let Some(MessageState { + approval_state: ApprovalState::UnassignedApproval(vote, source), + .. + }) = cached_approval_vote + { + gum::info!( + target: LOG_TARGET, + ?message_subject, + "Delayed approval processed" + ); + self.import_and_circulate_approval(ctx, metrics, source, vote).await + } } async fn import_and_circulate_approval( @@ -1090,7 +1121,48 @@ impl State { let message_kind = MessageKind::Approval; if let Some(peer_id) = source.peer_id() { + let sender_matches_validator_index = self + .topologies + .get_topology(entry.session) + .map(|topology| { + topology.get().peer_id_matches_validator_index(validator_index, peer_id) + }) + .unwrap_or(false); + if !entry.knowledge.contains(&message_subject, MessageKind::Assignment) { + metrics.on_unassigned_approval(); + + match entry.candidates.get_mut(candidate_index as usize) { + // Approvals might arrive sooner than their corresponding assignment because we are sending them directly + // to all peers instead of relaying on them being gossiped, so we need to save them untill the assignment + // arrives. + // We could also receive gossiped assignments in the case when aggression is triggered, but in that + // case we do not care about saving them because they should arrive before their corresponding assignments, + // since they are sent on the same route. + Some(candidate_entry) + if sender_matches_validator_index && + !candidate_entry.messages.contains_key(&validator_index) => + { + candidate_entry.messages.entry(validator_index).or_insert_with(|| { + MessageState { + required_routing_assignment: RequiredRouting::None, + required_routing_approval: RequiredRouting::None, + local: false, + random_routing: Default::default(), + approval_state: ApprovalState::UnassignedApproval(vote, source), + } + }); + gum::info!( + target: LOG_TARGET, + ?peer_id, + ?message_subject, + "Saving approval to process later on", + ); + return + }, + _ => {}, + }; + gum::debug!( target: LOG_TARGET, ?peer_id, @@ -1165,9 +1237,19 @@ impl State { } let (tx, rx) = oneshot::channel(); + if !sender_matches_validator_index { + gum::info!( + target: LOG_TARGET, + "Received gossiped approval" + ) + } - ctx.send_message(ApprovalVotingMessage::CheckAndImportApproval(vote.clone(), tx)) - .await; + ctx.send_message(ApprovalVotingMessage::CheckAndImportApproval( + vote.clone(), + tx, + sender_matches_validator_index, + )) + .await; let timer = metrics.time_awaiting_approval_voting(); let result = match rx.await { @@ -1238,17 +1320,19 @@ impl State { // Invariant: to our knowledge, none of the peers except for the `source` know about the approval. metrics.on_approval_imported(); - - let required_routing = match entry.candidates.get_mut(candidate_index as usize) { + let should_trigger_aggression = + self.aggression_config.should_trigger_aggression(self.approval_checking_lag); + let required_routing_approval = match entry.candidates.get_mut(candidate_index as usize) { Some(candidate_entry) => { // set the approval state for validator_index to Approved // it should be in assigned state already match candidate_entry.messages.remove(&validator_index) { Some(MessageState { approval_state: ApprovalState::Assigned(cert), - required_routing, + required_routing_assignment, local, random_routing, + required_routing_approval, }) => { candidate_entry.messages.insert( validator_index, @@ -1257,13 +1341,28 @@ impl State { cert, vote.signature.clone(), ), - required_routing, + required_routing_assignment, local, random_routing, + required_routing_approval, }, ); - - required_routing + // When aggression is enabled gossip the approvals we received from peers, so that we reach finality. + if required_routing_approval == RequiredRouting::None && + should_trigger_aggression + { + self.topologies + .get_topology(entry.session) + .zip(source.peer_id()) + .map(|(topology, peer_id)| { + topology + .local_grid_neighbors() + .required_routing_by_peer_id(peer_id, local) + }) + .unwrap_or(RequiredRouting::None) + } else { + required_routing_approval + } }, Some(_) => { unreachable!( @@ -1309,7 +1408,8 @@ impl State { return false } - // Here we're leaning on a few behaviors of assignment propagation: + // When approvals are gossiped(l1 agression enabled) we are leaning on a few behaviors of + // assignment propagation: // 1. At this point, the only peer we're aware of which has the approval // message is the source peer. // 2. We have sent the assignment message to every peer in the required routing @@ -1318,9 +1418,12 @@ impl State { // the assignment to all aware peers in the required routing _except_ the original // source of the assignment. Hence the `in_topology_check`. // 3. Any randomly selected peers have been sent the assignment already. - let in_topology = topology - .map_or(false, |t| t.local_grid_neighbors().route_to_peer(required_routing, peer)); - in_topology || knowledge.sent.contains(message_subject, MessageKind::Assignment) + let in_topology = topology.map_or(false, |t| { + t.local_grid_neighbors().route_to_peer(required_routing_approval, peer) + }); + in_topology || + (knowledge.sent.contains(message_subject, MessageKind::Assignment) && + should_trigger_aggression) }; let peers = entry @@ -1403,7 +1506,8 @@ impl State { candidate_entry.messages.iter().filter_map(|(validator_index, message_state)| { match &message_state.approval_state { ApprovalState::Approved(_, sig) => Some((*validator_index, sig.clone())), - ApprovalState::Assigned(_) => None, + ApprovalState::Assigned(_) | ApprovalState::UnassignedApproval(_, _) => + None, } }); all_sigs.extend(sigs); @@ -1420,6 +1524,7 @@ impl State { peer_id: PeerId, view: View, rng: &mut (impl CryptoRng + Rng), + should_trigger_aggression: bool, ) { metrics.on_unify_with_peer(); let _timer = metrics.time_unify_with_peer(); @@ -1454,39 +1559,36 @@ impl State { }) { // Propagate the message to all peers in the required routing set OR // randomly sample peers. - { - let random_routing = &mut message_state.random_routing; - let required_routing = message_state.required_routing; - let rng = &mut *rng; - let mut peer_filter = move |peer_id| { - let in_topology = topology.as_ref().map_or(false, |t| { - t.local_grid_neighbors().route_to_peer(required_routing, peer_id) - }); - in_topology || { - let route_random = random_routing.sample(total_peers, rng); - if route_random { - random_routing.inc_sent(); - } - - route_random + let random_routing = &mut message_state.random_routing; + let required_routing_assignment = message_state.required_routing_assignment; + let rng = &mut *rng; + let mut peer_filter_assignment = move |peer_id| { + let in_topology = topology.as_ref().map_or(false, |t| { + t.local_grid_neighbors() + .route_to_peer(required_routing_assignment, peer_id) + }); + in_topology || { + let route_random = random_routing.sample(total_peers, rng); + if route_random { + random_routing.inc_sent(); } - }; - - if !peer_filter(&peer_id) { - continue + route_random } - } + }; let message_subject = MessageSubject(block, candidate_index, *validator); - let assignment_message = ( - IndirectAssignmentCert { - block_hash: block, - validator: *validator, - cert: message_state.approval_state.assignment_cert().clone(), - }, - candidate_index, - ); + let assignment_message = + message_state.approval_state.assignment_cert().map(|assignmentcert| { + ( + IndirectAssignmentCert { + block_hash: block, + validator: *validator, + cert: assignmentcert.clone(), + }, + candidate_index, + ) + }); let approval_message = message_state.approval_state.approval_signature().map(|signature| { @@ -1498,19 +1600,39 @@ impl State { } }); - if !peer_knowledge.contains(&message_subject, MessageKind::Assignment) { - peer_knowledge - .sent - .insert(message_subject.clone(), MessageKind::Assignment); - assignments_to_send.push(assignment_message); + let mut assignment_sent = false; + if peer_filter_assignment(&peer_id) { + if let Some(assignment_message) = assignment_message { + if !peer_knowledge.contains(&message_subject, MessageKind::Assignment) { + peer_knowledge + .sent + .insert(message_subject.clone(), MessageKind::Assignment); + assignments_to_send.push(assignment_message); + assignment_sent = true; + } + } } - if let Some(approval_message) = approval_message { - if !peer_knowledge.contains(&message_subject, MessageKind::Approval) { - peer_knowledge - .sent - .insert(message_subject.clone(), MessageKind::Approval); - approvals_to_send.push(approval_message); + let peer_filter_approval = move |peer_id, required_routing, local| { + let in_topology = topology.as_ref().map_or(false, |t| { + t.local_grid_neighbors().route_to_peer(required_routing, peer_id) + }); + + in_topology || local || (should_trigger_aggression && assignment_sent) + }; + + if peer_filter_approval( + &peer_id, + message_state.required_routing_approval, + message_state.local, + ) { + if let Some(approval_message) = approval_message { + if !peer_knowledge.contains(&message_subject, MessageKind::Approval) { + peer_knowledge + .sent + .insert(message_subject.clone(), MessageKind::Approval); + approvals_to_send.push(approval_message); + } } } } @@ -1685,9 +1807,15 @@ async fn adjust_required_routing_and_propagate); struct MetricsInner { assignments_imported_total: prometheus::Counter, approvals_imported_total: prometheus::Counter, + unassigned_approval_total: prometheus::Counter, + delayed_approvals_processed_total: prometheus::Counter, unified_with_peer_total: prometheus::Counter, aggression_l1_messages_total: prometheus::Counter, aggression_l2_messages_total: prometheus::Counter, @@ -46,6 +48,18 @@ impl Metrics { } } + pub(crate) fn on_unassigned_approval(&self) { + if let Some(metrics) = &self.0 { + metrics.unassigned_approval_total.inc(); + } + } + + pub(crate) fn on_delayed_approval_processed(&self) { + if let Some(metrics) = &self.0 { + metrics.delayed_approvals_processed_total.inc(); + } + } + pub(crate) fn on_unify_with_peer(&self) { if let Some(metrics) = &self.0 { metrics.unified_with_peer_total.inc(); @@ -95,6 +109,20 @@ impl MetricsTrait for Metrics { )?, registry, )?, + unassigned_approval_total: prometheus::register( + prometheus::Counter::new( + "polkadot_parachain_unassigned_approval_total", + "Number of approvals received for which we didn't received the approval yet.", + )?, + registry, + )?, + delayed_approvals_processed_total: prometheus::register( + prometheus::Counter::new( + "polkadot_parachain_delayed_approvals_processedtotal", + "Number of approvals processed with delay after we received the assignment.", + )?, + registry, + )?, approvals_imported_total: prometheus::register( prometheus::Counter::new( "polkadot_parachain_approvals_imported_total", diff --git a/node/network/approval-distribution/src/tests.rs b/node/network/approval-distribution/src/tests.rs index 979f0ada4ee6..5dc474493c1a 100644 --- a/node/network/approval-distribution/src/tests.rs +++ b/node/network/approval-distribution/src/tests.rs @@ -689,26 +689,15 @@ fn import_approval_happy_path() { AllMessages::ApprovalVoting(ApprovalVotingMessage::CheckAndImportApproval( vote, tx, + _, )) => { assert_eq!(vote, approval); tx.send(ApprovalCheckResult::Accepted).unwrap(); } ); - + // We expect only reputation changes, because approvals from peers are not gossiped unless agression is enabled. expect_reputation_change(overseer, &peer_b, BENEFIT_VALID_MESSAGE_FIRST).await; - assert_matches!( - overseer_recv(overseer).await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( - peers, - Versioned::V1(protocol_v1::ValidationProtocol::ApprovalDistribution( - protocol_v1::ApprovalDistributionMessage::Approvals(approvals) - )) - )) => { - assert_eq!(peers.len(), 1); - assert_eq!(approvals.len(), 1); - } - ); virtual_overseer }); } @@ -783,6 +772,7 @@ fn import_approval_bad() { AllMessages::ApprovalVoting(ApprovalVotingMessage::CheckAndImportApproval( vote, tx, + false, )) => { assert_eq!(vote, approval); tx.send(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownBlock(hash))).unwrap(); @@ -1094,6 +1084,7 @@ fn import_remotely_then_locally() { AllMessages::ApprovalVoting(ApprovalVotingMessage::CheckAndImportApproval( vote, tx, + false )) => { assert_eq!(vote, approval); tx.send(ApprovalCheckResult::Accepted).unwrap(); @@ -1363,7 +1354,10 @@ fn propagates_locally_generated_assignment_to_both_dimensions() { )) )) => { // Random sampling is reused from the assignment. - assert_eq!(sent_peers, assignment_sent_peers); + for peer in &peers { + assert!(sent_peers.contains(&peer.0)); + } + assert_eq!(sent_peers.len(), peers.len()); assert_eq!(sent_approvals, approvals); } ); @@ -1519,12 +1513,11 @@ fn propagates_to_required_after_connect() { let hash = Hash::repeat_byte(0xAA); let peers = make_peers_and_authority_ids(100); + let omitted = [0, 10, 50, 51]; let _ = test_harness(State::default(), |mut virtual_overseer| async move { let overseer = &mut virtual_overseer; - let omitted = [0, 10, 50, 51]; - // Connect all peers except omitted. for (i, (peer, _)) in peers.iter().enumerate() { if !omitted.contains(&i) { @@ -1610,8 +1603,10 @@ fn propagates_to_required_after_connect() { protocol_v1::ApprovalDistributionMessage::Approvals(sent_approvals) )) )) => { - // Random sampling is reused from the assignment. - assert_eq!(sent_peers, assignment_sent_peers); + // Approvals should be sent to all peers. + for (index, peer) in peers.iter().enumerate().filter(|(index, _)| !omitted.contains(index)){ + assert!(sent_peers.contains(&peer.0)); + } assert_eq!(sent_approvals, approvals); } ); @@ -1706,7 +1701,12 @@ fn sends_to_more_peers_after_getting_topology() { let assignments = vec![(cert.clone(), candidate_index)]; let approvals = vec![approval.clone()]; - let mut expected_indices = vec![0, 10, 20, 30, 50, 51, 52, 53]; + let mut expected_indices_assignments = vec![0, 10, 20, 30, 50, 51, 52, 53]; + + // Approvals are sent to all peers + let mut expected_indices_approvals: Vec = (0..peers.len()).collect(); + + // We sent only assignment when we don't a topology set yet. let assignment_sent_peers = assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( @@ -1722,27 +1722,14 @@ fn sends_to_more_peers_after_getting_topology() { // Random gossip before topology can send to topology-targeted peers. // Remove them from the expected indices so we don't expect // them to get the messages again after the assignment. - expected_indices.retain(|&i2| i2 != i); + expected_indices_assignments.retain(|&i2| i2 != i); + } assert_eq!(sent_assignments, assignments); sent_peers } ); - assert_matches!( - overseer_recv(overseer).await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( - sent_peers, - Versioned::V1(protocol_v1::ValidationProtocol::ApprovalDistribution( - protocol_v1::ApprovalDistributionMessage::Approvals(sent_approvals) - )) - )) => { - // Random sampling is reused from the assignment. - assert_eq!(sent_peers, assignment_sent_peers); - assert_eq!(sent_approvals, approvals); - } - ); - // Set up a gossip topology. setup_gossip_topology( overseer, @@ -1750,8 +1737,7 @@ fn sends_to_more_peers_after_getting_topology() { ) .await; - let mut expected_indices_assignments = expected_indices.clone(); - let mut expected_indices_approvals = expected_indices.clone(); + let mut expected_indices_assignments = expected_indices_assignments.clone(); for _ in 0..expected_indices_assignments.len() { assert_matches!( @@ -1790,7 +1776,6 @@ fn sends_to_more_peers_after_getting_topology() { let pos = expected_indices_approvals.iter() .position(|i| &peers[*i].0 == &sent_peers[0]) .unwrap(); - expected_indices_approvals.remove(pos); } ); @@ -1879,14 +1864,18 @@ fn originator_aggression_l1() { } ); - assert_matches!( + let prev_sent_approvals = assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( - _, + sent_peers, Versioned::V1(protocol_v1::ValidationProtocol::ApprovalDistribution( protocol_v1::ApprovalDistributionMessage::Approvals(_) )) - )) => { } + )) => { + sent_peers.into_iter() + .filter_map(|sp| peers.iter().position(|p| &p.0 == &sp)) + .collect::>() + } ); // Add blocks until aggression L1 is triggered. @@ -1917,6 +1906,10 @@ fn originator_aggression_l1() { let unsent_indices = (0..peers.len()).filter(|i| !prev_sent_indices.contains(&i)).collect::>(); + let unsent_indices_approvals = (0..peers.len()) + .filter(|i| !prev_sent_approvals.contains(&i)) + .collect::>(); + for _ in 0..unsent_indices.len() { assert_matches!( overseer_recv(overseer).await, @@ -1937,7 +1930,7 @@ fn originator_aggression_l1() { ); } - for _ in 0..unsent_indices.len() { + for _ in 0..unsent_indices_approvals.len() { assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( diff --git a/node/network/protocol/src/grid_topology.rs b/node/network/protocol/src/grid_topology.rs index 1b356f67617b..316d0dd437c8 100644 --- a/node/network/protocol/src/grid_topology.rs +++ b/node/network/protocol/src/grid_topology.rs @@ -108,6 +108,33 @@ impl SessionGridTopology { Some(grid_subset) } + + /// Gets the list of known peer Ids for a given validator index + /// + /// Returns `None` if the validator index is out of bound + pub fn get_known_peer_ids_by_validator_index( + &self, + validator_index: ValidatorIndex, + ) -> Option<&Vec> { + let peer_index = self.shuffled_indices.get(validator_index.0 as usize)?; + + self.canonical_shuffling + .get(*peer_index) + .map(|topology_peer_info| &topology_peer_info.peer_ids) + } + + /// Checks if the passed validator_index matches the peer_id + /// + /// Returns true if they match and false otherwise + pub fn peer_id_matches_validator_index( + &self, + validator_index: ValidatorIndex, + peer_id: PeerId, + ) -> bool { + self.get_known_peer_ids_by_validator_index(validator_index) + .map(|known_peer_ids| known_peer_ids.contains(&peer_id)) + .unwrap_or(false) + } } struct MatrixNeighbors { diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 8419763789dc..6294670f8db7 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -827,8 +827,13 @@ pub enum ApprovalVotingMessage { /// Check if the approval vote is valid and can be accepted by our view of the /// protocol. /// + /// * `IndirectSignedApprovalVote` - The vote to be imported + /// * `oneshot::Sender` - The channel used for sending the reply back + /// * `bool` - If the sender of the messages is the originator of it, when false it means + /// the message has been gossipped. + /// /// Should not be sent unless the block hash within the indirect vote is known. - CheckAndImportApproval(IndirectSignedApprovalVote, oneshot::Sender), + CheckAndImportApproval(IndirectSignedApprovalVote, oneshot::Sender, bool), /// Returns the highest possible ancestor hash of the provided block hash which is /// acceptable to vote on finality for. /// The `BlockNumber` provided is the number of the block's ancestor which is the From 1245d18752d8e0f54b3c8af4438f59e2af043842 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 11 Jul 2023 13:52:21 +0300 Subject: [PATCH 02/15] fix warning in test-linux-stable Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/tests.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/node/network/approval-distribution/src/tests.rs b/node/network/approval-distribution/src/tests.rs index 5dc474493c1a..9bc8366c2fa7 100644 --- a/node/network/approval-distribution/src/tests.rs +++ b/node/network/approval-distribution/src/tests.rs @@ -1324,7 +1324,7 @@ fn propagates_locally_generated_assignment_to_both_dimensions() { let assignments = vec![(cert.clone(), candidate_index)]; let approvals = vec![approval.clone()]; - let assignment_sent_peers = assert_matches!( + let _assignment_sent_peers = assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( sent_peers, @@ -1574,7 +1574,7 @@ fn propagates_to_required_after_connect() { let assignments = vec![(cert.clone(), candidate_index)]; let approvals = vec![approval.clone()]; - let assignment_sent_peers = assert_matches!( + let _assignment_sent_peers = assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( sent_peers, @@ -1604,7 +1604,7 @@ fn propagates_to_required_after_connect() { )) )) => { // Approvals should be sent to all peers. - for (index, peer) in peers.iter().enumerate().filter(|(index, _)| !omitted.contains(index)){ + for (_, peer) in peers.iter().enumerate().filter(|(index, _)| !omitted.contains(index)){ assert!(sent_peers.contains(&peer.0)); } assert_eq!(sent_approvals, approvals); @@ -1701,13 +1701,13 @@ fn sends_to_more_peers_after_getting_topology() { let assignments = vec![(cert.clone(), candidate_index)]; let approvals = vec![approval.clone()]; - let mut expected_indices_assignments = vec![0, 10, 20, 30, 50, 51, 52, 53]; + let mut expected_indices = vec![0, 10, 20, 30, 50, 51, 52, 53]; // Approvals are sent to all peers let mut expected_indices_approvals: Vec = (0..peers.len()).collect(); // We sent only assignment when we don't a topology set yet. - let assignment_sent_peers = assert_matches!( + let _assignment_sent_peers = assert_matches!( overseer_recv(overseer).await, AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendValidationMessage( sent_peers, @@ -1722,7 +1722,7 @@ fn sends_to_more_peers_after_getting_topology() { // Random gossip before topology can send to topology-targeted peers. // Remove them from the expected indices so we don't expect // them to get the messages again after the assignment. - expected_indices_assignments.retain(|&i2| i2 != i); + expected_indices.retain(|&i2| i2 != i); } assert_eq!(sent_assignments, assignments); @@ -1737,7 +1737,7 @@ fn sends_to_more_peers_after_getting_topology() { ) .await; - let mut expected_indices_assignments = expected_indices_assignments.clone(); + let mut expected_indices_assignments = expected_indices.clone(); for _ in 0..expected_indices_assignments.len() { assert_matches!( From e28153ffb015066542fc848a2d46106b70fbcede Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 13 Jul 2023 20:30:38 +0300 Subject: [PATCH 03/15] Hack: Disable gossiping at a certain block number ... to ease up quick testing in versi Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index d4bc7cc8634e..24f4fa57b965 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -48,6 +48,12 @@ use std::{ time::Duration, }; +const ACTIVATION_BLOCK_NUMBER: u32 = 20; + +fn disable_gossiping(block: u32) -> bool { + block > ACTIVATION_BLOCK_NUMBER +} + use self::metrics::Metrics; mod metrics; @@ -970,10 +976,14 @@ impl State { candidate_entry.messages.entry(validator_index).or_insert_with(|| { MessageState { required_routing_assignment, - required_routing_approval: if local { - RequiredRouting::All + required_routing_approval: if disable_gossiping(entry.number) { + if local { + RequiredRouting::All + } else { + RequiredRouting::None + } } else { - RequiredRouting::None + required_routing_assignment }, local, random_routing: Default::default(), @@ -1403,6 +1413,7 @@ impl State { let source_peer = source.peer_id(); let message_subject = &message_subject; + let block_number = entry.number; let peer_filter = move |peer, knowledge: &PeerKnowledge| { if Some(peer) == source_peer.as_ref() { return false @@ -1423,7 +1434,7 @@ impl State { }); in_topology || (knowledge.sent.contains(message_subject, MessageKind::Assignment) && - should_trigger_aggression) + (should_trigger_aggression || !disable_gossiping(block_number))) }; let peers = entry @@ -1612,13 +1623,15 @@ impl State { } } } - + let block_number = entry.number; let peer_filter_approval = move |peer_id, required_routing, local| { let in_topology = topology.as_ref().map_or(false, |t| { t.local_grid_neighbors().route_to_peer(required_routing, peer_id) }); - in_topology || local || (should_trigger_aggression && assignment_sent) + in_topology || + local || ((should_trigger_aggression || !disable_gossiping(block_number)) && + assignment_sent) }; if peer_filter_approval( From 7f9a314abbeb37d0a0c88c7f4650eb432638a3d4 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 13 Jul 2023 20:39:13 +0300 Subject: [PATCH 04/15] HACK: Add versi block to start test Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 24f4fa57b965..89950bfbef43 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -48,7 +48,8 @@ use std::{ time::Duration, }; -const ACTIVATION_BLOCK_NUMBER: u32 = 20; +// TODO: Disable will be removed in the final version and will be replaced with +const ACTIVATION_BLOCK_NUMBER: u32 = 335539; fn disable_gossiping(block: u32) -> bool { block > ACTIVATION_BLOCK_NUMBER From db7a84014afd66198a03ff339abd353900fde9ca Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 09:20:53 +0300 Subject: [PATCH 05/15] Hack: update versi enablement block number Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 89950bfbef43..54e99be6995e 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -48,10 +48,16 @@ use std::{ time::Duration, }; -// TODO: Disable will be removed in the final version and will be replaced with -const ACTIVATION_BLOCK_NUMBER: u32 = 335539; +// TODO: Disable will be removed in the final version and will be replaced with a runtime configuration +const ACTIVATION_BLOCK_NUMBER: u32 = 10519; fn disable_gossiping(block: u32) -> bool { + if block == ACTIVATION_BLOCK_NUMBER { + gum::info!( + target: LOG_TARGET, + "Disable gossiping for nodes" + ) + } block > ACTIVATION_BLOCK_NUMBER } From f0a2472de228662ec80279eb84d3ee95314a1a3a Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 09:27:47 +0300 Subject: [PATCH 06/15] Fix Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 54e99be6995e..fec4cfb40eec 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -49,7 +49,7 @@ use std::{ }; // TODO: Disable will be removed in the final version and will be replaced with a runtime configuration -const ACTIVATION_BLOCK_NUMBER: u32 = 10519; +const ACTIVATION_BLOCK_NUMBER: u32 = 11019; fn disable_gossiping(block: u32) -> bool { if block == ACTIVATION_BLOCK_NUMBER { From 9ebee8cba402bd9adf06e7c359fad6afe30de658 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 11:19:28 +0300 Subject: [PATCH 07/15] Test block time Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index fec4cfb40eec..42c0c98f0126 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -49,7 +49,7 @@ use std::{ }; // TODO: Disable will be removed in the final version and will be replaced with a runtime configuration -const ACTIVATION_BLOCK_NUMBER: u32 = 11019; +const ACTIVATION_BLOCK_NUMBER: u32 = 9000; fn disable_gossiping(block: u32) -> bool { if block == ACTIVATION_BLOCK_NUMBER { From 69b7e8e41b87b51a09d1bb21d8d64c1926a2f2ab Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 14:04:48 +0300 Subject: [PATCH 08/15] Fixed on delayed approval processed Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 42c0c98f0126..fafa7e25805b 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -978,7 +978,6 @@ impl State { Some(candidate_entry) => { // We might have received the approval before the assignment, so save it for processing after the assignment let cached_approval_vote = candidate_entry.messages.remove(&validator_index); - metrics.on_delayed_approval_processed(); ( candidate_entry.messages.entry(validator_index).or_insert_with(|| { MessageState { @@ -1085,6 +1084,8 @@ impl State { ?message_subject, "Delayed approval processed" ); + metrics.on_delayed_approval_processed(); + self.import_and_circulate_approval(ctx, metrics, source, vote).await } } From f24fc2f861fbfd1ac05f7d6ea33a390ccf024847 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 14:33:46 +0300 Subject: [PATCH 09/15] Add more metrics regarding gossipped approvals Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index fafa7e25805b..d0bdd08154d3 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1079,7 +1079,7 @@ impl State { .. }) = cached_approval_vote { - gum::info!( + gum::debug!( target: LOG_TARGET, ?message_subject, "Delayed approval processed" @@ -1170,7 +1170,7 @@ impl State { approval_state: ApprovalState::UnassignedApproval(vote, source), } }); - gum::info!( + gum::debug!( target: LOG_TARGET, ?peer_id, ?message_subject, @@ -1256,10 +1256,11 @@ impl State { let (tx, rx) = oneshot::channel(); if !sender_matches_validator_index { - gum::info!( + gum::debug!( target: LOG_TARGET, "Received gossiped approval" - ) + ); + metrics.on_gossipped_approval(); } ctx.send_message(ApprovalVotingMessage::CheckAndImportApproval( From 1c312edd2b0c937c795d4968faa0c66904b58f30 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 14:43:50 +0300 Subject: [PATCH 10/15] Add metric as well Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/metrics.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/node/network/approval-distribution/src/metrics.rs b/node/network/approval-distribution/src/metrics.rs index dab36c18cd5a..4d59ce34abbe 100644 --- a/node/network/approval-distribution/src/metrics.rs +++ b/node/network/approval-distribution/src/metrics.rs @@ -26,6 +26,7 @@ struct MetricsInner { approvals_imported_total: prometheus::Counter, unassigned_approval_total: prometheus::Counter, delayed_approvals_processed_total: prometheus::Counter, + gossipped_approval_total: prometheus::Counter, unified_with_peer_total: prometheus::Counter, aggression_l1_messages_total: prometheus::Counter, aggression_l2_messages_total: prometheus::Counter, @@ -54,6 +55,12 @@ impl Metrics { } } + pub(crate) fn on_gossipped_approval(&self) { + if let Some(metrics) = &self.0 { + metrics.gossipped_approval_total.inc(); + } + } + pub(crate) fn on_delayed_approval_processed(&self) { if let Some(metrics) = &self.0 { metrics.delayed_approvals_processed_total.inc(); @@ -123,6 +130,13 @@ impl MetricsTrait for Metrics { )?, registry, )?, + gossipped_approval_total: prometheus::register( + prometheus::Counter::new( + "polkadot_parachain_gossipped_approvals_processedtotal", + "Number of approvals processed that were gossiped", + )?, + registry, + )?, approvals_imported_total: prometheus::register( prometheus::Counter::new( "polkadot_parachain_approvals_imported_total", From 953bc16857f39512ccfb3f27fe9e4186c34f62ca Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 14 Jul 2023 15:14:33 +0300 Subject: [PATCH 11/15] Add more logs to understand why gossiping is still happening Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 14 +++++++------- node/network/protocol/src/grid_topology.rs | 11 ++++++++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index d0bdd08154d3..b0dc85304361 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1147,6 +1147,13 @@ impl State { }) .unwrap_or(false); + if !sender_matches_validator_index { + gum::debug!( + target: LOG_TARGET, + "Received gossiped approval topology some {:}", self.topologies.get_topology(entry.session).is_some() + ); + metrics.on_gossipped_approval(); + } if !entry.knowledge.contains(&message_subject, MessageKind::Assignment) { metrics.on_unassigned_approval(); @@ -1255,13 +1262,6 @@ impl State { } let (tx, rx) = oneshot::channel(); - if !sender_matches_validator_index { - gum::debug!( - target: LOG_TARGET, - "Received gossiped approval" - ); - metrics.on_gossipped_approval(); - } ctx.send_message(ApprovalVotingMessage::CheckAndImportApproval( vote.clone(), diff --git a/node/network/protocol/src/grid_topology.rs b/node/network/protocol/src/grid_topology.rs index 316d0dd437c8..3469c0f216e8 100644 --- a/node/network/protocol/src/grid_topology.rs +++ b/node/network/protocol/src/grid_topology.rs @@ -132,7 +132,16 @@ impl SessionGridTopology { peer_id: PeerId, ) -> bool { self.get_known_peer_ids_by_validator_index(validator_index) - .map(|known_peer_ids| known_peer_ids.contains(&peer_id)) + .map(|known_peer_ids| { + let res = known_peer_ids.contains(&peer_id); + if !res { + gum::debug!( + target: LOG_TARGET, + "Received gossiped approval peer id {:?} known_peer_ids {:?}", peer_id, known_peer_ids + ); + } + res + }) .unwrap_or(false) } } From 5f6edefb84530fa5644329978970142a7ffd5583 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 18 Jul 2023 12:38:19 +0300 Subject: [PATCH 12/15] add more metrics: to understand versi behaviour Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 50 +++++++++++++++---- .../approval-distribution/src/metrics.rs | 26 ++++++++-- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index b0dc85304361..4a4980e858e8 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -49,16 +49,17 @@ use std::{ }; // TODO: Disable will be removed in the final version and will be replaced with a runtime configuration -const ACTIVATION_BLOCK_NUMBER: u32 = 9000; - -fn disable_gossiping(block: u32) -> bool { - if block == ACTIVATION_BLOCK_NUMBER { - gum::info!( - target: LOG_TARGET, - "Disable gossiping for nodes" - ) - } - block > ACTIVATION_BLOCK_NUMBER +// const ACTIVATION_BLOCK_NUMBER: u32 = 10; + +fn disable_gossiping(_block: u32) -> bool { + // if block == ACTIVATION_BLOCK_NUMBER { + // gum::info!( + // target: LOG_TARGET, + // "Disable gossiping for nodes" + // ) + // } + // block > ACTIVATION_BLOCK_NUMBER + return true } use self::metrics::Metrics; @@ -378,6 +379,7 @@ impl State { topology.session, topology.topology, topology.local_index, + metrics, ) .await; }, @@ -547,6 +549,7 @@ impl State { session: SessionIndex, topology: SessionGridTopology, local_index: Option, + metrics: &Metrics, ) { if local_index.is_none() { // this subsystem only matters to validators. @@ -568,6 +571,7 @@ impl State { .required_routing_by_index(*validator_index, local); } }, + metrics, ) .await; } @@ -1152,7 +1156,7 @@ impl State { target: LOG_TARGET, "Received gossiped approval topology some {:}", self.topologies.get_topology(entry.session).is_some() ); - metrics.on_gossipped_approval(); + metrics.on_gossipped_received_approval(); } if !entry.knowledge.contains(&message_subject, MessageKind::Assignment) { metrics.on_unassigned_approval(); @@ -1464,6 +1468,13 @@ impl State { if !peers.is_empty() { let approvals = vec![vote]; + if source.peer_id().is_some() { + gum::debug!( + target: LOG_TARGET, + "Gossiped approval sent" + ); + metrics.on_gossipped_sent_approval(); + } gum::trace!( target: LOG_TARGET, ?block_hash, @@ -1650,6 +1661,13 @@ impl State { ) { if let Some(approval_message) = approval_message { if !peer_knowledge.contains(&message_subject, MessageKind::Approval) { + if !message_state.local { + gum::debug!( + target: LOG_TARGET, + "Approval gossiped in unify with peer", + ); + metrics.on_gossipped_sent_approval(); + } peer_knowledge .sent .insert(message_subject.clone(), MessageKind::Approval); @@ -1740,6 +1758,7 @@ impl State { } }, |_, _, _| {}, + metrics, ) .await; @@ -1784,6 +1803,7 @@ impl State { } } }, + metrics, ) .await; } @@ -1808,6 +1828,7 @@ async fn adjust_required_routing_and_propagate bool, RoutingModifier: Fn(&mut RequiredRouting, bool, &ValidatorIndex), @@ -1894,6 +1915,13 @@ async fn adjust_required_routing_and_propagate); struct MetricsInner { assignments_imported_total: prometheus::Counter, approvals_imported_total: prometheus::Counter, + unassigned_approval_total: prometheus::Counter, delayed_approvals_processed_total: prometheus::Counter, - gossipped_approval_total: prometheus::Counter, + gossipped_approval_received_total: prometheus::Counter, + gossipped_approval_sent_total: prometheus::Counter, + unified_with_peer_total: prometheus::Counter, aggression_l1_messages_total: prometheus::Counter, aggression_l2_messages_total: prometheus::Counter, @@ -55,9 +58,15 @@ impl Metrics { } } - pub(crate) fn on_gossipped_approval(&self) { + pub(crate) fn on_gossipped_received_approval(&self) { + if let Some(metrics) = &self.0 { + metrics.gossipped_approval_received_total.inc(); + } + } + + pub(crate) fn on_gossipped_sent_approval(&self) { if let Some(metrics) = &self.0 { - metrics.gossipped_approval_total.inc(); + metrics.gossipped_approval_sent_total.inc(); } } @@ -130,9 +139,16 @@ impl MetricsTrait for Metrics { )?, registry, )?, - gossipped_approval_total: prometheus::register( + gossipped_approval_received_total: prometheus::register( + prometheus::Counter::new( + "polkadot_parachain_gossipped_approvals_receivedtotal", + "Number of approvals processed that were gossiped", + )?, + registry, + )?, + gossipped_approval_sent_total: prometheus::register( prometheus::Counter::new( - "polkadot_parachain_gossipped_approvals_processedtotal", + "polkadot_parachain_gossipped_approvals_senttotal", "Number of approvals processed that were gossiped", )?, registry, From b09a619d6d8cb986c49a4ca4902d59dce76f96b9 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 18 Jul 2023 15:30:45 +0300 Subject: [PATCH 13/15] fix: gossiped approvals received metric Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 4a4980e858e8..670e0b2e1eac 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1151,13 +1151,6 @@ impl State { }) .unwrap_or(false); - if !sender_matches_validator_index { - gum::debug!( - target: LOG_TARGET, - "Received gossiped approval topology some {:}", self.topologies.get_topology(entry.session).is_some() - ); - metrics.on_gossipped_received_approval(); - } if !entry.knowledge.contains(&message_subject, MessageKind::Assignment) { metrics.on_unassigned_approval(); @@ -1264,7 +1257,13 @@ impl State { } return } - + if !sender_matches_validator_index { + gum::debug!( + target: LOG_TARGET, + "Received gossiped approval topology some {:}", self.topologies.get_topology(entry.session).is_some() + ); + metrics.on_gossipped_received_approval(); + } let (tx, rx) = oneshot::channel(); ctx.send_message(ApprovalVotingMessage::CheckAndImportApproval( @@ -1471,7 +1470,7 @@ impl State { if source.peer_id().is_some() { gum::debug!( target: LOG_TARGET, - "Gossiped approval sent" + "Gossiped approval sent num_peers {:} approvals {:?}", peers.len(), approvals, ); metrics.on_gossipped_sent_approval(); } From 2af3525da4d6c06e693bfc2bd35ce6b9d1926990 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 18 Jul 2023 15:41:02 +0300 Subject: [PATCH 14/15] Add more logs to debug metrics Signed-off-by: Alexandru Gheorghe --- node/network/approval-distribution/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/node/network/approval-distribution/src/lib.rs b/node/network/approval-distribution/src/lib.rs index 670e0b2e1eac..0e625ff7930b 100644 --- a/node/network/approval-distribution/src/lib.rs +++ b/node/network/approval-distribution/src/lib.rs @@ -1260,7 +1260,9 @@ impl State { if !sender_matches_validator_index { gum::debug!( target: LOG_TARGET, - "Received gossiped approval topology some {:}", self.topologies.get_topology(entry.session).is_some() + "Received gossiped approval topology some {:} peer_id {:}", + self.topologies.get_topology(entry.session).is_some(), + source.peer_id().map(|peer_id| peer_id.to_string()).unwrap_or("unknown".into()) ); metrics.on_gossipped_received_approval(); } From 8abed2a1a3b9730a359ffcfd1c832523565214d3 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 20 Jul 2023 15:48:46 +0300 Subject: [PATCH 15/15] Add scaffolding to be able to test aggression works ... add option to malus nodes that witholds messages sent by approval-distribution to simulate approvals couldn't be sent to some of the peers and test finality is not lagging Signed-off-by: Alexandru Gheorghe --- Cargo.lock | 1 + node/malus/Cargo.toml | 2 + node/malus/src/interceptor.rs | 2 +- node/malus/src/malus.rs | 12 + node/malus/src/variants/common.rs | 2 +- node/malus/src/variants/mod.rs | 4 + .../src/variants/suggest_garbage_candidate.rs | 2 +- .../withold_approvals_distribution.rs | 311 ++++++++++++++++++ node/service/src/relay_chain_selection.rs | 1 + .../0005-parachains-aggression-enabled.toml | 130 ++++++++ .../0005-parachains-aggression-enabled.zndsl | 21 ++ 11 files changed, 485 insertions(+), 3 deletions(-) create mode 100644 node/malus/src/variants/withold_approvals_distribution.rs create mode 100644 zombienet_tests/functional/0005-parachains-aggression-enabled.toml create mode 100644 zombienet_tests/functional/0005-parachains-aggression-enabled.zndsl diff --git a/Cargo.lock b/Cargo.lock index f319a8db8e71..c2480647ccef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8257,6 +8257,7 @@ dependencies = [ "polkadot-node-core-dispute-coordinator", "polkadot-node-core-pvf-execute-worker", "polkadot-node-core-pvf-prepare-worker", + "polkadot-node-network-protocol", "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-node-subsystem-test-helpers", diff --git a/node/malus/Cargo.toml b/node/malus/Cargo.toml index 8e23e623174f..0606fa6cd36e 100644 --- a/node/malus/Cargo.toml +++ b/node/malus/Cargo.toml @@ -24,6 +24,8 @@ polkadot-node-core-pvf-execute-worker = { path = "../core/pvf/execute-worker" } polkadot-node-core-pvf-prepare-worker = { path = "../core/pvf/prepare-worker" } polkadot-node-primitives = { path = "../primitives" } polkadot-primitives = { path = "../../primitives" } +polkadot-node-network-protocol = { path = "../network/protocol" } + color-eyre = { version = "0.6.1", default-features = false } assert_matches = "1.5" async-trait = "0.1.57" diff --git a/node/malus/src/interceptor.rs b/node/malus/src/interceptor.rs index cbf39bccd160..854100dbc3ae 100644 --- a/node/malus/src/interceptor.rs +++ b/node/malus/src/interceptor.rs @@ -40,7 +40,7 @@ where /// For non-trivial cases, the `sender` can be used to send /// multiple messages after doing some additional processing. fn intercept_incoming( - &self, + &mut self, _sender: &mut Sender, msg: FromOrchestra, ) -> Option> { diff --git a/node/malus/src/malus.rs b/node/malus/src/malus.rs index d09f8be990a4..0be3ae9e921c 100644 --- a/node/malus/src/malus.rs +++ b/node/malus/src/malus.rs @@ -36,6 +36,8 @@ enum NemesisVariant { BackGarbageCandidate(BackGarbageCandidateOptions), /// Delayed disputing of ancestors that are perfectly fine. DisputeAncestor(DisputeAncestorOptions), + /// Do not distribute approvals to all nodes + WitholdApprovalsDistribution(WitholdApprovalsDistributionOptions), #[allow(missing_docs)] #[command(name = "prepare-worker", hide = true)] @@ -59,6 +61,7 @@ impl MalusCli { /// Launch a malus node. fn launch(self) -> eyre::Result<()> { let finality_delay = self.finality_delay; + match self.variant { NemesisVariant::BackGarbageCandidate(opts) => { let BackGarbageCandidateOptions { percentage, cli } = opts; @@ -88,6 +91,15 @@ impl MalusCli { finality_delay, )? }, + NemesisVariant::WitholdApprovalsDistribution(WitholdApprovalsDistributionOptions { + num_network_groups, + assigned_network_group, + cli, + }) => polkadot_cli::run_node( + cli, + WitholdApprovalsDistribution { num_network_groups, assigned_network_group }, + finality_delay, + )?, NemesisVariant::PvfPrepareWorker(cmd) => { #[cfg(target_os = "android")] { diff --git a/node/malus/src/variants/common.rs b/node/malus/src/variants/common.rs index 4ea8b88b56a5..f07043e56025 100644 --- a/node/malus/src/variants/common.rs +++ b/node/malus/src/variants/common.rs @@ -205,7 +205,7 @@ where // Capture all (approval and backing) candidate validation requests and depending on configuration fail them. fn intercept_incoming( - &self, + &mut self, subsystem_sender: &mut Sender, msg: FromOrchestra, ) -> Option> { diff --git a/node/malus/src/variants/mod.rs b/node/malus/src/variants/mod.rs index 3789f33ac98b..8144665e9f66 100644 --- a/node/malus/src/variants/mod.rs +++ b/node/malus/src/variants/mod.rs @@ -20,10 +20,14 @@ mod back_garbage_candidate; mod common; mod dispute_valid_candidates; mod suggest_garbage_candidate; +mod withold_approvals_distribution; pub(crate) use self::{ back_garbage_candidate::{BackGarbageCandidateOptions, BackGarbageCandidates}, dispute_valid_candidates::{DisputeAncestorOptions, DisputeValidCandidates}, suggest_garbage_candidate::{SuggestGarbageCandidateOptions, SuggestGarbageCandidates}, + withold_approvals_distribution::{ + WitholdApprovalsDistribution, WitholdApprovalsDistributionOptions, + }, }; pub(crate) use common::*; diff --git a/node/malus/src/variants/suggest_garbage_candidate.rs b/node/malus/src/variants/suggest_garbage_candidate.rs index 049cfc2b153d..4326c0ce62bc 100644 --- a/node/malus/src/variants/suggest_garbage_candidate.rs +++ b/node/malus/src/variants/suggest_garbage_candidate.rs @@ -72,7 +72,7 @@ where /// Intercept incoming `Second` requests from the `collator-protocol` subsystem. fn intercept_incoming( - &self, + &mut self, subsystem_sender: &mut Sender, msg: FromOrchestra, ) -> Option> { diff --git a/node/malus/src/variants/withold_approvals_distribution.rs b/node/malus/src/variants/withold_approvals_distribution.rs new file mode 100644 index 000000000000..20e81ebe1800 --- /dev/null +++ b/node/malus/src/variants/withold_approvals_distribution.rs @@ -0,0 +1,311 @@ +// Copyright (C) Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +//! This variant of Malus withold sending of approvals coming from approvals +//! distribution subsystem to just a subset of nodes. It is meant to be used for testing +//! finality is reached even nodes can not reach directly each other. +//! It enforces that peers are grouped in groups of fixed size, then it arranges +//! the groups in a ring topology. +//! +//! +//! Peers can send their messages only to peers in their group and to peers from next +//! group in the ring topology. +//! E.g If we 16 nodes split in 4 groups then: +//! (1, 2, 3, 4) -> (5, 6, 7, 8) -> (9, 10, 11, 12) -> (13, 14, 15, 16 ) +//! ^ | +//! |<------------------------------------------------------| +//! +//! Then node 5 will be able to send messages only to nodes in it's group (6, 7, 8) and to nodes +//! in the next group 9, 10, 11, 12 + +use polkadot_cli::{ + prepared_overseer_builder, + service::{ + AuthorityDiscoveryApi, AuxStore, BabeApi, Block, Error, HeaderBackend, Overseer, + OverseerConnector, OverseerGen, OverseerGenArgs, OverseerHandle, ParachainHost, + ProvideRuntimeApi, + }, + Cli, +}; +use polkadot_node_subsystem::SpawnGlue; +use polkadot_node_subsystem_types::messages::network_bridge_event::PeerId; +use sp_core::traits::SpawnNamed; + +use crate::interceptor::*; +use polkadot_node_network_protocol::{v1 as protocol_v1, Versioned}; + +use std::sync::Arc; +const LOG_TARGET: &str = "parachain::withold-approvals"; + +#[derive(Debug, clap::Parser)] +#[clap(rename_all = "kebab-case")] +#[allow(missing_docs)] +pub struct WitholdApprovalsDistributionOptions { + /// Determines how many groups to create, the groups are connected in a ring shape, so + /// a node from group N can send messages only to groups from N+1 and will receive messages from N-1 + /// This should helps us test that approval distribution works correctly even in situations where + /// all nodes can't reach each other. + #[clap(short, long, ignore_case = true, default_value_t = 4, value_parser = clap::value_parser!(u8).range(0..=100))] + pub num_network_groups: u8, + + /// The group to which this node is assigned + #[clap(short, long, ignore_case = true, default_value_t = 0, value_parser = clap::value_parser!(u8).range(0..=100))] + pub assigned_network_group: u8, + + #[clap(flatten)] + pub cli: Cli, +} + +pub(crate) struct WitholdApprovalsDistribution { + pub num_network_groups: u8, + pub assigned_network_group: u8, +} + +impl OverseerGen for WitholdApprovalsDistribution { + fn generate<'a, Spawner, RuntimeClient>( + &self, + connector: OverseerConnector, + args: OverseerGenArgs<'a, Spawner, RuntimeClient>, + ) -> Result<(Overseer, Arc>, OverseerHandle), Error> + where + RuntimeClient: 'static + ProvideRuntimeApi + HeaderBackend + AuxStore, + RuntimeClient::Api: ParachainHost + BabeApi + AuthorityDiscoveryApi, + Spawner: 'static + SpawnNamed + Clone + Unpin, + { + let spawner = args.spawner.clone(); + let validation_filter = ApprovalsDistributionInterceptor::new( + SpawnGlue(spawner), + self.num_network_groups, + self.assigned_network_group, + ); + + prepared_overseer_builder(args)? + .replace_network_bridge_tx(move |cv_subsystem| { + InterceptedSubsystem::new(cv_subsystem, validation_filter) + }) + .build_with_connector(connector) + .map_err(|e| e.into()) + } +} + +#[derive(Clone, Debug)] +/// Replaces `NetworkBridgeTx`. +pub struct ApprovalsDistributionInterceptor { + spawner: Spawner, + known_peer_ids: Vec, + num_network_groups: u8, + assigned_network_group: u8, +} + +impl ApprovalsDistributionInterceptor +where + Spawner: overseer::gen::Spawner, +{ + pub fn new(spawner: Spawner, num_network_groups: u8, assigned_network_group: u8) -> Self { + Self { spawner, known_peer_ids: vec![], num_network_groups, assigned_network_group } + } + + fn can_send(&self, peer_id: PeerId) -> bool { + let group_size = self.known_peer_ids.len() / (self.num_network_groups as usize) + + if self.known_peer_ids.len() % self.num_network_groups as usize != 0 { 1 } else { 0 }; + + let my_group_in_ring = self + .known_peer_ids + .chunks(group_size) + .skip(self.assigned_network_group as usize) + .next(); + + let next_group_in_ring = self + .known_peer_ids + .chunks(group_size) + .skip((self.assigned_network_group as usize + 1) % self.num_network_groups as usize) + .next(); + + my_group_in_ring + .map(|my_group_peers| my_group_peers.contains(&peer_id)) + .unwrap_or(false) || + next_group_in_ring + .map(|next_group_peers| next_group_peers.contains(&peer_id)) + .unwrap_or(false) + } +} + +impl MessageInterceptor for ApprovalsDistributionInterceptor +where + Sender: overseer::NetworkBridgeTxSenderTrait + Clone + Send + 'static, + Spawner: overseer::gen::Spawner + Clone + 'static, +{ + type Message = NetworkBridgeTxMessage; + + // Capture all (approval and backing) candidate validation requests and depending on configuration fail them. + fn intercept_incoming( + &mut self, + _subsystem_sender: &mut Sender, + msg: FromOrchestra, + ) -> Option> { + match msg { + // Message sent by the approval voting subsystem + FromOrchestra::Communication { + msg: NetworkBridgeTxMessage::SendValidationMessage(peers, message), + } => { + let new_peers: Vec<&PeerId> = + peers.iter().filter(|peer_id| !self.known_peer_ids.contains(peer_id)).collect(); + if !new_peers.is_empty() { + self.known_peer_ids.extend(new_peers); + self.known_peer_ids.sort(); + } + + match &message { + Versioned::V1(protocol_v1::ValidationProtocol::ApprovalDistribution( + protocol_v1::ApprovalDistributionMessage::Approvals(approvals), + )) => { + let num_peers_we_wanted = peers.len(); + let peers_we_can_send: Vec = + peers.into_iter().filter(|peer_id| self.can_send(*peer_id)).collect(); + gum::info!( + target: LOG_TARGET, + "Malus message intercepted num_peers_we_can_send {:} num_peers_we_wanted_to_send {:} known_peers {:} peers {:} approval {:?}", + peers_we_can_send.len(), + num_peers_we_wanted, + self.known_peer_ids.len(), + peers_we_can_send + .clone() + .into_iter() + .fold(String::new(), |accumulator, peer| { + format!("{}:{}", accumulator, peer) + }), + approvals + ); + Some(FromOrchestra::Communication { + msg: NetworkBridgeTxMessage::SendValidationMessage( + peers_we_can_send, + message, + ), + }) + }, + _ => Some(FromOrchestra::Communication { + msg: NetworkBridgeTxMessage::SendValidationMessage(peers, message), + }), + } + }, + msg => Some(msg), + } + } + + fn intercept_outgoing( + &self, + msg: overseer::NetworkBridgeTxOutgoingMessages, + ) -> Option { + Some(msg) + } +} + +#[cfg(test)] +mod tests { + use polkadot_node_network_protocol::PeerId; + use polkadot_node_subsystem::Spawner; + + use super::ApprovalsDistributionInterceptor; + + #[derive(Debug, Clone)] + struct DummySpanner; + impl Spawner for DummySpanner { + fn spawn_blocking( + &self, + _name: &'static str, + _group: Option<&'static str>, + _future: futures::future::BoxFuture<'static, ()>, + ) { + todo!() + } + + fn spawn( + &self, + _name: &'static str, + _group: Option<&'static str>, + _future: futures::future::BoxFuture<'static, ()>, + ) { + todo!() + } + } + + #[test] + fn test_can_send() { + let test_topologies: Vec> = vec![ + (1..20).map(|_| PeerId::random()).collect(), + (1..21).map(|_| PeerId::random()).collect(), + ]; + + for peer_ids in test_topologies { + let withold_approval_distribution = ApprovalsDistributionInterceptor { + spawner: DummySpanner {}, + known_peer_ids: peer_ids.clone(), + assigned_network_group: 1, + num_network_groups: 4, + }; + + assert!(!withold_approval_distribution.can_send(peer_ids[0])); + assert!(!withold_approval_distribution.can_send(peer_ids[4])); + + assert!(withold_approval_distribution.can_send(peer_ids[5])); + assert!(withold_approval_distribution.can_send(peer_ids[9])); + + assert!(withold_approval_distribution.can_send(peer_ids[10])); + assert!(withold_approval_distribution.can_send(peer_ids[14])); + + assert!(!withold_approval_distribution.can_send(peer_ids[15])); + assert!(!withold_approval_distribution.can_send(peer_ids[18])); + + let withold_approval_distribution = ApprovalsDistributionInterceptor { + spawner: DummySpanner {}, + known_peer_ids: peer_ids.clone(), + assigned_network_group: 3, + num_network_groups: 4, + }; + + assert!(withold_approval_distribution.can_send(peer_ids[0])); + assert!(withold_approval_distribution.can_send(peer_ids[4])); + + assert!(!withold_approval_distribution.can_send(peer_ids[5])); + assert!(!withold_approval_distribution.can_send(peer_ids[9])); + + assert!(!withold_approval_distribution.can_send(peer_ids[10])); + assert!(!withold_approval_distribution.can_send(peer_ids[14])); + + assert!(withold_approval_distribution.can_send(peer_ids[15])); + assert!(withold_approval_distribution.can_send(peer_ids[18])); + + let withold_approval_distribution = ApprovalsDistributionInterceptor { + spawner: DummySpanner {}, + known_peer_ids: peer_ids.clone(), + assigned_network_group: 0, + num_network_groups: 4, + }; + + assert!(withold_approval_distribution.can_send(peer_ids[0])); + assert!(withold_approval_distribution.can_send(peer_ids[4])); + + assert!(withold_approval_distribution.can_send(peer_ids[5])); + assert!(withold_approval_distribution.can_send(peer_ids[9])); + + assert!(!withold_approval_distribution.can_send(peer_ids[10])); + assert!(!withold_approval_distribution.can_send(peer_ids[14])); + + assert!(!withold_approval_distribution.can_send(peer_ids[15])); + assert!(!withold_approval_distribution.can_send(peer_ids[18])); + } + } +} diff --git a/node/service/src/relay_chain_selection.rs b/node/service/src/relay_chain_selection.rs index afc0ce320610..4d5f4d2c91b0 100644 --- a/node/service/src/relay_chain_selection.rs +++ b/node/service/src/relay_chain_selection.rs @@ -471,6 +471,7 @@ where let lag = initial_leaf_number.saturating_sub(subchain_number); self.metrics.note_approval_checking_finality_lag(lag); + gum::debug!(target: LOG_TARGET, ?subchain_head, "Approval checking lag {:}", lag); // Messages sent to `approval-distrbution` are known to have high `ToF`, we need to spawn a task for sending // the message to not block here and delay finality. diff --git a/zombienet_tests/functional/0005-parachains-aggression-enabled.toml b/zombienet_tests/functional/0005-parachains-aggression-enabled.toml new file mode 100644 index 000000000000..1d94465d00d3 --- /dev/null +++ b/zombienet_tests/functional/0005-parachains-aggression-enabled.toml @@ -0,0 +1,130 @@ +[settings] +timeout = 1000 + +[relaychain.genesis.runtime.runtime_genesis_config.configuration.config] + needed_approvals = 8 + +[relaychain] +default_image = "{{ZOMBIENET_INTEGRATION_TEST_IMAGE}}" +chain = "rococo-local" +chain_spec_command = "polkadot build-spec --chain rococo-local --disable-default-bootnode" + +[relaychain.default_resources] +limits = { memory = "4G", cpu = "2" } +requests = { memory = "2G", cpu = "1" } + + [[relaychain.nodes]] + name = "alice" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=0" + args = [ "--alice", "-lparachain=debug,runtime=debug" ] + + [[relaychain.nodes]] + name = "bob" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=0" + args = [ "--bob", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "charlie" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=0" + args = [ "--charlie", "-lparachain=debug,runtime=debug" ] + + [[relaychain.nodes]] + name = "dave" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=0" + args = [ "--dave", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "ferdie" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=1" + args = [ "--ferdie", "-lparachain=debug,runtime=debug" ] + + [[relaychain.nodes]] + name = "eve" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=1" + args = [ "--eve", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "one" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=1" + args = [ "--one", "-lparachain=debug,runtime=debug" ] + + [[relaychain.nodes]] + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=1" + name = "two" + args = [ "--two", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode8" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=2" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode9" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=2" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode10" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=2" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode11" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=2" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode12" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=3" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode13" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=3" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode14" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=3" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode15" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=3" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode16" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=4" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode17" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=4" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode18" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=4" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + + [[relaychain.nodes]] + name = "newnode19" + command = "malus withold-approvals-distribution --num-network-groups=5 --assigned-network-group=4" + args = [ "--validator", "-lparachain=debug,runtime=debug"] + +[[parachains]] +id = 2000 +addToGenesis = true +genesis_state_generator = "undying-collator export-genesis-state --pov-size=100000 --pvf-complexity=1" + + [parachains.collator] + name = "collator01" + image = "{{COL_IMAGE}}" + command = "undying-collator" + args = ["-lparachain=debug", "--pov-size=100000", "--pvf-complexity=1", "--parachain-id=2000"] + +[types.Header] +number = "u64" +parent_hash = "Hash" +post_state = "Hash" \ No newline at end of file diff --git a/zombienet_tests/functional/0005-parachains-aggression-enabled.zndsl b/zombienet_tests/functional/0005-parachains-aggression-enabled.zndsl new file mode 100644 index 000000000000..c8c7956d69c6 --- /dev/null +++ b/zombienet_tests/functional/0005-parachains-aggression-enabled.zndsl @@ -0,0 +1,21 @@ +Description: PVF preparation & execution time +Network: ./0005-parachains-aggression-enabled.toml +Creds: config + +# Check authority status. +alice: reports node_roles is 4 +bob: reports node_roles is 4 +charlie: reports node_roles is 4 +dave: reports node_roles is 4 +eve: reports node_roles is 4 +ferdie: reports node_roles is 4 +one: reports node_roles is 4 +two: reports node_roles is 4 + +# Ensure parachains are registered. +alice: parachain 2000 is registered within 60 seconds + + +# Ensure parachains made progress. +# alice: parachain 2000 block height is at least 30 within 600 seconds +alice: reports substrate_block_height{status="finalized"} is at least 40 within 400 seconds \ No newline at end of file