diff --git a/core/consensus/babe/primitives/src/digest.rs b/core/consensus/babe/primitives/src/digest.rs index f99f7bf9c89d1..3f275ecdb6341 100644 --- a/core/consensus/babe/primitives/src/digest.rs +++ b/core/consensus/babe/primitives/src/digest.rs @@ -246,6 +246,10 @@ impl CompatibleDigestItem for DigestItem where fn as_next_epoch_descriptor(&self) -> Option { self.try_to(OpaqueDigestItemId::Consensus(&BABE_ENGINE_ID)) + .and_then(|x: super::ConsensusLog| match x { + super::ConsensusLog::NextEpochData(n) => Some(n), + _ => None, + }) } } diff --git a/core/consensus/babe/primitives/src/lib.rs b/core/consensus/babe/primitives/src/lib.rs index 4a0a592519615..1293b7c8baa00 100644 --- a/core/consensus/babe/primitives/src/lib.rs +++ b/core/consensus/babe/primitives/src/lib.rs @@ -105,6 +105,12 @@ impl Epoch { randomness: descriptor.randomness, } } + + /// Produce the "end slot" of the epoch. This is NOT inclusive to the epoch, + // i.e. the slots covered by the epoch are `self.start_slot .. self.end_slot()`. + pub fn end_slot(&self) -> SlotNumber { + self.start_slot + self.duration + } } /// An consensus log item for BABE. diff --git a/core/consensus/babe/src/aux_schema.rs b/core/consensus/babe/src/aux_schema.rs index 05d5721118b5c..67f61050fa31a 100644 --- a/core/consensus/babe/src/aux_schema.rs +++ b/core/consensus/babe/src/aux_schema.rs @@ -24,7 +24,7 @@ use client::error::{Result as ClientResult, Error as ClientError}; use sr_primitives::traits::Block as BlockT; use babe_primitives::BabeBlockWeight; -use super::{EpochChanges, SharedEpochChanges}; +use super::{epoch_changes::EpochChangesFor, SharedEpochChanges}; const BABE_EPOCH_CHANGES: &[u8] = b"babe_epoch_changes"; @@ -50,7 +50,7 @@ fn load_decode(backend: &B, key: &[u8]) -> ClientResult> pub(crate) fn load_epoch_changes( backend: &B, ) -> ClientResult> { - let epoch_changes = load_decode::<_, EpochChanges>(backend, BABE_EPOCH_CHANGES)? + let epoch_changes = load_decode::<_, EpochChangesFor>(backend, BABE_EPOCH_CHANGES)? .map(Into::into) .unwrap_or_else(|| { info!(target: "babe", @@ -64,7 +64,7 @@ pub(crate) fn load_epoch_changes( /// Update the epoch changes on disk after a change. pub(crate) fn write_epoch_changes( - epoch_changes: &EpochChanges, + epoch_changes: &EpochChangesFor, write_aux: F, ) -> R where F: FnOnce(&[(&'static [u8], &[u8])]) -> R, diff --git a/core/consensus/babe/src/epoch_changes.rs b/core/consensus/babe/src/epoch_changes.rs index 8ce99ee3224d5..776c30c05e02c 100644 --- a/core/consensus/babe/src/epoch_changes.rs +++ b/core/consensus/babe/src/epoch_changes.rs @@ -23,20 +23,21 @@ use std::sync::Arc; use babe_primitives::{Epoch, SlotNumber}; use fork_tree::ForkTree; use parking_lot::{Mutex, MutexGuard}; -use sr_primitives::traits::{Block as BlockT, NumberFor}; +use sr_primitives::traits::{Block as BlockT, NumberFor, One, Zero}; use codec::{Encode, Decode}; use client::error::Error as ClientError; use client::utils as client_utils; use client::blockchain::HeaderBackend; use primitives::H256; +use std::ops::Add; /// A builder for `is_descendent_of` functions. -pub trait IsDescendentOfBuilder { +pub trait IsDescendentOfBuilder { /// The error returned by the function. type Error: std::error::Error; /// A function that can tell you if the second parameter is a descendent of /// the first. - type IsDescendentOf: Fn(&Block::Hash, &Block::Hash) -> Result; + type IsDescendentOf: Fn(&Hash, &Hash) -> Result; /// Build an `is_descendent_of` function. /// @@ -44,14 +45,25 @@ pub trait IsDescendentOfBuilder { /// details aren't yet stored, but its parent is. /// /// The format of `current` when `Some` is `(current, current_parent)`. - fn build_is_descendent_of(&self, current: Option<(Block::Hash, Block::Hash)>) + fn build_is_descendent_of(&self, current: Option<(Hash, Hash)>) -> Self::IsDescendentOf; } +/// Produce a descendent query object given the client. +pub(crate) fn descendent_query(client: &H) -> HeaderBackendDescendentBuilder<&H, Block> { + HeaderBackendDescendentBuilder(client, std::marker::PhantomData) +} + +/// Wrapper to get around unconstrained type errors when implementing +/// `IsDescendentOfBuilder` for header backends. +pub(crate) struct HeaderBackendDescendentBuilder(H, std::marker::PhantomData); + // TODO: relying on Hash = H256 is awful. // https://github.com/paritytech/substrate/issues/3624 -impl<'a, Block: BlockT, T> IsDescendentOfBuilder for &'a T - where T: HeaderBackend +impl<'a, H, Block> IsDescendentOfBuilder + for HeaderBackendDescendentBuilder<&'a H, Block> where + H: HeaderBackend, + Block: BlockT, { type Error = ClientError; type IsDescendentOf = Box Result + 'a>; @@ -59,7 +71,7 @@ impl<'a, Block: BlockT, T> IsDescendentOfBuilder for &'a T fn build_is_descendent_of(&self, current: Option<(H256, H256)>) -> Self::IsDescendentOf { - Box::new(client_utils::is_descendent_of(*self, current)) + Box::new(client_utils::is_descendent_of(self.0, current)) } } @@ -67,8 +79,8 @@ impl<'a, Block: BlockT, T> IsDescendentOfBuilder for &'a T /// the hash and block number of the block signaling the epoch change, and the /// epoch that was signalled at that block. #[derive(Clone, Encode, Decode)] -pub struct EpochChanges { - inner: ForkTree, Epoch>, +pub struct EpochChanges { + inner: ForkTree, } // create a fake header hash which hasn't been included in the chain. @@ -80,44 +92,47 @@ fn fake_head_hash + AsMut<[u8]> + Clone>(parent_hash: &H) -> H { h } -impl EpochChanges { +impl EpochChanges where + Hash: PartialEq + AsRef<[u8]> + AsMut<[u8]> + Copy, + Number: Ord + One + Zero + Add + Copy, +{ /// Create a new epoch-change tracker. fn new() -> Self { EpochChanges { inner: ForkTree::new() } } /// Prune out finalized epochs, except for the ancestor of the finalized block. - pub fn prune_finalized>( + pub fn prune_finalized>( &mut self, descendent_of_builder: D, - _hash: &Block::Hash, - _number: NumberFor, + _hash: &Hash, + _number: Number, ) -> Result<(), fork_tree::Error> { let _is_descendent_of = descendent_of_builder .build_is_descendent_of(None); // TODO: + // https://github.com/paritytech/substrate/issues/3651 + // // prune any epochs which could not be _live_ as of the children of the // finalized block. - // i.e. re-root the fork tree to the earliest ancestor of (hash, number) - // where epoch.start_slot + epoch.duration >= slot(hash) + // i.e. re-root the fork tree to the oldest ancestor of (hash, number) + // where epoch.end_slot() >= slot(hash) Ok(()) } /// Finds the epoch for a child of the given block, assuming the given slot number. - pub fn epoch_for_child_of, G>( + pub fn epoch_for_child_of, G>( &mut self, descendent_of_builder: D, - parent_hash: &Block::Hash, - parent_number: NumberFor, + parent_hash: &Hash, + parent_number: Number, slot_number: SlotNumber, make_genesis: G, ) -> Result, fork_tree::Error> where G: FnOnce(SlotNumber) -> Epoch { - use sr_primitives::traits::{One, Zero}; - // find_node_where will give you the node in the fork-tree which is an ancestor // of the `parent_hash` by default. if the last epoch was signalled at the parent_hash, // then it won't be returned. we need to create a new fake chain head hash which @@ -160,12 +175,12 @@ impl EpochChanges { /// This assumes that the given block is prospective (i.e. has not been /// imported yet), but its parent has. This is why the parent hash needs /// to be provided. - pub fn import>( + pub fn import>( &mut self, descendent_of_builder: D, - hash: Block::Hash, - number: NumberFor, - parent_hash: Block::Hash, + hash: Hash, + number: Number, + parent_hash: Hash, epoch: Epoch, ) -> Result<(), fork_tree::Error> { let is_descendent_of = descendent_of_builder @@ -185,28 +200,30 @@ impl EpochChanges { } } +pub type EpochChangesFor = EpochChanges<::Hash, NumberFor>; + /// A shared epoch changes tree. #[derive(Clone)] pub struct SharedEpochChanges { - inner: Arc>>, + inner: Arc>>, } impl SharedEpochChanges { /// Create a new instance of the `SharedEpochChanges`. pub fn new() -> Self { SharedEpochChanges { - inner: Arc::new(Mutex::new(EpochChanges::::new())) + inner: Arc::new(Mutex::new(EpochChanges::<_, _>::new())) } } /// Lock the shared epoch changes, - pub fn lock(&self) -> MutexGuard> { + pub fn lock(&self) -> MutexGuard> { self.inner.lock() } } -impl From> for SharedEpochChanges { - fn from(epoch_changes: EpochChanges) -> Self { +impl From> for SharedEpochChanges { + fn from(epoch_changes: EpochChangesFor) -> Self { SharedEpochChanges { inner: Arc::new(Mutex::new(epoch_changes)) } @@ -222,3 +239,189 @@ impl From> for SharedEpochChanges { // epoch end slot and another after // // 3. Test that this always gives you the right epoch based on the fork you're on. +#[cfg(test)] +mod tests { + use super::*; + + #[derive(Debug, PartialEq)] + pub struct TestError; + + impl std::fmt::Display for TestError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "TestError") + } + } + + impl std::error::Error for TestError {} + + impl<'a, F: 'a , H: 'a + PartialEq + std::fmt::Debug> IsDescendentOfBuilder for &'a F + where F: Fn(&H, &H) -> Result + { + type Error = TestError; + type IsDescendentOf = Box Result + 'a>; + + fn build_is_descendent_of(&self, current: Option<(H, H)>) + -> Self::IsDescendentOf + { + let f = *self; + Box::new(move |base, head| { + let mut head = head; + + if let Some((ref c_head, ref c_parent)) = current { + if head == c_head { + if base == c_parent { + return Ok(true); + } else { + head = c_parent; + } + } + } + + f(base, head) + }) + } + } + + type Hash = [u8; 1]; + + #[test] + fn genesis_epoch_is_inserted_and_persisted() { + // + // A - B + // \ + // — C + // + let is_descendent_of = |base: &Hash, block: &Hash| -> Result { + match (base, *block) { + (b"A", b) => Ok(b == *b"B" || b == *b"C" || b == *b"D"), + (b"B", b) | (b"C", b) => Ok(b == *b"D"), + (b"0", _) => Ok(true), + _ => Ok(false), + } + }; + + let make_genesis = |slot| Epoch { + epoch_index: 0, + start_slot: slot, + duration: 100, + authorities: Vec::new(), + randomness: [0; 32], + }; + + let mut epoch_changes = EpochChanges::new(); + let genesis_epoch = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"0", + 0, + 10101, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(genesis_epoch, make_genesis(10101)); + + let block_in_genesis_epoch = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"A", + 1, + 10102, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(block_in_genesis_epoch, genesis_epoch); + } + + #[test] + fn epoch_changes_between_blocks() { + // + // A - B + // \ + // — C + // + let is_descendent_of = |base: &Hash, block: &Hash| -> Result { + match (base, *block) { + (b"A", b) => Ok(b == *b"B" || b == *b"C" || b == *b"D"), + (b"B", b) | (b"C", b) => Ok(b == *b"D"), + (b"0", _) => Ok(true), + _ => Ok(false), + } + }; + + let make_genesis = |slot| Epoch { + epoch_index: 0, + start_slot: slot, + duration: 100, + authorities: Vec::new(), + randomness: [0; 32], + }; + + let mut epoch_changes = EpochChanges::new(); + let genesis_epoch = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"0", + 0, + 100, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(genesis_epoch, make_genesis(100)); + + let epoch_1 = genesis_epoch.increment(babe_primitives::NextEpochDescriptor { + authorities: Vec::new(), + randomness: [1; 32], + }); + + epoch_changes.import( + &is_descendent_of, + *b"A", + 1, + *b"0", + epoch_1.clone(), + ).unwrap(); + + assert!(is_descendent_of(b"0", b"A").unwrap()); + + let end_slot = genesis_epoch.end_slot(); + assert_eq!(end_slot, epoch_1.start_slot); + + { + // x is still within the genesis epoch. + let x = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"A", + 1, + end_slot - 1, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(x, genesis_epoch); + } + + { + // x is now at the next epoch, because the block is now at the + // start slot of epoch 1. + let x = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"A", + 1, + end_slot, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(x, epoch_1); + } + + { + // x is now at the next epoch, because the block is now after + // start slot of epoch 1. + let x = epoch_changes.epoch_for_child_of( + &is_descendent_of, + b"A", + 1, + epoch_1.end_slot() - 1, + &make_genesis, + ).unwrap().unwrap(); + + assert_eq!(x, epoch_1); + } + } +} diff --git a/core/consensus/babe/src/lib.rs b/core/consensus/babe/src/lib.rs index 7c86cf6346be3..2127369823637 100644 --- a/core/consensus/babe/src/lib.rs +++ b/core/consensus/babe/src/lib.rs @@ -110,6 +110,7 @@ use futures::prelude::*; use log::{warn, debug, info, trace}; use slots::{SlotWorker, SlotData, SlotInfo, SlotCompatible}; +use epoch_changes::descendent_query; mod aux_schema; mod epoch_changes; @@ -266,7 +267,7 @@ pub fn start_babe(BabeParams { // TODO: supply is-descendent-of and maybe write to disk _now_ // as opposed to waiting for the next epoch? let res = epoch_changes.lock().prune_finalized( - &*client, + descendent_query(&*client), ¬ification.hash, *notification.header.number(), ); @@ -329,7 +330,7 @@ impl slots::SimpleSlotWorker for BabeWorker Result { self.epoch_changes.lock().epoch_for_child_of( - &*self.client, + descendent_query(&*self.client), &parent.hash(), parent.number().clone(), slot_number, @@ -841,7 +842,7 @@ impl Verifier for BabeVerifier BlockImport for BabeBlockImport BlockImport for BabeBlockImport, { let epoch = link.epoch_changes.lock().epoch_for_child_of( - client, + descendent_query(client), &parent.hash(), parent.number().clone(), slot_number, diff --git a/core/consensus/babe/src/tests.rs b/core/consensus/babe/src/tests.rs index 409f6cce0f86a..10d4dbb0b187b 100644 --- a/core/consensus/babe/src/tests.rs +++ b/core/consensus/babe/src/tests.rs @@ -37,7 +37,7 @@ use keyring::sr25519::Keyring; use client::BlockchainEvents; use test_client; use log::debug; -use std::{time::Duration, borrow::Borrow, cell::RefCell}; +use std::{time::Duration, cell::RefCell}; type Item = DigestItem; @@ -50,11 +50,20 @@ type TestClient = client::Client< test_client::runtime::RuntimeApi, >; +#[derive(Copy, Clone, PartialEq)] +enum Stage { + PreSeal, + PostSeal, +} + +type Mutator = Arc; + #[derive(Clone)] struct DummyFactory { client: Arc, epoch_changes: crate::SharedEpochChanges, config: Config, + mutator: Mutator, } struct DummyProposer { @@ -112,7 +121,7 @@ impl Proposer for DummyProposer { // doesn't. let mut epoch_changes = self.factory.epoch_changes.lock(); let epoch = epoch_changes.epoch_for_child_of( - &*self.factory.client, + descendent_query(&*self.factory.client), &self.parent_hash, self.parent_number, this_slot, @@ -135,14 +144,38 @@ impl Proposer for DummyProposer { block.header.digest_mut().push(digest) } + // mutate the block header according to the mutator. + (self.factory.mutator)(&mut block.header, Stage::PreSeal); + future::ready(Ok(block)) } } -type Mutator = Arc Fn(&'r mut TestHeader) + Send + Sync>; - thread_local! { - static MUTATOR: RefCell = RefCell::new(Arc::new(|_|())); + static MUTATOR: RefCell = RefCell::new(Arc::new(|_, _|())); +} + +#[derive(Clone)] +struct PanickingBlockImport(B); + +impl> BlockImport for PanickingBlockImport { + type Error = B::Error; + + fn import_block( + &mut self, + block: BlockImportParams, + new_cache: HashMap>, + ) -> Result { + Ok(self.0.import_block(block, new_cache).expect("importing block failed")) + } + + fn check_block( + &mut self, + hash: Hash, + parent_hash: Hash, + ) -> Result { + Ok(self.0.check_block(hash, parent_hash).expect("checking block failed")) + } } pub struct BabeTestNet { @@ -174,8 +207,8 @@ impl Verifier for TestVerifier { justification: Option, body: Option>, ) -> Result<(BlockImportParams, Option)>>), String> { - let cb: &(dyn Fn(&mut TestHeader) + Send + Sync) = self.mutator.borrow(); - cb(&mut header); + // apply post-sealing mutations (i.e. stripping seal, if desired). + (self.mutator)(&mut header, Stage::PostSeal); Ok(self.inner.verify(origin, header, justification, body).expect("verification failed!")) } } @@ -219,6 +252,8 @@ impl TestNetFactory for BabeTestNet { client.clone(), ).expect("can initialize block-import"); + let block_import = PanickingBlockImport(block_import); + let data_block_import = Mutex::new(Some(Box::new(block_import.clone()) as BoxBlockImport<_>)); ( Box::new(block_import), @@ -229,7 +264,6 @@ impl TestNetFactory for BabeTestNet { ) } - /// KLUDGE: this function gets the mutator from thread-local storage. fn make_verifier( &self, client: PeersClient, @@ -253,7 +287,7 @@ impl TestNetFactory for BabeTestNet { epoch_changes: data.link.epoch_changes.clone(), time_source: data.link.time_source.clone(), }, - mutator: MUTATOR.with(|s| s.borrow().clone()), + mutator: MUTATOR.with(|m| m.borrow().clone()), } } @@ -288,8 +322,13 @@ fn rejects_empty_block() { }) } -fn run_one_test() { +fn run_one_test( + mutator: impl Fn(&mut TestHeader, Stage) + Send + Sync + 'static, +) { let _ = env_logger::try_init(); + let mutator = Arc::new(mutator) as Mutator; + + MUTATOR.with(|m| *m.borrow_mut() = mutator.clone()); let net = BabeTestNet::new(3); let peers = &[ @@ -302,6 +341,7 @@ fn run_one_test() { let mut import_notifications = Vec::new(); let mut runtime = current_thread::Runtime::new().unwrap(); let mut keystore_paths = Vec::new(); + for (peer_id, seed) in peers { let mut net = net.lock(); let peer = net.peer(*peer_id); @@ -322,6 +362,7 @@ fn run_one_test() { client: client.clone(), config: data.link.config.clone(), epoch_changes: data.link.epoch_changes.clone(), + mutator: mutator.clone(), }; import_notifications.push( @@ -367,47 +408,40 @@ fn run_one_test() { #[test] fn authoring_blocks() { - MUTATOR.with(|s| *s.borrow_mut() = Arc::new(move |_| ())); - run_one_test() + run_one_test(|_, _| ()) } #[test] #[should_panic] fn rejects_missing_inherent_digest() { - MUTATOR.with(|s| *s.borrow_mut() = Arc::new(move |header: &mut TestHeader| { + run_one_test(|header: &mut TestHeader, stage| { let v = std::mem::replace(&mut header.digest_mut().logs, vec![]); header.digest_mut().logs = v.into_iter() - .filter(|v| v.as_babe_pre_digest().is_none()) + .filter(|v| stage == Stage::PostSeal || v.as_babe_pre_digest().is_none()) .collect() - })); - run_one_test() + }) } #[test] #[should_panic] fn rejects_missing_seals() { - MUTATOR.with(|s| *s.borrow_mut() = Arc::new(move |header: &mut TestHeader| { + run_one_test(|header: &mut TestHeader, stage| { let v = std::mem::replace(&mut header.digest_mut().logs, vec![]); header.digest_mut().logs = v.into_iter() - .filter(|v| v.as_babe_seal().is_none()) + .filter(|v| stage == Stage::PreSeal || v.as_babe_seal().is_none()) .collect() - })); - run_one_test() + }) } -// TODO: this test assumes that the test runtime will trigger epoch changes -// which isn't the case since it doesn't include the session module. #[test] #[should_panic] -#[ignore] fn rejects_missing_consensus_digests() { - MUTATOR.with(|s| *s.borrow_mut() = Arc::new(move |header: &mut TestHeader| { + run_one_test(|header: &mut TestHeader, stage| { let v = std::mem::replace(&mut header.digest_mut().logs, vec![]); header.digest_mut().logs = v.into_iter() - .filter(|v| v.as_next_epoch_descriptor().is_none()) + .filter(|v| stage == Stage::PostSeal || v.as_next_epoch_descriptor().is_none()) .collect() - })); - run_one_test() + }); } #[test] diff --git a/core/utils/fork-tree/src/lib.rs b/core/utils/fork-tree/src/lib.rs index 90532181aa85b..42999187558fa 100644 --- a/core/utils/fork-tree/src/lib.rs +++ b/core/utils/fork-tree/src/lib.rs @@ -210,7 +210,7 @@ impl ForkTree where self.node_iter().map(|node| (&node.hash, &node.number, &node.data)) } - /// Find a node in the tree that is the lowest ancestor of the given + /// Find a node in the tree that is the deepest ancestor of the given /// block hash and which passes the given predicate. The given function /// `is_descendent_of` should return `true` if the second hash (target) /// is a descendent of the first hash (base).