Babe epoch newtype (paritytech#1596)

Removal of verbatim duplication of BABE's `Epoch` struct in the client.

I think is better to have one single definition and wrap the primitive
`Epoch` in a newtype (required because we need to implement the `Epoch`
trait).

substrate/client/consensus/babe/Cargo.toml

@@ -15,7 +15,6 @@ targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
 async-trait = "0.1.57"
-scale-info = { version = "2.5.0", features = ["derive"] }
 codec = { package = "parity-scale-codec", version = "3.6.1", features = ["derive"] }
 futures = "0.3.21"
 log = "0.4.17"
@@ -45,10 +44,8 @@ sp-keystore = { path = "../../../primitives/keystore" }
 sp-runtime = { path = "../../../primitives/runtime" }
 
 [dev-dependencies]
-rand_chacha = "0.2.2"
 sc-block-builder = { path = "../../block-builder" }
 sp-keyring = { path = "../../../primitives/keyring" }
-sc-network = { path = "../../network" }
 sc-network-test = { path = "../../network/test" }
 sp-timestamp = { path = "../../../primitives/timestamp" }
 sp-tracing = { path = "../../../primitives/tracing" }

substrate/client/consensus/babe/src/authorship.rs

@@ -132,23 +132,22 @@ fn claim_secondary_slot(
 	keystore: &KeystorePtr,
 	author_secondary_vrf: bool,
 ) -> Option<(PreDigest, AuthorityId)> {
-	let Epoch { authorities, randomness, mut epoch_index, .. } = epoch;
-
-	if authorities.is_empty() {
+	if epoch.authorities.is_empty() {
 		return None
 	}
 
+	let mut epoch_index = epoch.epoch_index;
 	if epoch.end_slot() <= slot {
 		// Slot doesn't strictly belong to the epoch, create a clone with fixed values.
 		epoch_index = epoch.clone_for_slot(slot).epoch_index;
 	}
 
-	let expected_author = secondary_slot_author(slot, authorities, *randomness)?;
+	let expected_author = secondary_slot_author(slot, &epoch.authorities, epoch.randomness)?;
 
 	for (authority_id, authority_index) in keys {
 		if authority_id == expected_author {
 			let pre_digest = if author_secondary_vrf {
-				let data = make_vrf_sign_data(randomness, slot, epoch_index);
+				let data = make_vrf_sign_data(&epoch.randomness, slot, epoch_index);
 				let result =
 					keystore.sr25519_vrf_sign(AuthorityId::ID, authority_id.as_ref(), &data);
 				if let Ok(Some(vrf_signature)) = result {
@@ -232,19 +231,18 @@ fn claim_primary_slot(
 	keystore: &KeystorePtr,
 	keys: &[(AuthorityId, usize)],
 ) -> Option<(PreDigest, AuthorityId)> {
-	let Epoch { authorities, randomness, mut epoch_index, .. } = epoch;
-
+	let mut epoch_index = epoch.epoch_index;
 	if epoch.end_slot() <= slot {
 		// Slot doesn't strictly belong to the epoch, create a clone with fixed values.
 		epoch_index = epoch.clone_for_slot(slot).epoch_index;
 	}
 
-	let data = make_vrf_sign_data(randomness, slot, epoch_index);
+	let data = make_vrf_sign_data(&epoch.randomness, slot, epoch_index);
 
 	for (authority_id, authority_index) in keys {
 		let result = keystore.sr25519_vrf_sign(AuthorityId::ID, authority_id.as_ref(), &data);
 		if let Ok(Some(vrf_signature)) = result {
-			let threshold = calculate_primary_threshold(c, authorities, *authority_index);
+			let threshold = calculate_primary_threshold(c, &epoch.authorities, *authority_index);
 
 			let can_claim = authority_id
 				.as_inner_ref()
@@ -274,7 +272,7 @@ fn claim_primary_slot(
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use sp_consensus_babe::{AllowedSlots, AuthorityId, BabeEpochConfiguration};
+	use sp_consensus_babe::{AllowedSlots, AuthorityId, BabeEpochConfiguration, Epoch};
 	use sp_core::{crypto::Pair as _, sr25519::Pair};
 	use sp_keystore::testing::MemoryKeystore;
 
@@ -300,7 +298,8 @@ mod tests {
 				c: (3, 10),
 				allowed_slots: AllowedSlots::PrimaryAndSecondaryPlainSlots,
 			},
-		};
+		}
+		.into();
 
 		assert!(claim_slot(10.into(), &epoch, &keystore).is_none());
 

substrate/client/consensus/babe/src/lib.rs

@@ -69,6 +69,7 @@
 use std::{
 	collections::HashSet,
 	future::Future,
+	ops::{Deref, DerefMut},
 	pin::Pin,
 	sync::Arc,
 	task::{Context, Poll},
@@ -156,20 +157,27 @@ const AUTHORING_SCORE_VRF_CONTEXT: &[u8] = b"substrate-babe-vrf";
 const AUTHORING_SCORE_LENGTH: usize = 16;
 
 /// BABE epoch information
-#[derive(Decode, Encode, PartialEq, Eq, Clone, Debug, scale_info::TypeInfo)]
-pub struct Epoch {
-	/// The epoch index.
-	pub epoch_index: u64,
-	/// The starting slot of the epoch.
-	pub start_slot: Slot,
-	/// The duration of this epoch.
-	pub duration: u64,
-	/// The authorities and their weights.
-	pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>,
-	/// Randomness for this epoch.
-	pub randomness: Randomness,
-	/// Configuration of the epoch.
-	pub config: BabeEpochConfiguration,
+#[derive(Clone, Debug, PartialEq, Eq, Encode, Decode)]
+pub struct Epoch(sp_consensus_babe::Epoch);
+
+impl Deref for Epoch {
+	type Target = sp_consensus_babe::Epoch;
+
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
+}
+
+impl DerefMut for Epoch {
+	fn deref_mut(&mut self) -> &mut Self::Target {
+		&mut self.0
+	}
+}
+
+impl From<sp_consensus_babe::Epoch> for Epoch {
+	fn from(epoch: sp_consensus_babe::Epoch) -> Self {
+		Epoch(epoch)
+	}
 }
 
 impl EpochT for Epoch {
@@ -180,14 +188,15 @@ impl EpochT for Epoch {
 		&self,
 		(descriptor, config): (NextEpochDescriptor, BabeEpochConfiguration),
 	) -> Epoch {
-		Epoch {
+		sp_consensus_babe::Epoch {
 			epoch_index: self.epoch_index + 1,
 			start_slot: self.start_slot + self.duration,
 			duration: self.duration,
 			authorities: descriptor.authorities,
 			randomness: descriptor.randomness,
 			config,
 		}
+		.into()
 	}
 
 	fn start_slot(&self) -> Slot {
@@ -199,25 +208,12 @@ impl EpochT for Epoch {
 	}
 }
 
-impl From<sp_consensus_babe::Epoch> for Epoch {
-	fn from(epoch: sp_consensus_babe::Epoch) -> Self {
-		Epoch {
-			epoch_index: epoch.epoch_index,
-			start_slot: epoch.start_slot,
-			duration: epoch.duration,
-			authorities: epoch.authorities,
-			randomness: epoch.randomness,
-			config: epoch.config,
-		}
-	}
-}
-
 impl Epoch {
 	/// Create the genesis epoch (epoch #0).
 	///
 	/// This is defined to start at the slot of the first block, so that has to be provided.
 	pub fn genesis(genesis_config: &BabeConfiguration, slot: Slot) -> Epoch {
-		Epoch {
+		sp_consensus_babe::Epoch {
 			epoch_index: 0,
 			start_slot: slot,
 			duration: genesis_config.epoch_length,
@@ -228,6 +224,7 @@ impl Epoch {
 				allowed_slots: genesis_config.allowed_slots,
 			},
 		}
+		.into()
 	}
 
 	/// Clone and tweak epoch information to refer to the specified slot.

substrate/client/consensus/babe/src/migration.rs

@@ -62,16 +62,18 @@ impl EpochT for EpochV0 {
 	}
 }
 
+// Implement From<EpochV0> for Epoch
 impl EpochV0 {
 	/// Migrate the sturct to current epoch version.
 	pub fn migrate(self, config: &BabeConfiguration) -> Epoch {
-		Epoch {
+		sp_consensus_babe::Epoch {
 			epoch_index: self.epoch_index,
 			start_slot: self.start_slot,
 			duration: self.duration,
 			authorities: self.authorities,
 			randomness: self.randomness,
 			config: BabeEpochConfiguration { c: config.c, allowed_slots: config.allowed_slots },
 		}
+		.into()
 	}
 }

substrate/client/consensus/babe/src/tests.rs

@@ -501,7 +501,7 @@ fn claim_epoch_slots() {
 	let authority = Sr25519Keyring::Alice;
 	let keystore = create_keystore(authority);
 
-	let mut epoch = Epoch {
+	let mut epoch: Epoch = sp_consensus_babe::Epoch {
 		start_slot: 0.into(),
 		authorities: vec![(authority.public().into(), 1)],
 		randomness: [0; 32],
@@ -511,7 +511,8 @@ fn claim_epoch_slots() {
 			c: (3, 10),
 			allowed_slots: AllowedSlots::PrimaryAndSecondaryPlainSlots,
 		},
-	};
+	}
+	.into();
 
 	let claim_slot_wrap = |s, e| match claim_slot(Slot::from(s as u64), &e, &keystore) {
 		None => 0,
@@ -551,7 +552,7 @@ fn claim_vrf_check() {
 
 	let public = authority.public();
 
-	let epoch = Epoch {
+	let epoch: Epoch = sp_consensus_babe::Epoch {
 		start_slot: 0.into(),
 		authorities: vec![(public.into(), 1)],
 		randomness: [0; 32],
@@ -561,7 +562,8 @@ fn claim_vrf_check() {
 			c: (3, 10),
 			allowed_slots: AllowedSlots::PrimaryAndSecondaryVRFSlots,
 		},
-	};
+	}
+	.into();
 
 	// We leverage the predictability of claim types given a constant randomness.