consensus-slots: cleanup SlotDuration config (#10878)

* consensus-slots: cleanup the SlotDuration config

* fix tests

* address review comments

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

@@ -66,9 +66,7 @@
 #![forbid(unsafe_code)]
 #![warn(missing_docs)]
 
-use std::{
-	borrow::Cow, collections::HashMap, convert::TryInto, pin::Pin, sync::Arc, time::Duration, u64,
-};
+use std::{borrow::Cow, collections::HashMap, convert::TryInto, pin::Pin, sync::Arc, u64};
 
 use codec::{Decode, Encode};
 use futures::{
@@ -106,10 +104,10 @@ use sp_block_builder::BlockBuilder as BlockBuilderApi;
 use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult};
 use sp_consensus::{
 	BlockOrigin, CacheKeyId, CanAuthorWith, Environment, Error as ConsensusError, Proposer,
-	SelectChain, SlotData,
+	SelectChain,
 };
 use sp_consensus_babe::inherents::BabeInherentData;
-use sp_consensus_slots::Slot;
+use sp_consensus_slots::{Slot, SlotDuration};
 use sp_core::{crypto::ByteArray, ExecutionContext};
 use sp_inherents::{CreateInherentDataProviders, InherentData, InherentDataProvider};
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
@@ -328,17 +326,15 @@ pub struct BabeIntermediate<B: BlockT> {
 /// Intermediate key for Babe engine.
 pub static INTERMEDIATE_KEY: &[u8] = b"babe1";
 
-/// A slot duration.
-///
-/// Create with [`Self::get`].
-// FIXME: Once Rust has higher-kinded types, the duplication between this
-// and `super::babe::Config` can be eliminated.
-// https://github.com/paritytech/substrate/issues/2434
+/// Configuration for BABE used for defining block verification parameters as
+/// well as authoring (e.g. the slot duration).
 #[derive(Clone)]
-pub struct Config(sc_consensus_slots::SlotDuration<BabeGenesisConfiguration>);
+pub struct Config {
+	genesis_config: BabeGenesisConfiguration,
+}
 
 impl Config {
-	/// Fetch the config from the runtime.
+	/// Create a new config by reading the genesis configuration from the runtime.
 	pub fn get<B: BlockT, C>(client: &C) -> ClientResult<Self>
 	where
 		C: AuxStore + ProvideRuntimeApi<B> + UsageProvider<B>,
@@ -355,7 +351,7 @@ impl Config {
 
 		let version = runtime_api.api_version::<dyn BabeApi<B>>(&best_block_id)?;
 
-		let slot_duration = if version == Some(1) {
+		let genesis_config = if version == Some(1) {
 			#[allow(deprecated)]
 			{
 				runtime_api.configuration_before_version_2(&best_block_id)?.into()
@@ -368,20 +364,17 @@ impl Config {
 			))
 		};
 
-		Ok(Self(sc_consensus_slots::SlotDuration::new(slot_duration)))
+		Ok(Config { genesis_config })
 	}
 
-	/// Get the inner slot duration
-	pub fn slot_duration(&self) -> Duration {
-		self.0.slot_duration()
+	/// Get the genesis configuration.
+	pub fn genesis_config(&self) -> &BabeGenesisConfiguration {
+		&self.genesis_config
 	}
-}
 
-impl std::ops::Deref for Config {
-	type Target = BabeGenesisConfiguration;
-
-	fn deref(&self) -> &BabeGenesisConfiguration {
-		&*self.0
+	/// Get the slot duration defined in the genesis configuration.
+	pub fn slot_duration(&self) -> SlotDuration {
+		SlotDuration::from_millis(self.genesis_config.slot_duration)
 	}
 }
 
@@ -488,7 +481,6 @@ where
 {
 	const HANDLE_BUFFER_SIZE: usize = 1024;
 
-	let config = babe_link.config;
 	let slot_notification_sinks = Arc::new(Mutex::new(Vec::new()));
 
 	let worker = BabeSlotWorker {
@@ -502,7 +494,7 @@ where
 		keystore,
 		epoch_changes: babe_link.epoch_changes.clone(),
 		slot_notification_sinks: slot_notification_sinks.clone(),
-		config: config.clone(),
+		config: babe_link.config.clone(),
 		block_proposal_slot_portion,
 		max_block_proposal_slot_portion,
 		telemetry,
@@ -510,7 +502,7 @@ where
 
 	info!(target: "babe", "👶 Starting BABE Authorship worker");
 	let inner = sc_consensus_slots::start_slot_worker(
-		config.0.clone(),
+		babe_link.config.slot_duration(),
 		select_chain,
 		worker,
 		sync_oracle,
@@ -521,7 +513,8 @@ where
 	let (worker_tx, worker_rx) = channel(HANDLE_BUFFER_SIZE);
 
 	let answer_requests =
-		answer_requests(worker_rx, config.0, client, babe_link.epoch_changes.clone());
+		answer_requests(worker_rx, babe_link.config, client, babe_link.epoch_changes.clone());
+
 	Ok(BabeWorker {
 		inner: Box::pin(future::join(inner, answer_requests).map(|_| ())),
 		slot_notification_sinks,
@@ -531,7 +524,7 @@ where
 
 async fn answer_requests<B: BlockT, C>(
 	mut request_rx: Receiver<BabeRequest<B>>,
-	genesis_config: sc_consensus_slots::SlotDuration<BabeGenesisConfiguration>,
+	config: Config,
 	client: Arc<C>,
 	epoch_changes: SharedEpochChanges<B, Epoch>,
 ) where
@@ -561,7 +554,7 @@ async fn answer_requests<B: BlockT, C>(
 
 					let viable_epoch = epoch_changes
 						.viable_epoch(&epoch_descriptor, |slot| {
-							Epoch::genesis(&genesis_config, slot)
+							Epoch::genesis(&config.genesis_config, slot)
 						})
 						.ok_or_else(|| Error::<B>::FetchEpoch(parent_hash))?;
 
@@ -720,7 +713,9 @@ where
 	fn authorities_len(&self, epoch_descriptor: &Self::EpochData) -> Option<usize> {
 		self.epoch_changes
 			.shared_data()
-			.viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot))
+			.viable_epoch(&epoch_descriptor, |slot| {
+				Epoch::genesis(&self.config.genesis_config, slot)
+			})
 			.map(|epoch| epoch.as_ref().authorities.len())
 	}
 
@@ -735,7 +730,9 @@ where
 			slot,
 			self.epoch_changes
 				.shared_data()
-				.viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot))?
+				.viable_epoch(&epoch_descriptor, |slot| {
+					Epoch::genesis(&self.config.genesis_config, slot)
+				})?
 				.as_ref(),
 			&self.keystore,
 		);
@@ -1165,7 +1162,9 @@ where
 				.map_err(|e| Error::<Block>::ForkTree(Box::new(e)))?
 				.ok_or_else(|| Error::<Block>::FetchEpoch(parent_hash))?;
 			let viable_epoch = epoch_changes
-				.viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot))
+				.viable_epoch(&epoch_descriptor, |slot| {
+					Epoch::genesis(&self.config.genesis_config, slot)
+				})
 				.ok_or_else(|| Error::<Block>::FetchEpoch(parent_hash))?;
 
 			// We add one to the current slot to allow for some small drift.
@@ -1498,7 +1497,9 @@ where
 				old_epoch_changes = Some((*epoch_changes).clone());
 
 				let viable_epoch = epoch_changes
-					.viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot))
+					.viable_epoch(&epoch_descriptor, |slot| {
+						Epoch::genesis(&self.config.genesis_config, slot)
+					})
 					.ok_or_else(|| {
 						ConsensusError::ClientImport(Error::<Block>::FetchEpoch(parent_hash).into())
 					})?;
@@ -1684,7 +1685,8 @@ pub fn block_import<Client, Block: BlockT, I>(
 where
 	Client: AuxStore + HeaderBackend<Block> + HeaderMetadata<Block, Error = sp_blockchain::Error>,
 {
-	let epoch_changes = aux_schema::load_epoch_changes::<Block, _>(&*client, &config)?;
+	let epoch_changes =
+		aux_schema::load_epoch_changes::<Block, _>(&*client, &config.genesis_config)?;
 	let link = BabeLink { epoch_changes: epoch_changes.clone(), config: config.clone() };
 
 	// NOTE: this isn't entirely necessary, but since we didn't use to prune the

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

@@ -143,7 +143,7 @@ impl DummyProposer {
 				&self.parent_hash,
 				self.parent_number,
 				this_slot,
-				|slot| Epoch::genesis(&self.factory.config, slot),
+				|slot| Epoch::genesis(self.factory.config.genesis_config(), slot),
 			)
 			.expect("client has data to find epoch")
 			.expect("can compute epoch for baked block");
@@ -336,9 +336,9 @@ impl TestNetFactory for BabeTestNet {
 				select_chain: longest_chain,
 				create_inherent_data_providers: Box::new(|_, _| async {
 					let timestamp = TimestampInherentDataProvider::from_system_time();
-					let slot = InherentDataProvider::from_timestamp_and_duration(
+					let slot = InherentDataProvider::from_timestamp_and_slot_duration(
 						*timestamp,
-						Duration::from_secs(6),
+						SlotDuration::from_millis(6000),
 					);
 
 					Ok((timestamp, slot))
@@ -449,9 +449,9 @@ fn run_one_test(mutator: impl Fn(&mut TestHeader, Stage) + Send + Sync + 'static
 				sync_oracle: DummyOracle,
 				create_inherent_data_providers: Box::new(|_, _| async {
 					let timestamp = TimestampInherentDataProvider::from_system_time();
-					let slot = InherentDataProvider::from_timestamp_and_duration(
+					let slot = InherentDataProvider::from_timestamp_and_slot_duration(
 						*timestamp,
-						Duration::from_secs(6),
+						SlotDuration::from_millis(6000),
 					);
 
 					Ok((timestamp, slot))
@@ -699,12 +699,12 @@ fn importing_block_one_sets_genesis_epoch() {
 		&mut block_import,
 	);
 
-	let genesis_epoch = Epoch::genesis(&data.link.config, 999.into());
+	let genesis_epoch = Epoch::genesis(data.link.config.genesis_config(), 999.into());
 
 	let epoch_changes = data.link.epoch_changes.shared_data();
 	let epoch_for_second_block = epoch_changes
 		.epoch_data_for_child_of(descendent_query(&*client), &block_hash, 1, 1000.into(), |slot| {
-			Epoch::genesis(&data.link.config, slot)
+			Epoch::genesis(data.link.config.genesis_config(), slot)
 		})
 		.unwrap()
 		.unwrap();