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

bin/node/runtime/src/lib.rs

@@ -813,7 +813,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;
@@ -880,8 +879,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::{IsMember, One, SaturatedConversion, Saturating, Zero},
-	ConsensusEngineId, KeyTypeId,
+	ConsensusEngineId, KeyTypeId, Percent,
 };
 use sp_session::{GetSessionNumber, GetValidatorCount};
 use sp_std::prelude::*;
@@ -781,14 +781,25 @@ impl<T: Config> frame_support::traits::EstimateNextSessionRotation<T::BlockNumbe
 		T::EpochDuration::get().saturated_into()
 	}
 
-	fn estimate_next_session_rotation(now: T::BlockNumber) -> Option<T::BlockNumber> {
-		Self::next_expected_epoch_change(now)
+	fn estimate_current_session_progress(_now: T::BlockNumber) -> (Option<Percent>, Weight) {
+		let elapsed = CurrentSlot::get().saturating_sub(Self::current_epoch_start()) + 1;
+
+		(
+			Some(Percent::from_rational_approximation(
+				*elapsed,
+				T::EpochDuration::get(),
+			)),
+			// Read: Current Slot, Epoch Index, Genesis Slot
+			T::DbWeight::get().reads(3),
+		)
 	}
 
-	// The validity of this weight depends on the implementation of `estimate_next_session_rotation`
-	fn weight(_now: T::BlockNumber) -> Weight {
-		// Read: Current Slot, Epoch Index, Genesis Slot
-		T::DbWeight::get().reads(3)
+	fn estimate_next_session_rotation(now: T::BlockNumber) -> (Option<T::BlockNumber>, Weight) {
+		(
+			Self::next_expected_epoch_change(now),
+			// Read: Current Slot, Epoch Index, Genesis Slot
+			T::DbWeight::get().reads(3),
+		)
 	}
 }
 

frame/babe/src/mock.rs

@@ -260,7 +260,7 @@ impl Config for Test {
 pub fn go_to_block(n: u64, s: u64) {
 	use frame_support::traits::OnFinalize;
 
-	System::on_finalize(System::block_number());
+	Babe::on_finalize(System::block_number());
 	Session::on_finalize(System::block_number());
 	Staking::on_finalize(System::block_number());
 
@@ -274,14 +274,8 @@ pub fn go_to_block(n: u64, s: u64) {
 	let pre_digest = make_secondary_plain_pre_digest(0, s.into());
 
 	System::initialize(&n, &parent_hash, &pre_digest, InitKind::Full);
-	System::set_block_number(n);
-	Timestamp::set_timestamp(n);
 
-	if s > 1 {
-		CurrentSlot::put(Slot::from(s));
-	}
-
-	System::on_initialize(n);
+	Babe::on_initialize(n);
 	Session::on_initialize(n);
 	Staking::on_initialize(n);
 }

frame/babe/src/tests.rs

@@ -20,12 +20,12 @@
 use super::{Call, *};
 use frame_support::{
 	assert_err, assert_ok,
-	traits::{Currency, OnFinalize},
+	traits::{Currency, EstimateNextSessionRotation, OnFinalize},
 	weights::{GetDispatchInfo, Pays},
 };
 use mock::*;
 use pallet_session::ShouldEndSession;
-use sp_consensus_babe::{AllowedSlots, Slot, BabeEpochConfiguration};
+use sp_consensus_babe::{AllowedSlots, BabeEpochConfiguration, Slot};
 use sp_core::crypto::Pair;
 
 const EMPTY_RANDOMNESS: [u8; 32] = [
@@ -220,6 +220,38 @@ fn can_predict_next_epoch_change() {
 	})
 }
 
+#[test]
+fn can_estimate_current_epoch_progress() {
+	new_test_ext(1).execute_with(|| {
+		assert_eq!(<Test as Config>::EpochDuration::get(), 3);
+
+		// with BABE the genesis block is not part of any epoch, the first epoch starts at block #1,
+		// therefore its last block should be #3
+		for i in 1u64..4 {
+			progress_to_block(i);
+
+			assert_eq!(Babe::estimate_next_session_rotation(i).0.unwrap(), 4);
+
+			// the last block of the epoch must have 100% progress.
+			if Babe::estimate_next_session_rotation(i).0.unwrap() - 1 == i {
+				assert_eq!(
+					Babe::estimate_current_session_progress(i).0.unwrap(),
+					Percent::from_percent(100)
+				);
+			} else {
+				assert!(Babe::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100));
+			}
+		}
+
+		// the first block of the new epoch counts towards the epoch progress as well
+		progress_to_block(4);
+		assert_eq!(
+			Babe::estimate_current_session_progress(4).0.unwrap(),
+			Percent::from_percent(33),
+		);
+	})
+}
+
 #[test]
 fn can_enact_next_config() {
 	new_test_ext(1).execute_with(|| {

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 {