From 5291601ff4948c9181e4f8edddb00c18fab7ba19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 17 Oct 2023 18:14:13 +0200 Subject: [PATCH 1/8] Do not force collators to update after enabling async backing --- .../src/validator_side/mod.rs | 68 +++++++++++-------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index fcb408d54b1b..f38df3e4d78c 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -224,6 +224,7 @@ impl PeerData { &mut self, on_relay_parent: Hash, relay_parent_mode: ProspectiveParachainsMode, + use_non_prospective_chain_mode: bool, candidate_hash: Option, implicit_view: &ImplicitView, active_leaves: &HashMap, @@ -241,33 +242,44 @@ impl PeerData { return Err(InsertAdvertisementError::OutOfOurView) } - match (relay_parent_mode, candidate_hash) { - (ProspectiveParachainsMode::Disabled, candidate_hash) => { + let insert_non_prospective_chains_mode = + |state: &mut CollatingPeerState, candidate_hash: Option| { if state.advertisements.contains_key(&on_relay_parent) { return Err(InsertAdvertisementError::Duplicate) } state .advertisements .insert(on_relay_parent, HashSet::from_iter(candidate_hash)); + Ok(()) + }; + + match (relay_parent_mode, candidate_hash) { + (ProspectiveParachainsMode::Disabled, candidate_hash) => { + insert_non_prospective_chains_mode(&mut state, candidate_hash)?; }, ( ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, Some(candidate_hash), - ) => { - if state - .advertisements - .get(&on_relay_parent) - .map_or(false, |candidates| candidates.contains(&candidate_hash)) - { - return Err(InsertAdvertisementError::Duplicate) - } - let candidates = state.advertisements.entry(on_relay_parent).or_default(); - - if candidates.len() > max_candidate_depth { - return Err(InsertAdvertisementError::PeerLimitReached) - } - candidates.insert(candidate_hash); - }, + ) => + if use_non_prospective_chain_mode { + insert_non_prospective_chains_mode(&mut state, candidate_hash)?; + } else { + if state + .advertisements + .get(&on_relay_parent) + .map_or(false, |candidates| candidates.contains(&candidate_hash)) + { + return Err(InsertAdvertisementError::Duplicate) + } + + let candidates = + state.advertisements.entry(on_relay_parent).or_default(); + + if candidates.len() > max_candidate_depth { + return Err(InsertAdvertisementError::PeerLimitReached) + } + candidates.insert(candidate_hash); + }, _ => return Err(InsertAdvertisementError::ProtocolMismatch), } @@ -1042,6 +1054,10 @@ where .get(&relay_parent) .map(|s| s.child("advertise-collation")); + // If the collator is speaking to us using `v1` networking protocol, it can not use prospective + // chains mode, even if it is enabled in the runtime. + let use_non_prospective_chains_mode = prospective_candidate.is_none(); + let per_relay_parent = state .per_relay_parent .get(&relay_parent) @@ -1054,16 +1070,9 @@ where let collator_para_id = peer_data.collating_para().ok_or(AdvertisementError::UndeclaredCollator)?; - match assignment.current { - Some(id) if id == collator_para_id => { - // Our assignment. - }, - _ => return Err(AdvertisementError::InvalidAssignment), - }; - - if relay_parent_mode.is_enabled() && prospective_candidate.is_none() { - // Expected v2 advertisement. - return Err(AdvertisementError::ProtocolMismatch) + // Check if this is assigned to us. + if assignment.current.map_or(true, |id| id != collator_para_id) { + return Err(AdvertisementError::InvalidAssignment) } // Always insert advertisements that pass all the checks for spam protection. @@ -1072,17 +1081,19 @@ where .insert_advertisement( relay_parent, relay_parent_mode, + use_non_prospective_chains_mode, candidate_hash, &state.implicit_view, &state.active_leaves, ) .map_err(AdvertisementError::Invalid)?; + if !per_relay_parent.collations.is_seconded_limit_reached(relay_parent_mode) { return Err(AdvertisementError::SecondedLimitReached) } if let Some((candidate_hash, parent_head_data_hash)) = prospective_candidate { - let is_seconding_allowed = !relay_parent_mode.is_enabled() || + let is_seconding_allowed = !use_non_prospective_chains_mode || can_second( sender, collator_para_id, @@ -1125,6 +1136,7 @@ where prospective_candidate, ) .await; + if let Err(fetch_error) = result { gum::debug!( target: LOG_TARGET, From 66a092506a134f8fbd98bcb079bb49a8f9238c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 17 Oct 2023 21:30:29 +0200 Subject: [PATCH 2/8] Some review comments --- .../src/validator_side/mod.rs | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index f38df3e4d78c..901eb2d5286e 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -224,7 +224,6 @@ impl PeerData { &mut self, on_relay_parent: Hash, relay_parent_mode: ProspectiveParachainsMode, - use_non_prospective_chain_mode: bool, candidate_hash: Option, implicit_view: &ImplicitView, active_leaves: &HashMap, @@ -255,14 +254,14 @@ impl PeerData { match (relay_parent_mode, candidate_hash) { (ProspectiveParachainsMode::Disabled, candidate_hash) => { - insert_non_prospective_chains_mode(&mut state, candidate_hash)?; + insert_non_prospective_chains_mode(state, candidate_hash)?; }, ( ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, Some(candidate_hash), ) => - if use_non_prospective_chain_mode { - insert_non_prospective_chains_mode(&mut state, candidate_hash)?; + if self.version == CollationVersion::V1 { + insert_non_prospective_chains_mode(state, Some(candidate_hash))?; } else { if state .advertisements @@ -932,8 +931,6 @@ enum AdvertisementError { UndeclaredCollator, /// We're assigned to a different para at the given relay parent. InvalidAssignment, - /// An advertisement format doesn't match the relay parent. - ProtocolMismatch, /// Para reached a limit of seconded candidates for this relay parent. SecondedLimitReached, /// Advertisement is invalid. @@ -946,7 +943,7 @@ impl AdvertisementError { match self { InvalidAssignment => Some(COST_WRONG_PARA), RelayParentUnknown | UndeclaredCollator | Invalid(_) => Some(COST_UNEXPECTED_MESSAGE), - UnknownPeer | ProtocolMismatch | SecondedLimitReached => None, + UnknownPeer | SecondedLimitReached => None, } } } @@ -1054,10 +1051,6 @@ where .get(&relay_parent) .map(|s| s.child("advertise-collation")); - // If the collator is speaking to us using `v1` networking protocol, it can not use prospective - // chains mode, even if it is enabled in the runtime. - let use_non_prospective_chains_mode = prospective_candidate.is_none(); - let per_relay_parent = state .per_relay_parent .get(&relay_parent) @@ -1081,7 +1074,6 @@ where .insert_advertisement( relay_parent, relay_parent_mode, - use_non_prospective_chains_mode, candidate_hash, &state.implicit_view, &state.active_leaves, @@ -1093,15 +1085,14 @@ where } if let Some((candidate_hash, parent_head_data_hash)) = prospective_candidate { - let is_seconding_allowed = !use_non_prospective_chains_mode || - can_second( - sender, - collator_para_id, - relay_parent, - candidate_hash, - parent_head_data_hash, - ) - .await; + let is_seconding_allowed = can_second( + sender, + collator_para_id, + relay_parent, + candidate_hash, + parent_head_data_hash, + ) + .await; if !is_seconding_allowed { gum::debug!( From a18078c6a24269b0adf602b0c45ce7e69081c7ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 18 Oct 2023 11:31:16 +0200 Subject: [PATCH 3/8] Fix tests --- .../src/validator_side/mod.rs | 72 ++++++++++--------- .../src/validator_side/tests/mod.rs | 23 ++++-- .../tests/prospective_parachains.rs | 14 ++-- 3 files changed, 63 insertions(+), 46 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 901eb2d5286e..4fd563d713e3 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -22,7 +22,6 @@ use std::{ collections::{hash_map::Entry, HashMap, HashSet}, convert::TryInto, future::Future, - iter::FromIterator, time::{Duration, Instant}, }; use tokio_util::sync::CancellationToken; @@ -241,45 +240,50 @@ impl PeerData { return Err(InsertAdvertisementError::OutOfOurView) } - let insert_non_prospective_chains_mode = - |state: &mut CollatingPeerState, candidate_hash: Option| { - if state.advertisements.contains_key(&on_relay_parent) { - return Err(InsertAdvertisementError::Duplicate) - } - state - .advertisements - .insert(on_relay_parent, HashSet::from_iter(candidate_hash)); - Ok(()) - }; - match (relay_parent_mode, candidate_hash) { - (ProspectiveParachainsMode::Disabled, candidate_hash) => { - insert_non_prospective_chains_mode(state, candidate_hash)?; - }, - ( - ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, - Some(candidate_hash), - ) => + (ProspectiveParachainsMode::Enabled { .. }, None) | + (ProspectiveParachainsMode::Disabled, None) => if self.version == CollationVersion::V1 { - insert_non_prospective_chains_mode(state, Some(candidate_hash))?; - } else { - if state - .advertisements - .get(&on_relay_parent) - .map_or(false, |candidates| candidates.contains(&candidate_hash)) - { + if state.advertisements.contains_key(&on_relay_parent) { return Err(InsertAdvertisementError::Duplicate) } + state.advertisements.insert(on_relay_parent, HashSet::new()); + } else { + gum::error!( + target: LOG_TARGET, + "Inserting advertisment: V2 without a candidate hash", + ); + return Err(InsertAdvertisementError::ProtocolMismatch) + }, + ( + ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, + Some(candidate_hash), + ) => { + if state + .advertisements + .get(&on_relay_parent) + .map_or(false, |candidates| candidates.contains(&candidate_hash)) + { + return Err(InsertAdvertisementError::Duplicate) + } - let candidates = - state.advertisements.entry(on_relay_parent).or_default(); + let candidates = state.advertisements.entry(on_relay_parent).or_default(); - if candidates.len() > max_candidate_depth { - return Err(InsertAdvertisementError::PeerLimitReached) - } - candidates.insert(candidate_hash); - }, - _ => return Err(InsertAdvertisementError::ProtocolMismatch), + if candidates.len() > max_candidate_depth { + return Err(InsertAdvertisementError::PeerLimitReached) + } + candidates.insert(candidate_hash); + }, + (mode, candidate_hash) => { + gum::error!( + target: LOG_TARGET, + ?mode, + hash = ?candidate_hash, + "Inserting advertisment: protocol mismatch", + ); + + return Err(InsertAdvertisementError::ProtocolMismatch) + }, } state.last_active = Instant::now(); diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs index 9812998aab76..ea67aeaa2604 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs @@ -510,17 +510,14 @@ fn act_on_advertisement_v2() { let pair = CollatorPair::generate().0; gum::trace!("activating"); - overseer_send( + prospective_parachains::update_view( &mut virtual_overseer, - CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange( - our_view![test_state.relay_parent], - )), + &test_state, + vec![(test_state.relay_parent, 0)], + 1, ) .await; - assert_async_backing_params_request(&mut virtual_overseer, test_state.relay_parent).await; - respond_to_core_info_queries(&mut virtual_overseer, &test_state).await; - let peer_b = PeerId::random(); connect_and_declare_collator( @@ -543,6 +540,18 @@ fn act_on_advertisement_v2() { ) .await; + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CandidateBacking( + CandidateBackingMessage::CanSecond(request, tx), + ) => { + assert_eq!(request.candidate_hash, candidate_hash); + assert_eq!(request.candidate_para_id, test_state.chain_ids[0]); + assert_eq!(request.parent_head_data_hash, parent_head_data_hash); + tx.send(true).expect("receiving side should be alive"); + } + ); + assert_fetch_collation_request( &mut virtual_overseer, test_state.relay_parent, diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs index 4da0f11da390..80f48f860bfc 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs @@ -74,7 +74,7 @@ async fn assert_assign_incoming( } /// Handle a view update. -async fn update_view( +pub(super) async fn update_view( virtual_overseer: &mut VirtualOverseer, test_state: &TestState, new_view: Vec<(Hash, u32)>, // Hash and block number. @@ -240,7 +240,7 @@ async fn assert_collation_seconded( } #[test] -fn v1_advertisement_rejected() { +fn v1_advertisement_accepted() { let test_state = TestState::default(); test_harness(ReputationAggregator::new(|_| true), |test_harness| async move { @@ -267,9 +267,13 @@ fn v1_advertisement_rejected() { advertise_collation(&mut virtual_overseer, peer_a, head_b, None).await; - // Not reported. - test_helpers::Yield::new().await; - assert_matches!(virtual_overseer.recv().now_or_never(), None); + assert_fetch_collation_request( + &mut virtual_overseer, + head_b, + test_state.chain_ids[0], + None, + ) + .await; virtual_overseer }); From 506d690ef3c15ba51b75a8526bf77a1f85ee282c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 18 Oct 2023 13:30:21 +0200 Subject: [PATCH 4/8] Fix more bugs --- .../src/validator_side/collation.rs | 7 ++ .../src/validator_side/mod.rs | 66 ++++++----- .../src/validator_side/tests/mod.rs | 14 +-- .../tests/prospective_parachains.rs | 106 ++++++++++++++---- 4 files changed, 132 insertions(+), 61 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/collation.rs b/polkadot/node/network/collator-protocol/src/validator_side/collation.rs index a53e0028b9e7..d6f34fc81b82 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/collation.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/collation.rs @@ -31,6 +31,7 @@ use std::{collections::VecDeque, future::Future, pin::Pin, task::Poll}; use futures::{future::BoxFuture, FutureExt}; use polkadot_node_network_protocol::{ + peer_set::CollationVersion, request_response::{outgoing::RequestError, v1 as request_v1, OutgoingResult}, PeerId, }; @@ -160,6 +161,8 @@ pub fn fetched_collation_sanity_check( pub struct CollationEvent { /// Collator id. pub collator_id: CollatorId, + /// The network protocol version the collator is using. + pub collator_protocol_version: CollationVersion, /// The requested collation data. pub pending_collation: PendingCollation, } @@ -307,6 +310,8 @@ pub(super) struct CollationFetchRequest { pub pending_collation: PendingCollation, /// Collator id. pub collator_id: CollatorId, + /// The network protocol version the collator is using. + pub collator_protocol_version: CollationVersion, /// Responses from collator. pub from_collator: BoxFuture<'static, OutgoingResult>, /// Handle used for checking if this request was cancelled. @@ -334,6 +339,7 @@ impl Future for CollationFetchRequest { self.span.as_mut().map(|s| s.add_string_tag("success", "false")); return Poll::Ready(( CollationEvent { + collator_protocol_version: self.collator_protocol_version, collator_id: self.collator_id.clone(), pending_collation: self.pending_collation, }, @@ -344,6 +350,7 @@ impl Future for CollationFetchRequest { let res = self.from_collator.poll_unpin(cx).map(|res| { ( CollationEvent { + collator_protocol_version: self.collator_protocol_version, collator_id: self.collator_id.clone(), pending_collation: self.pending_collation, }, diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 4fd563d713e3..02edbd75812e 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -720,6 +720,7 @@ async fn request_collation( let collation_request = CollationFetchRequest { pending_collation, collator_id: collator_id.clone(), + collator_protocol_version: peer_protocol_version, from_collator: response_recv.boxed(), cancellation_token: cancellation_token.clone(), span: state @@ -1484,7 +1485,7 @@ async fn process_msg( }, }; let fetched_collation = FetchedCollation::from(&receipt.to_plain()); - if let Some(CollationEvent { collator_id, pending_collation }) = + if let Some(CollationEvent { collator_id, pending_collation, .. }) = state.fetched_candidates.remove(&fetched_collation) { let PendingCollation { relay_parent, peer_id, prospective_candidate, .. } = @@ -1642,7 +1643,7 @@ async fn run_inner( Ok(res) => res }; - let CollationEvent {collator_id, pending_collation} = res.collation_event.clone(); + let CollationEvent {collator_id, pending_collation, .. } = res.collation_event.clone(); if let Err(err) = kick_off_seconding(&mut ctx, &mut state, res).await { gum::warn!( target: LOG_TARGET, @@ -1790,39 +1791,37 @@ async fn kick_off_seconding( }, }; let collations = &mut per_relay_parent.collations; - let relay_parent_mode = per_relay_parent.prospective_parachains_mode; let fetched_collation = FetchedCollation::from(&candidate_receipt); if let Entry::Vacant(entry) = state.fetched_candidates.entry(fetched_collation) { collation_event.pending_collation.commitments_hash = Some(candidate_receipt.commitments_hash); - let pvd = - match (relay_parent_mode, collation_event.pending_collation.prospective_candidate) { - ( - ProspectiveParachainsMode::Enabled { .. }, - Some(ProspectiveCandidate { parent_head_data_hash, .. }), - ) => - request_prospective_validation_data( - ctx.sender(), - relay_parent, - parent_head_data_hash, - pending_collation.para_id, - ) - .await?, - (ProspectiveParachainsMode::Disabled, _) => - request_persisted_validation_data( - ctx.sender(), - candidate_receipt.descriptor().relay_parent, - candidate_receipt.descriptor().para_id, - ) - .await?, - _ => { - // `handle_advertisement` checks for protocol mismatch. - return Ok(()) - }, - } - .ok_or(SecondingError::PersistedValidationDataNotFound)?; + let pvd = match ( + collation_event.collator_protocol_version, + collation_event.pending_collation.prospective_candidate, + ) { + (CollationVersion::V2, Some(ProspectiveCandidate { parent_head_data_hash, .. })) => + request_prospective_validation_data( + ctx.sender(), + relay_parent, + parent_head_data_hash, + pending_collation.para_id, + ) + .await?, + (CollationVersion::V1, _) => + request_persisted_validation_data( + ctx.sender(), + candidate_receipt.descriptor().relay_parent, + candidate_receipt.descriptor().para_id, + ) + .await?, + _ => { + // `handle_advertisement` checks for protocol mismatch. + return Ok(()) + }, + } + .ok_or(SecondingError::PersistedValidationDataNotFound)?; fetched_collation_sanity_check( &collation_event.pending_collation, @@ -1871,7 +1870,8 @@ async fn handle_collation_fetch_response( network_error_freq: &mut gum::Freq, canceled_freq: &mut gum::Freq, ) -> std::result::Result> { - let (CollationEvent { collator_id, pending_collation }, response) = response; + let (CollationEvent { collator_id, collator_protocol_version, pending_collation }, response) = + response; // Remove the cancellation handle, as the future already completed. state.collation_requests_cancel_handles.remove(&pending_collation); @@ -1977,7 +1977,11 @@ async fn handle_collation_fetch_response( metrics_result = Ok(()); Ok(PendingCollationFetch { - collation_event: CollationEvent { collator_id, pending_collation }, + collation_event: CollationEvent { + collator_id, + pending_collation, + collator_protocol_version, + }, candidate_receipt, pov, }) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs index ea67aeaa2604..3ddb194d0b82 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs @@ -269,15 +269,15 @@ async fn assert_candidate_backing_second( expected_relay_parent: Hash, expected_para_id: ParaId, expected_pov: &PoV, - mode: ProspectiveParachainsMode, + version: CollationVersion, ) -> CandidateReceipt { let pvd = dummy_pvd(); // Depending on relay parent mode pvd will be either requested // from the Runtime API or Prospective Parachains. let msg = overseer_recv(virtual_overseer).await; - match mode { - ProspectiveParachainsMode::Disabled => assert_matches!( + match version { + CollationVersion::V1 => assert_matches!( msg, AllMessages::RuntimeApi(RuntimeApiMessage::Request( hash, @@ -289,7 +289,7 @@ async fn assert_candidate_backing_second( tx.send(Ok(Some(pvd.clone()))).unwrap(); } ), - ProspectiveParachainsMode::Enabled { .. } => assert_matches!( + CollationVersion::V2 => assert_matches!( msg, AllMessages::ProspectiveParachains( ProspectiveParachainsMessage::GetProspectiveValidationData(request, tx), @@ -757,7 +757,7 @@ fn fetch_one_collation_at_a_time() { test_state.relay_parent, test_state.chain_ids[0], &pov, - ProspectiveParachainsMode::Disabled, + CollationVersion::V1, ) .await; @@ -889,7 +889,7 @@ fn fetches_next_collation() { second, test_state.chain_ids[0], &pov, - ProspectiveParachainsMode::Disabled, + CollationVersion::V1, ) .await; @@ -1019,7 +1019,7 @@ fn fetch_next_collation_on_invalid_collation() { test_state.relay_parent, test_state.chain_ids[0], &pov, - ProspectiveParachainsMode::Disabled, + CollationVersion::V1, ) .await; diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs index 80f48f860bfc..1a0277c15ce3 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs @@ -212,6 +212,7 @@ async fn assert_collation_seconded( virtual_overseer: &mut VirtualOverseer, relay_parent: Hash, peer_id: PeerId, + version: CollationVersion, ) { assert_matches!( overseer_recv(virtual_overseer).await, @@ -222,29 +223,51 @@ async fn assert_collation_seconded( assert_eq!(rep.value, BENEFIT_NOTIFY_GOOD.cost_or_benefit()); } ); - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendCollationMessage( - peers, - Versioned::V2(protocol_v2::CollationProtocol::CollatorProtocol( - protocol_v2::CollatorProtocolMessage::CollationSeconded( - _relay_parent, - .., - ), - )), - )) => { - assert_eq!(peers, vec![peer_id]); - assert_eq!(relay_parent, _relay_parent); - } - ); + + match version { + CollationVersion::V1 => { + assert_matches!( + overseer_recv(virtual_overseer).await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendCollationMessage( + peers, + Versioned::V1(protocol_v1::CollationProtocol::CollatorProtocol( + protocol_v1::CollatorProtocolMessage::CollationSeconded( + _relay_parent, + .., + ), + )), + )) => { + assert_eq!(peers, vec![peer_id]); + assert_eq!(relay_parent, _relay_parent); + } + ); + }, + CollationVersion::V2 => { + assert_matches!( + overseer_recv(virtual_overseer).await, + AllMessages::NetworkBridgeTx(NetworkBridgeTxMessage::SendCollationMessage( + peers, + Versioned::V2(protocol_v2::CollationProtocol::CollatorProtocol( + protocol_v2::CollatorProtocolMessage::CollationSeconded( + _relay_parent, + .., + ), + )), + )) => { + assert_eq!(peers, vec![peer_id]); + assert_eq!(relay_parent, _relay_parent); + } + ); + }, + } } #[test] -fn v1_advertisement_accepted() { +fn v1_advertisement_accepted_and_seconded() { let test_state = TestState::default(); test_harness(ReputationAggregator::new(|_| true), |test_harness| async move { - let TestHarness { mut virtual_overseer, .. } = test_harness; + let TestHarness { mut virtual_overseer, keystore } = test_harness; let pair_a = CollatorPair::generate().0; @@ -267,7 +290,7 @@ fn v1_advertisement_accepted() { advertise_collation(&mut virtual_overseer, peer_a, head_b, None).await; - assert_fetch_collation_request( + let response_channel = assert_fetch_collation_request( &mut virtual_overseer, head_b, test_state.chain_ids[0], @@ -275,6 +298,45 @@ fn v1_advertisement_accepted() { ) .await; + let mut candidate = dummy_candidate_receipt_bad_sig(head_b, Some(Default::default())); + candidate.descriptor.para_id = test_state.chain_ids[0]; + candidate.descriptor.persisted_validation_data_hash = dummy_pvd().hash(); + let commitments = CandidateCommitments { + head_data: HeadData(vec![1 as u8]), + horizontal_messages: Default::default(), + upward_messages: Default::default(), + new_validation_code: None, + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + candidate.commitments_hash = commitments.hash(); + + let pov = PoV { block_data: BlockData(vec![1]) }; + + response_channel + .send(Ok(request_v2::CollationFetchingResponse::Collation( + candidate.clone(), + pov.clone(), + ) + .encode())) + .expect("Sending response should succeed"); + + assert_candidate_backing_second( + &mut virtual_overseer, + head_b, + test_state.chain_ids[0], + &pov, + CollationVersion::V1, + ) + .await; + + let candidate = CommittedCandidateReceipt { descriptor: candidate.descriptor, commitments }; + + send_seconded_statement(&mut virtual_overseer, keystore.clone(), &candidate).await; + + assert_collation_seconded(&mut virtual_overseer, head_b, peer_a, CollationVersion::V1) + .await; + virtual_overseer }); } @@ -473,10 +535,7 @@ fn second_multiple_candidates_per_relay_parent() { head_c, test_state.chain_ids[0], &pov, - ProspectiveParachainsMode::Enabled { - max_candidate_depth: ASYNC_BACKING_PARAMETERS.max_candidate_depth as _, - allowed_ancestry_len: ASYNC_BACKING_PARAMETERS.allowed_ancestry_len as _, - }, + CollationVersion::V2, ) .await; @@ -485,7 +544,8 @@ fn second_multiple_candidates_per_relay_parent() { send_seconded_statement(&mut virtual_overseer, keystore.clone(), &candidate).await; - assert_collation_seconded(&mut virtual_overseer, head_c, peer_a).await; + assert_collation_seconded(&mut virtual_overseer, head_c, peer_a, CollationVersion::V2) + .await; } // No more advertisements can be made for this relay parent. From 7634b20a151118d4464da6536d924d57f7c60b42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 18 Oct 2023 23:31:40 +0200 Subject: [PATCH 5/8] Ensure v1 can only advertise for active leaves --- .../src/validator_side/mod.rs | 14 ++++++- .../tests/prospective_parachains.rs | 42 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 02edbd75812e..82529c9ce035 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -84,6 +84,8 @@ const COST_NETWORK_ERROR: Rep = Rep::CostMinor("Some network error"); const COST_INVALID_SIGNATURE: Rep = Rep::Malicious("Invalid network message signature"); const COST_REPORT_BAD: Rep = Rep::Malicious("A collator was reported by another subsystem"); const COST_WRONG_PARA: Rep = Rep::Malicious("A collator provided a collation for the wrong para"); +const COST_PROTOCOL_MISUSE: Rep = + Rep::Malicious("A collator advertising a collation for an async backing relay parent using V1"); const COST_UNNEEDED_COLLATOR: Rep = Rep::CostMinor("An unneeded collator connected"); const BENEFIT_NOTIFY_GOOD: Rep = Rep::BenefitMinor("A collator was noted good by another subsystem"); @@ -938,6 +940,9 @@ enum AdvertisementError { InvalidAssignment, /// Para reached a limit of seconded candidates for this relay parent. SecondedLimitReached, + /// Collator trying to advertise a collation using V1 protocol for an async backing relay + /// parent. + ProtocolMisuse, /// Advertisement is invalid. Invalid(InsertAdvertisementError), } @@ -947,6 +952,7 @@ impl AdvertisementError { use AdvertisementError::*; match self { InvalidAssignment => Some(COST_WRONG_PARA), + ProtocolMisuse => Some(COST_PROTOCOL_MISUSE), RelayParentUnknown | UndeclaredCollator | Invalid(_) => Some(COST_UNEXPECTED_MESSAGE), UnknownPeer | SecondedLimitReached => None, } @@ -1056,6 +1062,13 @@ where .get(&relay_parent) .map(|s| s.child("advertise-collation")); + let peer_data = state.peer_data.get_mut(&peer_id).ok_or(AdvertisementError::UnknownPeer)?; + + if peer_data.version == CollationVersion::V1 && !state.active_leaves.contains_key(&relay_parent) + { + return Err(AdvertisementError::ProtocolMisuse) + } + let per_relay_parent = state .per_relay_parent .get(&relay_parent) @@ -1064,7 +1077,6 @@ where let relay_parent_mode = per_relay_parent.prospective_parachains_mode; let assignment = &per_relay_parent.assignment; - let peer_data = state.peer_data.get_mut(&peer_id).ok_or(AdvertisementError::UnknownPeer)?; let collator_para_id = peer_data.collating_para().ok_or(AdvertisementError::UndeclaredCollator)?; diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs index 1a0277c15ce3..c5236ef3eb21 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/prospective_parachains.rs @@ -341,6 +341,48 @@ fn v1_advertisement_accepted_and_seconded() { }); } +#[test] +fn v1_advertisement_rejected_on_non_active_leave() { + let test_state = TestState::default(); + + test_harness(ReputationAggregator::new(|_| true), |test_harness| async move { + let TestHarness { mut virtual_overseer, .. } = test_harness; + + let pair_a = CollatorPair::generate().0; + + let head_b = Hash::from_low_u64_be(128); + let head_b_num: u32 = 5; + + update_view(&mut virtual_overseer, &test_state, vec![(head_b, head_b_num)], 1).await; + + let peer_a = PeerId::random(); + + // Accept both collators from the implicit view. + connect_and_declare_collator( + &mut virtual_overseer, + peer_a, + pair_a.clone(), + test_state.chain_ids[0], + CollationVersion::V1, + ) + .await; + + advertise_collation(&mut virtual_overseer, peer_a, get_parent_hash(head_b), None).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::NetworkBridgeTx( + NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(peer, rep)), + ) => { + assert_eq!(peer, peer_a); + assert_eq!(rep.value, COST_PROTOCOL_MISUSE.cost_or_benefit()); + } + ); + + virtual_overseer + }); +} + #[test] fn accept_advertisements_from_implicit_view() { let test_state = TestState::default(); From 8735f5210653d21fe38397caa58019df4cceb034 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 19 Oct 2023 11:09:12 +0200 Subject: [PATCH 6/8] Don't queue the advertisement if async backing isn't enabled --- .../src/validator_side/mod.rs | 59 ++++++++----------- .../src/validator_side/tests/mod.rs | 23 +++----- 2 files changed, 30 insertions(+), 52 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 82529c9ce035..8c6331e57347 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -22,6 +22,7 @@ use std::{ collections::{hash_map::Entry, HashMap, HashSet}, convert::TryInto, future::Future, + iter::FromIterator, time::{Duration, Instant}, }; use tokio_util::sync::CancellationToken; @@ -145,9 +146,6 @@ enum InsertAdvertisementError { UndeclaredCollator, /// A limit for announcements per peer is reached. PeerLimitReached, - /// Mismatch of relay parent mode and advertisement arguments. - /// An internal error that should not happen. - ProtocolMismatch, } #[derive(Debug)] @@ -243,20 +241,15 @@ impl PeerData { } match (relay_parent_mode, candidate_hash) { - (ProspectiveParachainsMode::Enabled { .. }, None) | - (ProspectiveParachainsMode::Disabled, None) => - if self.version == CollationVersion::V1 { - if state.advertisements.contains_key(&on_relay_parent) { - return Err(InsertAdvertisementError::Duplicate) - } - state.advertisements.insert(on_relay_parent, HashSet::new()); - } else { - gum::error!( - target: LOG_TARGET, - "Inserting advertisment: V2 without a candidate hash", - ); - return Err(InsertAdvertisementError::ProtocolMismatch) - }, + (ProspectiveParachainsMode::Enabled { .. }, candidate_hash) | + (ProspectiveParachainsMode::Disabled, candidate_hash) => { + if state.advertisements.contains_key(&on_relay_parent) { + return Err(InsertAdvertisementError::Duplicate) + } + state + .advertisements + .insert(on_relay_parent, HashSet::from_iter(candidate_hash)); + }, ( ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, Some(candidate_hash), @@ -276,16 +269,6 @@ impl PeerData { } candidates.insert(candidate_hash); }, - (mode, candidate_hash) => { - gum::error!( - target: LOG_TARGET, - ?mode, - hash = ?candidate_hash, - "Inserting advertisment: protocol mismatch", - ); - - return Err(InsertAdvertisementError::ProtocolMismatch) - }, } state.last_active = Instant::now(); @@ -1102,16 +1085,20 @@ where } if let Some((candidate_hash, parent_head_data_hash)) = prospective_candidate { - let is_seconding_allowed = can_second( - sender, - collator_para_id, - relay_parent, - candidate_hash, - parent_head_data_hash, - ) - .await; + // We need to queue the advertisement if we are not allowed to second it. + // + // This is also only important when async backing is enabled. + let queue_advertisement = relay_parent_mode.is_enabled() && + !can_second( + sender, + collator_para_id, + relay_parent, + candidate_hash, + parent_head_data_hash, + ) + .await; - if !is_seconding_allowed { + if queue_advertisement { gum::debug!( target: LOG_TARGET, relay_parent = ?relay_parent, diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs index 3ddb194d0b82..a44212f4504b 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs @@ -510,14 +510,17 @@ fn act_on_advertisement_v2() { let pair = CollatorPair::generate().0; gum::trace!("activating"); - prospective_parachains::update_view( + overseer_send( &mut virtual_overseer, - &test_state, - vec![(test_state.relay_parent, 0)], - 1, + CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange( + our_view![test_state.relay_parent], + )), ) .await; + assert_async_backing_params_request(&mut virtual_overseer, test_state.relay_parent).await; + respond_to_core_info_queries(&mut virtual_overseer, &test_state).await; + let peer_b = PeerId::random(); connect_and_declare_collator( @@ -540,18 +543,6 @@ fn act_on_advertisement_v2() { ) .await; - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CandidateBacking( - CandidateBackingMessage::CanSecond(request, tx), - ) => { - assert_eq!(request.candidate_hash, candidate_hash); - assert_eq!(request.candidate_para_id, test_state.chain_ids[0]); - assert_eq!(request.parent_head_data_hash, parent_head_data_hash); - tx.send(true).expect("receiving side should be alive"); - } - ); - assert_fetch_collation_request( &mut virtual_overseer, test_state.relay_parent, From 7ca2a26ddd36c5b44346fa3efea1bfe1cbbf24a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 19 Oct 2023 12:15:01 +0200 Subject: [PATCH 7/8] Fixes fixes --- .../src/validator_side/mod.rs | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 8c6331e57347..78f33881d45d 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -241,7 +241,6 @@ impl PeerData { } match (relay_parent_mode, candidate_hash) { - (ProspectiveParachainsMode::Enabled { .. }, candidate_hash) | (ProspectiveParachainsMode::Disabled, candidate_hash) => { if state.advertisements.contains_key(&on_relay_parent) { return Err(InsertAdvertisementError::Duplicate) @@ -252,22 +251,40 @@ impl PeerData { }, ( ProspectiveParachainsMode::Enabled { max_candidate_depth, .. }, - Some(candidate_hash), + candidate_hash, ) => { - if state - .advertisements - .get(&on_relay_parent) - .map_or(false, |candidates| candidates.contains(&candidate_hash)) - { - return Err(InsertAdvertisementError::Duplicate) - } - - let candidates = state.advertisements.entry(on_relay_parent).or_default(); - - if candidates.len() > max_candidate_depth { - return Err(InsertAdvertisementError::PeerLimitReached) - } - candidates.insert(candidate_hash); + if let Some(candidate_hash) = candidate_hash { + if state + .advertisements + .get(&on_relay_parent) + .map_or(false, |candidates| candidates.contains(&candidate_hash)) + { + return Err(InsertAdvertisementError::Duplicate) + } + + let candidates = + state.advertisements.entry(on_relay_parent).or_default(); + + if candidates.len() > max_candidate_depth { + return Err(InsertAdvertisementError::PeerLimitReached) + } + candidates.insert(candidate_hash); + } else { + if self.version != CollationVersion::V1 { + gum::error!( + target: LOG_TARGET, + "Programming error, `candidate_hash` can not be `None` \ + for non `V1` networking.", + ); + } + + if state.advertisements.contains_key(&on_relay_parent) { + return Err(InsertAdvertisementError::Duplicate) + } + state + .advertisements + .insert(on_relay_parent, HashSet::from_iter(candidate_hash)); + }; }, } From f9700a8ef665a9f5b4a7756e0ff877adbe472064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 19 Oct 2023 20:59:21 +0200 Subject: [PATCH 8/8] Fixy fix --- .../src/validator_side/mod.rs | 6 ++-- .../src/validator_side/tests/mod.rs | 29 +++++++++++++++++-- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs index 78f33881d45d..00bf50013a53 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/mod.rs @@ -1817,7 +1817,8 @@ async fn kick_off_seconding( collation_event.collator_protocol_version, collation_event.pending_collation.prospective_candidate, ) { - (CollationVersion::V2, Some(ProspectiveCandidate { parent_head_data_hash, .. })) => + (CollationVersion::V2, Some(ProspectiveCandidate { parent_head_data_hash, .. })) + if per_relay_parent.prospective_parachains_mode.is_enabled() => request_prospective_validation_data( ctx.sender(), relay_parent, @@ -1825,7 +1826,8 @@ async fn kick_off_seconding( pending_collation.para_id, ) .await?, - (CollationVersion::V1, _) => + // Support V2 collators without async backing enabled. + (CollationVersion::V2, Some(_)) | (CollationVersion::V1, _) => request_persisted_validation_data( ctx.sender(), candidate_receipt.descriptor().relay_parent, diff --git a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs index a44212f4504b..3a9740149948 100644 --- a/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs +++ b/polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs @@ -532,7 +532,14 @@ fn act_on_advertisement_v2() { ) .await; - let candidate_hash = CandidateHash::default(); + let pov = PoV { block_data: BlockData(vec![]) }; + let mut candidate_a = + dummy_candidate_receipt_bad_sig(dummy_hash(), Some(Default::default())); + candidate_a.descriptor.para_id = test_state.chain_ids[0]; + candidate_a.descriptor.relay_parent = test_state.relay_parent; + candidate_a.descriptor.persisted_validation_data_hash = dummy_pvd().hash(); + + let candidate_hash = candidate_a.hash(); let parent_head_data_hash = Hash::zero(); // v2 advertisement. advertise_collation( @@ -543,7 +550,7 @@ fn act_on_advertisement_v2() { ) .await; - assert_fetch_collation_request( + let response_channel = assert_fetch_collation_request( &mut virtual_overseer, test_state.relay_parent, test_state.chain_ids[0], @@ -551,6 +558,24 @@ fn act_on_advertisement_v2() { ) .await; + response_channel + .send(Ok(request_v1::CollationFetchingResponse::Collation( + candidate_a.clone(), + pov.clone(), + ) + .encode())) + .expect("Sending response should succeed"); + + assert_candidate_backing_second( + &mut virtual_overseer, + test_state.relay_parent, + test_state.chain_ids[0], + &pov, + // Async backing isn't enabled and thus it should do it the old way. + CollationVersion::V1, + ) + .await; + virtual_overseer }); }