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

@@ -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<TaskManager, ServiceError>
 				keystore: keystore_container.sync_keystore(),
 				can_author_with,
 				sync_oracle: network.clone(),
+				block_proposal_slot_portion: SlotProportion::new(2f32 / 3f32),
 			},
 		)?;
 

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<Block, RuntimeApi, Executor>;
 type FullBackend = sc_service::TFullBackend<Block>;
@@ -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)?;

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<P> = <P as Pair>::Public;
 
@@ -142,6 +143,12 @@ pub struct StartAuraParams<C, SC, I, PF, SO, BS, CAW> {
 	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<P, B, C, SC, PF, I, SO, CAW, BS, Error>(
 		backoff_authoring_blocks,
 		keystore,
 		can_author_with,
+		block_proposal_slot_portion,
 	}: StartAuraParams<C, SC, I, PF, SO, BS, CAW>,
 ) -> Result<impl Future<Output = ()>, sp_consensus::Error> where
 	B: BlockT,
@@ -184,6 +192,7 @@ pub fn start_aura<P, B, C, SC, PF, I, SO, CAW, BS, Error>(
 		force_authoring,
 		backoff_authoring_blocks,
 		_key_type: PhantomData::<P>,
+		block_proposal_slot_portion,
 	};
 	register_aura_inherent_data_provider(
 		&inherent_data_providers,
@@ -208,6 +217,7 @@ struct AuraWorker<C, E, I, P, SO, BS> {
 	sync_oracle: SO,
 	force_authoring: bool,
 	backoff_authoring_blocks: Option<BS>,
+	block_proposal_slot_portion: SlotProportion,
 	_key_type: PhantomData<P>,
 }
 
@@ -365,11 +375,22 @@ where
 		&self,
 		head: &B::Header,
 		slot_info: &SlotInfo,
-	) -> Option<std::time::Duration> {
-		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::<B, P::Signature>(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::<AuthorityPair>,
+			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::<AuthorityPair>,
+			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();
 

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<B: BlockT, C, E, I, SO, SC, CAW, BS> {
 
 	/// 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<B, C, SC, E, I, SO, CAW, BS, Error>(BabeParams {
 	backoff_authoring_blocks,
 	babe_link,
 	can_author_with,
+	block_proposal_slot_portion,
 }: BabeParams<B, C, E, I, SO, SC, CAW, BS>) -> Result<
 	BabeWorker<B>,
 	sp_consensus::Error,
@@ -443,6 +452,7 @@ pub fn start_babe<B, C, SC, E, I, SO, CAW, BS, Error>(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<B: BlockT, C, E, I, SO, BS> {
 	epoch_changes: SharedEpochChanges<B, Epoch>,
 	slot_notification_sinks: SlotNotificationSinks<B>,
 	config: Config,
+	block_proposal_slot_portion: SlotProportion,
 }
 
 impl<B, C, E, I, Error, SO, BS> sc_consensus_slots::SimpleSlotWorker<B>
@@ -791,16 +802,22 @@ where
 		&self,
 		parent_head: &B::Header,
 		slot_info: &SlotInfo,
-	) -> Option<std::time::Duration> {
-		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::<B>(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
 		}
 	}
 }

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(