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

…#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 <[email protected]> * 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 <[email protected]>
paritytech · Mar 10, 2021 · 2461349 · 2461349
1 parent b6c2c5d
commit 2461349
Show file tree

Hide file tree

Showing 9 changed files with 187 additions and 17 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs
@@ -305,6 +305,7 @@ pub fn testnet_genesis(
  },
  pallet_babe: BabeConfig {
  authorities: vec![],
+ epoch_config: Some(node_runtime::BABE_GENESIS_EPOCH_CONFIG),
  },
  pallet_im_online: ImOnlineConfig {
  keys: vec![],

diff --git a/bin/node/runtime/src/lib.rs b/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 {
@@ -1274,10 +1281,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,
  }
  }
 

diff --git a/bin/node/testing/src/genesis.rs b/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;
@@ -100,7 +100,10 @@ pub fn config_endowed(
  pallet_contracts: ContractsConfig {
  current_schedule: Default::default(),
  },
- pallet_babe: Default::default(),
+ pallet_babe: BabeConfig {
+ authorities: vec![],
+ epoch_config: Some(BABE_GENESIS_EPOCH_CONFIG),
+ },
  pallet_grandpa: GrandpaConfig {
  authorities: vec![],
  },

diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs
@@ -517,6 +517,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(),
  })
  };
 

diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs
@@ -42,7 +42,8 @@ use sp_timestamp::OnTimestampSet;
 
 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;
 
@@ -187,8 +188,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;
@@ -225,10 +226,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"));
+ })
  }
 }
 
@@ -326,7 +338,7 @@ decl_module! {
  config: NextConfigDescriptor,
  ) {
  ensure_root(origin)?;
- NextEpochConfig::put(config);
+ PendingEpochConfigChange::put(config);
  }
  }
 }
@@ -490,8 +502,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));
  }
  }
 
@@ -510,6 +530,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"),
  }
  }
 
@@ -527,6 +548,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")
+ }),
  }
  }
 
@@ -835,3 +859,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)
+ }
+}
diff --git a/frame/babe/src/tests.rs b/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));
+ });
+}
diff --git a/primitives/consensus/babe/Cargo.toml b/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",
 ]
diff --git a/primitives/consensus/babe/src/lib.rs b/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! {