Return babe configuration information in the babe api epoch functions (…

…paritytech#8072)

* Make changes

* Add serialize/deserialize, copy babe epoch config defaults from node runtime

* Fix line widths and turn default features off for serde

* Remove ser/deser from Epoch, fix node-cli

* Apply suggestions

* Add comment to BABE_GENESIS_EPOCH_CONFIG in bin

* Apply suggestions

* Add a sketchy migration function

* Add a migration test

* Check for PendingEpochConfigChange as well

* Make epoch_config in node-cli

* Move updating EpochConfig out of the if

* Fix executor tests

* Calculate weight for add_epoch_configurations

* Fix babe test

* Apply suggestions from code review

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

* Add more asserts to tests, remove unused changes to primitives/slots

* Allow setting the migration pallet prefix

* Rename to BabePalletPrefix

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

bin/node/cli/src/chain_spec.rs

@@ -305,8 +305,9 @@ pub fn testnet_genesis(
 		}),
 		pallet_babe: Some(BabeConfig {
 			authorities: vec![],
-		}),
-		pallet_im_online: Some(ImOnlineConfig {
+			epoch_config: Some(node_runtime::BABE_GENESIS_EPOCH_CONFIG),
+		},
+		pallet_im_online: ImOnlineConfig {
 			keys: vec![],
 		}),
 		pallet_authority_discovery: Some(AuthorityDiscoveryConfig {

bin/node/runtime/src/lib.rs

@@ -119,6 +119,13 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	transaction_version: 2,
 };
 
+/// The BABE epoch configuration at genesis.
+pub const BABE_GENESIS_EPOCH_CONFIG: sp_consensus_babe::BabeEpochConfiguration =
+	sp_consensus_babe::BabeEpochConfiguration {
+		c: PRIMARY_PROBABILITY,
+		allowed_slots: sp_consensus_babe::AllowedSlots::PrimaryAndSecondaryPlainSlots
+	};
+
 /// Native version.
 #[cfg(any(feature = "std", test))]
 pub fn native_version() -> NativeVersion {
@@ -1272,10 +1279,10 @@ impl_runtime_apis! {
 			sp_consensus_babe::BabeGenesisConfiguration {
 				slot_duration: Babe::slot_duration(),
 				epoch_length: EpochDuration::get(),
-				c: PRIMARY_PROBABILITY,
+				c: BABE_GENESIS_EPOCH_CONFIG.c,
 				genesis_authorities: Babe::authorities(),
 				randomness: Babe::randomness(),
-				allowed_slots: sp_consensus_babe::AllowedSlots::PrimaryAndSecondaryPlainSlots,
+				allowed_slots: BABE_GENESIS_EPOCH_CONFIG.allowed_slots,
 			}
 		}
 

bin/node/testing/src/genesis.rs

@@ -23,7 +23,7 @@ use sp_keyring::{Ed25519Keyring, Sr25519Keyring};
 use node_runtime::{
 	GenesisConfig, BalancesConfig, SessionConfig, StakingConfig, SystemConfig,
 	GrandpaConfig, IndicesConfig, ContractsConfig, SocietyConfig, wasm_binary_unwrap,
-	AccountId, StakerStatus,
+	AccountId, StakerStatus, BabeConfig, BABE_GENESIS_EPOCH_CONFIG,
 };
 use node_runtime::constants::currency::*;
 use sp_core::ChangesTrieConfiguration;
@@ -99,9 +99,12 @@ pub fn config_endowed(
 		}),
 		pallet_contracts: Some(ContractsConfig {
 			current_schedule: Default::default(),
-		}),
-		pallet_babe: Some(Default::default()),
-		pallet_grandpa: Some(GrandpaConfig {
+		},
+		pallet_babe: BabeConfig {
+			authorities: vec![],
+			epoch_config: Some(BABE_GENESIS_EPOCH_CONFIG),
+		},
+		pallet_grandpa: GrandpaConfig {
 			authorities: vec![],
 		}),
 		pallet_im_online: Some(Default::default()),

client/consensus/babe/src/lib.rs

@@ -523,6 +523,7 @@ async fn answer_requests<B: BlockT, C>(
 						duration: viable_epoch.as_ref().duration,
 						authorities: viable_epoch.as_ref().authorities.clone(),
 						randomness: viable_epoch.as_ref().randomness,
+						config: viable_epoch.as_ref().config.clone(),
 					})
 				};
 

frame/babe/src/lib.rs

@@ -41,7 +41,8 @@ use sp_std::prelude::*;
 
 use sp_consensus_babe::{
 	digests::{NextConfigDescriptor, NextEpochDescriptor, PreDigest},
-	BabeAuthorityWeight, ConsensusLog, Epoch, EquivocationProof, Slot, BABE_ENGINE_ID,
+	BabeAuthorityWeight, BabeEpochConfiguration, ConsensusLog, Epoch,
+	EquivocationProof, Slot, BABE_ENGINE_ID,
 };
 use sp_consensus_vrf::schnorrkel;
 
@@ -186,8 +187,8 @@ decl_storage! {
 		// variable to its underlying value.
 		pub Randomness get(fn randomness): schnorrkel::Randomness;
 
-		/// Next epoch configuration, if changed.
-		NextEpochConfig: Option<NextConfigDescriptor>;
+		/// Pending epoch configuration change that will be applied when the next epoch is enacted.
+		PendingEpochConfigChange: Option<NextConfigDescriptor>;
 
 		/// Next epoch randomness.
 		NextRandomness: schnorrkel::Randomness;
@@ -224,10 +225,21 @@ decl_storage! {
 		/// on block finalization. Querying this storage entry outside of block
 		/// execution context should always yield zero.
 		Lateness get(fn lateness): T::BlockNumber;
+
+		/// The configuration for the current epoch. Should never be `None` as it is initialized in genesis.
+		EpochConfig: Option<BabeEpochConfiguration>;
+
+		/// The configuration for the next epoch, `None` if the config will not change
+		/// (you can fallback to `EpochConfig` instead in that case).
+		NextEpochConfig: Option<BabeEpochConfiguration>;
 	}
 	add_extra_genesis {
 		config(authorities): Vec<(AuthorityId, BabeAuthorityWeight)>;
-		build(|config| Module::<T>::initialize_authorities(&config.authorities))
+		config(epoch_config): Option<BabeEpochConfiguration>;
+		build(|config| {
+			Module::<T>::initialize_authorities(&config.authorities);
+			EpochConfig::put(config.epoch_config.clone().expect("epoch_config must not be None"));
+		})
 	}
 }
 
@@ -325,7 +337,7 @@ decl_module! {
 			config: NextConfigDescriptor,
 		) {
 			ensure_root(origin)?;
-			NextEpochConfig::put(config);
+			PendingEpochConfigChange::put(config);
 		}
 	}
 }
@@ -489,8 +501,16 @@ impl<T: Config> Module<T> {
 		};
 		Self::deposit_consensus(ConsensusLog::NextEpochData(next_epoch));
 
-		if let Some(next_config) = NextEpochConfig::take() {
-			Self::deposit_consensus(ConsensusLog::NextConfigData(next_config));
+		if let Some(next_config) = NextEpochConfig::get() {
+			EpochConfig::put(next_config);
+		}
+
+		if let Some(pending_epoch_config_change) = PendingEpochConfigChange::take() {
+			let next_epoch_config: BabeEpochConfiguration =
+				pending_epoch_config_change.clone().into();
+			NextEpochConfig::put(next_epoch_config);
+
+			Self::deposit_consensus(ConsensusLog::NextConfigData(pending_epoch_config_change));
 		}
 	}
 
@@ -509,6 +529,7 @@ impl<T: Config> Module<T> {
 			duration: T::EpochDuration::get(),
 			authorities: Self::authorities(),
 			randomness: Self::randomness(),
+			config: EpochConfig::get().expect("EpochConfig is initialized in genesis; we never `take` or `kill` it; qed"),
 		}
 	}
 
@@ -526,6 +547,9 @@ impl<T: Config> Module<T> {
 			duration: T::EpochDuration::get(),
 			authorities: NextAuthorities::get(),
 			randomness: NextRandomness::get(),
+			config: NextEpochConfig::get().unwrap_or_else(|| {
+				EpochConfig::get().expect("EpochConfig is initialized in genesis; we never `take` or `kill` it; qed")
+			}),
 		}
 	}
 
@@ -834,3 +858,49 @@ fn compute_randomness(
 
 	sp_io::hashing::blake2_256(&s)
 }
+
+pub mod migrations {
+	use super::*;
+	use frame_support::pallet_prelude::{ValueQuery, StorageValue};
+
+	/// Something that can return the storage prefix of the `Babe` pallet.
+	pub trait BabePalletPrefix: Config {
+		fn pallet_prefix() -> &'static str;
+	}
+
+	struct __OldNextEpochConfig<T>(sp_std::marker::PhantomData<T>);
+	impl<T: BabePalletPrefix> frame_support::traits::StorageInstance for __OldNextEpochConfig<T> {
+		fn pallet_prefix() -> &'static str { T::pallet_prefix() }
+		const STORAGE_PREFIX: &'static str = "NextEpochConfig";
+	}
+
+	type OldNextEpochConfig<T> = StorageValue<
+		__OldNextEpochConfig<T>, Option<NextConfigDescriptor>, ValueQuery
+	>;
+
+	/// A storage migration that adds the current epoch configuration for Babe
+	/// to storage.
+	pub fn add_epoch_configuration<T: BabePalletPrefix>(
+		epoch_config: BabeEpochConfiguration,
+	) -> Weight {
+		let mut writes = 0;
+		let mut reads = 0;
+
+		if let Some(pending_change) = OldNextEpochConfig::<T>::get() {
+			PendingEpochConfigChange::put(pending_change);
+
+			writes += 1;
+		}
+
+		reads += 1;
+
+		OldNextEpochConfig::<T>::kill();
+
+		EpochConfig::put(epoch_config.clone());
+		NextEpochConfig::put(epoch_config);
+
+		writes += 3;
+
+		T::DbWeight::get().writes(writes) + T::DbWeight::get().reads(reads)
+	}
+}

frame/babe/src/tests.rs

@@ -25,7 +25,7 @@ use frame_support::{
 };
 use mock::*;
 use pallet_session::ShouldEndSession;
-use sp_consensus_babe::{AllowedSlots, Slot};
+use sp_consensus_babe::{AllowedSlots, Slot, BabeEpochConfiguration};
 use sp_core::crypto::Pair;
 
 const EMPTY_RANDOMNESS: [u8; 32] = [
@@ -231,22 +231,45 @@ fn can_enact_next_config() {
 		assert_eq!(Babe::epoch_index(), 0);
 		go_to_block(2, 7);
 
+		let current_config = BabeEpochConfiguration {
+			c: (0, 4),
+			allowed_slots: sp_consensus_babe::AllowedSlots::PrimarySlots,
+		};
+
+		let next_config = BabeEpochConfiguration {
+			c: (1, 4),
+			allowed_slots: sp_consensus_babe::AllowedSlots::PrimarySlots,
+		};
+
+		let next_next_config = BabeEpochConfiguration {
+			c: (2, 4),
+			allowed_slots: sp_consensus_babe::AllowedSlots::PrimarySlots,
+		};
+
+		EpochConfig::put(current_config);
+		NextEpochConfig::put(next_config.clone());
+
+		assert_eq!(NextEpochConfig::get(), Some(next_config.clone()));
+
 		Babe::plan_config_change(
 			Origin::root(),
 			NextConfigDescriptor::V1 {
-				c: (1, 4),
-				allowed_slots: AllowedSlots::PrimarySlots,
+				c: next_next_config.c,
+				allowed_slots: next_next_config.allowed_slots,
 			},
 		).unwrap();
 
 		progress_to_block(4);
 		Babe::on_finalize(9);
 		let header = System::finalize();
 
+		assert_eq!(EpochConfig::get(), Some(next_config));
+		assert_eq!(NextEpochConfig::get(), Some(next_next_config.clone()));
+
 		let consensus_log = sp_consensus_babe::ConsensusLog::NextConfigData(
-			sp_consensus_babe::digests::NextConfigDescriptor::V1 {
-				c: (1, 4),
-				allowed_slots: AllowedSlots::PrimarySlots,
+			NextConfigDescriptor::V1 {
+				c: next_next_config.c,
+				allowed_slots: next_next_config.allowed_slots,
 			}
 		);
 		let consensus_digest = DigestItem::Consensus(BABE_ENGINE_ID, consensus_log.encode());
@@ -291,6 +314,11 @@ fn only_root_can_enact_config_change() {
 #[test]
 fn can_fetch_current_and_next_epoch_data() {
 	new_test_ext(5).execute_with(|| {
+		EpochConfig::put(BabeEpochConfiguration {
+			c: (1, 4),
+			allowed_slots: sp_consensus_babe::AllowedSlots::PrimarySlots,
+		});
+
 		// genesis authorities should be used for the first and second epoch
 		assert_eq!(
 			Babe::current_epoch().authorities,
@@ -809,3 +837,54 @@ fn valid_equivocation_reports_dont_pay_fees() {
 		assert_eq!(post_info.pays_fee, Pays::Yes);
 	})
 }
+
+#[test]
+fn add_epoch_configurations_migration_works() {
+	use frame_support::storage::migration::{
+		put_storage_value, get_storage_value,
+	};
+
+	impl crate::migrations::BabePalletPrefix for Test {
+		fn pallet_prefix() -> &'static str {
+			"Babe"
+		}
+	}
+
+	new_test_ext(1).execute_with(|| {
+		let next_config_descriptor = NextConfigDescriptor::V1 {
+			c: (3, 4),
+			allowed_slots: AllowedSlots::PrimarySlots
+		};
+
+		put_storage_value(
+			b"Babe",
+			b"NextEpochConfig",
+			&[],
+			Some(next_config_descriptor.clone())
+		);
+
+		assert!(get_storage_value::<Option<NextConfigDescriptor>>(
+			b"Babe",
+			b"NextEpochConfig",
+			&[],
+		).is_some());
+
+		let current_epoch = BabeEpochConfiguration {
+			c: (1, 4),
+			allowed_slots: sp_consensus_babe::AllowedSlots::PrimarySlots,
+		};
+
+		crate::migrations::add_epoch_configuration::<Test>(
+			current_epoch.clone()
+		);
+
+		assert!(get_storage_value::<Option<NextConfigDescriptor>>(
+			b"Babe",
+			b"NextEpochConfig",
+			&[],
+		).is_none());
+
+		assert_eq!(EpochConfig::get(), Some(current_epoch));
+		assert_eq!(PendingEpochConfigChange::get(), Some(next_config_descriptor));
+	});
+}

primitives/consensus/babe/Cargo.toml

@@ -26,6 +26,7 @@ sp-inherents = { version = "3.0.0", default-features = false, path = "../../inhe
 sp-keystore = { version = "0.9.0", default-features = false, path = "../../keystore", optional = true }
 sp-runtime = { version = "3.0.0", default-features = false, path = "../../runtime" }
 sp-timestamp = { version = "3.0.0", default-features = false, path = "../../timestamp" }
+serde = { version = "1.0.123", features = ["derive"], optional = true }
 
 [features]
 default = ["std"]
@@ -43,4 +44,5 @@ std = [
 	"sp-keystore",
 	"sp-runtime/std",
 	"sp-timestamp/std",
+	"serde",
 ]

primitives/consensus/babe/src/lib.rs

@@ -30,6 +30,8 @@ pub use sp_consensus_vrf::schnorrkel::{
 
 use codec::{Decode, Encode};
 #[cfg(feature = "std")]
+use serde::{Serialize, Deserialize};
+#[cfg(feature = "std")]
 use sp_keystore::vrf::{VRFTranscriptData, VRFTranscriptValue};
 use sp_runtime::{traits::Header, ConsensusEngineId, RuntimeDebug};
 use sp_std::vec::Vec;
@@ -216,6 +218,7 @@ pub struct BabeGenesisConfiguration {
 
 /// Types of allowed slots.
 #[derive(Clone, Copy, PartialEq, Eq, Encode, Decode, RuntimeDebug)]
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 pub enum AllowedSlots {
 	/// Only allow primary slots.
 	PrimarySlots,
@@ -248,6 +251,7 @@ impl sp_consensus::SlotData for BabeGenesisConfiguration {
 
 /// Configuration data used by the BABE consensus engine.
 #[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)]
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 pub struct BabeEpochConfiguration {
 	/// A constant value that is used in the threshold calculation formula.
 	/// Expressed as a rational where the first member of the tuple is the
@@ -362,6 +366,8 @@ pub struct Epoch {
 	pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>,
 	/// Randomness for this epoch.
 	pub randomness: [u8; VRF_OUTPUT_LENGTH],
+	/// Configuration of the epoch.
+	pub config: BabeEpochConfiguration,
 }
 
 sp_api::decl_runtime_apis! {