SlotDuration: Always fetch the slot duration from the runtime (#10509)

* SlotDuration: Always fetch the slot duration from the runtime

The slot duration should always be fetched from the runtime instead of being cached in the db. The
slot duration is only fetched on startup of the node, so the performance isn't that important. This
is especially helpful for the case when the slot duration of a chain should be changed through a
runtime upgrade (there be dragons, so take care).

* Fix docs

* Remove logging

* Fix warning

substrate/Cargo.lock

@@ -8014,7 +8014,6 @@ dependencies = [
  "sc-client-api",
  "sc-consensus",
  "sc-telemetry",
- "sp-api",
  "sp-arithmetic",
  "sp-blockchain",
  "sp-consensus",

substrate/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(),
 	)?;

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

@@ -85,7 +85,7 @@ 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,
@@ -93,7 +93,10 @@ where
 	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.

substrate/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");
 

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

@@ -329,7 +329,9 @@ 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
@@ -337,39 +339,33 @@ pub static INTERMEDIATE_KEY: &[u8] = b"babe1";
 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

substrate/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");
 

substrate/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.

substrate/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" }

substrate/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};
 

substrate/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";
 }

substrate/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.

substrate/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];
 }

substrate/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,

substrate/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(),
 //! 		)?;