im-online: use EstimateNextSessionRotation to get better estimates of session progress

bin/node/runtime/src/lib.rs

@@ -806,7 +806,6 @@ impl pallet_sudo::Config for Runtime {
 }
 
 parameter_types! {
-	pub const SessionDuration: BlockNumber = EPOCH_DURATION_IN_SLOTS as _;
 	pub const ImOnlineUnsignedPriority: TransactionPriority = TransactionPriority::max_value();
 	/// We prioritize im-online heartbeats over election solution submission.
 	pub const StakingUnsignedPriority: TransactionPriority = TransactionPriority::max_value() / 2;
@@ -873,8 +872,8 @@ impl<C> frame_system::offchain::SendTransactionTypes<C> for Runtime where
 impl pallet_im_online::Config for Runtime {
 	type AuthorityId = ImOnlineId;
 	type Event = Event;
+	type NextSessionRotation = Babe;
 	type ValidatorSet = Historical;
-	type SessionDuration = SessionDuration;
 	type ReportUnresponsiveness = Offences;
 	type UnsignedPriority = ImOnlineUnsignedPriority;
 	type WeightInfo = pallet_im_online::weights::SubstrateWeight<Runtime>;

frame/babe/src/lib.rs

@@ -34,7 +34,7 @@ use sp_application_crypto::Public;
 use sp_runtime::{
 	generic::DigestItem,
 	traits::{Hash, IsMember, One, SaturatedConversion, Saturating, Zero},
-	ConsensusEngineId, KeyTypeId,
+	ConsensusEngineId, KeyTypeId, Percent,
 };
 use sp_session::{GetSessionNumber, GetValidatorCount};
 use sp_std::prelude::*;
@@ -765,12 +765,26 @@ impl<T: Config> frame_support::traits::EstimateNextSessionRotation<T::BlockNumbe
 		T::EpochDuration::get().saturated_into()
 	}
 
+	fn estimate_current_session_progress(_now: T::BlockNumber) -> Option<Percent> {
+		let elapsed = CurrentSlot::get().saturating_sub(Self::current_epoch_start());
+		Some(Percent::from_rational_approximation(
+			*elapsed,
+			T::EpochDuration::get(),
+		))
+	}
+
 	fn estimate_next_session_rotation(now: T::BlockNumber) -> Option<T::BlockNumber> {
 		Self::next_expected_epoch_change(now)
 	}
 
 	// The validity of this weight depends on the implementation of `estimate_next_session_rotation`
-	fn weight(_now: T::BlockNumber) -> Weight {
+	fn estimate_next_session_rotation_weight(_now: T::BlockNumber) -> Weight {
+		// Read: Current Slot, Epoch Index, Genesis Slot
+		T::DbWeight::get().reads(3)
+	}
+
+	// The validity of this weight depends on the implementation of `estimate_current_session_progress`
+	fn estimate_current_session_progress_weight(_now: T::BlockNumber) -> Weight {
 		// Read: Current Slot, Epoch Index, Genesis Slot
 		T::DbWeight::get().reads(3)
 	}

frame/im-online/src/lib.rs

@@ -81,20 +81,24 @@ use sp_std::prelude::*;
 use sp_std::convert::TryInto;
 use sp_runtime::{
 	offchain::storage::StorageValueRef,
-	RuntimeDebug,
-	traits::{Convert, Member, Saturating, AtLeast32BitUnsigned}, Perbill,
+	traits::{AtLeast32BitUnsigned, Convert, Member, Saturating},
 	transaction_validity::{
-		TransactionValidity, ValidTransaction, InvalidTransaction, TransactionSource,
-		TransactionPriority,
+		InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity,
+		ValidTransaction,
 	},
+	Perbill, Percent, RuntimeDebug,
 };
 use sp_staking::{
 	SessionIndex,
 	offence::{ReportOffence, Offence, Kind},
 };
 use frame_support::{
-	decl_module, decl_event, decl_storage, Parameter, decl_error,
-	traits::{Get, ValidatorSet, ValidatorSetWithIdentification, OneSessionHandler},
+	decl_error, decl_event, decl_module, decl_storage,
+	traits::{
+		EstimateNextSessionRotation, Get, OneSessionHandler, ValidatorSet,
+		ValidatorSetWithIdentification,
+	},
+	Parameter,
 };
 use frame_system::ensure_none;
 use frame_system::offchain::{
@@ -181,7 +185,7 @@ impl<BlockNumber: PartialEq + AtLeast32BitUnsigned + Copy> HeartbeatStatus<Block
 /// Error which may occur while executing the off-chain code.
 #[cfg_attr(test, derive(PartialEq))]
 enum OffchainErr<BlockNumber> {
-	TooEarly(BlockNumber),
+	TooEarly,
 	WaitingForInclusion(BlockNumber),
 	AlreadyOnline(u32),
 	FailedSigning,
@@ -193,8 +197,8 @@ enum OffchainErr<BlockNumber> {
 impl<BlockNumber: sp_std::fmt::Debug> sp_std::fmt::Debug for OffchainErr<BlockNumber> {
 	fn fmt(&self, fmt: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
 		match *self {
-			OffchainErr::TooEarly(ref block) =>
-				write!(fmt, "Too early to send heartbeat, next expected at {:?}", block),
+			OffchainErr::TooEarly =>
+				write!(fmt, "Too early to send heartbeat."),
 			OffchainErr::WaitingForInclusion(ref block) =>
 				write!(fmt, "Heartbeat already sent at {:?}. Waiting for inclusion.", block),
 			OffchainErr::AlreadyOnline(auth_idx) =>
@@ -245,24 +249,24 @@ pub trait Config: SendTransactionTypes<Call<Self>> + frame_system::Config {
 	/// The overarching event type.
 	type Event: From<Event<Self>> + Into<<Self as frame_system::Config>::Event>;
 
-	/// An expected duration of the session.
-	///
-	/// This parameter is used to determine the longevity of `heartbeat` transaction
-	/// and a rough time when we should start considering sending heartbeats,
-	/// since the workers avoids sending them at the very beginning of the session, assuming
-	/// there is a chance the authority will produce a block and they won't be necessary.
-	type SessionDuration: Get<Self::BlockNumber>;
-
 	/// A type for retrieving the validators supposed to be online in a session.
 	type ValidatorSet: ValidatorSetWithIdentification<Self::AccountId>;
 
+	/// A trait that allows us to estimate the current session progress and also the
+	/// average session length.
+	///
+	/// This parameter is used to determine the longevity of `heartbeat` transaction and a
+	/// rough time when we should start considering sending heartbeats, since the workers
+	/// avoids sending them at the very beginning of the session, assuming there is a
+	/// chance the authority will produce a block and they won't be necessary.
+	type NextSessionRotation: EstimateNextSessionRotation<Self::BlockNumber>;
+
 	/// A type that gives us the ability to submit unresponsiveness offence reports.
-	type ReportUnresponsiveness:
-		ReportOffence<
-			Self::AccountId,
-			IdentificationTuple<Self>,
-			UnresponsivenessOffence<IdentificationTuple<Self>>,
-		>;
+	type ReportUnresponsiveness: ReportOffence<
+		Self::AccountId,
+		IdentificationTuple<Self>,
+		UnresponsivenessOffence<IdentificationTuple<Self>>,
+	>;
 
 	/// A configuration for base priority of unsigned transactions.
 	///
@@ -290,12 +294,17 @@ decl_event!(
 
 decl_storage! {
 	trait Store for Module<T: Config> as ImOnline {
-		/// The block number after which it's ok to send heartbeats in current session.
+		/// The block number after which it's ok to send heartbeats in the current
+		/// session.
+		///
+		/// At the beginning of each session we set this to a value that should fall
+		/// roughly in the middle of the session duration. The idea is to first wait for
+		/// the validators to produce a block in the current session, so that the
+		/// heartbeat later on will not be necessary.
 		///
-		/// At the beginning of each session we set this to a value that should
-		/// fall roughly in the middle of the session duration.
-		/// The idea is to first wait for the validators to produce a block
-		/// in the current session, so that the heartbeat later on will not be necessary.
+		/// This value will only be used as a fallback if we fail to get a proper session
+		/// progress estimate from `NextSessionRotation`, as those estimates should be
+		/// more accurate then the value we calculate for `HeartbeatAfter`.
 		HeartbeatAfter get(fn heartbeat_after): T::BlockNumber;
 
 		/// The current set of keys that may issue a heartbeat.
@@ -469,27 +478,43 @@ impl<T: Config> Module<T> {
 		);
 	}
 
-	pub(crate) fn send_heartbeats(block_number: T::BlockNumber)
-		-> OffchainResult<T, impl Iterator<Item=OffchainResult<T, ()>>>
-	{
-		let heartbeat_after = <HeartbeatAfter<T>>::get();
-		if block_number < heartbeat_after {
-			return Err(OffchainErr::TooEarly(heartbeat_after))
+	pub(crate) fn send_heartbeats(
+		block_number: T::BlockNumber,
+	) -> OffchainResult<T, impl Iterator<Item = OffchainResult<T, ()>>> {
+		const HALF_SESSION: Percent = Percent::from_percent(50);
+
+		let too_early = if let Some(progress) =
+			T::NextSessionRotation::estimate_current_session_progress(block_number)
+		{
+			// we try to get an estimate of the current session progress first since it
+			// should provide more accurate results and send the heartbeat if we're halfway
+			// through the session.
+			progress < HALF_SESSION
+		} else {
+			// otherwise we fallback to using the block number calculated at the beginning
+			// of the session that should roughly correspond to the middle of the session
+			let heartbeat_after = <HeartbeatAfter<T>>::get();
+			block_number < heartbeat_after
+		};
+
+		if too_early {
+			return Err(OffchainErr::TooEarly);
 		}
 
 		let session_index = T::ValidatorSet::session_index();
 		let validators_len = Keys::<T>::decode_len().unwrap_or_default() as u32;
 
-		Ok(Self::local_authority_keys()
-			.map(move |(authority_index, key)|
+		Ok(
+			Self::local_authority_keys().map(move |(authority_index, key)| {
 				Self::send_single_heartbeat(
 					authority_index,
 					key,
 					session_index,
 					block_number,
 					validators_len,
 				)
-			))
+			}),
+		)
 	}
 
 
@@ -648,7 +673,7 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Module<T> {
 		// Since we consider producing blocks as being online,
 		// the heartbeat is deferred a bit to prevent spamming.
 		let block_number = <frame_system::Module<T>>::block_number();
-		let half_session = T::SessionDuration::get() / 2u32.into();
+		let half_session = T::NextSessionRotation::average_session_length() / 2u32.into();
 		<HeartbeatAfter<T>>::put(block_number + half_session);
 
 		// Remember who the authorities are for the new session.
@@ -699,10 +724,7 @@ const INVALID_VALIDATORS_LEN: u8 = 10;
 impl<T: Config> frame_support::unsigned::ValidateUnsigned for Module<T> {
 	type Call = Call<T>;
 
-	fn validate_unsigned(
-		_source: TransactionSource,
-		call: &Self::Call,
-	) -> TransactionValidity {
+	fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity {
 		if let Call::heartbeat(heartbeat, signature) = call {
 			if <Module<T>>::is_online(heartbeat.authority_index) {
 				// we already received a heartbeat for this authority
@@ -737,9 +759,12 @@ impl<T: Config> frame_support::unsigned::ValidateUnsigned for Module<T> {
 			ValidTransaction::with_tag_prefix("ImOnline")
 				.priority(T::UnsignedPriority::get())
 				.and_provides((current_session, authority_id))
-				.longevity(TryInto::<u64>::try_into(
-					T::SessionDuration::get() / 2u32.into()
-				).unwrap_or(64_u64))
+				.longevity(
+					TryInto::<u64>::try_into(
+						T::NextSessionRotation::average_session_length() / 2u32.into(),
+					)
+					.unwrap_or(64_u64),
+				)
 				.propagate(true)
 				.build()
 		} else {

frame/session/src/lib.rs

@@ -116,8 +116,10 @@ pub mod weights;
 
 use sp_std::{prelude::*, marker::PhantomData, ops::{Sub, Rem}};
 use codec::Decode;
-use sp_runtime::{KeyTypeId, Perbill, RuntimeAppPublic};
-use sp_runtime::traits::{Convert, Zero, Member, OpaqueKeys, Saturating};
+use sp_runtime::{
+	traits::{AtLeast32BitUnsigned, Convert, Member, OpaqueKeys, Zero},
+	KeyTypeId, Perbill, Percent, RuntimeAppPublic,
+};
 use sp_staking::SessionIndex;
 use frame_support::{
 	ensure, decl_module, decl_event, decl_storage, decl_error, ConsensusEngineId, Parameter,
@@ -142,31 +144,51 @@ pub trait ShouldEndSession<BlockNumber> {
 /// The first session will have length of `Offset`, and
 /// the following sessions will have length of `Period`.
 /// This may prove nonsensical if `Offset` >= `Period`.
-pub struct PeriodicSessions<
-	Period,
-	Offset,
->(PhantomData<(Period, Offset)>);
+pub struct PeriodicSessions<Period, Offset>(PhantomData<(Period, Offset)>);
 
 impl<
-	BlockNumber: Rem<Output=BlockNumber> + Sub<Output=BlockNumber> + Zero + PartialOrd,
+	BlockNumber: Rem<Output = BlockNumber> + Sub<Output = BlockNumber> + Zero + PartialOrd,
 	Period: Get<BlockNumber>,
 	Offset: Get<BlockNumber>,
-> ShouldEndSession<BlockNumber> for PeriodicSessions<Period, Offset> {
+> ShouldEndSession<BlockNumber> for PeriodicSessions<Period, Offset>
+{
 	fn should_end_session(now: BlockNumber) -> bool {
 		let offset = Offset::get();
 		now >= offset && ((now - offset) % Period::get()).is_zero()
 	}
 }
 
 impl<
-	BlockNumber: Rem<Output=BlockNumber> + Sub<Output=BlockNumber> + Zero + PartialOrd + Saturating + Clone,
+	BlockNumber: AtLeast32BitUnsigned + Clone,
 	Period: Get<BlockNumber>,
-	Offset: Get<BlockNumber>,
-> EstimateNextSessionRotation<BlockNumber> for PeriodicSessions<Period, Offset> {
+	Offset: Get<BlockNumber>
+> EstimateNextSessionRotation<BlockNumber> for PeriodicSessions<Period, Offset>
+{
+	fn average_session_length() -> BlockNumber {
+		Period::get()
+	}
+
+	fn estimate_current_session_progress(now: BlockNumber) -> Option<Percent> {
+		let offset = Offset::get();
+		let period = Period::get();
+
+		if now > offset {
+			let current = (now - offset) % period.clone();
+
+			// NOTE: when this is called on the last block of the session we will return 0% since we
+			// make the assumption that the session has already been rotated (or will rotate). This
+			// is consistent with the results from `estimate_next_session_rotation` below.
+			Some(Percent::from_rational_approximation(current, period))
+		} else {
+			Some(Percent::from_rational_approximation(now, offset))
+		}
+	}
+
 	fn estimate_next_session_rotation(now: BlockNumber) -> Option<BlockNumber> {
 		let offset = Offset::get();
 		let period = Period::get();
-		Some(if now > offset {
+
+		let next_session = if now > offset {
 			let block_after_last_session = (now.clone() - offset) % period.clone();
 			if block_after_last_session > Zero::zero() {
 				now.saturating_add(period.saturating_sub(block_after_last_session))
@@ -179,19 +201,25 @@ impl<
 			}
 		} else {
 			offset
-		})
+		};
+
+		Some(next_session)
 	}
 
-	fn weight(_now: BlockNumber) -> Weight {
-		// Weight note: `estimate_next_session_rotation` has no storage reads and trivial
+	fn estimate_current_session_progress_weight(_now: BlockNumber) -> Weight {
+		// Weight note: `estimate_current_session_progress` has no storage reads and trivial
 		// computational overhead. There should be no risk to the chain having this weight value be
 		// zero for now. However, this value of zero was not properly calculated, and so it would be
 		// reasonable to come back here and properly calculate the weight of this function.
-		0
+		Zero::zero()
 	}
 
-	fn average_session_length() -> BlockNumber {
-		Period::get()
+	fn estimate_next_session_rotation_weight(_now: BlockNumber) -> Weight {
+		// Weight note: `estimate_next_session_rotation` has no storage reads and trivial
+		// computational overhead. There should be no risk to the chain having this weight value be
+		// zero for now. However, this value of zero was not properly calculated, and so it would be
+		// reasonable to come back here and properly calculate the weight of this function.
+		Zero::zero()
 	}
 }
 
@@ -833,17 +861,17 @@ impl<T: Config, Inner: FindAuthor<u32>> FindAuthor<T::ValidatorId>
 }
 
 impl<T: Config> EstimateNextNewSession<T::BlockNumber> for Module<T> {
+	fn average_session_length() -> T::BlockNumber {
+		T::NextSessionRotation::average_session_length()
+	}
+
 	/// This session module always calls new_session and next_session at the same time, hence we
 	/// do a simple proxy and pass the function to next rotation.
 	fn estimate_next_new_session(now: T::BlockNumber) -> Option<T::BlockNumber> {
 		T::NextSessionRotation::estimate_next_session_rotation(now)
 	}
 
-	fn average_session_length() -> T::BlockNumber {
-		T::NextSessionRotation::average_session_length()
-	}
-
-	fn weight(now: T::BlockNumber) -> Weight {
-		T::NextSessionRotation::weight(now)
+	fn estimate_next_new_session_weight(now: T::BlockNumber) -> Weight {
+		T::NextSessionRotation::estimate_next_session_rotation_weight(now)
 	}
 }

frame/staking/src/lib.rs

@@ -1385,7 +1385,7 @@ decl_module! {
 				} else {
 					log!(warn, "Estimating next session change failed.");
 				}
-				add_weight(0, 0, T::NextNewSession::weight(now))
+				add_weight(0, 0, T::NextNewSession::estimate_next_new_session_weight(now))
 			}
 			// For `era_election_status`, `is_current_session_final`, `will_era_be_forced`
 			add_weight(3, 0, 0);