slots: slot lenience must take into account block proposal portion (#9138)

* aura, babe: proposal slot lenience must take into account proposal portion

* slots: add support for max_block_proposal_slot_portion

* fix compilation

* slots: add tests

* aura: fix comment

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* slots: log the actual proposing duration after lenience is applied

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/bin/node-template/node/src/service.rs

@@ -245,6 +245,7 @@ pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError>
 				sync_oracle: network.clone(),
 				justification_sync_link: network.clone(),
 				block_proposal_slot_portion: SlotProportion::new(2f32 / 3f32),
+				max_block_proposal_slot_portion: None,
 				telemetry: telemetry.as_ref().map(|x| x.handle()),
 			},
 		)?;

substrate/bin/node/cli/src/service.rs

@@ -333,6 +333,7 @@ pub fn new_full_base(
 			babe_link,
 			can_author_with,
 			block_proposal_slot_portion: SlotProportion::new(0.5),
+			max_block_proposal_slot_portion: None,
 			telemetry: telemetry.as_ref().map(|x| x.handle()),
 		};
 

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

@@ -140,6 +140,9 @@ pub struct StartAuraParams<C, SC, I, PF, SO, L, CIDP, BS, CAW> {
 	/// 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,
+	/// The maximum proportion of the slot dedicated to proposing with any lenience factor applied
+	/// due to no blocks being produced.
+	pub max_block_proposal_slot_portion: Option<SlotProportion>,
 	/// Telemetry instance used to report telemetry metrics.
 	pub telemetry: Option<TelemetryHandle>,
 }
@@ -160,9 +163,11 @@ pub fn start_aura<P, B, C, SC, I, PF, SO, L, CIDP, BS, CAW, Error>(
 		keystore,
 		can_author_with,
 		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
 		telemetry,
 	}: StartAuraParams<C, SC, I, PF, SO, L, CIDP, BS, CAW>,
-) -> Result<impl Future<Output = ()>, sp_consensus::Error> where
+) -> Result<impl Future<Output = ()>, sp_consensus::Error>
+where
 	P: Pair + Send + Sync,
 	P::Public: AppPublic + Hash + Member + Encode + Decode,
 	P::Signature: TryFrom<Vec<u8>> + Hash + Member + Encode + Decode,
@@ -192,6 +197,7 @@ pub fn start_aura<P, B, C, SC, I, PF, SO, L, CIDP, BS, CAW, Error>(
 		backoff_authoring_blocks,
 		telemetry,
 		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
 	});
 
 	Ok(sc_consensus_slots::start_slot_worker(
@@ -228,6 +234,9 @@ pub struct BuildAuraWorkerParams<C, I, PF, SO, L, BS> {
 	/// 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,
+	/// The maximum proportion of the slot dedicated to proposing with any lenience factor applied
+	/// due to no blocks being produced.
+	pub max_block_proposal_slot_portion: Option<SlotProportion>,
 	/// Telemetry instance used to report telemetry metrics.
 	pub telemetry: Option<TelemetryHandle>,
 }
@@ -245,10 +254,12 @@ pub fn build_aura_worker<P, B, C, PF, I, SO, L, BS, Error>(
 		backoff_authoring_blocks,
 		keystore,
 		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
 		telemetry,
 		force_authoring,
 	}: BuildAuraWorkerParams<C, I, PF, SO, L, BS>,
-) -> impl sc_consensus_slots::SlotWorker<B, <PF::Proposer as Proposer<B>>::Proof> where
+) -> impl sc_consensus_slots::SlotWorker<B, <PF::Proposer as Proposer<B>>::Proof>
+where
 	B: BlockT,
 	C: ProvideRuntimeApi<B> + BlockOf + ProvideCache<B> + AuxStore + HeaderBackend<B> + Send + Sync,
 	C::Api: AuraApi<B, AuthorityId<P>>,
@@ -274,6 +285,7 @@ pub fn build_aura_worker<P, B, C, PF, I, SO, L, BS, Error>(
 		backoff_authoring_blocks,
 		telemetry,
 		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
 		_key_type: PhantomData::<P>,
 	}
 }
@@ -288,6 +300,7 @@ struct AuraWorker<C, E, I, P, SO, L, BS> {
 	force_authoring: bool,
 	backoff_authoring_blocks: Option<BS>,
 	block_proposal_slot_portion: SlotProportion,
+	max_block_proposal_slot_portion: Option<SlotProportion>,
 	telemetry: Option<TelemetryHandle>,
 	_key_type: PhantomData<P>,
 }
@@ -452,42 +465,17 @@ where
 		self.telemetry.clone()
 	}
 
-	fn proposing_remaining_duration(
-		&self,
-		slot_info: &SlotInfo<B>,
-	) -> std::time::Duration {
-		let max_proposing = slot_info.duration.mul_f32(self.block_proposal_slot_portion.get());
+	fn proposing_remaining_duration(&self, slot_info: &SlotInfo<B>) -> std::time::Duration {
+		let parent_slot = find_pre_digest::<B, P::Signature>(&slot_info.chain_head).ok();
 
-		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 slot_info.chain_head.number().is_zero() {
-			return slot_remaining
-		}
-
-		let parent_slot = match find_pre_digest::<B, P::Signature>(&slot_info.chain_head) {
-			Err(_) => return slot_remaining,
-			Ok(d) => d,
-		};
-
-		if let Some(slot_lenience) =
-			sc_consensus_slots::slot_lenience_exponential(parent_slot, slot_info)
-		{
-			debug!(
-				target: "aura",
-				"No block for {} slots. Applying linear lenience of {}s",
-				slot_info.slot.saturating_sub(parent_slot + 1),
-				slot_lenience.as_secs(),
-			);
-
-			slot_remaining + slot_lenience
-		} else {
-			slot_remaining
-		}
+		sc_consensus_slots::proposing_remaining_duration(
+			parent_slot,
+			slot_info,
+			&self.block_proposal_slot_portion,
+			self.max_block_proposal_slot_portion.as_ref(),
+			sc_consensus_slots::SlotLenienceType::Exponential,
+			self.logging_target(),
+		)
 	}
 }
 
@@ -759,6 +747,7 @@ mod tests {
 				keystore,
 				can_author_with: sp_consensus::AlwaysCanAuthor,
 				block_proposal_slot_portion: SlotProportion::new(0.5),
+				max_block_proposal_slot_portion: None,
 				telemetry: None,
 			}).expect("Starts aura"));
 		}
@@ -823,6 +812,7 @@ mod tests {
 			telemetry: None,
 			_key_type: PhantomData::<AuthorityPair>,
 			block_proposal_slot_portion: SlotProportion::new(0.5),
+			max_block_proposal_slot_portion: None,
 		};
 
 		let head = Header::new(
@@ -873,6 +863,7 @@ mod tests {
 			telemetry: None,
 			_key_type: PhantomData::<AuthorityPair>,
 			block_proposal_slot_portion: SlotProportion::new(0.5),
+			max_block_proposal_slot_portion: None,
 		};
 
 		let head = client.header(&BlockId::Number(0)).unwrap().unwrap();

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

@@ -412,30 +412,35 @@ pub struct BabeParams<B: BlockT, C, SC, E, I, SO, L, CIDP, BS, CAW> {
 	/// because there were no blocks produced for some slots.
 	pub block_proposal_slot_portion: SlotProportion,
 
+	/// The maximum proportion of the slot dedicated to proposing with any lenience factor applied
+	/// due to no blocks being produced.
+	pub max_block_proposal_slot_portion: Option<SlotProportion>,
+
 	/// Handle use to report telemetries.
 	pub telemetry: Option<TelemetryHandle>,
 }
 
 /// Start the babe worker.
-pub fn start_babe<B, C, SC, E, I, SO, CIDP, BS, CAW, L, Error>(BabeParams {
-	keystore,
-	client,
-	select_chain,
-	env,
-	block_import,
-	sync_oracle,
-	justification_sync_link,
-	create_inherent_data_providers,
-	force_authoring,
-	backoff_authoring_blocks,
-	babe_link,
-	can_author_with,
-	block_proposal_slot_portion,
-	telemetry,
-}: BabeParams<B, C, SC, E, I, SO, L, CIDP, BS, CAW>) -> Result<
-	BabeWorker<B>,
-	sp_consensus::Error,
-> where
+pub fn start_babe<B, C, SC, E, I, SO, CIDP, BS, CAW, L, Error>(
+	BabeParams {
+		keystore,
+		client,
+		select_chain,
+		env,
+		block_import,
+		sync_oracle,
+		justification_sync_link,
+		create_inherent_data_providers,
+		force_authoring,
+		backoff_authoring_blocks,
+		babe_link,
+		can_author_with,
+		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
+		telemetry,
+	}: BabeParams<B, C, SC, E, I, SO, L, CIDP, BS, CAW>,
+) -> Result<BabeWorker<B>, sp_consensus::Error>
+where
 	B: BlockT,
 	C: ProvideRuntimeApi<B>
 		+ ProvideCache<B>
@@ -480,6 +485,7 @@ pub fn start_babe<B, C, SC, E, I, SO, CIDP, BS, CAW, L, Error>(BabeParams {
 		slot_notification_sinks: slot_notification_sinks.clone(),
 		config: config.clone(),
 		block_proposal_slot_portion,
+		max_block_proposal_slot_portion,
 		telemetry,
 	};
 
@@ -630,6 +636,7 @@ struct BabeSlotWorker<B: BlockT, C, E, I, SO, L, BS> {
 	slot_notification_sinks: SlotNotificationSinks<B>,
 	config: Config,
 	block_proposal_slot_portion: SlotProportion,
+	max_block_proposal_slot_portion: Option<SlotProportion>,
 	telemetry: Option<TelemetryHandle>,
 }
 
@@ -637,10 +644,10 @@ impl<B, C, E, I, Error, SO, L, BS> sc_consensus_slots::SimpleSlotWorker<B>
 	for BabeSlotWorker<B, C, E, I, SO, L, BS>
 where
 	B: BlockT,
-	C: ProvideRuntimeApi<B> +
-		ProvideCache<B> +
-		HeaderBackend<B> +
-		HeaderMetadata<B, Error = ClientError>,
+	C: ProvideRuntimeApi<B>
+		+ ProvideCache<B>
+		+ HeaderBackend<B>
+		+ HeaderMetadata<B, Error = ClientError>,
 	C::Api: BabeApi<B>,
 	E: Environment<B, Error = Error>,
 	E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
@@ -832,42 +839,17 @@ where
 		self.telemetry.clone()
 	}
 
-	fn proposing_remaining_duration(
-		&self,
-		slot_info: &SlotInfo<B>,
-	) -> std::time::Duration {
-		let max_proposing = slot_info.duration.mul_f32(self.block_proposal_slot_portion.get());
+	fn proposing_remaining_duration(&self, slot_info: &SlotInfo<B>) -> std::time::Duration {
+		let parent_slot = find_pre_digest::<B>(&slot_info.chain_head).ok().map(|d| d.slot());
 
-		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 slot_info.chain_head.number().is_zero() {
-			return slot_remaining
-		}
-
-		let parent_slot = match find_pre_digest::<B>(&slot_info.chain_head) {
-			Err(_) => return slot_remaining,
-			Ok(d) => d.slot(),
-		};
-
-		if let Some(slot_lenience) =
-			sc_consensus_slots::slot_lenience_exponential(parent_slot, slot_info)
-		{
-			debug!(
-				target: "babe",
-				"No block for {} slots. Applying exponential lenience of {}s",
-				slot_info.slot.saturating_sub(parent_slot + 1),
-				slot_lenience.as_secs(),
-			);
-
-			slot_remaining + slot_lenience
-		} else {
-			slot_remaining
-		}
+		sc_consensus_slots::proposing_remaining_duration(
+			parent_slot,
+			slot_info,
+			&self.block_proposal_slot_portion,
+			self.max_block_proposal_slot_portion.as_ref(),
+			sc_consensus_slots::SlotLenienceType::Exponential,
+			self.logging_target(),
+		)
 	}
 }
 

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

@@ -473,6 +473,7 @@ fn run_one_test(mutator: impl Fn(&mut TestHeader, Stage) + Send + Sync + 'static
 			can_author_with: sp_consensus::AlwaysCanAuthor,
 			justification_sync_link: (),
 			block_proposal_slot_portion: SlotProportion::new(0.5),
+			max_block_proposal_slot_portion: None,
 			telemetry: None,
 		}).expect("Starts babe"));
 	}

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

@@ -666,6 +666,96 @@ impl SlotProportion {
 	}
 }
 
+/// The strategy used to calculate the slot lenience used to increase the block proposal time when
+/// slots have been skipped with no blocks authored.
+pub enum SlotLenienceType {
+	/// Increase the lenience linearly with the number of skipped slots.
+	Linear,
+	/// Increase the lenience exponentially with the number of skipped slots.
+	Exponential,
+}
+
+impl SlotLenienceType {
+	fn as_str(&self) -> &'static str {
+		match self {
+			SlotLenienceType::Linear => "linear",
+			SlotLenienceType::Exponential => "exponential",
+		}
+	}
+}
+
+/// Calculate the remaining duration for block proposal taking into account whether any slots have
+/// been skipped and applying the given lenience strategy. If `max_block_proposal_slot_portion` is
+/// not none this method guarantees that the returned duration must be lower or equal to
+/// `slot_info.duration * max_block_proposal_slot_portion`.
+pub fn proposing_remaining_duration<Block: BlockT>(
+	parent_slot: Option<Slot>,
+	slot_info: &SlotInfo<Block>,
+	block_proposal_slot_portion: &SlotProportion,
+	max_block_proposal_slot_portion: Option<&SlotProportion>,
+	slot_lenience_type: SlotLenienceType,
+	log_target: &str,
+) -> Duration {
+	use sp_runtime::traits::Zero;
+
+	let proposing_duration = slot_info
+		.duration
+		.mul_f32(block_proposal_slot_portion.get());
+
+	let slot_remaining = slot_info
+		.ends_at
+		.checked_duration_since(std::time::Instant::now())
+		.unwrap_or_default();
+
+	let proposing_duration = std::cmp::min(slot_remaining, proposing_duration);
+
+	// If parent is genesis block, we don't require any lenience factor.
+	if slot_info.chain_head.number().is_zero() {
+		return proposing_duration;
+	}
+
+	let parent_slot = match parent_slot {
+		Some(parent_slot) => parent_slot,
+		None => return proposing_duration,
+	};
+
+	let slot_lenience = match slot_lenience_type {
+		SlotLenienceType::Exponential => slot_lenience_exponential(parent_slot, slot_info),
+		SlotLenienceType::Linear => slot_lenience_linear(parent_slot, slot_info),
+	};
+
+	if let Some(slot_lenience) = slot_lenience {
+		let lenient_proposing_duration =
+			proposing_duration + slot_lenience.mul_f32(block_proposal_slot_portion.get());
+
+		// if we defined a maximum portion of the slot for proposal then we must make sure the
+		// lenience doesn't go over it
+		let lenient_proposing_duration =
+			if let Some(ref max_block_proposal_slot_portion) = max_block_proposal_slot_portion {
+				std::cmp::min(
+					lenient_proposing_duration,
+					slot_info
+						.duration
+						.mul_f32(max_block_proposal_slot_portion.get()),
+				)
+			} else {
+				lenient_proposing_duration
+			};
+
+		debug!(
+			target: log_target,
+			"No block for {} slots. Applying {} lenience, total proposing duration: {}",
+			slot_info.slot.saturating_sub(parent_slot + 1),
+			slot_lenience_type.as_str(),
+			lenient_proposing_duration.as_secs(),
+		);
+
+		lenient_proposing_duration
+	} else {
+		proposing_duration
+	}
+}
+
 /// 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
@@ -703,7 +793,7 @@ pub fn slot_lenience_exponential<Block: BlockT>(
 /// a linear backoff of at most `20 * slot_duration`, if no slots were skipped
 /// this method will return `None.`
 pub fn slot_lenience_linear<Block: BlockT>(
-	parent_slot: u64,
+	parent_slot: Slot,
 	slot_info: &SlotInfo<Block>,
 ) -> Option<Duration> {
 	// never give more than 20 times more lenience.
@@ -839,7 +929,7 @@ mod test {
 			duration: SLOT_DURATION,
 			timestamp: Default::default(),
 			inherent_data: Default::default(),
-			ends_at: Instant::now(),
+			ends_at: Instant::now() + SLOT_DURATION,
 			chain_head: Header::new(
 				1,
 				Default::default(),
@@ -897,6 +987,36 @@ mod test {
 		);
 	}
 
+	#[test]
+	fn proposing_remaining_duration_should_apply_lenience_based_on_proposal_slot_proportion() {
+		assert_eq!(
+			proposing_remaining_duration(
+				Some(0.into()),
+				&slot(2),
+				&SlotProportion(0.25),
+				None,
+				SlotLenienceType::Linear,
+				"test",
+			),
+			SLOT_DURATION.mul_f32(0.25 * 2.0),
+		);
+	}
+
+	#[test]
+	fn proposing_remaining_duration_should_never_exceed_max_proposal_slot_proportion() {
+		assert_eq!(
+			proposing_remaining_duration(
+				Some(0.into()),
+				&slot(100),
+				&SlotProportion(0.25),
+				Some(SlotProportion(0.9)).as_ref(),
+				SlotLenienceType::Exponential,
+				"test",
+			),
+			SLOT_DURATION.mul_f32(0.9),
+		);
+	}
+
 	#[derive(PartialEq, Debug)]
 	struct HeadState {
 		head_number: NumberFor<Block>,