diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs index e6360a3c8408e..86ef1a235454d 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs @@ -201,7 +201,7 @@ where continue; }; - let Ok(rp_data) = offset_relay_parent_find_descendants( + let Ok(Some(rp_data)) = offset_relay_parent_find_descendants( &mut relay_chain_data_cache, relay_best_hash, relay_parent_offset, @@ -467,7 +467,7 @@ pub(crate) async fn offset_relay_parent_find_descendants( relay_chain_data_cache: &mut RelayChainDataCache, relay_best_block: RelayHash, relay_parent_offset: u32, -) -> Result +) -> Result, ()> where RelayClient: RelayChainInterface + Clone + 'static, { @@ -481,7 +481,12 @@ where }; if relay_parent_offset == 0 { - return Ok(RelayParentData::new(relay_header)); + return Ok(Some(RelayParentData::new(relay_header))); + } + + if sc_consensus_babe::contains_epoch_change::(&relay_header) { + tracing::debug!(target: LOG_TARGET, ?relay_best_block, relay_best_block_number = relay_header.number(), "Relay parent is in previous session."); + return Ok(None); } let mut required_ancestors: VecDeque = Default::default(); @@ -492,6 +497,10 @@ where .await? .relay_parent_header .clone(); + if sc_consensus_babe::contains_epoch_change::(&next_header) { + tracing::debug!(target: LOG_TARGET, ?relay_best_block, ancestor = %next_header.hash(), ancestor_block_number = next_header.number(), "Ancestor of best block is in previous session."); + return Ok(None); + } required_ancestors.push_front(next_header.clone()); relay_header = next_header; } @@ -510,7 +519,7 @@ where "Relay parent descendants." ); - Ok(RelayParentData::new_with_descendants(relay_parent, required_ancestors.into())) + Ok(Some(RelayParentData::new_with_descendants(relay_parent, required_ancestors.into()))) } /// Return value of [`determine_core`]. diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/tests.rs b/cumulus/client/consensus/aura/src/collators/slot_based/tests.rs index a26ac2c581e92..e0ba35e558afe 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/tests.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/tests.rs @@ -20,6 +20,7 @@ use super::{ relay_chain_data_cache::{RelayChainData, RelayChainDataCache}, }; use async_trait::async_trait; +use codec::Encode; use cumulus_primitives_core::{ClaimQueueOffset, CoreInfo, CoreSelector, CumulusDigestItem}; use cumulus_relay_chain_interface::*; use futures::Stream; @@ -28,6 +29,11 @@ use polkadot_primitives::{ CandidateEvent, CommittedCandidateReceiptV2, CoreIndex, Hash as RelayHash, Header as RelayHeader, Id as ParaId, }; +use rstest::rstest; +use sc_consensus_babe::{ + AuthorityId, ConsensusLog as BabeConsensusLog, NextEpochDescriptor, BABE_ENGINE_ID, +}; +use sp_core::sr25519; use sp_runtime::{generic::BlockId, testing::Header as TestHeader, traits::Header}; use sp_version::RuntimeVersion; use std::{ @@ -45,7 +51,7 @@ async fn offset_test_zero_offset() { let result = offset_relay_parent_find_descendants(&mut cache, best_hash, 0).await; assert!(result.is_ok()); - let data = result.unwrap(); + let data = result.unwrap().unwrap(); assert_eq!(data.descendants_len(), 0); assert_eq!(data.relay_parent().hash(), best_hash); assert!(data.into_inherent_descendant_list().is_empty()); @@ -61,7 +67,7 @@ async fn offset_test_two_offset() { let result = offset_relay_parent_find_descendants(&mut cache, best_hash, 2).await; assert!(result.is_ok()); - let data = result.unwrap(); + let data = result.unwrap().unwrap(); assert_eq!(data.descendants_len(), 2); assert_eq!(*data.relay_parent().number(), 98); let descendant_list = data.into_inherent_descendant_list(); @@ -80,7 +86,7 @@ async fn offset_test_five_offset() { let result = offset_relay_parent_find_descendants(&mut cache, best_hash, 5).await; assert!(result.is_ok()); - let data = result.unwrap(); + let data = result.unwrap().unwrap(); assert_eq!(data.descendants_len(), 5); assert_eq!(*data.relay_parent().number(), 95); let descendant_list = data.into_inherent_descendant_list(); @@ -104,6 +110,33 @@ async fn offset_test_too_long() { assert!(result.is_err()); } +#[derive(PartialEq)] +enum HasEpochChange { + Yes, + No, +} + +#[rstest] +#[case::in_best( + &[HasEpochChange::No, HasEpochChange::No, HasEpochChange::Yes], +)] +#[case::in_first_ancestor( + &[HasEpochChange::No, HasEpochChange::Yes, HasEpochChange::No], +)] +#[case::in_second_ancestor( + &[HasEpochChange::Yes, HasEpochChange::No, HasEpochChange::No], +)] +#[tokio::test] +async fn offset_returns_none_when_epoch_change_encountered(#[case] flags: &[HasEpochChange]) { + let (headers, best_hash) = build_headers_with_epoch_flags(flags); + let client = TestRelayClient::new(headers); + let mut cache = RelayChainDataCache::new(client, 1.into()); + + let result = offset_relay_parent_find_descendants(&mut cache, best_hash, 3).await; + assert!(result.is_ok()); + assert!(result.unwrap().is_none()); +} + #[tokio::test] async fn determine_core_new_relay_parent() { let (headers, _best_hash) = create_header_chain(); @@ -593,6 +626,50 @@ impl RelayChainInterface for TestRelayClient { } } +/// Build a consecutive set of relay headers whose digest entries optionally carry a BABE +/// epoch-change marker, returning the underlying map and the hash of the last header. +fn build_headers_with_epoch_flags( + flags: &[HasEpochChange], +) -> (HashMap, RelayHash) { + let mut headers = HashMap::new(); + let mut parent_hash = RelayHash::default(); + let mut last_hash = RelayHash::default(); + + for (index, has_epoch_change) in flags.iter().enumerate() { + let digest = if *has_epoch_change == HasEpochChange::Yes { + babe_epoch_change_digest() + } else { + Default::default() + }; + + let header = RelayHeader { + parent_hash, + number: (index as u32 + 1), + state_root: Default::default(), + extrinsics_root: Default::default(), + digest, + }; + + let hash = header.hash(); + headers.insert(hash, header); + parent_hash = hash; + last_hash = hash; + } + + (headers, last_hash) +} + +/// Create a digest containing a single BABE `NextEpochData` item for use in tests. +fn babe_epoch_change_digest() -> sp_runtime::generic::Digest { + let mut digest = sp_runtime::generic::Digest::default(); + let authority_id = AuthorityId::from(sr25519::Public::from_raw([1u8; 32])); + let next_epoch = + NextEpochDescriptor { authorities: vec![(authority_id, 1u64)], randomness: [0u8; 32] }; + let log = BabeConsensusLog::NextEpochData(next_epoch); + digest.push(sp_runtime::generic::DigestItem::Consensus(BABE_ENGINE_ID, log.encode())); + digest +} + fn create_header_chain() -> (HashMap, RelayHash) { let mut headers = HashMap::new(); let mut current_parent = None; diff --git a/cumulus/client/parachain-inherent/src/lib.rs b/cumulus/client/parachain-inherent/src/lib.rs index 47040d4782b5b..f851954fd2cd2 100644 --- a/cumulus/client/parachain-inherent/src/lib.rs +++ b/cumulus/client/parachain-inherent/src/lib.rs @@ -171,12 +171,10 @@ impl ParachainInherentDataProvider { ) -> Option { // Only include next epoch authorities when the descendants include an epoch digest. // Skip the first entry because this is the relay parent itself. - let include_next_authorities = relay_parent_descendants.iter().skip(1).any(|header| { - sc_consensus_babe::find_next_epoch_digest::(header) - .ok() - .flatten() - .is_some() - }); + let include_next_authorities = relay_parent_descendants + .iter() + .skip(1) + .any(sc_consensus_babe::contains_epoch_change::); let relay_chain_state = collect_relay_storage_proof( relay_chain_interface, para_id, diff --git a/prdoc/pr_9990.prdoc b/prdoc/pr_9990.prdoc new file mode 100644 index 0000000000000..fa7f237466b66 --- /dev/null +++ b/prdoc/pr_9990.prdoc @@ -0,0 +1,16 @@ +title: '`cumulus`: Skip building on blocks on relay parents in old session' +doc: +- audience: Node Dev + description: |- + Collators building on older relay parents must skip building blocks when the chosen RP session is different than best block session. + + The relay chain will not accept such candidate. We can see this happening on the Kusama Canary parachain at each session boundary. + + Slot-based Aura has been modified in Cumulus to skip building blocks on relay parents in old session. +crates: +- name: cumulus-client-consensus-aura + bump: patch +- name: cumulus-client-parachain-inherent + bump: patch +- name: sc-consensus-babe + bump: minor \ No newline at end of file diff --git a/substrate/client/consensus/babe/src/lib.rs b/substrate/client/consensus/babe/src/lib.rs index a7cad251379bf..a21aa92871b8c 100644 --- a/substrate/client/consensus/babe/src/lib.rs +++ b/substrate/client/consensus/babe/src/lib.rs @@ -913,6 +913,11 @@ pub fn find_pre_digest(header: &B::Header) -> Result(header: &B::Header) -> bool { + find_next_epoch_digest::(header).ok().flatten().is_some() +} + /// Extract the BABE epoch change digest from the given header, if it exists. pub fn find_next_epoch_digest( header: &B::Header,