Parachains: Use relay chain slot for velocity measurement

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

@@ -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/mod.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,33 @@ 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?;
+
+	let Ok(Some(api_version)) =
+		runtime_api.api_version::<dyn AuraUnincludedSegmentApi<Block>>(parent_hash)
+	else {
+		return Some(SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp))
+	};
+
+	let slot = if api_version > 1 { relay_slot } else { para_slot };
 
 	// 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()?
+	if parent_hash == included_block ||
+		runtime_api.can_build_upon(parent_hash, included_block, slot).ok()?
 	{
-		return None
+		Some(SlotClaim::unchecked::<P>(author_pub, para_slot, timestamp))
+	} else {
+		None
 	}
-
-	Some(SlotClaim::unchecked::<P>(author_pub, slot, timestamp))
 }
 
 /// Use [`cumulus_client_consensus_common::find_potential_parents`] to find parachain blocks that

cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.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/parachain-inherent/src/mock.rs

@@ -17,7 +17,8 @@
 use crate::{ParachainInherentData, INHERENT_IDENTIFIER};
 use codec::Decode;
 use cumulus_primitives_core::{
-	relay_chain, InboundDownwardMessage, InboundHrmpMessage, ParaId, PersistedValidationData,
+	relay_chain, relay_chain::Slot, InboundDownwardMessage, InboundHrmpMessage, ParaId,
+	PersistedValidationData,
 };
 use cumulus_primitives_parachain_inherent::MessageQueueChain;
 use sc_client_api::{Backend, StorageProvider};
@@ -28,9 +29,6 @@ use std::collections::BTreeMap;
 
 use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder;
 
-/// 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.
@@ -173,8 +171,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);
 
 		// Process the downward messages and set up the correct head
 		let mut downward_messages = Vec::new();

cumulus/pallets/aura-ext/Cargo.toml

@@ -28,9 +28,15 @@ sp-runtime = { workspace = true }
 cumulus-pallet-parachain-system = { workspace = true }
 
 [dev-dependencies]
-
 # Cumulus
 cumulus-pallet-parachain-system = { workspace = true, default-features = true }
+cumulus-test-relay-sproof-builder = { workspace = true, default-features = true }
+cumulus-primitives-core = { workspace = true, default-features = true }
+
+# Substrate
+sp-io = { workspace = true, default-features = true }
+sp-core = { workspace = true, default-features = true }
+sp-version = { workspace = true, default-features = true }
 
 [features]
 default = ["std"]

cumulus/pallets/aura-ext/src/consensus_hook.rs

@@ -18,7 +18,6 @@
 //! block velocity.
 //!
 //! The velocity `V` refers to the rate of block processing by the relay chain.
-
 use super::{pallet, Aura};
 use core::{marker::PhantomData, num::NonZeroU32};
 use cumulus_pallet_parachain_system::{
@@ -54,8 +53,23 @@ where
 		let velocity = V.max(1);
 		let relay_chain_slot = state_proof.read_slot().expect("failed to read relay chain slot");
 
-		let (slot, authored) =
-			pallet::SlotInfo::<T>::get().expect("slot info is inserted on block initialization");
+		let (relay_chain_slot, authored_in_relay) = match pallet::RelaySlotInfo::<T>::get() {
+			Some((slot, authored)) if slot == relay_chain_slot => (slot, authored + 1),
+			Some((slot, _)) if slot < relay_chain_slot => (relay_chain_slot, 1),
+			Some(..) => {
+				panic!("slot moved backwards")
+			},
+			None => (relay_chain_slot, 1),
+		};
+
+		// We need to allow one additional block to be built to fill the unincluded segment.
+		if authored_in_relay > velocity + 1 {
+			panic!("authored blocks limit is reached for the slot: relay_chain_slot={relay_chain_slot:?}, authored={authored_in_relay:?}, velocity={velocity:?}");
+		}
+
+		pallet::RelaySlotInfo::<T>::put((relay_chain_slot, authored_in_relay));
+
+		let para_slot = pallet_aura::CurrentSlot::<T>::get();
 
 		// Convert relay chain timestamp.
 		let relay_chain_timestamp =
@@ -67,19 +81,16 @@ where
 
 		// Check that we are not too far in the future. Since we expect `V` parachain blocks
 		// during the relay chain slot, we can allow for `V` parachain slots into the future.
-		if *slot > *para_slot_from_relay + u64::from(velocity) {
+		if *para_slot > *para_slot_from_relay + u64::from(velocity) {
 			panic!(
-				"Parachain slot is too far in the future: parachain_slot: {:?}, derived_from_relay_slot: {:?} velocity: {:?}",
-				slot,
+				"Parachain slot is too far in the future: parachain_slot={:?}, derived_from_relay_slot={:?} velocity={:?}, relay_chain_slot={:?}",
+				para_slot,
 				para_slot_from_relay,
-				velocity
+				velocity,
+				relay_chain_slot
 			);
 		}
 
-		// We need to allow authoring multiple blocks in the same slot.
-		if slot != para_slot_from_relay && authored > velocity {
-			panic!("authored blocks limit is reached for the slot")
-		}
 		let weight = T::DbWeight::get().reads(1);
 
 		(
@@ -110,7 +121,7 @@ impl<
 	/// is more recent than the included block itself.
 	pub fn can_build_upon(included_hash: T::Hash, new_slot: Slot) -> bool {
 		let velocity = V.max(1);
-		let (last_slot, authored_so_far) = match pallet::SlotInfo::<T>::get() {
+		let (last_slot, authored_so_far) = match pallet::RelaySlotInfo::<T>::get() {
 			None => return true,
 			Some(x) => x,
 		};
@@ -123,11 +134,8 @@ impl<
 			return false
 		}
 
-		// TODO: This logic needs to be adjusted.
-		// It checks that we have not authored more than `V + 1` blocks in the slot.
-		// As a slot however, we take the parachain slot here. Velocity should
-		// be measured in relation to the relay chain slot.
-		// https://github.com/paritytech/polkadot-sdk/issues/3967
+		// Check that we have not authored more than `V + 1` parachain blocks in the current relay
+		// chain slot.
 		if last_slot == new_slot {
 			authored_so_far < velocity + 1
 		} else {

cumulus/pallets/aura-ext/src/lib.rs

@@ -40,6 +40,9 @@ use sp_consensus_aura::{digests::CompatibleDigestItem, Slot};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
 pub mod consensus_hook;
+pub mod migration;
+mod test;
+
 pub use consensus_hook::FixedVelocityConsensusHook;
 
 type Aura<T> = pallet_aura::Pallet<T>;
@@ -57,6 +60,7 @@ pub mod pallet {
 	pub trait Config: pallet_aura::Config + frame_system::Config {}
 
 	#[pallet::pallet]
+	#[pallet::storage_version(migration::STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
 
 	#[pallet::hooks]
@@ -70,20 +74,7 @@ pub mod pallet {
 			// Fetch the authorities once to get them into the storage proof of the PoV.
 			Authorities::<T>::get();
 
-			let new_slot = pallet_aura::CurrentSlot::<T>::get();
-
-			let (new_slot, authored) = match SlotInfo::<T>::get() {
-				Some((slot, authored)) if slot == new_slot => (slot, authored + 1),
-				Some((slot, _)) if slot < new_slot => (new_slot, 1),
-				Some(..) => {
-					panic!("slot moved backwards")
-				},
-				None => (new_slot, 1),
-			};
-
-			SlotInfo::<T>::put((new_slot, authored));
-
-			T::DbWeight::get().reads_writes(4, 2)
+			T::DbWeight::get().reads_writes(1, 0)
 		}
 	}
 
@@ -99,11 +90,12 @@ pub mod pallet {
 		ValueQuery,
 	>;
 
-	/// Current slot paired with a number of authored blocks.
+	/// Current relay chain slot paired with a number of authored blocks.
 	///
-	/// Updated on each block initialization.
+	/// This is updated in [`FixedVelocityConsensusHook::on_state_proof`] with the current relay
+	/// chain slot as provided by the relay chain state proof.
 	#[pallet::storage]
-	pub(crate) type SlotInfo<T: Config> = StorageValue<_, (Slot, u32), OptionQuery>;
+	pub(crate) type RelaySlotInfo<T: Config> = StorageValue<_, (Slot, u32), OptionQuery>;
 
 	#[pallet::genesis_config]
 	#[derive(frame_support::DefaultNoBound)]

cumulus/pallets/aura-ext/src/migration.rs

@@ -0,0 +1,74 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// This file is part of Cumulus.
+
+// Cumulus 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.
+
+// Cumulus 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 Cumulus.  If not, see <http://www.gnu.org/licenses/>.
+extern crate alloc;
+
+use crate::{Config, Pallet};
+#[cfg(feature = "try-runtime")]
+use alloc::vec::Vec;
+use frame_support::{migrations::VersionedMigration, pallet_prelude::StorageVersion};
+
+/// The in-code storage version.
+pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
+mod v0 {
+	use super::*;
+	use frame_support::{pallet_prelude::OptionQuery, storage_alias};
+	use sp_consensus_aura::Slot;
+
+	/// Current slot paired with a number of authored blocks.
+	///
+	/// Updated on each block initialization.
+	#[storage_alias]
+	pub(super) type SlotInfo<T: Config> = StorageValue<Pallet<T>, (Slot, u32), OptionQuery>;
+}
+mod v1 {
+	use super::*;
+	use frame_support::{pallet_prelude::*, traits::UncheckedOnRuntimeUpgrade};
+
+	pub struct UncheckedMigrationToV1<T: Config>(PhantomData<T>);
+
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV1<T> {
+		fn on_runtime_upgrade() -> Weight {
+			let mut weight: Weight = Weight::zero();
+			weight += migrate::<T>();
+			weight
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
+			Ok(Vec::new())
+		}
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_state: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
+			ensure!(!v0::SlotInfo::<T>::exists(), "SlotInfo should not exist");
+			Ok(())
+		}
+	}
+
+	pub fn migrate<T: Config>() -> Weight {
+		v0::SlotInfo::<T>::kill();
+		T::DbWeight::get().writes(1)
+	}
+}
+
+/// Migrate `V0` to `V1`.
+pub type MigrateV0ToV1<T> = VersionedMigration<
+	0,
+	1,
+	v1::UncheckedMigrationToV1<T>,
+	Pallet<T>,
+	<T as frame_system::Config>::DbWeight,
+>;