From 2c9296d7dca897c84328520fd2d78778e0c68b79 Mon Sep 17 00:00:00 2001 From: Chris Sosnin <48099298+slumber@users.noreply.github.com> Date: Wed, 13 Jul 2022 07:24:46 +0300 Subject: [PATCH] Track occupied depth in backing per parachain (#5778) --- node/core/backing/src/lib.rs | 39 +++- node/core/backing/src/tests/mod.rs | 14 +- .../src/tests/prospective_parachains.rs | 217 +++++++++++++++--- 3 files changed, 214 insertions(+), 56 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index 9d5f521da1f8..2b619aca5a16 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -244,12 +244,13 @@ impl ProspectiveParachainsMode { struct ActiveLeafState { prospective_parachains_mode: ProspectiveParachainsMode, /// The candidates seconded at various depths under this active - /// leaf. A candidate can only be seconded when its hypothetical - /// depth under every active leaf has an empty entry in this map. + /// leaf with respect to parachain id. A candidate can only be + /// seconded when its hypothetical depth under every active leaf + /// has an empty entry in this map. /// /// When prospective parachains are disabled, the only depth /// which is allowed is 0. - seconded_at_depth: BTreeMap, + seconded_at_depth: HashMap>, } /// The state of the subsystem. @@ -869,7 +870,7 @@ async fn handle_active_leaves_update( // when prospective parachains are disabled is the leaf hash and 0, // respectively. We've just learned about the leaf hash, so we cannot // have any candidates seconded with it as a relay-parent yet. - seconded_at_depth: BTreeMap::new(), + seconded_at_depth: HashMap::new(), }, ); @@ -895,7 +896,8 @@ async fn handle_active_leaves_update( for (candidate_hash, para_id) in remaining_seconded { let (tx, rx) = oneshot::channel(); - membership_answers.push(rx.map_ok(move |membership| (candidate_hash, membership))); + membership_answers + .push(rx.map_ok(move |membership| (para_id, candidate_hash, membership))); ctx.send_message(ProspectiveParachainsMessage::GetTreeMembership( para_id, @@ -905,7 +907,7 @@ async fn handle_active_leaves_update( .await; } - let mut seconded_at_depth = BTreeMap::new(); + let mut seconded_at_depth = HashMap::new(); for response in membership_answers.next().await { match response { Err(oneshot::Canceled) => { @@ -916,15 +918,17 @@ async fn handle_active_leaves_update( continue }, - Ok((candidate_hash, membership)) => { + Ok((para_id, candidate_hash, membership)) => { // This request gives membership in all fragment trees. We have some // wasted data here, and it can be optimized if it proves // relevant to performance. if let Some((_, depths)) = membership.into_iter().find(|(leaf_hash, _)| leaf_hash == &leaf.hash) { + let para_entry: &mut BTreeMap = + seconded_at_depth.entry(para_id).or_default(); for depth in depths { - seconded_at_depth.insert(depth, candidate_hash); + para_entry.insert(depth, candidate_hash); } } }, @@ -1163,7 +1167,11 @@ async fn seconding_sanity_check( responses.push(rx.map_ok(move |depths| (depths, head, leaf_state)).boxed()); } else { if head == &candidate_relay_parent { - if leaf_state.seconded_at_depth.contains_key(&0) { + if leaf_state + .seconded_at_depth + .get(&candidate_para) + .map_or(false, |occupied| occupied.contains_key(&0)) + { // The leaf is already occupied. return SecondingAllowed::No } @@ -1188,7 +1196,11 @@ async fn seconding_sanity_check( }, Ok((depths, head, leaf_state)) => { for depth in &depths { - if leaf_state.seconded_at_depth.contains_key(&depth) { + if leaf_state + .seconded_at_depth + .get(&candidate_para) + .map_or(false, |occupied| occupied.contains_key(&depth)) + { gum::debug!( target: LOG_TARGET, ?candidate_hash, @@ -1323,8 +1335,13 @@ async fn handle_validated_candidate_command( Some(d) => d, }; + let seconded_at_depth = leaf_data + .seconded_at_depth + .entry(candidate.descriptor().para_id) + .or_default(); + for depth in depths { - leaf_data.seconded_at_depth.insert(depth, candidate_hash); + seconded_at_depth.insert(depth, candidate_hash); } } diff --git a/node/core/backing/src/tests/mod.rs b/node/core/backing/src/tests/mod.rs index 402462913749..59334fc179bc 100644 --- a/node/core/backing/src/tests/mod.rs +++ b/node/core/backing/src/tests/mod.rs @@ -32,8 +32,7 @@ use polkadot_node_subsystem::{ }; use polkadot_node_subsystem_test_helpers as test_helpers; use polkadot_primitives::v2::{ - CandidateDescriptor, CollatorId, GroupRotationInfo, HeadData, PersistedValidationData, - ScheduledCore, + CandidateDescriptor, GroupRotationInfo, HeadData, PersistedValidationData, ScheduledCore, }; use sp_application_crypto::AppKey; use sp_keyring::Sr25519Keyring; @@ -90,9 +89,8 @@ impl Default for TestState { fn default() -> Self { let chain_a = ParaId::from(1); let chain_b = ParaId::from(2); - let thread_a = ParaId::from(3); - let chain_ids = vec![chain_a, chain_b, thread_a]; + let chain_ids = vec![chain_a, chain_b]; let validators = vec![ Sr25519Keyring::Alice, @@ -114,25 +112,21 @@ impl Default for TestState { let validator_public = validator_pubkeys(&validators); - let validator_groups = vec![vec![2, 0, 3, 5], vec![1], vec![4]] + let validator_groups = vec![vec![2, 0, 3, 5], vec![1]] .into_iter() .map(|g| g.into_iter().map(ValidatorIndex).collect()) .collect(); let group_rotation_info = GroupRotationInfo { session_start_block: 0, group_rotation_frequency: 100, now: 1 }; - let thread_collator: CollatorId = Sr25519Keyring::Two.public().into(); let availability_cores = vec![ CoreState::Scheduled(ScheduledCore { para_id: chain_a, collator: None }), CoreState::Scheduled(ScheduledCore { para_id: chain_b, collator: None }), - CoreState::Scheduled(ScheduledCore { - para_id: thread_a, - collator: Some(thread_collator.clone()), - }), ]; let mut head_data = HashMap::new(); head_data.insert(chain_a, HeadData(vec![4, 5, 6])); + head_data.insert(chain_b, HeadData(vec![5, 6, 7])); let relay_parent = Hash::repeat_byte(5); diff --git a/node/core/backing/src/tests/prospective_parachains.rs b/node/core/backing/src/tests/prospective_parachains.rs index 5be62b344980..749da6f10937 100644 --- a/node/core/backing/src/tests/prospective_parachains.rs +++ b/node/core/backing/src/tests/prospective_parachains.rs @@ -78,46 +78,49 @@ async fn activate_leaf( let ancestry_hashes = std::iter::successors(Some(leaf_hash), |h| Some(get_parent_hash(*h))) .take(ancestry_len as usize); let ancestry_numbers = (min_min..=leaf_number).rev(); - let mut ancestry_iter = ancestry_hashes.clone().zip(ancestry_numbers).peekable(); + let ancestry_iter = ancestry_hashes.zip(ancestry_numbers).peekable(); let mut next_overseer_message = None; // How many blocks were actually requested. let mut requested_len = 0; - loop { - let (hash, number) = match ancestry_iter.next() { - Some((hash, number)) => (hash, number), - None => break, - }; + { + let mut ancestry_iter = ancestry_iter.clone(); + loop { + let (hash, number) = match ancestry_iter.next() { + Some((hash, number)) => (hash, number), + None => break, + }; + + // May be `None` for the last element. + let parent_hash = + ancestry_iter.peek().map(|(h, _)| *h).unwrap_or_else(|| get_parent_hash(hash)); + + let msg = virtual_overseer.recv().await; + // It may happen that some blocks were cached by implicit view, + // reuse the message. + if !matches!(&msg, AllMessages::ChainApi(ChainApiMessage::BlockHeader(..))) { + next_overseer_message.replace(msg); + break + } - // May be `None` for the last element. - let parent_hash = - ancestry_iter.peek().map(|(h, _)| *h).unwrap_or_else(|| get_parent_hash(hash)); + assert_matches!( + msg, + AllMessages::ChainApi( + ChainApiMessage::BlockHeader(_hash, tx) + ) if _hash == hash => { + let header = Header { + parent_hash, + number, + state_root: Hash::zero(), + extrinsics_root: Hash::zero(), + digest: Default::default(), + }; - let msg = virtual_overseer.recv().await; - // It may happen that some blocks were cached by implicit view, - // reuse the message. - if !matches!(&msg, AllMessages::ChainApi(ChainApiMessage::BlockHeader(..))) { - next_overseer_message.replace(msg); - break + tx.send(Ok(Some(header))).unwrap(); + } + ); + requested_len += 1; } - - assert_matches!( - msg, - AllMessages::ChainApi( - ChainApiMessage::BlockHeader(_hash, tx) - ) if _hash == hash => { - let header = Header { - parent_hash, - number, - state_root: Hash::zero(), - extrinsics_root: Hash::zero(), - digest: Default::default(), - }; - - tx.send(Ok(Some(header))).unwrap(); - } - ); - requested_len += 1; } for _ in 0..seconded_in_view { @@ -135,7 +138,7 @@ async fn activate_leaf( ); } - for hash in ancestry_hashes.take(requested_len) { + for (hash, number) in ancestry_iter.take(requested_len) { // Check that subsystem job issues a request for a validator set. let msg = match next_overseer_message.take() { Some(msg) => msg, @@ -156,7 +159,9 @@ async fn activate_leaf( AllMessages::RuntimeApi( RuntimeApiMessage::Request(parent, RuntimeApiRequest::ValidatorGroups(tx)) ) if parent == hash => { - tx.send(Ok(test_state.validator_groups.clone())).unwrap(); + let (validator_groups, mut group_rotation_info) = test_state.validator_groups.clone(); + group_rotation_info.now = number; + tx.send(Ok((validator_groups, group_rotation_info))).unwrap(); } ); @@ -1350,3 +1355,145 @@ fn concurrent_dependent_candidates() { virtual_overseer }); } + +// Test that multiple candidates from different paras can occupy the same depth +// in a given relay parent. +#[test] +fn seconding_sanity_check_occupy_same_depth() { + let test_state = TestState::default(); + test_harness(test_state.keystore.clone(), |mut virtual_overseer| async move { + // Candidate `a` is seconded in a parent of the activated `leaf`. + const LEAF_BLOCK_NUMBER: BlockNumber = 100; + const LEAF_DEPTH: BlockNumber = 3; + + let para_id_a = test_state.chain_ids[0]; + let para_id_b = test_state.chain_ids[1]; + + let leaf_hash = Hash::from_low_u64_be(130); + let leaf_parent = get_parent_hash(leaf_hash); + + let activated = ActivatedLeaf { + hash: leaf_hash, + number: LEAF_BLOCK_NUMBER, + status: LeafStatus::Fresh, + span: Arc::new(jaeger::Span::Disabled), + }; + + let min_block_number = LEAF_BLOCK_NUMBER - LEAF_DEPTH; + let min_relay_parents = vec![(para_id_a, min_block_number), (para_id_b, min_block_number)]; + let test_leaf_a = TestLeaf { activated, min_relay_parents }; + + activate_leaf(&mut virtual_overseer, test_leaf_a, &test_state, 0).await; + + let pov = PoV { block_data: BlockData(vec![42, 43, 44]) }; + let pvd = dummy_pvd(); + let validation_code = ValidationCode(vec![1, 2, 3]); + + let expected_head_data_a = test_state.head_data.get(¶_id_a).unwrap(); + let expected_head_data_b = test_state.head_data.get(¶_id_b).unwrap(); + + let pov_hash = pov.hash(); + let candidate_a = TestCandidateBuilder { + para_id: para_id_a, + relay_parent: leaf_parent, + pov_hash, + head_data: expected_head_data_a.clone(), + erasure_root: make_erasure_root(&test_state, pov.clone(), pvd.clone()), + persisted_validation_data_hash: pvd.hash(), + validation_code: validation_code.0.clone(), + ..Default::default() + }; + + let mut candidate_b = candidate_a.clone(); + candidate_b.para_id = para_id_b; + candidate_b.head_data = expected_head_data_b.clone(); + // A rotation happens, test validator is assigned to second para here. + candidate_b.relay_parent = leaf_hash; + + let candidate_a = (candidate_a.build(), expected_head_data_a, para_id_a); + let candidate_b = (candidate_b.build(), expected_head_data_b, para_id_b); + + for candidate in &[candidate_a, candidate_b] { + let (candidate, expected_head_data, para_id) = candidate; + let second = CandidateBackingMessage::Second( + leaf_hash, + candidate.to_plain(), + pvd.clone(), + pov.clone(), + ); + + virtual_overseer.send(FromOrchestra::Communication { msg: second }).await; + + assert_validate_seconded_candidate( + &mut virtual_overseer, + candidate.descriptor().relay_parent, + &candidate, + &pov, + &pvd, + &validation_code, + *expected_head_data, + false, + ) + .await; + + // `seconding_sanity_check` + let expected_request_a = vec![( + HypotheticalDepthRequest { + candidate_hash: candidate.hash(), + candidate_para: *para_id, + parent_head_data_hash: pvd.parent_head.hash(), + candidate_relay_parent: candidate.descriptor().relay_parent, + fragment_tree_relay_parent: leaf_hash, + }, + vec![0, 1], // Send the same membership for both candidates. + )]; + + assert_hypothetical_depth_requests(&mut virtual_overseer, expected_request_a.clone()) + .await; + + // Prospective parachains are notified. + assert_matches!( + virtual_overseer.recv().await, + AllMessages::ProspectiveParachains( + ProspectiveParachainsMessage::CandidateSeconded( + candidate_para, + candidate_receipt, + _pvd, + tx, + ), + ) if &candidate_receipt == candidate && candidate_para == *para_id && pvd == _pvd => { + // Any non-empty response will do. + tx.send(vec![(leaf_hash, vec![0, 2, 3])]).unwrap(); + } + ); + + test_dispute_coordinator_notifications( + &mut virtual_overseer, + candidate.hash(), + test_state.session(), + vec![ValidatorIndex(0)], + ) + .await; + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::StatementDistribution( + StatementDistributionMessage::Share( + parent_hash, + _signed_statement, + ) + ) if parent_hash == candidate.descriptor().relay_parent => {} + ); + + assert_matches!( + virtual_overseer.recv().await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::Seconded(hash, statement)) => { + assert_eq!(candidate.descriptor().relay_parent, hash); + assert_matches!(statement.payload(), Statement::Seconded(_)); + } + ); + } + + virtual_overseer + }); +}