Fix periodic sessions and document behavior (#2974)

* Fix periodic sessions and document behavior

* bump versions and test

* Apply suggestions from code review

* update docs

* update docs again

substrate/node/runtime/src/lib.rs

@@ -67,7 +67,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// to equal spec_version. If only runtime implementation changes and behavior does not, then
 	// leave spec_version as is and increment impl_version.
 	spec_version: 102,
-	impl_version: 103,
+	impl_version: 104,
 	apis: RUNTIME_API_VERSIONS,
 };
 

substrate/srml/session/src/lib.rs

@@ -115,11 +115,11 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use rstd::{prelude::*, marker::PhantomData, ops::Rem};
+use rstd::{prelude::*, marker::PhantomData, ops::{Sub, Rem}};
 #[cfg(not(feature = "std"))]
 use rstd::alloc::borrow::ToOwned;
 use parity_codec::Decode;
-use primitives::traits::{Zero, Saturating, Member, OpaqueKeys};
+use primitives::traits::{Zero, Member, OpaqueKeys};
 use srml_support::{
 	ConsensusEngineId, StorageValue, StorageMap, for_each_tuple, decl_module,
 	decl_event, decl_storage,
@@ -134,18 +134,24 @@ pub trait ShouldEndSession<BlockNumber> {
 	fn should_end_session(now: BlockNumber) -> bool;
 }
 
+/// Ends the session after a fixed period of blocks.
+///
+/// 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)>);
 
 impl<
-	BlockNumber: Rem<Output=BlockNumber> + Saturating + Zero,
+	BlockNumber: Rem<Output=BlockNumber> + Sub<Output=BlockNumber> + Zero + PartialOrd,
 	Period: Get<BlockNumber>,
 	Offset: Get<BlockNumber>,
 > ShouldEndSession<BlockNumber> for PeriodicSessions<Period, Offset> {
 	fn should_end_session(now: BlockNumber) -> bool {
-		((now.saturating_sub(Offset::get())) % Period::get()).is_zero()
+		let offset = Offset::get();
+		now >= offset && ((now - offset) % Period::get()).is_zero()
 	}
 }
 
@@ -590,4 +596,33 @@ mod tests {
 			assert_eq!(authorities(), vec![UintAuthorityId(1), UintAuthorityId(5), UintAuthorityId(3)]);
 		});
 	}
+
+	#[test]
+	fn periodic_session_works() {
+		struct Period;
+		struct Offset;
+
+		impl Get<u64> for Period {
+			fn get() -> u64 { 10 }
+		}
+
+		impl Get<u64> for Offset {
+			fn get() -> u64 { 3 }
+		}
+
+
+		type P = PeriodicSessions<Period, Offset>;
+
+		for i in 0..3 {
+			assert!(!P::should_end_session(i));
+		}
+
+		assert!(P::should_end_session(3));
+
+		for i in (1..10).map(|i| 3 + i) {
+			assert!(!P::should_end_session(i));
+		}
+
+		assert!(P::should_end_session(13));
+	}
 }