Use PostInherents to validate timestamp (#83)

* Use PostInherents to validate timestamp

* fix tests

* align nimbus basic collator implementation with the latest AuRa changes

* handle AllowMultipleBlocksPerSlot

Author: RomarQ

Cargo.lock

@@ -16,6 +16,7 @@ sp-blockchain = { workspace = true }
 sp-consensus = { workspace = true }
 sp-core = { workspace = true }
 sp-inherents = { workspace = true }
+sp-timestamp = { workspace = true }
 sp-keystore = { workspace = true }
 sp-runtime = { workspace = true }
 sp-version = { workspace = true }

client/consensus/nimbus-consensus/Cargo.toml

@@ -27,12 +27,16 @@ use crate::*;
 use cumulus_client_collator::service::ServiceInterface as CollatorServiceInterface;
 use cumulus_client_consensus_common::{ParachainBlockImportMarker, ParachainCandidate};
 use cumulus_client_consensus_proposer::ProposerInterface;
-use cumulus_primitives_core::ParachainBlockData;
+use cumulus_primitives_core::{
+	relay_chain::{OccupiedCoreAssumption, ValidationCodeHash},
+	ParachainBlockData,
+};
 use cumulus_primitives_parachain_inherent::ParachainInherentData;
 use futures::prelude::*;
 use log::{debug, info};
 use nimbus_primitives::{CompatibleDigestItem, DigestsProvider, NimbusId, NIMBUS_KEY_ID};
 use polkadot_node_primitives::{Collation, MaybeCompressedPoV};
+use polkadot_primitives::Hash as RelayHash;
 use sc_consensus::{BlockImport, BlockImportParams};
 use sp_application_crypto::ByteArray;
 use sp_consensus::{BlockOrigin, Proposal};
@@ -195,3 +199,54 @@ where
 
 	<DigestItem as CompatibleDigestItem>::nimbus_seal(signature)
 }
+
+/// Check the `local_validation_code_hash` against the validation code hash in the relay chain
+/// state.
+///
+/// If the code hashes do not match, it prints a warning.
+async fn check_validation_code_or_log(
+	local_validation_code_hash: &ValidationCodeHash,
+	para_id: ParaId,
+	relay_client: &impl RelayChainInterface,
+	relay_parent: RelayHash,
+) {
+	let state_validation_code_hash = match relay_client
+		.validation_code_hash(relay_parent, para_id, OccupiedCoreAssumption::Included)
+		.await
+	{
+		Ok(hash) => hash,
+		Err(error) => {
+			tracing::debug!(
+				target: super::LOG_TARGET,
+				%error,
+				?relay_parent,
+				%para_id,
+				"Failed to fetch validation code hash",
+			);
+			return;
+		}
+	};
+
+	match state_validation_code_hash {
+		Some(state) => {
+			if state != *local_validation_code_hash {
+				tracing::warn!(
+					target: super::LOG_TARGET,
+					%para_id,
+					?relay_parent,
+					?local_validation_code_hash,
+					relay_validation_code_hash = ?state,
+					"Parachain code doesn't match validation code stored in the relay chain state.",
+				);
+			}
+		}
+		None => {
+			tracing::warn!(
+				target: super::LOG_TARGET,
+				%para_id,
+				?relay_parent,
+				"Could not find validation code for parachain in the relay chain state.",
+			);
+		}
+	}
+}

client/consensus/nimbus-consensus/src/collators.rs

@@ -16,20 +16,21 @@
 
 use crate::*;
 use cumulus_client_collator::service::ServiceInterface as CollatorServiceInterface;
-use cumulus_client_consensus_common::ParachainBlockImportMarker;
+use cumulus_client_consensus_common::{self as consensus_common, ParachainBlockImportMarker};
 use cumulus_client_consensus_proposer::ProposerInterface;
 use cumulus_primitives_core::{
-	relay_chain::{BlockId as RBlockId, Hash as PHash},
+	relay_chain::{BlockId as RBlockId, Hash as PHash, ValidationCode},
 	CollectCollationInfo, ParaId, PersistedValidationData,
 };
 use cumulus_relay_chain_interface::{OverseerHandle, RelayChainInterface};
 use futures::prelude::*;
 use nimbus_primitives::{DigestsProvider, NimbusApi, NimbusId};
 use polkadot_node_primitives::CollationResult;
 use polkadot_primitives::CollatorPair;
-use sc_client_api::{BlockBackend, BlockOf};
-use sp_api::ProvideRuntimeApi;
+use sc_client_api::{AuxStore, BlockBackend, BlockOf, StateBackend};
+use sp_api::{CallApiAt, ProvideRuntimeApi};
 use sp_blockchain::HeaderBackend;
+use sp_consensus_slots::{Slot, SlotDuration};
 use sp_core::Decode;
 use sp_inherents::CreateInherentDataProviders;
 use sp_keystore::KeystorePtr;
@@ -44,6 +45,11 @@ pub struct Params<Proposer, BI, ParaClient, RClient, CIDP, CS, ADP = ()> {
 	pub para_id: ParaId,
 	/// A handle to the relay-chain client's "Overseer" or task orchestrator.
 	pub overseer_handle: OverseerHandle,
+	/// The length of slots in the relay chain.
+	pub relay_chain_slot_duration: Duration,
+	/// The length of slots in this parachain.
+	/// If the parachain doesn't have slot and rely only on relay slots, set it to None.
+	pub slot_duration: Option<SlotDuration>,
 	/// The underlying block proposer this should call into.
 	pub proposer: Proposer,
 	/// The block import handle.
@@ -79,8 +85,10 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 	BI: BlockImport<Block> + ParachainBlockImportMarker + Send + Sync + 'static,
 	Client: ProvideRuntimeApi<Block>
 		+ BlockOf
+		+ AuxStore
 		+ HeaderBackend<Block>
 		+ BlockBackend<Block>
+		+ CallApiAt<Block>
 		+ Send
 		+ Sync
 		+ 'static,
@@ -112,6 +120,9 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 		..
 	} = params;
 
+	let mut last_processed_slot = 0;
+	let mut last_relay_chain_block = Default::default();
+
 	while let Some(request) = collation_requests.next().await {
 		macro_rules! reject_with_error {
 			($err:expr) => {{
@@ -142,6 +153,25 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 			continue;
 		}
 
+		let Ok(Some(code)) = para_client
+			.state_at(parent_hash)
+			.map_err(drop)
+			.and_then(|s| {
+				s.storage(&sp_core::storage::well_known_keys::CODE)
+					.map_err(drop)
+			})
+		else {
+			continue;
+		};
+
+		super::check_validation_code_or_log(
+			&ValidationCode::from(code).hash(),
+			para_id,
+			&relay_client,
+			*request.relay_parent(),
+		)
+		.await;
+
 		let relay_parent_header = match relay_client
 			.header(RBlockId::hash(*request.relay_parent()))
 			.await
@@ -165,6 +195,54 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 			Err(e) => reject_with_error!(e),
 		};
 
+		// Determine which is the current slot
+		let (slot_now, timestamp) = match consensus_common::relay_slot_and_timestamp(
+			&relay_parent_header,
+			params.relay_chain_slot_duration,
+		) {
+			None => {
+				tracing::trace!(
+					target: crate::LOG_TARGET,
+					relay_parent = ?relay_parent_header,
+					relay_chain_slot_duration = ?params.relay_chain_slot_duration,
+					"Fail to get the relay slot for this relay block!"
+				);
+				continue;
+			}
+			Some((relay_slot, relay_timestamp)) => {
+				let our_slot = if let Some(slot_duration) = params.slot_duration {
+					Slot::from_timestamp(relay_timestamp, slot_duration)
+				} else {
+					// If there is no slot duration, we assume that the parachain use the relay slot directly
+					relay_slot
+				};
+				tracing::debug!(
+					target: crate::LOG_TARGET,
+					?relay_slot,
+					para_slot = ?our_slot,
+					?relay_timestamp,
+					slot_duration = ?params.slot_duration,
+					relay_chain_slot_duration = ?params.relay_chain_slot_duration,
+					"Adjusted relay-chain slot to parachain slot"
+				);
+				(our_slot, relay_timestamp)
+			}
+		};
+
+		// With async backing this function will be called every relay chain block.
+		//
+		// Most parachains currently run with 12 seconds slots and thus, they would try to
+		// produce multiple blocks per slot which very likely would fail on chain. Thus, we have
+		// this "hack" to only produce one block per slot per relay chain fork.
+		//
+		// With https://github.com/paritytech/polkadot-sdk/issues/3168 this implementation will be
+		// obsolete and also the underlying issue will be fixed.
+		if last_processed_slot >= *slot_now
+			&& last_relay_chain_block < *relay_parent_header.number()
+		{
+			continue;
+		}
+
 		let inherent_data = try_request!(
 			create_inherent_data(
 				&create_inherent_data_providers,
@@ -174,6 +252,7 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 				&relay_client,
 				*request.relay_parent(),
 				nimbus_id.clone(),
+				Some(timestamp)
 			)
 			.await
 		);
@@ -216,5 +295,8 @@ pub async fn run<Block, BI, CIDP, Backend, Client, RClient, Proposer, CS, ADP>(
 			request.complete(None);
 			tracing::debug!(target: crate::LOG_TARGET, "No block proposal");
 		}
+
+		last_processed_slot = *slot_now;
+		last_relay_chain_block = *relay_parent_header.number();
 	}
 }

client/consensus/nimbus-consensus/src/collators/basic.rs

@@ -207,7 +207,7 @@ where
 			};
 
 			// Determine which is the current slot
-			let slot_now = match consensus_common::relay_slot_and_timestamp(
+			let (slot_now, timestamp) = match consensus_common::relay_slot_and_timestamp(
 				&relay_parent_header,
 				params.relay_chain_slot_duration,
 			) {
@@ -236,7 +236,7 @@ where
 						relay_chain_slot_duration = ?params.relay_chain_slot_duration,
 						"Adjusted relay-chain slot to parachain slot"
 					);
-					our_slot
+					(our_slot, relay_timestamp)
 				}
 			};
 
@@ -343,6 +343,7 @@ where
 						&params.relay_client,
 						relay_parent,
 						author_id.clone(),
+						Some(timestamp),
 					)
 					.await
 					{
@@ -355,19 +356,21 @@ where
 
 				// Compute the hash of the parachain runtime bytecode that we using to build the block.
 				// The hash will be send to relay validators alongside the candidate.
-				let validation_code_hash = match params.code_hash_provider.code_hash_at(parent_hash)
-				{
-					None => {
-						tracing::error!(
-							target: crate::LOG_TARGET,
-							?parent_hash,
-							"Could not fetch validation code hash"
-						);
-						break;
-					}
-					Some(validation_code_hash) => validation_code_hash,
+				let Some(validation_code_hash) =
+					params.code_hash_provider.code_hash_at(parent_hash)
+				else {
+					tracing::error!(target: crate::LOG_TARGET, ?parent_hash, "Could not fetch validation code hash");
+					break;
 				};
 
+				super::check_validation_code_or_log(
+					&validation_code_hash,
+					params.para_id,
+					&params.relay_client,
+					relay_parent,
+				)
+				.await;
+
 				let allowed_pov_size = {
 					// Cap the percentage at 85% (see https://github.com/paritytech/polkadot-sdk/issues/6020)
 					let capped_percentage = params.max_pov_percentage.min(85);

client/consensus/nimbus-consensus/src/collators/lookahead.rs

@@ -98,7 +98,8 @@ where
 	}
 }
 
-/// Explicitly creates the inherent data for parachain block authoring.
+/// Explicitly creates the inherent data for parachain block authoring and overrides
+/// the timestamp inherent data with the one provided, if any.
 pub(crate) async fn create_inherent_data<Block, CIDP, RClient>(
 	create_inherent_data_providers: &CIDP,
 	para_id: ParaId,
@@ -107,6 +108,7 @@ pub(crate) async fn create_inherent_data<Block, CIDP, RClient>(
 	relay_client: &RClient,
 	relay_parent: PHash,
 	author_id: NimbusId,
+	timestamp: impl Into<Option<sp_timestamp::Timestamp>>,
 ) -> Result<(ParachainInherentData, InherentData), Box<dyn Error + Send + Sync + 'static>>
 where
 	Block: BlockT,
@@ -131,14 +133,18 @@ where
 		}
 	};
 
-	let other_inherent_data = create_inherent_data_providers
+	let mut other_inherent_data = create_inherent_data_providers
 		.create_inherent_data_providers(parent, (relay_parent, validation_data.clone(), author_id))
 		.map_err(|e| e as Box<dyn Error + Send + Sync + 'static>)
 		.await?
 		.create_inherent_data()
 		.await
 		.map_err(Box::new)?;
 
+	if let Some(timestamp) = timestamp.into() {
+		other_inherent_data.replace_data(sp_timestamp::INHERENT_IDENTIFIER, &timestamp);
+	}
+
 	Ok((paras_inherent_data, other_inherent_data))
 }