From 494dee7c7b93a9d9e17ce75d25d50607d317e52d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 20 Dec 2021 20:49:18 +0100 Subject: [PATCH] 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 | 1 - substrate/bin/node/cli/src/service.rs | 2 +- substrate/client/consensus/aura/src/lib.rs | 7 ++- .../client/consensus/babe/rpc/src/lib.rs | 2 +- substrate/client/consensus/babe/src/lib.rs | 50 ++++++++--------- substrate/client/consensus/babe/src/tests.rs | 2 +- .../manual-seal/src/consensus/babe.rs | 4 +- substrate/client/consensus/slots/Cargo.toml | 1 - substrate/client/consensus/slots/src/lib.rs | 55 ++----------------- .../primitives/consensus/aura/src/lib.rs | 2 - .../primitives/consensus/babe/src/lib.rs | 2 - .../primitives/consensus/common/src/lib.rs | 3 - .../test-utils/test-runner/src/client.rs | 2 +- substrate/test-utils/test-runner/src/lib.rs | 2 +- 14 files changed, 40 insertions(+), 95 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 4af488ca98..f4ab495c24 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -8014,7 +8014,6 @@ dependencies = [ "sc-client-api", "sc-consensus", "sc-telemetry", - "sp-api", "sp-arithmetic", "sp-blockchain", "sp-consensus", diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index fbc91c5f7d..fabf8d0adf 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/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(), )?; diff --git a/substrate/client/consensus/aura/src/lib.rs b/substrate/client/consensus/aura/src/lib.rs index ec577b0844..4579b2d73d 100644 --- a/substrate/client/consensus/aura/src/lib.rs +++ b/substrate/client/consensus/aura/src/lib.rs @@ -85,7 +85,7 @@ type AuthorityId

=

::Public; /// Slot duration type for Aura. pub type SlotDuration = sc_consensus_slots::SlotDuration; -/// Get type of `SlotDuration` for Aura. +/// Get the slot duration for Aura. pub fn slot_duration(client: &C) -> CResult where A: Codec, @@ -93,7 +93,10 @@ where C: AuxStore + ProvideRuntimeApi + UsageProvider, C::Api: AuraApi, { - 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. diff --git a/substrate/client/consensus/babe/rpc/src/lib.rs b/substrate/client/consensus/babe/rpc/src/lib.rs index eeec7b86b1..463b05ef9c 100644 --- a/substrate/client/consensus/babe/rpc/src/lib.rs +++ b/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"); diff --git a/substrate/client/consensus/babe/src/lib.rs b/substrate/client/consensus/babe/src/lib.rs index 21c3c883a3..7a05c7a926 100644 --- a/substrate/client/consensus/babe/src/lib.rs +++ b/substrate/client/consensus/babe/src/lib.rs @@ -329,7 +329,9 @@ pub struct BabeIntermediate { /// 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); impl Config { - /// Either fetch the slot duration from disk or compute it from the genesis - /// state. - pub fn get_or_compute(client: &C) -> ClientResult + /// Fetch the config from the runtime. + pub fn get(client: &C) -> ClientResult where C: AuxStore + ProvideRuntimeApi + UsageProvider, C::Api: BabeApi, { 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::, _>(&b, |v| v == 1)?; - let has_api_v2 = a.has_api_with::, _>(&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::>(&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 diff --git a/substrate/client/consensus/babe/src/tests.rs b/substrate/client/consensus/babe/src/tests.rs index 73cc453812..23c34d21ec 100644 --- a/substrate/client/consensus/babe/src/tests.rs +++ b/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"); diff --git a/substrate/client/consensus/manual-seal/src/consensus/babe.rs b/substrate/client/consensus/manual-seal/src/consensus/babe.rs index e06c544aae..499a82c63e 100644 --- a/substrate/client/consensus/manual-seal/src/consensus/babe.rs +++ b/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 + ProvideRuntimeApi + UsageProvider, C::Api: BabeApi, { - 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. diff --git a/substrate/client/consensus/slots/Cargo.toml b/substrate/client/consensus/slots/Cargo.toml index bf2bd59bb9..782e979ed6 100644 --- a/substrate/client/consensus/slots/Cargo.toml +++ b/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" } diff --git a/substrate/client/consensus/slots/src/lib.rs b/substrate/client/consensus/slots/src/lib.rs index 3174eacaff..905165aa9e 100644 --- a/substrate/client/consensus/slots/src/lib.rs +++ b/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), } -/// 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); @@ -554,54 +551,12 @@ impl SlotData for SlotDuration { fn slot_duration(&self) -> std::time::Duration { self.0.slot_duration() } - - const SLOT_KEY: &'static [u8] = T::SLOT_KEY; } impl SlotDuration { - /// 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(client: &C, cb: CB) -> sp_blockchain::Result - where - C: sc_client_api::backend::AuxStore + sc_client_api::UsageProvider, - C: ProvideRuntimeApi, - CB: FnOnce(ApiRef, &BlockId) -> sp_blockchain::Result, - T: SlotData + Encode + Decode + Debug, - { - let slot_duration = match client.get_aux(T::SLOT_KEY)? { - Some(v) => ::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 BackoffAuthoringBlocksStrategy 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}; diff --git a/substrate/primitives/consensus/aura/src/lib.rs b/substrate/primitives/consensus/aura/src/lib.rs index e6a319c1d1..b85443e091 100644 --- a/substrate/primitives/consensus/aura/src/lib.rs +++ b/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"; } diff --git a/substrate/primitives/consensus/babe/src/lib.rs b/substrate/primitives/consensus/babe/src/lib.rs index 560866cfb2..1971563ff1 100644 --- a/substrate/primitives/consensus/babe/src/lib.rs +++ b/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. diff --git a/substrate/primitives/consensus/common/src/lib.rs b/substrate/primitives/consensus/common/src/lib.rs index ce834fd0a4..456ba965f4 100644 --- a/substrate/primitives/consensus/common/src/lib.rs +++ b/substrate/primitives/consensus/common/src/lib.rs @@ -332,7 +332,4 @@ impl CanAuthorWith 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]; } diff --git a/substrate/test-utils/test-runner/src/client.rs b/substrate/test-utils/test-runner/src/client.rs index 21039d3bc4..b4a82b1c89 100644 --- a/substrate/test-utils/test-runner/src/client.rs +++ b/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, diff --git a/substrate/test-utils/test-runner/src/lib.rs b/substrate/test-utils/test-runner/src/lib.rs index 9f51442ed7..d5e2873fe4 100644 --- a/substrate/test-utils/test-runner/src/lib.rs +++ b/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(), //! )?;