Ensure timestamp config makes sense. (#2132)

* Timestamp period should be expressed as expected period

* Fix test

* Ensure value never illegal

* Rename

* Remove println

* Add default

* Comment

* Fix, bump and rebuild wasm

* Fix test

* Add TODOs

* Configure only minimum_period.

* Fix

* Update srml/timestamp/src/lib.rs

Co-Authored-By: gavofyork <github@gavwood.com>

* Update srml/timestamp/src/lib.rs

Co-Authored-By: gavofyork <github@gavwood.com>

substrate/node-template/src/chain_spec.rs

@@ -98,7 +98,7 @@ fn testnet_genesis(initial_authorities: Vec<AuthorityId>, endowed_accounts: Vec<
 		}),
 		system: None,
 		timestamp: Some(TimestampConfig {
-			period: 5,					// 5 second block time.
+			minimum_period: 5, // 10 second block time.
 		}),
 		indices: Some(IndicesConfig {
 			ids: endowed_accounts.clone(),

substrate/node/cli/src/chain_spec.rs

@@ -141,7 +141,7 @@ fn staging_testnet_config_genesis() -> GenesisConfig {
 			enact_delay_period: 0,
 		}),
 		timestamp: Some(TimestampConfig {
-			period: SECS_PER_BLOCK / 2, // due to the nature of aura the slots are 2*period
+			minimum_period: SECS_PER_BLOCK / 2, // due to the nature of aura the slots are 2*period
 		}),
 		treasury: Some(TreasuryConfig {
 			proposal_bond: Permill::from_percent(5),
@@ -298,7 +298,7 @@ pub fn testnet_genesis(
 			enact_delay_period: 0,
 		}),
 		timestamp: Some(TimestampConfig {
-			period: 2,                    // 2*2=4 second block time.
+			minimum_period: 2,                    // 2*2=4 second block time.
 		}),
 		treasury: Some(TreasuryConfig {
 			proposal_bond: Permill::from_percent(5),
@@ -367,7 +367,7 @@ mod tests {
 
 	fn local_testnet_genesis_instant() -> GenesisConfig {
 		let mut genesis = local_testnet_genesis();
-		genesis.timestamp = Some(TimestampConfig { period: 0 });
+		genesis.timestamp = Some(TimestampConfig { minimum_period: 1 });
 		genesis
 	}
 

substrate/node/runtime/src/lib.rs

@@ -58,7 +58,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 46,
+	spec_version: 47,
 	impl_version: 47,
 	apis: RUNTIME_API_VERSIONS,
 };

substrate/srml/aura/src/lib.rs

@@ -165,7 +165,7 @@ impl<T: Trait> Module<T> {
 	pub fn slot_duration() -> u64 {
 		// we double the minimum block-period so each author can always propose within
 		// the majority of their slot.
-		<timestamp::Module<T>>::block_period().as_().saturating_mul(2)
+		<timestamp::Module<T>>::minimum_period().as_().saturating_mul(2)
 	}
 
 	fn on_timestamp_set<H: HandleReport>(now: T::Moment, slot_duration: T::Moment) {

substrate/srml/aura/src/mock.rs

@@ -68,7 +68,7 @@ pub fn new_test_ext(authorities: Vec<u64>) -> runtime_io::TestExternalities<Blak
 		authorities: authorities.into_iter().map(|a| UintAuthorityId(a)).collect(),
 	}.build_storage().unwrap().0);
 	t.extend(timestamp::GenesisConfig::<Test>{
-		period: 1,
+		minimum_period: 1,
 	}.build_storage().unwrap().0);
 	t.into()
 }

substrate/srml/session/src/lib.rs

@@ -200,7 +200,7 @@ impl<T: Trait> Module<T> {
 
 	/// Get the time that should have elapsed over a session if everything was working perfectly.
 	pub fn ideal_session_duration() -> T::Moment {
-		let block_period: T::Moment = <timestamp::Module<T>>::block_period();
+		let block_period: T::Moment = <timestamp::Module<T>>::minimum_period();
 		let session_length: T::BlockNumber = Self::length();
 		Mul::<T::BlockNumber>::mul(block_period, session_length)
 	}
@@ -289,7 +289,7 @@ mod tests {
 			authorities: NEXT_VALIDATORS.with(|l| l.borrow().iter().cloned().map(UintAuthorityId).collect()),
 		}.build_storage().unwrap().0);
 		t.extend(timestamp::GenesisConfig::<Test>{
-			period: 5,
+			minimum_period: 5,
 		}.build_storage().unwrap().0);
 		t.extend(GenesisConfig::<Test>{
 			session_length: 2,

substrate/srml/staking/src/mock.rs

@@ -218,7 +218,7 @@ impl ExtBuilder {
 			invulnerables: vec![],
 		}.assimilate_storage(&mut t, &mut c);
 		let _ = timestamp::GenesisConfig::<Test>{
-			period: 5,
+			minimum_period: 5,
 		}.assimilate_storage(&mut t, &mut c);
 		t.into()
 	}

substrate/srml/timestamp/src/lib.rs

@@ -40,7 +40,7 @@
 //! 
 //! * `get` - Gets the current time for the current block. If this function is called prior the setting to timestamp, it will return the timestamp of the previous block.
 //! 
-//! * `block_period` - Gets the minimum (and advised) period between blocks for the chain.
+//! * `minimum_period` - Gets the minimum (and advised) period between blocks for the chain.
 //! 
 //! ## Usage
 //! 
@@ -211,15 +211,15 @@ decl_module! {
 		/// This call should be invoked exactly once per block. It will panic at the finalization phase,
 		/// if this call hasn't been invoked by that time.
 		///
-		/// The timestamp should be greater than the previous one by the amount specified by `block_period`.
+		/// The timestamp should be greater than the previous one by the amount specified by `minimum_period`.
 		/// 
 		/// The dispatch origin for this call must be `Inherent`.
 		fn set(origin, #[compact] now: T::Moment) {
 			ensure_inherent(origin)?;
 			assert!(!<Self as Store>::DidUpdate::exists(), "Timestamp must be updated only once in the block");
 			assert!(
-				Self::now().is_zero() || now >= Self::now() + Self::block_period(),
-				"Timestamp must increment by at least <BlockPeriod> between sequential blocks"
+				Self::now().is_zero() || now >= Self::now() + <MinimumPeriod<T>>::get(),
+				"Timestamp must increment by at least <MinimumPeriod> between sequential blocks"
 			);
 			<Self as Store>::Now::put(now.clone());
 			<Self as Store>::DidUpdate::put(true);
@@ -227,6 +227,16 @@ decl_module! {
 			<T::OnTimestampSet as OnTimestampSet<_>>::on_timestamp_set(now);
 		}
 
+		// Manage upgrade. Remove after all networks upgraded.
+		// TODO: #2133
+		fn on_initialise() {
+			if let Some(period) = <BlockPeriod<T>>::take() {
+				if !<MinimumPeriod<T>>::exists() {
+					<MinimumPeriod<T>>::put(period)
+				}
+			}
+		}
+
 		fn on_finalise() {
 			assert!(<Self as Store>::DidUpdate::take(), "Timestamp must be updated once in the block");
 		}
@@ -238,8 +248,15 @@ decl_storage! {
 		/// Current time for the current block.
 		pub Now get(now) build(|_| T::Moment::sa(0)): T::Moment;
 
-		/// The minimum (and advised) period between blocks.
-		pub BlockPeriod get(block_period) config(period): T::Moment = T::Moment::sa(5);
+		/// Old storage item provided for compatibility. Remove after all networks upgraded.
+		// TODO: #2133
+		pub BlockPeriod: Option<T::Moment>;
+
+		/// The minimum period between blocks. Beware that this is different to the *expected* period
+		/// that the block production apparatus provides. Your chosen consensus system will generally
+		/// work with this to determine a sensible block time. e.g. For Aura, it will be double this
+		/// period on default settings.
+		pub MinimumPeriod get(minimum_period) config(): T::Moment = T::Moment::sa(3);
 
 		/// Did the timestamp get updated in this block?
 		DidUpdate: bool;
@@ -276,7 +293,7 @@ impl<T: Trait> ProvideInherent for Module<T> {
 	fn create_inherent(data: &InherentData) -> Option<Self::Call> {
 		let data = extract_inherent_data(data).expect("Gets and decodes timestamp inherent data");
 
-		let next_time = cmp::max(As::sa(data), Self::now() + Self::block_period());
+		let next_time = cmp::max(As::sa(data), Self::now() + <MinimumPeriod<T>>::get());
 		Some(Call::set(next_time.into()))
 	}
 
@@ -290,7 +307,7 @@ impl<T: Trait> ProvideInherent for Module<T> {
 
 		let data = extract_inherent_data(data).map_err(|e| InherentError::Other(e))?;
 
-		let minimum = (Self::now() + Self::block_period()).as_();
+		let minimum = (Self::now() + <MinimumPeriod<T>>::get()).as_();
 		if t > data + MAX_TIMESTAMP_DRIFT {
 			Err(InherentError::Other("Timestamp too far in future to accept".into()))
 		} else if t < minimum {
@@ -341,7 +358,7 @@ mod tests {
 	fn timestamp_works() {
 		let mut t = system::GenesisConfig::<Test>::default().build_storage().unwrap().0;
 		t.extend(GenesisConfig::<Test> {
-			period: 5,
+			minimum_period: 5,
 		}.build_storage().unwrap().0);
 
 		with_externalities(&mut TestExternalities::new(t), || {
@@ -356,7 +373,7 @@ mod tests {
 	fn double_timestamp_should_fail() {
 		let mut t = system::GenesisConfig::<Test>::default().build_storage().unwrap().0;
 		t.extend(GenesisConfig::<Test> {
-			period: 5,
+			minimum_period: 5,
 		}.build_storage().unwrap().0);
 
 		with_externalities(&mut TestExternalities::new(t), || {
@@ -367,11 +384,11 @@ mod tests {
 	}
 
 	#[test]
-	#[should_panic(expected = "Timestamp must increment by at least <BlockPeriod> between sequential blocks")]
-	fn block_period_is_enforced() {
+	#[should_panic(expected = "Timestamp must increment by at least <MinimumPeriod> between sequential blocks")]
+	fn block_period_minimum_enforced() {
 		let mut t = system::GenesisConfig::<Test>::default().build_storage().unwrap().0;
 		t.extend(GenesisConfig::<Test> {
-			period: 5,
+			minimum_period: 5,
 		}.build_storage().unwrap().0);
 
 		with_externalities(&mut TestExternalities::new(t), || {