Independence of Slot-based algorithms from system Timestamp (#12224)

* Remove timestamp from SlotInfo

* Expose as millis instead of secs

* Nits

* Fix test after field removal

* Yet another test fix

* On the fly timestamp computation

* Removed slot timestamp from logs

* Removed reference to timestamp from slots subsystem

* Slot based algorithm tests do not require timstamp inherent anymore

* Remove junk files

* Further tests cleanup

* Trigger pipeline

* Apply code suggestions

* Trigger pipeline

Co-authored-by: André Silva <andrerfosilva@gmail.com>

substrate/Cargo.lock

@@ -8246,7 +8246,6 @@ dependencies = [
  "sp-inherents",
  "sp-runtime",
  "sp-state-machine",
- "sp-timestamp",
  "substrate-test-runtime-client",
  "thiserror",
 ]

substrate/bin/node-template/node/src/service.rs

@@ -126,7 +126,7 @@ pub fn new_partial(
 						slot_duration,
 					);
 
-				Ok((timestamp, slot))
+				Ok((slot, timestamp))
 			},
 			spawner: &task_manager.spawn_essential_handle(),
 			registry: config.prometheus_registry(),
@@ -269,7 +269,7 @@ pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError>
 							slot_duration,
 						);
 
-					Ok((timestamp, slot))
+					Ok((slot, timestamp))
 				},
 				force_authoring,
 				backoff_authoring_blocks,

substrate/bin/node/cli/src/service.rs

@@ -223,7 +223,7 @@ pub fn new_partial(
 			let uncles =
 				sp_authorship::InherentDataProvider::<<Block as BlockT>::Header>::check_inherents();
 
-			Ok((timestamp, slot, uncles))
+			Ok((slot, timestamp, uncles))
 		},
 		&task_manager.spawn_essential_handle(),
 		config.prometheus_registry(),
@@ -453,7 +453,7 @@ pub fn new_full_base(
 							&parent,
 						)?;
 
-					Ok((timestamp, slot, uncles, storage_proof))
+					Ok((slot, timestamp, uncles, storage_proof))
 				}
 			},
 			force_authoring,

substrate/client/consensus/aura/src/lib.rs

@@ -569,7 +569,7 @@ mod tests {
 		traits::{Block as BlockT, Header as _},
 		Digest,
 	};
-	use sp_timestamp::InherentDataProvider as TimestampInherentDataProvider;
+	use sp_timestamp::Timestamp;
 	use std::{
 		task::Poll,
 		time::{Duration, Instant},
@@ -579,6 +579,8 @@ mod tests {
 		TestClient,
 	};
 
+	const SLOT_DURATION_MS: u64 = 1000;
+
 	type Error = sp_blockchain::Error;
 
 	struct DummyFactory(Arc<TestClient>);
@@ -619,8 +621,6 @@ mod tests {
 		}
 	}
 
-	const SLOT_DURATION: u64 = 1000;
-
 	type AuraVerifier = import_queue::AuraVerifier<
 		PeersFullClient,
 		AuthorityPair,
@@ -628,7 +628,7 @@ mod tests {
 			dyn CreateInherentDataProviders<
 				TestBlock,
 				(),
-				InherentDataProviders = (TimestampInherentDataProvider, InherentDataProvider),
+				InherentDataProviders = (InherentDataProvider,),
 			>,
 		>,
 	>;
@@ -648,17 +648,15 @@ mod tests {
 			let client = client.as_client();
 			let slot_duration = slot_duration(&*client).expect("slot duration available");
 
-			assert_eq!(slot_duration.as_millis() as u64, SLOT_DURATION);
+			assert_eq!(slot_duration.as_millis() as u64, SLOT_DURATION_MS);
 			import_queue::AuraVerifier::new(
 				client,
 				Box::new(|_, _| async {
-					let timestamp = TimestampInherentDataProvider::from_system_time();
 					let slot = InherentDataProvider::from_timestamp_and_slot_duration(
-						*timestamp,
+						Timestamp::current(),
-						SlotDuration::from_millis(6000),
+						SlotDuration::from_millis(SLOT_DURATION_MS),
 					);
-
+					Ok((slot,))
-					Ok((timestamp, slot))
 				}),
 				CheckForEquivocation::Yes,
 				None,
@@ -736,13 +734,12 @@ mod tests {
 					sync_oracle: DummyOracle,
 					justification_sync_link: (),
 					create_inherent_data_providers: |_, _| async {
-						let timestamp = TimestampInherentDataProvider::from_system_time();
 						let slot = InherentDataProvider::from_timestamp_and_slot_duration(
-							*timestamp,
+							Timestamp::current(),
-							SlotDuration::from_millis(6000),
+							SlotDuration::from_millis(SLOT_DURATION_MS),
 						);
 
-						Ok((timestamp, slot))
+						Ok((slot,))
 					},
 					force_authoring: false,
 					backoff_authoring_blocks: Some(
@@ -875,7 +872,6 @@ mod tests {
 
 		let res = executor::block_on(worker.on_slot(SlotInfo {
 			slot: 0.into(),
-			timestamp: 0.into(),
 			ends_at: Instant::now() + Duration::from_secs(100),
 			inherent_data: InherentData::new(),
 			duration: Duration::from_millis(1000),

substrate/client/consensus/babe/src/tests.rs

@@ -43,7 +43,7 @@ use sp_runtime::{
 	generic::{Digest, DigestItem},
 	traits::Block as BlockT,
 };
-use sp_timestamp::InherentDataProvider as TimestampInherentDataProvider;
+use sp_timestamp::Timestamp;
 use std::{cell::RefCell, task::Poll, time::Duration};
 
 type Item = DigestItem;
@@ -68,6 +68,8 @@ type Mutator = Arc<dyn Fn(&mut TestHeader, Stage) + Send + Sync>;
 type BabeBlockImport =
 	PanickingBlockImport<crate::BabeBlockImport<TestBlock, TestClient, Arc<TestClient>>>;
 
+const SLOT_DURATION_MS: u64 = 1000;
+
 #[derive(Clone)]
 struct DummyFactory {
 	client: Arc<TestClient>,
@@ -239,7 +241,7 @@ pub struct TestVerifier {
 			dyn CreateInherentDataProviders<
 				TestBlock,
 				(),
-				InherentDataProviders = (TimestampInherentDataProvider, InherentDataProvider),
+				InherentDataProviders = (InherentDataProvider,),
 			>,
 		>,
 	>,
@@ -321,13 +323,11 @@ impl TestNetFactory for BabeTestNet {
 				client: client.clone(),
 				select_chain: longest_chain,
 				create_inherent_data_providers: Box::new(|_, _| async {
-					let timestamp = TimestampInherentDataProvider::from_system_time();
 					let slot = InherentDataProvider::from_timestamp_and_slot_duration(
-						*timestamp,
+						Timestamp::current(),
-						SlotDuration::from_millis(6000),
+						SlotDuration::from_millis(SLOT_DURATION_MS),
 					);
-
+					Ok((slot,))
-					Ok((timestamp, slot))
 				}),
 				config: data.link.config.clone(),
 				epoch_changes: data.link.epoch_changes.clone(),
@@ -433,13 +433,11 @@ fn run_one_test(mutator: impl Fn(&mut TestHeader, Stage) + Send + Sync + 'static
 				env: environ,
 				sync_oracle: DummyOracle,
 				create_inherent_data_providers: Box::new(|_, _| async {
-					let timestamp = TimestampInherentDataProvider::from_system_time();
 					let slot = InherentDataProvider::from_timestamp_and_slot_duration(
-						*timestamp,
+						Timestamp::current(),
-						SlotDuration::from_millis(6000),
+						SlotDuration::from_millis(SLOT_DURATION_MS),
 					);
-
+					Ok((slot,))
-					Ok((timestamp, slot))
 				}),
 				force_authoring: false,
 				backoff_authoring_blocks: Some(BackoffAuthoringOnFinalizedHeadLagging::default()),

substrate/client/consensus/slots/Cargo.toml

@@ -31,7 +31,6 @@ sp-core = { version = "6.0.0", path = "../../../primitives/core" }
 sp-inherents = { version = "4.0.0-dev", path = "../../../primitives/inherents" }
 sp-runtime = { version = "6.0.0", path = "../../../primitives/runtime" }
 sp-state-machine = { version = "0.12.0", path = "../../../primitives/state-machine" }
-sp-timestamp = { version = "4.0.0-dev", path = "../../../primitives/timestamp" }
 
 [dev-dependencies]
 substrate-test-runtime-client = { version = "2.0.0", path = "../../../test-utils/runtime/client" }

substrate/client/consensus/slots/src/lib.rs

@@ -42,7 +42,6 @@ use sp_consensus::{Proposal, Proposer, SelectChain, SyncOracle};
 use sp_consensus_slots::{Slot, SlotDuration};
 use sp_inherents::CreateInherentDataProviders;
 use sp_runtime::traits::{Block as BlockT, HashFor, Header as HeaderT};
-use sp_timestamp::Timestamp;
 use std::{fmt::Debug, ops::Deref, time::Duration};
 
 /// The changes that need to applied to the storage to create the state for a block.
@@ -252,7 +251,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 	where
 		Self: Sync,
 	{
-		let (timestamp, slot) = (slot_info.timestamp, slot_info.slot);
+		let slot = slot_info.slot;
 		let telemetry = self.telemetry();
 		let logging_target = self.logging_target();
 
@@ -316,23 +315,14 @@ pub trait SimpleSlotWorker<B: BlockT> {
 			return None
 		}
 
-		debug!(
+		debug!(target: logging_target, "Starting authorship at slot: {slot}");
-			target: logging_target,
-			"Starting authorship at slot {}; timestamp = {}", slot, *timestamp,
-		);
 
-		telemetry!(
+		telemetry!(telemetry; CONSENSUS_DEBUG; "slots.starting_authorship"; "slot_num" => slot);
-			telemetry;
-			CONSENSUS_DEBUG;
-			"slots.starting_authorship";
-			"slot_num" => *slot,
-			"timestamp" => *timestamp,
-		);
 
 		let proposer = match self.proposer(&slot_info.chain_head).await {
 			Ok(p) => p,
 			Err(err) => {
-				warn!(target: logging_target, "Unable to author block in slot {:?}: {}", slot, err,);
+				warn!(target: logging_target, "Unable to author block in slot {slot:?}: {err}");
 
 				telemetry!(
 					telemetry;
@@ -440,44 +430,35 @@ impl<T: SimpleSlotWorker<B> + Send + Sync, B: BlockT>
 
 /// Slot specific extension that the inherent data provider needs to implement.
 pub trait InherentDataProviderExt {
-	/// The current timestamp that will be found in the
-	/// [`InherentData`](`sp_inherents::InherentData`).
-	fn timestamp(&self) -> Timestamp;
-
 	/// The current slot that will be found in the [`InherentData`](`sp_inherents::InherentData`).
 	fn slot(&self) -> Slot;
 }
 
 /// Small macro for implementing `InherentDataProviderExt` for inherent data provider tuple.
 macro_rules! impl_inherent_data_provider_ext_tuple {
-	( T, S $(, $TN:ident)* $( , )?) => {
+	( S $(, $TN:ident)* $( , )?) => {
-		impl<T, S, $( $TN ),*>  InherentDataProviderExt for (T, S, $($TN),*)
+		impl<S, $( $TN ),*>  InherentDataProviderExt for (S, $($TN),*)
 		where
-			T: Deref<Target = Timestamp>,
 			S: Deref<Target = Slot>,
 		{
-			fn timestamp(&self) -> Timestamp {
-				*self.0.deref()
-			}
-
 			fn slot(&self) -> Slot {
-				*self.1.deref()
+				*self.0.deref()
 			}
 		}
 	}
 }
 
-impl_inherent_data_provider_ext_tuple!(T, S);
+impl_inherent_data_provider_ext_tuple!(S);
-impl_inherent_data_provider_ext_tuple!(T, S, A);
+impl_inherent_data_provider_ext_tuple!(S, A);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B);
+impl_inherent_data_provider_ext_tuple!(S, A, B);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E, F);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E, F);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E, F, G);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E, F, G);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E, F, G, H);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E, F, G, H);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E, F, G, H, I);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E, F, G, H, I);
-impl_inherent_data_provider_ext_tuple!(T, S, A, B, C, D, E, F, G, H, I, J);
+impl_inherent_data_provider_ext_tuple!(S, A, B, C, D, E, F, G, H, I, J);
 
 /// Start a new slot worker.
 ///
@@ -806,7 +787,6 @@ mod test {
 		super::slots::SlotInfo {
 			slot: slot.into(),
 			duration: SLOT_DURATION,
-			timestamp: Default::default(),
 			inherent_data: Default::default(),
 			ends_at: Instant::now() + SLOT_DURATION,
 			chain_head: Header::new(

substrate/client/consensus/slots/src/slots.rs

@@ -50,8 +50,6 @@ pub fn time_until_next_slot(slot_duration: Duration) -> Duration {
 pub struct SlotInfo<B: BlockT> {
 	/// The slot number as found in the inherent data.
 	pub slot: Slot,
-	/// Current timestamp as found in the inherent data.
-	pub timestamp: sp_timestamp::Timestamp,
 	/// The instant at which the slot ends.
 	pub ends_at: Instant,
 	/// The inherent data.
@@ -72,7 +70,6 @@ impl<B: BlockT> SlotInfo<B> {
 	/// `ends_at` is calculated using `timestamp` and `duration`.
 	pub fn new(
 		slot: Slot,
-		timestamp: sp_timestamp::Timestamp,
 		inherent_data: InherentData,
 		duration: Duration,
 		chain_head: B::Header,
@@ -80,7 +77,6 @@ impl<B: BlockT> SlotInfo<B> {
 	) -> Self {
 		Self {
 			slot,
-			timestamp,
 			inherent_data,
 			duration,
 			chain_head,
@@ -175,7 +171,6 @@ where
 				);
 			}
 
-			let timestamp = inherent_data_providers.timestamp();
 			let slot = inherent_data_providers.slot();
 			let inherent_data = inherent_data_providers.create_inherent_data()?;
 
@@ -183,14 +178,7 @@ where
 			if slot > self.last_slot {
 				self.last_slot = slot;
 
-				break Ok(SlotInfo::new(
+				break Ok(SlotInfo::new(slot, inherent_data, self.slot_duration, chain_head, None))
-					slot,
-					timestamp,
-					inherent_data,
-					self.slot_duration,
-					chain_head,
-					None,
-				))
 			}
 		}
 	}

substrate/primitives/timestamp/src/lib.rs

@@ -56,6 +56,17 @@ impl Timestamp {
 	pub fn checked_sub(self, other: Self) -> Option<Self> {
 		self.0.checked_sub(other.0).map(Self)
 	}
+
+	/// The current timestamp using the system time.
+	#[cfg(feature = "std")]
+	pub fn current() -> Self {
+		use std::time::SystemTime;
+
+		let now = SystemTime::now();
+		now.duration_since(SystemTime::UNIX_EPOCH)
+			.expect("Current time is always after unix epoch; qed")
+			.into()
+	}
 }
 
 impl sp_std::ops::Deref for Timestamp {
@@ -165,18 +176,6 @@ impl TimestampInherentData for InherentData {
 	}
 }
 
-/// The current timestamp using the system time.
-///
-/// This timestamp is the time since the UNIX epoch.
-#[cfg(feature = "std")]
-fn current_timestamp() -> std::time::Duration {
-	use std::time::SystemTime;
-
-	let now = SystemTime::now();
-	now.duration_since(SystemTime::UNIX_EPOCH)
-		.expect("Current time is always after unix epoch; qed")
-}
-
 /// Provide duration since unix epoch in millisecond for timestamp inherent.
 #[cfg(feature = "std")]
 pub struct InherentDataProvider {
@@ -190,7 +189,7 @@ impl InherentDataProvider {
 	pub fn from_system_time() -> Self {
 		Self {
 			max_drift: std::time::Duration::from_secs(60).into(),
-			timestamp: current_timestamp().into(),
+			timestamp: Timestamp::current(),
 		}
 	}