From 7599e0d6e808201ccb1c6b174cc970666a966403 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 9 Mar 2021 20:14:54 +0100 Subject: [PATCH] Introduce new concept of "slot portion for proposing" (#8280) * Introduce new concept of "slot portion for proposing" Currently when building a block we actually give the proposer all of the time in the slot, while this is wrong. The slot is actually split in at least two phases proposing and propagation or in the polkadot case into three phases validating pov's, proposing and propagation. As we don't want to bring that much polkadot concepts into Substrate, we only support splitting the slot into proposing and propagation. The portion can now be passed as parameter to AuRa and BABE to configure this value. However, this slot portion for propagation doesn't mean that the proposer can not go over this limit. When we miss slots we still apply the lenience factor to increase the proposing time, so that we have enough time to build a heavy block. Besides all what was said above, this is especially required for parachains. Parachains have a much more constraint proposing window. Currently the slot duration is at minimum 12 seconds, but we only have around 500ms for proposing. So, this slot portion for proposing is really required to make it working without hacks. * Offgit feedback * Cast cast cast --- .../bin/node-template/node/src/service.rs | 3 +- substrate/bin/node/cli/src/service.rs | 5 +- substrate/client/consensus/aura/src/lib.rs | 36 +++++++-- substrate/client/consensus/babe/src/lib.rs | 29 +++++-- substrate/client/consensus/babe/src/tests.rs | 1 + substrate/client/consensus/slots/src/lib.rs | 81 ++++++++++--------- substrate/client/consensus/slots/src/slots.rs | 14 ++-- 7 files changed, 112 insertions(+), 57 deletions(-) diff --git a/substrate/bin/node-template/node/src/service.rs b/substrate/bin/node-template/node/src/service.rs index a5030f1b35..368767fbdd 100644 --- a/substrate/bin/node-template/node/src/service.rs +++ b/substrate/bin/node-template/node/src/service.rs @@ -9,7 +9,7 @@ use sp_inherents::InherentDataProviders; use sc_executor::native_executor_instance; pub use sc_executor::NativeExecutor; use sp_consensus_aura::sr25519::AuthorityPair as AuraPair; -use sc_consensus_aura::{ImportQueueParams, StartAuraParams}; +use sc_consensus_aura::{ImportQueueParams, StartAuraParams, SlotProportion}; use sc_finality_grandpa::SharedVoterState; use sc_keystore::LocalKeystore; use sc_telemetry::TelemetrySpan; @@ -212,6 +212,7 @@ pub fn new_full(mut config: Configuration) -> Result keystore: keystore_container.sync_keystore(), can_author_with, sync_oracle: network.clone(), + block_proposal_slot_portion: SlotProportion::new(2f32 / 3f32), }, )?; diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 80561a78a0..6fed5bf5c6 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -25,8 +25,7 @@ use sc_consensus_babe; use node_primitives::Block; use node_runtime::RuntimeApi; use sc_service::{ - config::{Configuration}, error::{Error as ServiceError}, - RpcHandlers, TaskManager, + config::Configuration, error::Error as ServiceError, RpcHandlers, TaskManager, }; use sp_inherents::InherentDataProviders; use sc_network::{Event, NetworkService}; @@ -35,6 +34,7 @@ use futures::prelude::*; use sc_client_api::{ExecutorProvider, RemoteBackend}; use node_executor::Executor; use sc_telemetry::{TelemetryConnectionNotifier, TelemetrySpan}; +use sc_consensus_babe::SlotProportion; type FullClient = sc_service::TFullClient; type FullBackend = sc_service::TFullBackend; @@ -279,6 +279,7 @@ pub fn new_full_base( backoff_authoring_blocks, babe_link, can_author_with, + block_proposal_slot_portion: SlotProportion::new(0.5), }; let babe = sc_consensus_babe::start_babe(babe_config)?; diff --git a/substrate/client/consensus/aura/src/lib.rs b/substrate/client/consensus/aura/src/lib.rs index 1c30f136ea..12ce0e1697 100644 --- a/substrate/client/consensus/aura/src/lib.rs +++ b/substrate/client/consensus/aura/src/lib.rs @@ -69,6 +69,7 @@ pub use sp_consensus_aura::{ }; pub use sp_consensus::SyncOracle; pub use import_queue::{ImportQueueParams, import_queue, AuraBlockImport, CheckForEquivocation}; +pub use sc_consensus_slots::SlotProportion; type AuthorityId

=

::Public; @@ -142,6 +143,12 @@ pub struct StartAuraParams { pub keystore: SyncCryptoStorePtr, /// Can we author a block with this node? pub can_author_with: CAW, + /// The proportion of the slot dedicated to proposing. + /// + /// The block proposing will be limited to this proportion of the slot from the starting of the + /// slot. However, the proposing can still take longer when there is some lenience factor applied, + /// because there were no blocks produced for some slots. + pub block_proposal_slot_portion: SlotProportion, } /// Start the aura worker. The returned future should be run in a futures executor. @@ -158,6 +165,7 @@ pub fn start_aura( backoff_authoring_blocks, keystore, can_author_with, + block_proposal_slot_portion, }: StartAuraParams, ) -> Result, sp_consensus::Error> where B: BlockT, @@ -184,6 +192,7 @@ pub fn start_aura( force_authoring, backoff_authoring_blocks, _key_type: PhantomData::

, + block_proposal_slot_portion, }; register_aura_inherent_data_provider( &inherent_data_providers, @@ -208,6 +217,7 @@ struct AuraWorker { sync_oracle: SO, force_authoring: bool, backoff_authoring_blocks: Option, + block_proposal_slot_portion: SlotProportion, _key_type: PhantomData

, } @@ -365,11 +375,22 @@ where &self, head: &B::Header, slot_info: &SlotInfo, - ) -> Option { - let slot_remaining = self.slot_remaining_duration(slot_info); + ) -> std::time::Duration { + let max_proposing = slot_info.duration.mul_f32(self.block_proposal_slot_portion.get()); + + let slot_remaining = slot_info.ends_at + .checked_duration_since(std::time::Instant::now()) + .unwrap_or_default(); + + let slot_remaining = std::cmp::min(slot_remaining, max_proposing); + + // If parent is genesis block, we don't require any lenience factor. + if head.number().is_zero() { + return slot_remaining + } let parent_slot = match find_pre_digest::(head) { - Err(_) => return Some(slot_remaining), + Err(_) => return slot_remaining, Ok(d) => d, }; @@ -383,9 +404,9 @@ where slot_lenience.as_secs(), ); - Some(slot_remaining + slot_lenience) + slot_remaining + slot_lenience } else { - Some(slot_remaining) + slot_remaining } } } @@ -648,6 +669,7 @@ mod tests { backoff_authoring_blocks: Some(BackoffAuthoringOnFinalizedHeadLagging::default()), keystore, can_author_with: sp_consensus::AlwaysCanAuthor, + block_proposal_slot_portion: SlotProportion::new(0.5), }).expect("Starts aura")); } @@ -708,6 +730,7 @@ mod tests { force_authoring: false, backoff_authoring_blocks: Some(BackoffAuthoringOnFinalizedHeadLagging::default()), _key_type: PhantomData::, + block_proposal_slot_portion: SlotProportion::new(0.5), }; let head = Header::new( @@ -755,6 +778,7 @@ mod tests { force_authoring: false, backoff_authoring_blocks: Option::<()>::None, _key_type: PhantomData::, + block_proposal_slot_portion: SlotProportion::new(0.5), }; let head = client.header(&BlockId::Number(0)).unwrap().unwrap(); @@ -766,7 +790,7 @@ mod tests { timestamp: 0, ends_at: Instant::now() + Duration::from_secs(100), inherent_data: InherentData::new(), - duration: 1000, + duration: Duration::from_millis(1000), }, )).unwrap(); diff --git a/substrate/client/consensus/babe/src/lib.rs b/substrate/client/consensus/babe/src/lib.rs index 5622df48db..7f2f47da5d 100644 --- a/substrate/client/consensus/babe/src/lib.rs +++ b/substrate/client/consensus/babe/src/lib.rs @@ -74,6 +74,7 @@ pub use sp_consensus_babe::{ }, }; pub use sp_consensus::SyncOracle; +pub use sc_consensus_slots::SlotProportion; use std::{ collections::HashMap, sync::Arc, u64, pin::Pin, time::{Instant, Duration}, any::Any, borrow::Cow, convert::TryInto, @@ -394,6 +395,13 @@ pub struct BabeParams { /// Checks if the current native implementation can author with a runtime at a given block. pub can_author_with: CAW, + + /// The proportion of the slot dedicated to proposing. + /// + /// The block proposing will be limited to this proportion of the slot from the starting of the + /// slot. However, the proposing can still take longer when there is some lenience factor applied, + /// because there were no blocks produced for some slots. + pub block_proposal_slot_portion: SlotProportion, } /// Start the babe worker. @@ -409,6 +417,7 @@ pub fn start_babe(BabeParams { backoff_authoring_blocks, babe_link, can_author_with, + block_proposal_slot_portion, }: BabeParams) -> Result< BabeWorker, sp_consensus::Error, @@ -443,6 +452,7 @@ pub fn start_babe(BabeParams { epoch_changes: babe_link.epoch_changes.clone(), slot_notification_sinks: slot_notification_sinks.clone(), config: config.clone(), + block_proposal_slot_portion, }; register_babe_inherent_data_provider(&inherent_data_providers, config.slot_duration())?; @@ -597,6 +607,7 @@ struct BabeSlotWorker { epoch_changes: SharedEpochChanges, slot_notification_sinks: SlotNotificationSinks, config: Config, + block_proposal_slot_portion: SlotProportion, } impl sc_consensus_slots::SimpleSlotWorker @@ -791,16 +802,22 @@ where &self, parent_head: &B::Header, slot_info: &SlotInfo, - ) -> Option { - let slot_remaining = self.slot_remaining_duration(slot_info); + ) -> std::time::Duration { + let max_proposing = slot_info.duration.mul_f32(self.block_proposal_slot_portion.get()); + + let slot_remaining = slot_info.ends_at + .checked_duration_since(Instant::now()) + .unwrap_or_default(); + + let slot_remaining = std::cmp::min(slot_remaining, max_proposing); // If parent is genesis block, we don't require any lenience factor. if parent_head.number().is_zero() { - return Some(slot_remaining) + return slot_remaining } let parent_slot = match find_pre_digest::(parent_head) { - Err(_) => return Some(slot_remaining), + Err(_) => return slot_remaining, Ok(d) => d.slot(), }; @@ -814,9 +831,9 @@ where slot_lenience.as_secs(), ); - Some(slot_remaining + slot_lenience) + slot_remaining + slot_lenience } else { - Some(slot_remaining) + slot_remaining } } } diff --git a/substrate/client/consensus/babe/src/tests.rs b/substrate/client/consensus/babe/src/tests.rs index a33a509ddc..9ffffc37fd 100644 --- a/substrate/client/consensus/babe/src/tests.rs +++ b/substrate/client/consensus/babe/src/tests.rs @@ -431,6 +431,7 @@ fn run_one_test( babe_link: data.link.clone(), keystore, can_author_with: sp_consensus::AlwaysCanAuthor, + block_proposal_slot_portion: SlotProportion::new(0.5), }).expect("Starts babe")); } futures::executor::block_on(future::select( diff --git a/substrate/client/consensus/slots/src/lib.rs b/substrate/client/consensus/slots/src/lib.rs index 564d5c28c5..1b40ac102d 100644 --- a/substrate/client/consensus/slots/src/lib.rs +++ b/substrate/client/consensus/slots/src/lib.rs @@ -32,7 +32,7 @@ pub use slots::SlotInfo; use slots::Slots; pub use aux_schema::{check_equivocation, MAX_SLOT_CAPACITY, PRUNING_BOUND}; -use std::{fmt::Debug, ops::Deref, pin::Pin, sync::Arc, time::{Instant, Duration}}; +use std::{fmt::Debug, ops::Deref, pin::Pin, sync::Arc, time::Duration}; use codec::{Decode, Encode}; use futures::{prelude::*, future::{self, Either}}; use futures_timer::Delay; @@ -180,24 +180,12 @@ pub trait SimpleSlotWorker { /// Returns a `Proposer` to author on top of the given block. fn proposer(&mut self, block: &B::Header) -> Self::CreateProposer; - /// Remaining duration of the slot. - fn slot_remaining_duration(&self, slot_info: &SlotInfo) -> Duration { - let now = Instant::now(); - if now < slot_info.ends_at { - slot_info.ends_at.duration_since(now) - } else { - Duration::from_millis(0) - } - } - - /// Remaining duration for proposing. None means unlimited. + /// Remaining duration for proposing. fn proposing_remaining_duration( &self, - _head: &B::Header, + head: &B::Header, slot_info: &SlotInfo, - ) -> Option { - Some(self.slot_remaining_duration(slot_info)) - } + ) -> Duration; /// Implements [`SlotWorker::on_slot`]. fn on_slot( @@ -210,21 +198,19 @@ pub trait SimpleSlotWorker { { let (timestamp, slot) = (slot_info.timestamp, slot_info.slot); - let slot_remaining_duration = self.slot_remaining_duration(&slot_info); let proposing_remaining_duration = self.proposing_remaining_duration(&chain_head, &slot_info); - let proposing_remaining = match proposing_remaining_duration { - Some(r) if r.as_secs() == 0 && r.as_nanos() == 0 => { - debug!( - target: self.logging_target(), - "Skipping proposal slot {} since there's no time left to propose", - slot, - ); + let proposing_remaining = if proposing_remaining_duration == Duration::default() { + debug!( + target: self.logging_target(), + "Skipping proposal slot {} since there's no time left to propose", + slot, + ); - return Box::pin(future::ready(None)); - }, - Some(r) => Box::new(Delay::new(r)) as Box + Unpin + Send>, - None => Box::new(future::pending()) as Box<_>, + return Box::pin(future::ready(None)); + } else { + Box::new(Delay::new(proposing_remaining_duration)) + as Box + Unpin + Send> }; let epoch_data = match self.epoch_data(&chain_head, slot) { @@ -298,20 +284,25 @@ pub trait SimpleSlotWorker { let logs = self.pre_digest_data(slot, &claim); - // deadline our production to approx. the end of the slot + // deadline our production to 98% of the total time left for proposing. As we deadline + // the proposing below to the same total time left, the 2% margin should be enough for + // the result to be returned. let proposing = awaiting_proposer.and_then(move |proposer| proposer.propose( slot_info.inherent_data, sp_runtime::generic::Digest { logs, }, - slot_remaining_duration, + proposing_remaining_duration.mul_f32(0.98), ).map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e)))); let proposal_work = futures::future::select(proposing, proposing_remaining).map(move |v| match v { Either::Left((b, _)) => b.map(|b| (b, claim)), Either::Right(_) => { - info!("⌛️ Discarding proposal for slot {}; block production took too long", slot); + info!( + "⌛️ Discarding proposal for slot {}; block production took too long", + slot, + ); // If the node was compiled with debug, tell the user to use release optimizations. #[cfg(build_type="debug")] info!("👉 Recompile your node in `--release` mode to mitigate this problem."); @@ -381,8 +372,7 @@ pub trait SimpleSlotWorker { } } -impl> SlotWorker>::Proof> for T -{ +impl> SlotWorker>::Proof> for T { fn on_slot( &mut self, chain_head: B::Header, @@ -564,6 +554,24 @@ impl SlotDuration { } } +/// A unit type wrapper to express the proportion of a slot. +pub struct SlotProportion(f32); + +impl SlotProportion { + /// Create a new proportion. + /// + /// The given value `inner` should be in the range `[0,1]`. If the value is not in the required + /// range, it is clamped into the range. + pub fn new(inner: f32) -> Self { + Self(inner.clamp(0.0, 1.0)) + } + + /// Returns the inner that is guaranted to be in the range `[0,1]`. + pub fn get(&self) -> f32 { + self.0 + } +} + /// Calculate a slot duration lenience based on the number of missed slots from current /// to parent. If the number of skipped slots is greated than 0 this method will apply /// an exponential backoff of at most `2^7 * slot_duration`, if no slots were skipped @@ -589,7 +597,7 @@ pub fn slot_lenience_exponential(parent_slot: Slot, slot_info: &SlotInfo) -> Opt let slot_lenience = skipped_slots / BACKOFF_STEP; let slot_lenience = std::cmp::min(slot_lenience, BACKOFF_CAP); let slot_lenience = 1 << slot_lenience; - Some(Duration::from_millis(slot_lenience * slot_info.duration)) + Some(slot_lenience * slot_info.duration) } } @@ -613,7 +621,8 @@ pub fn slot_lenience_linear(parent_slot: Slot, slot_info: &SlotInfo) -> Option super::slots::SlotInfo { super::slots::SlotInfo { slot: slot.into(), - duration: SLOT_DURATION.as_millis() as u64, + duration: SLOT_DURATION, timestamp: Default::default(), inherent_data: Default::default(), ends_at: Instant::now(), diff --git a/substrate/client/consensus/slots/src/slots.rs b/substrate/client/consensus/slots/src/slots.rs index d3bddccce0..b23d676035 100644 --- a/substrate/client/consensus/slots/src/slots.rs +++ b/substrate/client/consensus/slots/src/slots.rs @@ -40,9 +40,11 @@ pub fn duration_now() -> Duration { } /// Returns the duration until the next slot, based on current duration since -pub fn time_until_next(now: Duration, slot_duration: u64) -> Duration { - let remaining_full_millis = slot_duration - (now.as_millis() as u64 % slot_duration) - 1; - Duration::from_millis(remaining_full_millis) +pub fn time_until_next(now: Duration, slot_duration: Duration) -> Duration { + let remaining_full_millis = slot_duration.as_millis() + - (now.as_millis() % slot_duration.as_millis()) + - 1; + Duration::from_millis(remaining_full_millis as u64) } /// Information about a slot. @@ -56,13 +58,13 @@ pub struct SlotInfo { /// The inherent data. pub inherent_data: InherentData, /// Slot duration. - pub duration: u64, + pub duration: Duration, } /// A stream that returns every time there is a new slot. pub(crate) struct Slots { last_slot: Slot, - slot_duration: u64, + slot_duration: Duration, inner_delay: Option, inherent_data_providers: InherentDataProviders, timestamp_extractor: SC, @@ -77,7 +79,7 @@ impl Slots { ) -> Self { Slots { last_slot: 0.into(), - slot_duration, + slot_duration: Duration::from_millis(slot_duration), inner_delay: None, inherent_data_providers, timestamp_extractor,