SlotDuration: Always fetch the slot duration from the runtime

bin/node/cli/src/service.rs

@@ -196,7 +196,7 @@ pub fn new_partial(
 	let justification_import = grandpa_block_import.clone();
 
 	let (block_import, babe_link) = sc_consensus_babe::block_import(
-		sc_consensus_babe::Config::get_or_compute(&*client)?,
+		sc_consensus_babe::Config::get(&*client)?,
 		grandpa_block_import,
 		client.clone(),
 	)?;

client/consensus/aura/src/lib.rs

@@ -85,15 +85,18 @@ type AuthorityId<P> = <P as Pair>::Public;
 /// Slot duration type for Aura.
 pub type SlotDuration = sc_consensus_slots::SlotDuration<sp_consensus_aura::SlotDuration>;
 
-/// Get type of `SlotDuration` for Aura.
+/// Get the slot duration for Aura.
 pub fn slot_duration<A, B, C>(client: &C) -> CResult<SlotDuration>
 where
 	A: Codec,
 	B: BlockT,
 	C: AuxStore + ProvideRuntimeApi<B> + UsageProvider<B>,
 	C::Api: AuraApi<B, A>,
 {
-	SlotDuration::get_or_compute(client, |a, b| a.slot_duration(b).map_err(Into::into))
+	let best_block_id = BlockId::Hash(client.usage_info().chain.best_hash);
+	let slot_duration = client.runtime_api().slot_duration(&best_block_id)?;
+
+	Ok(SlotDuration::new(slot_duration))
 }
 
 /// Get slot author for given block along with authorities.

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

@@ -247,7 +247,7 @@ mod tests {
 		let builder = TestClientBuilder::new();
 		let (client, longest_chain) = builder.build_with_longest_chain();
 		let client = Arc::new(client);
-		let config = Config::get_or_compute(&*client).expect("config available");
+		let config = Config::get(&*client).expect("config available");
 		let (_, link) = block_import(config.clone(), client.clone(), client.clone())
 			.expect("can initialize block-import");
 

client/consensus/babe/src/lib.rs

@@ -329,47 +329,43 @@ pub struct BabeIntermediate<B: BlockT> {
 /// Intermediate key for Babe engine.
 pub static INTERMEDIATE_KEY: &[u8] = b"babe1";
 
-/// A slot duration. Create with `get_or_compute`.
+/// 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
 #[derive(Clone)]
 pub struct Config(sc_consensus_slots::SlotDuration<BabeGenesisConfiguration>);
 
 impl Config {
-	/// Either fetch the slot duration from disk or compute it from the genesis
-	/// state.
-	pub fn get_or_compute<B: BlockT, C>(client: &C) -> ClientResult<Self>
+	/// Fetch the config from the runtime.
+	pub fn get<B: BlockT, C>(client: &C) -> ClientResult<Self>
 	where
 		C: AuxStore + ProvideRuntimeApi<B> + UsageProvider<B>,
 		C::Api: BabeApi<B>,
 	{
 		trace!(target: "babe", "Getting slot duration");
-		match sc_consensus_slots::SlotDuration::get_or_compute(client, |a, b| {
-			let has_api_v1 = a.has_api_with::<dyn BabeApi<B>, _>(&b, |v| v == 1)?;
-			let has_api_v2 = a.has_api_with::<dyn BabeApi<B>, _>(&b, |v| v == 2)?;
 
-			if has_api_v1 {
-				#[allow(deprecated)]
-				{
-					Ok(a.configuration_before_version_2(b)?.into())
-				}
-			} else if has_api_v2 {
-				a.configuration(b).map_err(Into::into)
-			} else {
-				Err(sp_blockchain::Error::VersionInvalid(
-					"Unsupported or invalid BabeApi version".to_string(),
-				))
+		let best_block_id = BlockId::Hash(client.usage_info().chain.best_hash);
+		let runtime_api = client.runtime_api();
+
+		let version = runtime_api.api_version::<dyn BabeApi<B>>(&best_block_id)?;
+
+		let slot_duration = if version == Some(1) {
+			#[allow(deprecated)]
+			{
+				runtime_api.configuration_before_version_2(&best_block_id)?.into()
 			}
-		})
-		.map(Self)
-		{
-			Ok(s) => Ok(s),
-			Err(s) => {
-				warn!(target: "babe", "Failed to get slot duration");
-				Err(s)
-			},
-		}
+		} else if version == Some(2) {
+			runtime_api.configuration(&best_block_id)?
+		} else {
+			return Err(sp_blockchain::Error::VersionInvalid(
+				"Unsupported or invalid BabeApi version".to_string(),
+			))
+		};
+
+		Ok(Self(sc_consensus_slots::SlotDuration::new(slot_duration)))
 	}
 
 	/// Get the inner slot duration

client/consensus/babe/src/tests.rs

@@ -297,7 +297,7 @@ impl TestNetFactory for BabeTestNet {
 	) {
 		let client = client.as_client();
 
-		let config = Config::get_or_compute(&*client).expect("config available");
+		let config = Config::get(&*client).expect("config available");
 		let (block_import, link) = crate::block_import(config, client.clone(), client.clone())
 			.expect("can initialize block-import");
 

client/consensus/manual-seal/src/consensus/babe.rs

@@ -154,7 +154,7 @@ where
 			return Err(Error::StringError("Cannot supply empty authority set!".into()))
 		}
 
-		let config = Config::get_or_compute(&*client)?;
+		let config = Config::get(&*client)?;
 
 		Ok(Self { config, client, keystore, epoch_changes, authorities })
 	}
@@ -327,7 +327,7 @@ impl SlotTimestampProvider {
 		C: AuxStore + HeaderBackend<B> + ProvideRuntimeApi<B> + UsageProvider<B>,
 		C::Api: BabeApi<B>,
 	{
-		let slot_duration = Config::get_or_compute(&*client)?.slot_duration;
+		let slot_duration = Config::get(&*client)?.slot_duration;
 		let info = client.info();
 
 		// looks like this isn't the first block, rehydrate the fake time.

client/consensus/slots/Cargo.toml

@@ -23,7 +23,6 @@ sc-consensus = { version = "0.10.0-dev", path = "../../../client/consensus/commo
 sp-consensus-slots = { version = "0.10.0-dev", path = "../../../primitives/consensus/slots" }
 sp-runtime = { version = "4.0.0", path = "../../../primitives/runtime" }
 sp-state-machine = { version = "0.10.0", path = "../../../primitives/state-machine" }
-sp-api = { version = "4.0.0-dev", path = "../../../primitives/api" }
 sc-telemetry = { version = "4.0.0-dev", path = "../../telemetry" }
 sp-consensus = { version = "0.10.0-dev", path = "../../../primitives/consensus/common" }
 sp-inherents = { version = "4.0.0-dev", path = "../../../primitives/inherents" }

client/consensus/slots/src/lib.rs

@@ -38,7 +38,6 @@ use futures_timer::Delay;
 use log::{debug, error, info, warn};
 use sc_consensus::{BlockImport, JustificationSyncLink};
 use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_DEBUG, CONSENSUS_INFO, CONSENSUS_WARN};
-use sp_api::{ApiRef, ProvideRuntimeApi};
 use sp_arithmetic::traits::BaseArithmetic;
 use sp_consensus::{CanAuthorWith, Proposer, SelectChain, SlotData, SyncOracle};
 use sp_consensus_slots::Slot;
@@ -537,9 +536,7 @@ where
 	SlotDurationInvalid(SlotDuration<T>),
 }
 
-/// A slot duration. Create with [`get_or_compute`](Self::get_or_compute).
-// The internal member should stay private here to maintain invariants of
-// `get_or_compute`.
+/// A slot duration. Create with [`Self::new`].
 #[derive(Clone, Copy, Debug, Encode, Decode, Hash, PartialOrd, Ord, PartialEq, Eq)]
 pub struct SlotDuration<T>(T);
 
@@ -554,54 +551,12 @@ impl<T: SlotData> SlotData for SlotDuration<T> {
 	fn slot_duration(&self) -> std::time::Duration {
 		self.0.slot_duration()
 	}
-
-	const SLOT_KEY: &'static [u8] = T::SLOT_KEY;
 }
 
 impl<T: Clone + Send + Sync + 'static> SlotDuration<T> {
-	/// Either fetch the slot duration from disk or compute it from the
-	/// genesis state.
-	///
-	/// `slot_key` is marked as `'static`, as it should really be a
-	/// compile-time constant.
-	pub fn get_or_compute<B: BlockT, C, CB>(client: &C, cb: CB) -> sp_blockchain::Result<Self>
-	where
-		C: sc_client_api::backend::AuxStore + sc_client_api::UsageProvider<B>,
-		C: ProvideRuntimeApi<B>,
-		CB: FnOnce(ApiRef<C::Api>, &BlockId<B>) -> sp_blockchain::Result<T>,
-		T: SlotData + Encode + Decode + Debug,
-	{
-		let slot_duration = match client.get_aux(T::SLOT_KEY)? {
-			Some(v) => <T as codec::Decode>::decode(&mut &v[..]).map(SlotDuration).map_err(|_| {
-				sp_blockchain::Error::Backend({
-					error!(target: "slots", "slot duration kept in invalid format");
-					"slot duration kept in invalid format".to_string()
-				})
-			}),
-			None => {
-				let best_hash = client.usage_info().chain.best_hash;
-				let slot_duration = cb(client.runtime_api(), &BlockId::hash(best_hash))?;
-
-				info!(
-					"⏱  Loaded block-time = {:?} from block {:?}",
-					slot_duration.slot_duration(),
-					best_hash,
-				);
-
-				slot_duration
-					.using_encoded(|s| client.insert_aux(&[(T::SLOT_KEY, &s[..])], &[]))?;
-
-				Ok(SlotDuration(slot_duration))
-			},
-		}?;
-
-		if slot_duration.slot_duration() == Default::default() {
-			return Err(sp_blockchain::Error::Application(Box::new(Error::SlotDurationInvalid(
-				slot_duration,
-			))))
-		}
-
-		Ok(slot_duration)
+	/// Create a new instance of `Self`.
+	pub fn new(val: T) -> Self {
+		Self(val)
 	}
 
 	/// Returns slot data value.
@@ -875,7 +830,7 @@ impl<N> BackoffAuthoringBlocksStrategy<N> for () {
 #[cfg(test)]
 mod test {
 	use super::*;
-	use sp_api::NumberFor;
+	use sp_runtime::traits::NumberFor;
 	use std::time::{Duration, Instant};
 	use substrate_test_runtime_client::runtime::{Block, Header};
 

primitives/consensus/aura/src/lib.rs

@@ -117,6 +117,4 @@ impl sp_consensus::SlotData for SlotDuration {
 	fn slot_duration(&self) -> std::time::Duration {
 		std::time::Duration::from_millis(self.0)
 	}
-
-	const SLOT_KEY: &'static [u8] = b"aura_slot_duration";
 }

primitives/consensus/babe/src/lib.rs

@@ -242,8 +242,6 @@ impl sp_consensus::SlotData for BabeGenesisConfiguration {
 	fn slot_duration(&self) -> std::time::Duration {
 		std::time::Duration::from_millis(self.slot_duration)
 	}
-
-	const SLOT_KEY: &'static [u8] = b"babe_configuration";
 }
 
 /// Configuration data used by the BABE consensus engine.

primitives/consensus/common/src/lib.rs

@@ -332,7 +332,4 @@ impl<Block: BlockT> CanAuthorWith<Block> for NeverCanAuthor {
 pub trait SlotData {
 	/// Gets the slot duration.
 	fn slot_duration(&self) -> sp_std::time::Duration;
-
-	/// The static slot key
-	const SLOT_KEY: &'static [u8];
 }

test-utils/test-runner/src/client.rs

@@ -127,7 +127,7 @@ where
 		None,
 	)?;
 
-	let slot_duration = sc_consensus_babe::Config::get_or_compute(&*client)?;
+	let slot_duration = sc_consensus_babe::Config::get(&*client)?;
 	let (block_import, babe_link) = sc_consensus_babe::block_import(
 		slot_duration.clone(),
 		grandpa_block_import,

test-utils/test-runner/src/lib.rs

@@ -154,7 +154,7 @@
 //! 			sc_finality_grandpa::block_import(client.clone(), &(client.clone() as Arc<_>), select_chain.clone())?;
 //!
 //! 		let (block_import, babe_link) = sc_consensus_babe::block_import(
-//! 			sc_consensus_babe::Config::get_or_compute(&*client)?,
+//! 			sc_consensus_babe::Config::get(&*client)?,
 //! 			grandpa_block_import,
 //! 			client.clone(),
 //! 		)?;