From b04cfee6dcdef38b8decd3269002a7ffd5731118 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 12 Sep 2019 13:48:01 +0200 Subject: [PATCH 1/5] rejig babe APIs --- core/client/src/client.rs | 6 ++-- core/consensus/babe/primitives/src/digest.rs | 31 ++++------------ core/consensus/babe/primitives/src/lib.rs | 38 ++++++++++---------- 3 files changed, 29 insertions(+), 46 deletions(-) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index eb071166903c1..46eb252796688 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -1528,7 +1528,7 @@ impl consensus::BlockImport for Client } } -impl Finalizer for Client where +impl Finalizer for Client where B: backend::Backend, E: CallExecutor, Block: BlockT, @@ -1552,7 +1552,7 @@ impl Finalizer for Client Finalizer for &Client where +impl Finalizer for &Client where B: backend::Backend, E: CallExecutor, Block: BlockT, @@ -1833,7 +1833,7 @@ impl backend::AuxStore for &Client B: backend::Backend, E: CallExecutor, Block: BlockT, -{ +{ fn insert_aux< 'a, diff --git a/core/consensus/babe/primitives/src/digest.rs b/core/consensus/babe/primitives/src/digest.rs index 427c4fec57e7d..45157e2e28216 100644 --- a/core/consensus/babe/primitives/src/digest.rs +++ b/core/consensus/babe/primitives/src/digest.rs @@ -22,7 +22,7 @@ use super::AuthoritySignature; use super::{BABE_ENGINE_ID, Epoch}; #[cfg(not(feature = "std"))] use super::{VRF_OUTPUT_LENGTH, VRF_PROOF_LENGTH}; -use super::{AuthorityIndex, BabeBlockWeight, SlotNumber}; +use super::{AuthorityIndex, SlotNumber}; #[cfg(feature = "std")] use sr_primitives::{DigestItem, generic::OpaqueDigestItemId}; #[cfg(feature = "std")] @@ -52,8 +52,6 @@ pub enum BabePreDigest { authority_index: super::AuthorityIndex, /// Slot number slot_number: SlotNumber, - /// Chain weight (measured in number of Primary blocks) - weight: BabeBlockWeight, }, /// A secondary deterministic slot assignment. Secondary { @@ -61,8 +59,6 @@ pub enum BabePreDigest { authority_index: super::AuthorityIndex, /// Slot number slot_number: SlotNumber, - /// Chain weight (measured in number of Primary blocks) - weight: BabeBlockWeight, }, } @@ -83,14 +79,6 @@ impl BabePreDigest { BabePreDigest::Secondary { slot_number, .. } => *slot_number, } } - - /// Returns the weight of the pre digest. - pub fn weight(&self) -> BabeBlockWeight { - match self { - BabePreDigest::Primary { weight, .. } => *weight, - BabePreDigest::Secondary { weight, .. } => *weight, - } - } } /// The prefix used by BABE for its VRF keys. @@ -100,26 +88,24 @@ pub const BABE_VRF_PREFIX: &'static [u8] = b"substrate-babe-vrf"; #[derive(Copy, Clone, Encode, Decode)] pub enum RawBabePreDigest { /// A primary VRF-based slot assignment. + #[codec(index = "1")] Primary { /// Authority index authority_index: AuthorityIndex, /// Slot number slot_number: SlotNumber, - /// Chain weight (measured in number of Primary blocks) - weight: BabeBlockWeight, /// VRF output vrf_output: [u8; VRF_OUTPUT_LENGTH], /// VRF proof vrf_proof: [u8; VRF_PROOF_LENGTH], }, /// A secondary deterministic slot assignment. + #[codec(index = "2")] Secondary { /// Authority index authority_index: AuthorityIndex, /// Slot number slot_number: SlotNumber, - /// Chain weight (measured in number of Primary blocks) - weight: BabeBlockWeight, }, } @@ -142,25 +128,21 @@ impl Encode for BabePreDigest { vrf_proof, authority_index, slot_number, - weight, } => { RawBabePreDigest::Primary { vrf_output: *vrf_output.as_bytes(), vrf_proof: vrf_proof.to_bytes(), authority_index: *authority_index, slot_number: *slot_number, - weight: *weight, } }, BabePreDigest::Secondary { authority_index, slot_number, - weight, } => { RawBabePreDigest::Secondary { authority_index: *authority_index, slot_number: *slot_number, - weight: *weight, } }, }; @@ -176,7 +158,7 @@ impl codec::EncodeLike for BabePreDigest {} impl Decode for BabePreDigest { fn decode(i: &mut R) -> Result { let pre_digest = match Decode::decode(i)? { - RawBabePreDigest::Primary { vrf_output, vrf_proof, authority_index, slot_number, weight } => { + RawBabePreDigest::Primary { vrf_output, vrf_proof, authority_index, slot_number } => { // Verify (at compile time) that the sizes in babe_primitives are correct let _: [u8; super::VRF_OUTPUT_LENGTH] = vrf_output; let _: [u8; super::VRF_PROOF_LENGTH] = vrf_proof; @@ -186,11 +168,10 @@ impl Decode for BabePreDigest { vrf_output: VRFOutput::from_bytes(&vrf_output).map_err(convert_error)?, authority_index, slot_number, - weight, } }, - RawBabePreDigest::Secondary { authority_index, slot_number, weight } => { - BabePreDigest::Secondary { authority_index, slot_number, weight } + RawBabePreDigest::Secondary { authority_index, slot_number } => { + BabePreDigest::Secondary { authority_index, slot_number } }, }; diff --git a/core/consensus/babe/primitives/src/lib.rs b/core/consensus/babe/primitives/src/lib.rs index 09ac2f20123ac..3f9d46ba2f4a9 100644 --- a/core/consensus/babe/primitives/src/lib.rs +++ b/core/consensus/babe/primitives/src/lib.rs @@ -59,6 +59,11 @@ pub const VRF_PROOF_LENGTH: usize = 64; /// The length of the public key pub const PUBLIC_KEY_LENGTH: usize = 32; +/// How many blocks to wait before running the median algorithm for relative time +/// This will not vary from chain to chain as it is not dependent on slot duration +/// or epoch length. +pub const MEDIAN_ALGORITHM_CARDINALITY: usize = 1200; // arbitrary suggestion by w3f-research. + /// The index of an authority. pub type AuthorityIndex = u32; @@ -106,7 +111,7 @@ pub enum ConsensusLog { } /// Configuration data used by the BABE consensus engine. -#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug, Encode, Decode)] +#[derive(Clone, Hash, PartialEq, Eq, Debug, Encode, Decode)] pub struct BabeConfiguration { /// The slot duration in milliseconds for BABE. Currently, only /// the value provided by this type at genesis will be used. @@ -114,30 +119,30 @@ pub struct BabeConfiguration { /// Dynamic slot duration may be supported in the future. pub slot_duration: u64, + /// The duration of epochs in slots. + pub epoch_length: SlotNumber, + /// A constant value that is used in the threshold calculation formula. - /// Expressed as a fraction where the first member of the tuple is the - /// numerator and the second is the denominator. The fraction should + /// Expressed as a rational where the first member of the tuple is the + /// numerator and the second is the denominator. The rational should /// represent a value between 0 and 1. /// In the threshold formula calculation, `1 - c` represents the probability /// of a slot being empty. pub c: (u64, u64), - /// The minimum number of blocks that must be received before running the - /// median algorithm to compute the offset between the on-chain time and the - /// local time. Currently, only the value provided by this type at genesis - /// will be used, but this is subject to change. - /// - /// Blocks less than `self.median_required_blocks` must be generated by an - /// *initial validator* ― that is, a node that was a validator at genesis. - pub median_required_blocks: u64, + /// The authorities for the genesis epoch. + pub genesis_authorities: Vec<(AuthorityId, BabeAuthorityWeight)>, + + /// The randomness for this epoch. + pub randomness: [u8; VRF_OUTPUT_LENGTH], + + /// Whether this chain should run with secondary slots, which are assigned + /// in round-robin manner. + pub secondary_slots: bool, } #[cfg(feature = "std")] impl slots::SlotData for BabeConfiguration { - /// Return the slot duration in milliseconds for BABE. Currently, only - /// the value provided by this type at genesis will be used. - /// - /// Dynamic slot duration may be supported in the future. fn slot_duration(&self) -> u64 { self.slot_duration } @@ -153,8 +158,5 @@ decl_runtime_apis! { /// /// Dynamic configuration may be supported in the future. fn startup_data() -> BabeConfiguration; - - /// Get the current epoch data for Babe. - fn epoch() -> Epoch; } } From be5ce8c2ce6e0524cd8a097229a70455fe231173 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 12 Sep 2019 14:44:55 +0200 Subject: [PATCH 2/5] introduce next-epoch-descriptor type --- core/consensus/babe/primitives/src/digest.rs | 21 ++++++++++++++------ core/consensus/babe/primitives/src/lib.rs | 2 +- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/core/consensus/babe/primitives/src/digest.rs b/core/consensus/babe/primitives/src/digest.rs index 45157e2e28216..40d7230bab03f 100644 --- a/core/consensus/babe/primitives/src/digest.rs +++ b/core/consensus/babe/primitives/src/digest.rs @@ -17,12 +17,10 @@ //! Private implementation details of BABE digests. #[cfg(feature = "std")] -use super::AuthoritySignature; -#[cfg(feature = "std")] -use super::{BABE_ENGINE_ID, Epoch}; +use super::{BABE_ENGINE_ID, AuthoritySignature}; #[cfg(not(feature = "std"))] use super::{VRF_OUTPUT_LENGTH, VRF_PROOF_LENGTH}; -use super::{AuthorityIndex, SlotNumber}; +use super::{AuthorityId, AuthorityIndex, SlotNumber, BabeAuthorityWeight}; #[cfg(feature = "std")] use sr_primitives::{DigestItem, generic::OpaqueDigestItemId}; #[cfg(feature = "std")] @@ -179,6 +177,17 @@ impl Decode for BabePreDigest { } } +/// Information about the next epoch. This is broadcast in the first block +/// of the epoch. +#[derive(Clone, Encode, Decode)] +pub struct NextEpochDescriptor { + /// The authorities. + pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>, + + /// The value of randomness to use for the slot-assignment. + pub randomness: [u8; VRF_OUTPUT_LENGTH], +} + /// A digest item which is usable with BABE consensus. #[cfg(feature = "std")] pub trait CompatibleDigestItem: Sized { @@ -195,7 +204,7 @@ pub trait CompatibleDigestItem: Sized { fn as_babe_seal(&self) -> Option; /// If this item is a BABE epoch, return it. - fn as_babe_epoch(&self) -> Option; + fn as_next_epoch_descriptor(&self) -> Option; } #[cfg(feature = "std")] @@ -218,7 +227,7 @@ impl CompatibleDigestItem for DigestItem where self.try_to(OpaqueDigestItemId::Seal(&BABE_ENGINE_ID)) } - fn as_babe_epoch(&self) -> Option { + fn as_next_epoch_descriptor(&self) -> Option { self.try_to(OpaqueDigestItemId::Consensus(&BABE_ENGINE_ID)) } } diff --git a/core/consensus/babe/primitives/src/lib.rs b/core/consensus/babe/primitives/src/lib.rs index 3f9d46ba2f4a9..8b38764f023f7 100644 --- a/core/consensus/babe/primitives/src/lib.rs +++ b/core/consensus/babe/primitives/src/lib.rs @@ -157,6 +157,6 @@ decl_runtime_apis! { /// only the value provided by this type at genesis will be used. /// /// Dynamic configuration may be supported in the future. - fn startup_data() -> BabeConfiguration; + fn configuration() -> BabeConfiguration; } } From 32c2147802fc5818aee744539dd4929c68180526 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 13 Sep 2019 12:28:02 +0200 Subject: [PATCH 3/5] overhaul srml-BABE epoch logic --- core/consensus/babe/primitives/src/digest.rs | 5 +- core/consensus/babe/primitives/src/lib.rs | 16 +-- node/runtime/src/lib.rs | 16 +-- srml/babe/src/lib.rs | 127 +++++++------------ 4 files changed, 64 insertions(+), 100 deletions(-) diff --git a/core/consensus/babe/primitives/src/digest.rs b/core/consensus/babe/primitives/src/digest.rs index 40d7230bab03f..7969509a047fe 100644 --- a/core/consensus/babe/primitives/src/digest.rs +++ b/core/consensus/babe/primitives/src/digest.rs @@ -33,6 +33,8 @@ use schnorrkel::{ SignatureError, errors::MultiSignatureStage, vrf::{VRFProof, VRFOutput, VRF_OUTPUT_LENGTH, VRF_PROOF_LENGTH} }; +use rstd::vec::Vec; + /// A BABE pre-runtime digest. This contains all data required to validate a /// block and for the BABE runtime module. Slots can be assigned to a primary @@ -179,7 +181,8 @@ impl Decode for BabePreDigest { /// Information about the next epoch. This is broadcast in the first block /// of the epoch. -#[derive(Clone, Encode, Decode)] +#[derive(Decode, Encode, Default, PartialEq, Eq, Clone)] +#[cfg_attr(any(feature = "std", test), derive(Debug))] pub struct NextEpochDescriptor { /// The authorities. pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>, diff --git a/core/consensus/babe/primitives/src/lib.rs b/core/consensus/babe/primitives/src/lib.rs index 8b38764f023f7..4c31ff6b1444f 100644 --- a/core/consensus/babe/primitives/src/lib.rs +++ b/core/consensus/babe/primitives/src/lib.rs @@ -28,7 +28,7 @@ use substrate_client::decl_runtime_apis; #[cfg(feature = "std")] pub use digest::{BabePreDigest, CompatibleDigestItem}; -pub use digest::{BABE_VRF_PREFIX, RawBabePreDigest}; +pub use digest::{BABE_VRF_PREFIX, RawBabePreDigest, NextEpochDescriptor}; mod app { use app_crypto::{app_crypto, key_types::BABE, sr25519}; @@ -92,26 +92,24 @@ pub struct Epoch { pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>, /// Randomness for this epoch pub randomness: [u8; VRF_OUTPUT_LENGTH], - /// Whether secondary slot assignments should be used during the epoch. - pub secondary_slots: bool, } /// An consensus log item for BABE. #[derive(Decode, Encode, Clone, PartialEq, Eq)] pub enum ConsensusLog { - /// The epoch has changed. This provides information about the - /// epoch _after_ next: what slot number it will start at, who are the authorities (and their weights) - /// and the next epoch randomness. The information for the _next_ epoch should already - /// be available. + /// The epoch has changed. This provides information about the _next_ + /// epoch - information about the _current_ epoch (i.e. the one we've just + /// entered) should already be available earlier in the chain. #[codec(index = "1")] - NextEpochData(Epoch), + NextEpochData(NextEpochDescriptor), /// Disable the authority with given index. #[codec(index = "2")] OnDisabled(AuthorityIndex), } /// Configuration data used by the BABE consensus engine. -#[derive(Clone, Hash, PartialEq, Eq, Debug, Encode, Decode)] +#[derive(Clone, PartialEq, Eq, Encode, Decode)] +#[cfg_attr(any(feature = "std", test), derive(Debug))] pub struct BabeConfiguration { /// The slot duration in milliseconds for BABE. Currently, only /// the value provided by this type at genesis will be used. diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 03d11111f1939..f67cdb5133295 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -586,27 +586,19 @@ impl_runtime_apis! { } impl babe_primitives::BabeApi for Runtime { - fn startup_data() -> babe_primitives::BabeConfiguration { + fn configuration() -> babe_primitives::BabeConfiguration { // The choice of `c` parameter (where `1 - c` represents the // probability of a slot being empty), is done in accordance to the // slot duration and expected target block time, for safely // resisting network delays of maximum two seconds. // babe_primitives::BabeConfiguration { - median_required_blocks: 1000, slot_duration: Babe::slot_duration(), + epoch_length: EpochDuration::get(), c: PRIMARY_PROBABILITY, - } - } - - fn epoch() -> babe_primitives::Epoch { - babe_primitives::Epoch { - start_slot: Babe::epoch_start_slot(), - authorities: Babe::authorities(), - epoch_index: Babe::epoch_index(), + genesis_authorities: Babe::authorities(), randomness: Babe::randomness(), - duration: EpochDuration::get(), - secondary_slots: Babe::secondary_slots().0, + secondary_slots: true, } } } diff --git a/srml/babe/src/lib.rs b/srml/babe/src/lib.rs index e21f083eb6ba9..e898ea77e28cf 100644 --- a/srml/babe/src/lib.rs +++ b/srml/babe/src/lib.rs @@ -20,9 +20,6 @@ #![cfg_attr(not(feature = "std"), no_std)] #![forbid(unused_must_use, unsafe_code, unused_variables)] -// TODO: @marcio uncomment this when BabeEquivocation is integrated. -// #![forbid(dead_code)] - pub use timestamp; use rstd::{result, prelude::*}; @@ -34,16 +31,17 @@ use sr_staking_primitives::{ SessionIndex, offence::{Offence, Kind}, }; -use sr_primitives::weights::SimpleDispatchInfo; #[cfg(feature = "std")] use timestamp::TimestampInherentData; use codec::{Encode, Decode}; use inherents::{RuntimeString, InherentIdentifier, InherentData, ProvideInherent, MakeFatalError}; #[cfg(feature = "std")] use inherents::{InherentDataProviders, ProvideInherentData}; -use babe_primitives::{BABE_ENGINE_ID, ConsensusLog, BabeAuthorityWeight, Epoch, RawBabePreDigest}; +use babe_primitives::{ + BABE_ENGINE_ID, ConsensusLog, BabeAuthorityWeight, NextEpochDescriptor, RawBabePreDigest, + SlotNumber, +}; pub use babe_primitives::{AuthorityId, VRF_OUTPUT_LENGTH, PUBLIC_KEY_LENGTH}; -use system::ensure_root; /// The BABE inherent identifier. pub const INHERENT_IDENTIFIER: InherentIdentifier = *b"babeslot"; @@ -118,7 +116,7 @@ impl ProvideInherentData for InherentDataProvider { } pub trait Trait: timestamp::Trait { - type EpochDuration: Get; + type EpochDuration: Get; type ExpectedBlockTime: Get; } @@ -135,22 +133,13 @@ decl_storage! { /// Current epoch authorities. pub Authorities get(authorities): Vec<(AuthorityId, BabeAuthorityWeight)>; - /// Slot at which the current epoch started. It is possible that no - /// block was authored at the given slot and the epoch change was - /// signalled later than this. - pub EpochStartSlot get(epoch_start_slot): u64; + /// The slot at which the first epoch actually started. This is 0 + /// until the first block of the chain. + pub GenesisSlot get(genesis_slot): u64; /// Current slot number. pub CurrentSlot get(current_slot): u64; - /// Whether secondary slots are enabled in case the VRF-based slot is - /// empty for the current epoch and the next epoch, respectively. - pub SecondarySlots get(secondary_slots): (bool, bool) = (true, true); - - /// Pending change to enable/disable secondary slots which will be - /// triggered at `current_epoch + 2`. - pub PendingSecondarySlotsChange get(pending_secondary_slots_change): Option = None; - /// The epoch randomness for the *current* epoch. /// /// # Security @@ -214,20 +203,6 @@ decl_module! { fn on_finalize() { Initialized::kill(); } - - /// Sets a pending change to enable / disable secondary slot assignment. - /// The pending change will be set at the end of the current epoch and - /// will be enacted at `current_epoch + 2`. - #[weight = SimpleDispatchInfo::FixedOperational(10_000)] - fn set_pending_secondary_slots_change(origin, change: Option) { - ensure_root(origin)?; - match change { - Some(change) => PendingSecondarySlotsChange::put(change), - None => { - PendingSecondarySlotsChange::take(); - }, - } - } } } @@ -269,15 +244,25 @@ impl IsMember for Module { } impl session::ShouldEndSession for Module { - fn should_end_session(_: T::BlockNumber) -> bool { + fn should_end_session(now: T::BlockNumber) -> bool { // it might be (and it is in current implementation) that session module is calling // should_end_session() from it's own on_initialize() handler // => because session on_initialize() is called earlier than ours, let's ensure // that we have synced with digest before checking if session should be ended Self::do_initialize(); - let diff = CurrentSlot::get().saturating_sub(EpochStartSlot::get()); - diff >= T::EpochDuration::get() + // The session has technically ended during the passage of time + // between this block and the last, but we have to "end" the session now, + // since there is no earlier possible block we could have done it. + // + // The exception is for block 1: the genesis has slot 0, so we treat + // epoch 0 as having started at the slot of block 1. We want to use + // the same randomness and validator set as signalled in the genesis, + // so we don't rotate the session. + now != sr_primitives::traits::One::one() && { + let diff = CurrentSlot::get().saturating_sub(Self::current_epoch_start()); + diff >= T::EpochDuration::get() + } } } @@ -336,6 +321,13 @@ impl Module { ::MinimumPeriod::get().saturating_mul(2.into()) } + // finds the start slot of the current epoch. only guaranteed to + // give correct results after `do_initialize` of the first block + // in the chain (as its result is based off of `GenesisSlot`). + fn current_epoch_start() -> SlotNumber { + (EpochIndex::get() * T::EpochDuration::get()) + GenesisSlot::get() + } + fn deposit_consensus(new: U) { let log: DigestItem = DigestItem::Consensus(BABE_ENGINE_ID, new.encode()); >::deposit_log(log.into()) @@ -379,8 +371,20 @@ impl Module { None }) { - if EpochStartSlot::get() == 0 { - EpochStartSlot::put(digest.slot_number()); + // on the first non-zero block (i.e. block #1) + // this is where the first epoch (epoch #0) actually starts. + // we need to adjust internal storage accordingly. + if GenesisSlot::get() == 0 { + GenesisSlot::put(digest.slot_number()); + + // deposit a log because this is the first block in epoch #0 + // we use the same values as genesis because + let next = NextEpochDescriptor { + authorities: Self::authorities(), + randomness: Self::randomness(), + }; + + Self::deposit_consensus(ConsensusLog::NextEpochData(next)) } CurrentSlot::put(digest.slot_number()); @@ -437,7 +441,12 @@ impl session::OneSessionHandler for Module { fn on_new_session<'a, I: 'a>(_changed: bool, validators: I, queued_validators: I) where I: Iterator { - Self::do_initialize(); + // PRECONDITION: `should_end_session` has done initialization and is guaranteed + // by the session module to be called before this. + #[cfg(debug_assertions)] + { + assert_eq!(Initialized::get(), Some(true)); + } // Update epoch index let epoch_index = EpochIndex::get() @@ -453,21 +462,6 @@ impl session::OneSessionHandler for Module { Authorities::put(authorities); - // Update epoch start slot. - let now = CurrentSlot::get(); - EpochStartSlot::mutate(|previous| { - loop { - // on the first epoch we must account for skipping at least one - // whole epoch, in case the first block is authored with a slot - // number far in the past. - if now.saturating_sub(*previous) < T::EpochDuration::get() { - break; - } - - *previous = previous.saturating_add(T::EpochDuration::get()); - } - }); - // Update epoch randomness. let next_epoch_index = epoch_index .checked_add(1) @@ -484,34 +478,11 @@ impl session::OneSessionHandler for Module { (k, 1) }).collect::>(); - let next_epoch_start_slot = EpochStartSlot::get().saturating_add(T::EpochDuration::get()); let next_randomness = NextRandomness::get(); - // Update any pending secondary slots change - let mut secondary_slots = SecondarySlots::get(); - - // change for E + 1 now becomes change at E - secondary_slots.0 = secondary_slots.1; - - if let Some(change) = PendingSecondarySlotsChange::take() { - // if there's a pending change schedule it for E + 1 - secondary_slots.1 = change; - } else { - // otherwise E + 1 will have the same value as E - secondary_slots.1 = secondary_slots.0; - } - - SecondarySlots::mutate(|secondary| { - *secondary = secondary_slots; - }); - - let next = Epoch { - epoch_index: next_epoch_index, - start_slot: next_epoch_start_slot, - duration: T::EpochDuration::get(), + let next = NextEpochDescriptor { authorities: next_authorities, randomness: next_randomness, - secondary_slots: secondary_slots.1, }; Self::deposit_consensus(ConsensusLog::NextEpochData(next)) From 0bd813a43270cb2982231cfefa73f40623548006 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 13 Sep 2019 14:26:29 +0200 Subject: [PATCH 4/5] ensure VRF outputs end up in the right epoch-randomness --- srml/babe/src/lib.rs | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/srml/babe/src/lib.rs b/srml/babe/src/lib.rs index e898ea77e28cf..4be8ff979d2bd 100644 --- a/srml/babe/src/lib.rs +++ b/srml/babe/src/lib.rs @@ -125,6 +125,8 @@ pub const RANDOMNESS_LENGTH: usize = 32; const UNDER_CONSTRUCTION_SEGMENT_LENGTH: usize = 256; +type MaybeVrf = Option<[u8; 32 /* VRF_OUTPUT_LENGTH */]>; + decl_storage! { trait Store for Module as Babe { /// Current epoch index. @@ -170,9 +172,9 @@ decl_storage! { SegmentIndex build(|_| 0): u32; UnderConstruction: map u32 => Vec<[u8; 32 /* VRF_OUTPUT_LENGTH */]>; - /// Temporary value (cleared at block finalization) which is true + /// Temporary value (cleared at block finalization) which is `Some` /// if per-block initialization has already been called for current block. - Initialized get(initialized): Option; + Initialized get(initialized): Option; } add_extra_genesis { config(authorities): Vec<(AuthorityId, BabeAuthorityWeight)>; @@ -201,7 +203,14 @@ decl_module! { /// Block finalization fn on_finalize() { - Initialized::kill(); + // at the end of the block, we can safely include the new VRF output + // from this block into the under-construction randomness. If we've determined + // that this block was the first in a new epoch, the changeover logic has + // already occurred at this point, so the under-construction randomness + // will only contain outputs from the right epoch. + if let Some(Some(vrf_output)) = Initialized::take() { + Self::deposit_vrf_output(&vrf_output); + } } } } @@ -355,12 +364,20 @@ impl Module { fn do_initialize() { // since do_initialize can be called twice (if session module is present) // => let's ensure that we only modify the storage once per block - let initialized = Self::initialized().unwrap_or(false); + let initialized = Self::initialized().is_some(); if initialized { return; } - Initialized::put(true); + #[derive(Default)] + struct Guard { maybe_vrf: MaybeVrf }; + impl Drop for Guard { + fn drop(&mut self) { + Initialized::put(self.maybe_vrf.take()) + } + } + + let mut guard = Guard::default(); for digest in Self::get_inherent_digests() .logs .iter() @@ -390,10 +407,13 @@ impl Module { CurrentSlot::put(digest.slot_number()); if let RawBabePreDigest::Primary { vrf_output, .. } = digest { - Self::deposit_vrf_output(&vrf_output); + // place the VRF output into the `Initialized` storage item + // and it'll be put onto the under-construction randomness + // later, once we've decided which epoch this block is in. + guard.maybe_vrf = Some(vrf_output); } - return; + break } } @@ -445,7 +465,7 @@ impl session::OneSessionHandler for Module { // by the session module to be called before this. #[cfg(debug_assertions)] { - assert_eq!(Initialized::get(), Some(true)); + assert!(Self::initialized().is_some()) } // Update epoch index From 8ec1f486f72b760423dd19ef75b24640956bc20b Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 16 Sep 2019 10:04:54 +0200 Subject: [PATCH 5/5] rewrite `do_initialize` to remove unnecessary loop --- srml/babe/src/lib.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/srml/babe/src/lib.rs b/srml/babe/src/lib.rs index 4be8ff979d2bd..019adbe52bc00 100644 --- a/srml/babe/src/lib.rs +++ b/srml/babe/src/lib.rs @@ -369,16 +369,7 @@ impl Module { return; } - #[derive(Default)] - struct Guard { maybe_vrf: MaybeVrf }; - impl Drop for Guard { - fn drop(&mut self) { - Initialized::put(self.maybe_vrf.take()) - } - } - - let mut guard = Guard::default(); - for digest in Self::get_inherent_digests() + let maybe_pre_digest = Self::get_inherent_digests() .logs .iter() .filter_map(|s| s.as_pre_runtime()) @@ -387,7 +378,9 @@ impl Module { } else { None }) - { + .next(); + + let maybe_vrf = maybe_pre_digest.and_then(|digest| { // on the first non-zero block (i.e. block #1) // this is where the first epoch (epoch #0) actually starts. // we need to adjust internal storage accordingly. @@ -410,11 +403,13 @@ impl Module { // place the VRF output into the `Initialized` storage item // and it'll be put onto the under-construction randomness // later, once we've decided which epoch this block is in. - guard.maybe_vrf = Some(vrf_output); + Some(vrf_output) + } else { + None } + }); - break - } + Initialized::put(maybe_vrf); } /// Call this function exactly once when an epoch changes, to update the