[stable2412] Partial backport of #6825 and #7205 (#7298)

This is a partial backport of #6825 and #7205. Only the node-side minor
changes were ported. However, this creates forward compatibility of the
omni-node from 2412 with runtimes built after #6825.

Author: skunert

Cargo.lock

@@ -82,6 +82,11 @@ polkadot-node-subsystem-util.default-features = true
 polkadot-overseer.workspace = true
 polkadot-overseer.default-features = true
 
+[dev-dependencies]
+cumulus-test-client = { workspace = true }
+cumulus-test-relay-sproof-builder = { workspace = true }
+sp-keyring = { workspace = true }
+
 [features]
 # Allows collator to use full PoV size for block building
 full-pov-size = []

cumulus/client/consensus/aura/Cargo.toml

@@ -336,6 +336,7 @@ where
 				);
 				Some(super::can_build_upon::<_, _, P>(
 					slot_now,
+					relay_slot,
 					timestamp,
 					block_hash,
 					included_block,

cumulus/client/consensus/aura/src/collators/lookahead.rs

@@ -34,7 +34,7 @@ use polkadot_primitives::{
 	ValidationCodeHash,
 };
 use sc_consensus_aura::{standalone as aura_internal, AuraApi};
-use sp_api::ProvideRuntimeApi;
+use sp_api::{ApiExt, ProvideRuntimeApi};
 use sp_core::Pair;
 use sp_keystore::KeystorePtr;
 use sp_timestamp::Timestamp;
@@ -160,7 +160,8 @@ async fn cores_scheduled_for_para(
 // Checks if we own the slot at the given block and whether there
 // is space in the unincluded segment.
 async fn can_build_upon<Block: BlockT, Client, P>(
-	slot: Slot,
+	para_slot: Slot,
+	relay_slot: Slot,
 	timestamp: Timestamp,
 	parent_hash: Block::Hash,
 	included_block: Block::Hash,
@@ -169,25 +170,35 @@ async fn can_build_upon<Block: BlockT, Client, P>(
 ) -> Option<SlotClaim<P::Public>>
 where
 	Client: ProvideRuntimeApi<Block>,
-	Client::Api: AuraApi<Block, P::Public> + AuraUnincludedSegmentApi<Block>,
+	Client::Api: AuraApi<Block, P::Public> + AuraUnincludedSegmentApi<Block> + ApiExt<Block>,
 	P: Pair,
 	P::Public: Codec,
 	P::Signature: Codec,
 {
 	let runtime_api = client.runtime_api();
 	let authorities = runtime_api.authorities(parent_hash).ok()?;
-	let author_pub = aura_internal::claim_slot::<P>(slot, &authorities, keystore).await?;
+	let author_pub = aura_internal::claim_slot::<P>(para_slot, &authorities, keystore).await?;
 
-	// Here we lean on the property that building on an empty unincluded segment must always
-	// be legal. Skipping the runtime API query here allows us to seamlessly run this
-	// collator against chains which have not yet upgraded their runtime.
-	if parent_hash != included_block &&
-		!runtime_api.can_build_upon(parent_hash, included_block, slot).ok()?
-	{
-		return None
+	// This function is typically called when we want to build block N. At that point, the
+	// unincluded segment in the runtime is unaware of the hash of block N-1. If the unincluded
+	// segment in the runtime is full, but block N-1 is the included block, the unincluded segment
+	// should have length 0 and we can build. Since the hash is not available to the runtime
+	// however, we need this extra check here.
+	if parent_hash == included_block {
+		return Some(SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp));
 	}
 
-	Some(SlotClaim::unchecked::<P>(author_pub, slot, timestamp))
+	let api_version = runtime_api
+		.api_version::<dyn AuraUnincludedSegmentApi<Block>>(parent_hash)
+		.ok()
+		.flatten()?;
+
+	let slot = if api_version > 1 { relay_slot } else { para_slot };
+
+	runtime_api
+		.can_build_upon(parent_hash, included_block, slot)
+		.ok()?
+		.then(|| SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp))
 }
 
 /// Use [`cumulus_client_consensus_common::find_potential_parents`] to find parachain blocks that
@@ -239,3 +250,116 @@ where
 		.max_by_key(|a| a.depth)
 		.map(|parent| (included_block, parent))
 }
+
+#[cfg(test)]
+mod tests {
+	use crate::collators::can_build_upon;
+	use codec::Encode;
+	use cumulus_primitives_aura::Slot;
+	use cumulus_primitives_core::BlockT;
+	use cumulus_relay_chain_interface::PHash;
+	use cumulus_test_client::{
+		runtime::{Block, Hash},
+		Client, DefaultTestClientBuilderExt, InitBlockBuilder, TestClientBuilder,
+		TestClientBuilderExt,
+	};
+	use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder;
+	use polkadot_primitives::HeadData;
+	use sc_consensus::{BlockImport, BlockImportParams, ForkChoiceStrategy};
+	use sp_consensus::BlockOrigin;
+	use sp_keystore::{Keystore, KeystorePtr};
+	use sp_timestamp::Timestamp;
+	use std::sync::Arc;
+
+	async fn import_block<I: BlockImport<Block>>(
+		importer: &I,
+		block: Block,
+		origin: BlockOrigin,
+		import_as_best: bool,
+	) {
+		let (header, body) = block.deconstruct();
+
+		let mut block_import_params = BlockImportParams::new(origin, header);
+		block_import_params.fork_choice = Some(ForkChoiceStrategy::Custom(import_as_best));
+		block_import_params.body = Some(body);
+		importer.import_block(block_import_params).await.unwrap();
+	}
+
+	fn sproof_with_parent_by_hash(client: &Client, hash: PHash) -> RelayStateSproofBuilder {
+		let header = client.header(hash).ok().flatten().expect("No header for parent block");
+		let included = HeadData(header.encode());
+		let mut builder = RelayStateSproofBuilder::default();
+		builder.para_id = cumulus_test_client::runtime::PARACHAIN_ID.into();
+		builder.included_para_head = Some(included);
+
+		builder
+	}
+	async fn build_and_import_block(client: &Client, included: Hash) -> Block {
+		let sproof = sproof_with_parent_by_hash(client, included);
+
+		let block_builder = client.init_block_builder(None, sproof).block_builder;
+
+		let block = block_builder.build().unwrap().block;
+
+		let origin = BlockOrigin::NetworkInitialSync;
+		import_block(client, block.clone(), origin, true).await;
+		block
+	}
+
+	fn set_up_components() -> (Arc<Client>, KeystorePtr) {
+		let keystore = Arc::new(sp_keystore::testing::MemoryKeystore::new()) as Arc<_>;
+		for key in sp_keyring::Sr25519Keyring::iter() {
+			Keystore::sr25519_generate_new(
+				&*keystore,
+				sp_application_crypto::key_types::AURA,
+				Some(&key.to_seed()),
+			)
+			.expect("Can insert key into MemoryKeyStore");
+		}
+		(Arc::new(TestClientBuilder::new().build()), keystore)
+	}
+
+	/// This tests a special scenario where the unincluded segment in the runtime
+	/// is full. We are calling `can_build_upon`, passing the last built block as the
+	/// included one. In the runtime we will not find the hash of the included block in the
+	/// unincluded segment. The `can_build_upon` runtime API would therefore return `false`, but
+	/// we are ensuring on the node side that we are are always able to build on the included block.
+	#[tokio::test]
+	async fn test_can_build_upon() {
+		let (client, keystore) = set_up_components();
+
+		let genesis_hash = client.chain_info().genesis_hash;
+		let mut last_hash = genesis_hash;
+
+		// Fill up the unincluded segment tracker in the runtime.
+		while can_build_upon::<_, _, sp_consensus_aura::sr25519::AuthorityPair>(
+			Slot::from(u64::MAX),
+			Slot::from(u64::MAX),
+			Timestamp::default(),
+			last_hash,
+			genesis_hash,
+			&*client,
+			&keystore,
+		)
+		.await
+		.is_some()
+		{
+			let block = build_and_import_block(&client, genesis_hash).await;
+			last_hash = block.header().hash();
+		}
+
+		// Blocks were built with the genesis hash set as included block.
+		// We call `can_build_upon` with the last built block as the included block.
+		let result = can_build_upon::<_, _, sp_consensus_aura::sr25519::AuthorityPair>(
+			Slot::from(u64::MAX),
+			Slot::from(u64::MAX),
+			Timestamp::default(),
+			last_hash,
+			last_hash,
+			&*client,
+			&keystore,
+		)
+		.await;
+		assert!(result.is_some());
+	}
+}

cumulus/client/consensus/aura/src/collators/mod.rs

@@ -25,8 +25,8 @@ use cumulus_relay_chain_interface::RelayChainInterface;
 
 use polkadot_primitives::{
 	vstaging::{ClaimQueueOffset, CoreSelector, DEFAULT_CLAIM_QUEUE_OFFSET},
-	BlockId, CoreIndex, Hash as RelayHash, Header as RelayHeader, Id as ParaId,
-	OccupiedCoreAssumption,
+	Block as RelayBlock, BlockId, CoreIndex, Hash as RelayHash, Header as RelayHeader,
+	Id as ParaId, OccupiedCoreAssumption,
 };
 
 use futures::prelude::*;
@@ -300,8 +300,17 @@ where
 			// on-chain data.
 			collator.collator_service().check_block_status(parent_hash, &parent_header);
 
+			let Ok(relay_slot) =
+				sc_consensus_babe::find_pre_digest::<RelayBlock>(relay_parent_header)
+					.map(|babe_pre_digest| babe_pre_digest.slot())
+			else {
+				tracing::error!(target: crate::LOG_TARGET, "Relay chain does not contain babe slot. This should never happen.");
+				continue;
+			};
+
 			let slot_claim = match crate::collators::can_build_upon::<_, _, P>(
 				para_slot.slot,
+				relay_slot,
 				para_slot.timestamp,
 				parent_hash,
 				included_block,

cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs

@@ -17,8 +17,9 @@
 use crate::{ParachainInherentData, INHERENT_IDENTIFIER};
 use codec::Decode;
 use cumulus_primitives_core::{
-	relay_chain, relay_chain::UpgradeGoAhead, InboundDownwardMessage, InboundHrmpMessage, ParaId,
-	PersistedValidationData,
+	relay_chain,
+	relay_chain::{Slot, UpgradeGoAhead},
+	InboundDownwardMessage, InboundHrmpMessage, ParaId, PersistedValidationData,
 };
 use cumulus_primitives_parachain_inherent::MessageQueueChain;
 use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder;
@@ -28,9 +29,6 @@ use sp_inherents::{InherentData, InherentDataProvider};
 use sp_runtime::traits::Block;
 use std::collections::BTreeMap;
 
-/// Relay chain slot duration, in milliseconds.
-pub const RELAY_CHAIN_SLOT_DURATION_MILLIS: u32 = 6000;
-
 /// Inherent data provider that supplies mocked validation data.
 ///
 /// This is useful when running a node that is not actually backed by any relay chain.
@@ -175,8 +173,7 @@ impl<R: Send + Sync + GenerateRandomness<u64>> InherentDataProvider
 		// Calculate the mocked relay block based on the current para block
 		let relay_parent_number =
 			self.relay_offset + self.relay_blocks_per_para_block * self.current_para_block;
-		sproof_builder.current_slot =
-			((relay_parent_number / RELAY_CHAIN_SLOT_DURATION_MILLIS) as u64).into();
+		sproof_builder.current_slot = Slot::from(relay_parent_number as u64);
 
 		sproof_builder.upgrade_go_ahead = self.upgrade_go_ahead;
 		// Process the downward messages and set up the correct head

cumulus/client/parachain-inherent/src/mock.rs

@@ -1118,6 +1118,7 @@ macro_rules! decl_test_networks {
 				) -> $crate::ParachainInherentData {
 					let mut sproof = $crate::RelayStateSproofBuilder::default();
 					sproof.para_id = para_id.into();
+					sproof.current_slot = $crate::polkadot_primitives::Slot::from(relay_parent_number as u64);
 
 					// egress channel
 					let e_index = sproof.hrmp_egress_channel_index.get_or_insert_with(Vec::new);

cumulus/xcm/xcm-emulator/src/lib.rs

@@ -0,0 +1,18 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Use relay chain slot for velocity measurement on parachains
+
+doc:
+  - audience: Runtime Dev
+    description: |
+        This is a partial backport of #6825. It makes the node side of omni-node forward compatible with runtimes that
+        are build with the changes from #6825.
+
+crates:
+  - name: cumulus-client-parachain-inherent
+    bump: patch
+  - name: cumulus-client-consensus-aura
+    bump: patch
+  - name: xcm-emulator
+    bump: patch

prdoc/pr_6825.prdoc

@@ -0,0 +1,10 @@
+title: 'Collator: Fix `can_build_upon` by always allowing to build on included block'
+doc:
+- audience: Node Dev
+  description: |-
+    Fixes a bug introduced in #6825.
+    We should always allow building on the included block of parachains. In situations where the unincluded segment
+    is full, but the included block moved to the most recent block, building was wrongly disallowed.
+crates:
+- name: cumulus-client-consensus-aura
+  bump: patch