From c7dbfc21b661c39f5dc2e181109d00945638350c Mon Sep 17 00:00:00 2001
From: Davide Galassi <davxy@datawok.net>
Date: Sun, 17 Sep 2023 22:06:19 +0200
Subject: [PATCH] Babe epoch newtype (#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).
---
Cargo.lock | 3 -
substrate/client/consensus/babe/Cargo.toml | 3 -
.../client/consensus/babe/src/authorship.rs | 21 ++++---
substrate/client/consensus/babe/src/lib.rs | 55 +++++++++----------
.../client/consensus/babe/src/migration.rs | 4 +-
substrate/client/consensus/babe/src/tests.rs | 10 ++--
6 files changed, 45 insertions(+), 51 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index a994d14a322..efefda5c878 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -14754,17 +14754,14 @@ dependencies = [
"num-traits",
"parity-scale-codec",
"parking_lot 0.12.1",
- "rand_chacha 0.2.2",
"sc-block-builder",
"sc-client-api",
"sc-consensus",
"sc-consensus-epochs",
"sc-consensus-slots",
- "sc-network",
"sc-network-test",
"sc-telemetry",
"sc-transaction-pool-api",
- "scale-info",
"sp-api",
"sp-application-crypto",
"sp-block-builder",
diff --git a/substrate/client/consensus/babe/Cargo.toml b/substrate/client/consensus/babe/Cargo.toml
index d9588794535..c8cff0981b3 100644
--- a/substrate/client/consensus/babe/Cargo.toml
+++ b/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" }
diff --git a/substrate/client/consensus/babe/src/authorship.rs b/substrate/client/consensus/babe/src/authorship.rs
index 758d5321a94..3580caba746 100644
--- a/substrate/client/consensus/babe/src/authorship.rs
+++ b/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());
diff --git a/substrate/client/consensus/babe/src/lib.rs b/substrate/client/consensus/babe/src/lib.rs
index 90b7523ec18..ccf72939631 100644
--- a/substrate/client/consensus/babe/src/lib.rs
+++ b/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,7 +188,7 @@ 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,
@@ -188,6 +196,7 @@ impl EpochT for Epoch {
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.
diff --git a/substrate/client/consensus/babe/src/migration.rs b/substrate/client/consensus/babe/src/migration.rs
index 2b1396c41c2..bec2d0a61f4 100644
--- a/substrate/client/consensus/babe/src/migration.rs
+++ b/substrate/client/consensus/babe/src/migration.rs
@@ -62,10 +62,11 @@ 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,
@@ -73,5 +74,6 @@ impl EpochV0 {
randomness: self.randomness,
config: BabeEpochConfiguration { c: config.c, allowed_slots: config.allowed_slots },
}
+ .into()
}
}
diff --git a/substrate/client/consensus/babe/src/tests.rs b/substrate/client/consensus/babe/src/tests.rs
index b3843f8acfa..02882d8baae 100644
--- a/substrate/client/consensus/babe/src/tests.rs
+++ b/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.
--
GitLab