diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 2043f59eb9..bb372f31c7 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/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 frame_system::offchain::SendTransactionTypes 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; diff --git a/substrate/frame/babe/src/lib.rs b/substrate/frame/babe/src/lib.rs index 75bdba78a9..00bfa4f265 100644 --- a/substrate/frame/babe/src/lib.rs +++ b/substrate/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::*; @@ -780,14 +780,25 @@ impl frame_support::traits::EstimateNextSessionRotation Option { - Self::next_expected_epoch_change(now) + fn estimate_current_session_progress(_now: T::BlockNumber) -> (Option, 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, Weight) { + ( + Self::next_expected_epoch_change(now), + // Read: Current Slot, Epoch Index, Genesis Slot + T::DbWeight::get().reads(3), + ) } } diff --git a/substrate/frame/babe/src/mock.rs b/substrate/frame/babe/src/mock.rs index 412f13f6a2..c46b55c2c4 100644 --- a/substrate/frame/babe/src/mock.rs +++ b/substrate/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); } diff --git a/substrate/frame/babe/src/tests.rs b/substrate/frame/babe/src/tests.rs index 82a7782448..0ccc3db4df 100644 --- a/substrate/frame/babe/src/tests.rs +++ b/substrate/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!(::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(|| { diff --git a/substrate/frame/im-online/src/lib.rs b/substrate/frame/im-online/src/lib.rs index f0df19d6ab..e00b5aa9d1 100644 --- a/substrate/frame/im-online/src/lib.rs +++ b/substrate/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 HeartbeatStatus { - TooEarly(BlockNumber), + TooEarly, WaitingForInclusion(BlockNumber), AlreadyOnline(u32), FailedSigning, @@ -193,8 +197,8 @@ enum OffchainErr { impl sp_std::fmt::Debug for OffchainErr { 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> + frame_system::Config { /// The overarching event type. type Event: From> + Into<::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; - /// A type for retrieving the validators supposed to be online in a session. type ValidatorSet: ValidatorSetWithIdentification; + /// 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; + /// A type that gives us the ability to submit unresponsiveness offence reports. - type ReportUnresponsiveness: - ReportOffence< - Self::AccountId, - IdentificationTuple, - UnresponsivenessOffence>, - >; + type ReportUnresponsiveness: ReportOffence< + Self::AccountId, + IdentificationTuple, + UnresponsivenessOffence>, + >; /// A configuration for base priority of unsigned transactions. /// @@ -290,12 +294,17 @@ decl_event!( decl_storage! { trait Store for Module 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,19 +478,34 @@ impl Module { ); } - pub(crate) fn send_heartbeats(block_number: T::BlockNumber) - -> OffchainResult>> - { - let heartbeat_after = >::get(); - if block_number < heartbeat_after { - return Err(OffchainErr::TooEarly(heartbeat_after)) + pub(crate) fn send_heartbeats( + block_number: T::BlockNumber, + ) -> OffchainResult>> { + 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 = >::get(); + block_number < heartbeat_after + }; + + if too_early { + return Err(OffchainErr::TooEarly); } let session_index = T::ValidatorSet::session_index(); let validators_len = Keys::::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, @@ -489,7 +513,8 @@ impl Module { block_number, validators_len, ) - )) + }), + ) } @@ -648,7 +673,7 @@ impl OneSessionHandler for Module { // Since we consider producing blocks as being online, // the heartbeat is deferred a bit to prevent spamming. let block_number = >::block_number(); - let half_session = T::SessionDuration::get() / 2u32.into(); + let half_session = T::NextSessionRotation::average_session_length() / 2u32.into(); >::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 frame_support::unsigned::ValidateUnsigned for Module { type Call = Call; - 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 >::is_online(heartbeat.authority_index) { // we already received a heartbeat for this authority @@ -737,9 +759,12 @@ impl frame_support::unsigned::ValidateUnsigned for Module { ValidTransaction::with_tag_prefix("ImOnline") .priority(T::UnsignedPriority::get()) .and_provides((current_session, authority_id)) - .longevity(TryInto::::try_into( - T::SessionDuration::get() / 2u32.into() - ).unwrap_or(64_u64)) + .longevity( + TryInto::::try_into( + T::NextSessionRotation::average_session_length() / 2u32.into(), + ) + .unwrap_or(64_u64), + ) .propagate(true) .build() } else { diff --git a/substrate/frame/im-online/src/mock.rs b/substrate/frame/im-online/src/mock.rs index 1b80f5b12d..f8346aa536 100644 --- a/substrate/frame/im-online/src/mock.rs +++ b/substrate/frame/im-online/src/mock.rs @@ -21,15 +21,19 @@ use std::cell::RefCell; -use crate::Config; -use sp_runtime::Perbill; -use sp_staking::{SessionIndex, offence::{ReportOffence, OffenceError}}; -use sp_runtime::testing::{Header, UintAuthorityId, TestXt}; -use sp_runtime::traits::{IdentityLookup, BlakeTwo256, ConvertInto}; -use sp_core::H256; -use frame_support::parameter_types; -use crate as imonline; +use frame_support::{parameter_types, weights::Weight}; use pallet_session::historical as pallet_session_historical; +use sp_core::H256; +use sp_runtime::testing::{Header, TestXt, UintAuthorityId}; +use sp_runtime::traits::{BlakeTwo256, ConvertInto, IdentityLookup}; +use sp_runtime::{Perbill, Percent}; +use sp_staking::{ + offence::{OffenceError, ReportOffence}, + SessionIndex, +}; + +use crate as imonline; +use crate::Config; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -176,6 +180,41 @@ impl pallet_authorship::Config for Runtime { type EventHandler = ImOnline; } +thread_local! { + pub static MOCK_CURRENT_SESSION_PROGRESS: RefCell>> = RefCell::new(None); +} + +thread_local! { + pub static MOCK_AVERAGE_SESSION_LENGTH: RefCell> = RefCell::new(None); +} + +pub struct TestNextSessionRotation; + +impl frame_support::traits::EstimateNextSessionRotation for TestNextSessionRotation { + fn average_session_length() -> u64 { + // take the mock result if any and return it + let mock = MOCK_AVERAGE_SESSION_LENGTH.with(|p| p.borrow_mut().take()); + + mock.unwrap_or(pallet_session::PeriodicSessions::::average_session_length()) + } + + fn estimate_current_session_progress(now: u64) -> (Option, Weight) { + let (estimate, weight) = + pallet_session::PeriodicSessions::::estimate_current_session_progress( + now, + ); + + // take the mock result if any and return it + let mock = MOCK_CURRENT_SESSION_PROGRESS.with(|p| p.borrow_mut().take()); + + (mock.unwrap_or(estimate), weight) + } + + fn estimate_next_session_rotation(now: u64) -> (Option, Weight) { + pallet_session::PeriodicSessions::::estimate_next_session_rotation(now) + } +} + parameter_types! { pub const UnsignedPriority: u64 = 1 << 20; } @@ -183,9 +222,9 @@ parameter_types! { impl Config for Runtime { type AuthorityId = UintAuthorityId; type Event = Event; - type ReportUnresponsiveness = OffenceHandler; type ValidatorSet = Historical; - type SessionDuration = Period; + type NextSessionRotation = TestNextSessionRotation; + type ReportUnresponsiveness = OffenceHandler; type UnsignedPriority = UnsignedPriority; type WeightInfo = (); } diff --git a/substrate/frame/im-online/src/tests.rs b/substrate/frame/im-online/src/tests.rs index 919b639dd6..f447a2ade5 100644 --- a/substrate/frame/im-online/src/tests.rs +++ b/substrate/frame/im-online/src/tests.rs @@ -357,3 +357,86 @@ fn should_not_send_a_report_if_already_online() { }); }); } + +#[test] +fn should_handle_missing_progress_estimates() { + use frame_support::traits::OffchainWorker; + + let mut ext = new_test_ext(); + let (offchain, _state) = TestOffchainExt::new(); + let (pool, state) = TestTransactionPoolExt::new(); + ext.register_extension(OffchainDbExt::new(offchain.clone())); + ext.register_extension(OffchainWorkerExt::new(offchain)); + ext.register_extension(TransactionPoolExt::new(pool)); + + ext.execute_with(|| { + let block = 1; + + System::set_block_number(block); + UintAuthorityId::set_all_keys(vec![0, 1, 2]); + + // buffer new validators + Session::rotate_session(); + + // enact the change and buffer another one + VALIDATORS.with(|l| *l.borrow_mut() = Some(vec![0, 1, 2])); + Session::rotate_session(); + + // we will return `None` on the next call to `estimate_current_session_progress` + // and the offchain worker should fallback to checking `HeartbeatAfter` + MOCK_CURRENT_SESSION_PROGRESS.with(|p| *p.borrow_mut() = Some(None)); + ImOnline::offchain_worker(block); + + assert_eq!(state.read().transactions.len(), 3); + }); +} + +#[test] +fn should_handle_non_linear_session_progress() { + // NOTE: this is the reason why we started using `EstimateNextSessionRotation` to figure out if + // we should send a heartbeat, it's possible that between successive blocks we progress through + // the session more than just one block increment (in BABE session length is defined in slots, + // not block numbers). + + let mut ext = new_test_ext(); + let (offchain, _state) = TestOffchainExt::new(); + let (pool, _) = TestTransactionPoolExt::new(); + ext.register_extension(OffchainDbExt::new(offchain.clone())); + ext.register_extension(OffchainWorkerExt::new(offchain)); + ext.register_extension(TransactionPoolExt::new(pool)); + + ext.execute_with(|| { + UintAuthorityId::set_all_keys(vec![0, 1, 2]); + + // buffer new validator + Session::rotate_session(); + + // mock the session length as being 10 blocks long, + // enact the change and buffer another one + VALIDATORS.with(|l| *l.borrow_mut() = Some(vec![0, 1, 2])); + + // mock the session length has being 10 which should make us assume the fallback for half + // session will be reached by block 5. + MOCK_AVERAGE_SESSION_LENGTH.with(|p| *p.borrow_mut() = Some(10)); + + Session::rotate_session(); + + // if we don't have valid results for the current session progres then + // we'll fallback to `HeartbeatAfter` and only heartbeat on block 5. + MOCK_CURRENT_SESSION_PROGRESS.with(|p| *p.borrow_mut() = Some(None)); + assert_eq!( + ImOnline::send_heartbeats(2).err(), + Some(OffchainErr::TooEarly), + ); + + MOCK_CURRENT_SESSION_PROGRESS.with(|p| *p.borrow_mut() = Some(None)); + assert!(ImOnline::send_heartbeats(5).ok().is_some()); + + // if we have a valid current session progress then we'll heartbeat as soon + // as we're past 50% of the session regardless of the block number + MOCK_CURRENT_SESSION_PROGRESS + .with(|p| *p.borrow_mut() = Some(Some(Percent::from_percent(51)))); + + assert!(ImOnline::send_heartbeats(2).ok().is_some()); + }); +} diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index 124e6b13b7..54d649381e 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/substrate/frame/offences/benchmarking/src/mock.rs @@ -186,7 +186,7 @@ impl pallet_im_online::Config for Test { type AuthorityId = UintAuthorityId; type Event = Event; type ValidatorSet = Historical; - type SessionDuration = Period; + type NextSessionRotation = pallet_session::PeriodicSessions; type ReportUnresponsiveness = Offences; type UnsignedPriority = (); type WeightInfo = (); diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index d95d99389f..45f3ae9dfc 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/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, One, 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,16 +144,14 @@ pub trait ShouldEndSession { /// 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(PhantomData<(Period, Offset)>); impl< - BlockNumber: Rem + Sub + Zero + PartialOrd, + BlockNumber: Rem + Sub + Zero + PartialOrd, Period: Get, Offset: Get, -> ShouldEndSession for PeriodicSessions { +> ShouldEndSession for PeriodicSessions +{ fn should_end_session(now: BlockNumber) -> bool { let offset = Offset::get(); now >= offset && ((now - offset) % Period::get()).is_zero() @@ -159,14 +159,47 @@ impl< } impl< - BlockNumber: Rem + Sub + Zero + PartialOrd + Saturating + Clone, + BlockNumber: AtLeast32BitUnsigned + Clone, Period: Get, - Offset: Get, -> EstimateNextSessionRotation for PeriodicSessions { - fn estimate_next_session_rotation(now: BlockNumber) -> Option { + Offset: Get +> EstimateNextSessionRotation for PeriodicSessions +{ + fn average_session_length() -> BlockNumber { + Period::get() + } + + fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight) { let offset = Offset::get(); let period = Period::get(); - Some(if now > offset { + + // NOTE: we add one since we assume that the current block has already elapsed, + // i.e. when evaluating the last block in the session the progress should be 100% + // (0% is never returned). + let progress = if now >= offset { + let current = (now - offset) % period.clone() + One::one(); + Some(Percent::from_rational_approximation( + current.clone(), + period.clone(), + )) + } else { + Some(Percent::from_rational_approximation( + now + One::one(), + offset, + )) + }; + + // 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. + (progress, Zero::zero()) + } + + fn estimate_next_session_rotation(now: BlockNumber) -> (Option, Weight) { + let offset = Offset::get(); + let period = Period::get(); + + 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 +212,13 @@ impl< } } else { offset - }) - } + }; - fn 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. - 0 - } - - fn average_session_length() -> BlockNumber { - Period::get() + (Some(next_session), Zero::zero()) } } @@ -833,17 +860,13 @@ impl> FindAuthor } impl EstimateNextNewSession for Module { - /// 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::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) + /// 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, Weight) { + T::NextSessionRotation::estimate_next_session_rotation(now) } } diff --git a/substrate/frame/session/src/tests.rs b/substrate/frame/session/src/tests.rs index b2e086aed9..a528b3293d 100644 --- a/substrate/frame/session/src/tests.rs +++ b/substrate/frame/session/src/tests.rs @@ -253,7 +253,6 @@ fn session_changed_flag_works() { #[test] fn periodic_session_works() { - frame_support::parameter_types! { const Period: u64 = 10; const Offset: u64 = 3; @@ -261,24 +260,67 @@ fn periodic_session_works() { type P = PeriodicSessions; + // make sure that offset phase behaves correctly for i in 0u64..3 { assert!(!P::should_end_session(i)); - assert_eq!(P::estimate_next_session_rotation(i).unwrap(), 3); + assert_eq!(P::estimate_next_session_rotation(i).0.unwrap(), 3); + + // the last block of the session (i.e. the one before session rotation) + // should have progress 100%. + if P::estimate_next_session_rotation(i).0.unwrap() - 1 == i { + assert_eq!( + P::estimate_current_session_progress(i).0.unwrap(), + Percent::from_percent(100) + ); + } else { + assert!( + P::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100) + ); + } } + // we end the session at block #3 and we consider this block the first one + // from the next session. since we're past the offset phase it represents + // 1/10 of progress. assert!(P::should_end_session(3u64)); - assert_eq!(P::estimate_next_session_rotation(3u64).unwrap(), 3); + assert_eq!(P::estimate_next_session_rotation(3u64).0.unwrap(), 3); + assert_eq!( + P::estimate_current_session_progress(3u64).0.unwrap(), + Percent::from_percent(10), + ); for i in (1u64..10).map(|i| 3 + i) { assert!(!P::should_end_session(i)); - assert_eq!(P::estimate_next_session_rotation(i).unwrap(), 13); + assert_eq!(P::estimate_next_session_rotation(i).0.unwrap(), 13); + + // as with the offset phase the last block of the session must have 100% + // progress. + if P::estimate_next_session_rotation(i).0.unwrap() - 1 == i { + assert_eq!( + P::estimate_current_session_progress(i).0.unwrap(), + Percent::from_percent(100) + ); + } else { + assert!( + P::estimate_current_session_progress(i).0.unwrap() < Percent::from_percent(100) + ); + } } + // the new session starts and we proceed in 1/10 increments. assert!(P::should_end_session(13u64)); - assert_eq!(P::estimate_next_session_rotation(13u64).unwrap(), 23); + assert_eq!(P::estimate_next_session_rotation(13u64).0.unwrap(), 23); + assert_eq!( + P::estimate_current_session_progress(13u64).0.unwrap(), + Percent::from_percent(10) + ); assert!(!P::should_end_session(14u64)); - assert_eq!(P::estimate_next_session_rotation(14u64).unwrap(), 23); + assert_eq!(P::estimate_next_session_rotation(14u64).0.unwrap(), 23); + assert_eq!( + P::estimate_current_session_progress(14u64).0.unwrap(), + Percent::from_percent(20) + ); } #[test] diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 544ae29b0e..05511be63b 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1365,7 +1365,10 @@ decl_module! { // either current session final based on the plan, or we're forcing. (Self::is_current_session_final() || Self::will_era_be_forced()) { - if let Some(next_session_change) = T::NextNewSession::estimate_next_new_session(now) { + let (maybe_next_session_change, estimate_next_new_session_weight) = + T::NextNewSession::estimate_next_new_session(now); + + if let Some(next_session_change) = maybe_next_session_change { if let Some(remaining) = next_session_change.checked_sub(&now) { if remaining <= T::ElectionLookahead::get() && !remaining.is_zero() { // create snapshot. @@ -1387,7 +1390,7 @@ decl_module! { } else { log!(warn, "Estimating next session change failed."); } - add_weight(0, 0, T::NextNewSession::weight(now)) + add_weight(0, 0, estimate_next_new_session_weight) } // For `era_election_status`, `is_current_session_final`, `will_era_be_forced` add_weight(3, 0, 0); @@ -3365,6 +3368,7 @@ impl sp_election_providers::ElectionDataProvider { /// Return the average length of a session. /// /// This may or may not be accurate. fn average_session_length() -> BlockNumber; + /// Return an estimate of the current session progress. + /// + /// None should be returned if the estimation fails to come to an answer. + fn estimate_current_session_progress(now: BlockNumber) -> (Option, Weight); + /// Return the block number at which the next session rotation is estimated to happen. /// - /// None should be returned if the estimation fails to come to an answer - fn estimate_next_session_rotation(now: BlockNumber) -> Option; - - /// Return the weight of calling `estimate_next_session_rotation` - fn weight(now: BlockNumber) -> Weight; + /// None should be returned if the estimation fails to come to an answer. + fn estimate_next_session_rotation(now: BlockNumber) -> (Option, Weight); } -impl EstimateNextSessionRotation for () { +impl EstimateNextSessionRotation for () { fn average_session_length() -> BlockNumber { - Default::default() + Zero::zero() } - fn estimate_next_session_rotation(_: BlockNumber) -> Option { - Default::default() + fn estimate_current_session_progress(_: BlockNumber) -> (Option, Weight) { + (None, Zero::zero()) } - fn weight(_: BlockNumber) -> Weight { - 0 + fn estimate_next_session_rotation(_: BlockNumber) -> (Option, Weight) { + (None, Zero::zero()) } } -/// Something that can estimate at which block the next `new_session` will be triggered. +/// Something that can estimate at which block scheduling of the next session will happen (i.e when +/// we will try to fetch new validators). /// -/// This must always be implemented by the session module. +/// This only refers to the point when we fetch the next session details and not when we enact them +/// (for enactment there's `EstimateNextSessionRotation`). With `pallet-session` this should be +/// triggered whenever `SessionManager::new_session` is called. +/// +/// For example, if we are using a staking module this would be the block when the session module +/// would ask staking what the next validator set will be, as such this must always be implemented +/// by the session module. pub trait EstimateNextNewSession { /// Return the average length of a session. /// @@ -533,23 +547,18 @@ pub trait EstimateNextNewSession { fn average_session_length() -> BlockNumber; /// Return the block number at which the next new session is estimated to happen. - fn estimate_next_new_session(now: BlockNumber) -> Option; - - /// Return the weight of calling `estimate_next_new_session` - fn weight(now: BlockNumber) -> Weight; + /// + /// None should be returned if the estimation fails to come to an answer. + fn estimate_next_new_session(_: BlockNumber) -> (Option, Weight); } -impl EstimateNextNewSession for () { +impl EstimateNextNewSession for () { fn average_session_length() -> BlockNumber { - Default::default() + Zero::zero() } - fn estimate_next_new_session(_: BlockNumber) -> Option { - Default::default() - } - - fn weight(_: BlockNumber) -> Weight { - 0 + fn estimate_next_new_session(_: BlockNumber) -> (Option, Weight) { + (None, Zero::zero()) } }