diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 41c00169e5412..c9e171dbed887 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -111,7 +111,7 @@ use sp_api::{ApiExt, ProvideRuntimeApi}; use sp_application_crypto::AppKey; use sp_block_builder::BlockBuilder as BlockBuilderApi; use sp_blockchain::{ - Backend as _, Error as ClientError, ForkBackend, HeaderBackend, HeaderMetadata, + Backend as _, BlockStatus, Error as ClientError, ForkBackend, HeaderBackend, HeaderMetadata, Result as ClientResult, }; use sp_consensus::{ @@ -1133,11 +1133,17 @@ where let hash = block.header.hash(); let parent_hash = *block.header.parent_hash(); - if block.with_state() { - // When importing whole state we don't calculate epoch descriptor, but rather - // read it from the state after import. We also skip all verifications - // because there's no parent state and we trust the sync module to verify - // that the state is correct and finalized. + let info = self.client.info(); + let number = *block.header.number(); + + if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) || block.with_state() { + // Verification for imported blocks is skipped in two cases: + // 1. When importing blocks below the last finalized block during network initial + // synchronization. + // 2. When importing whole state we don't calculate epoch descriptor, but rather + // read it from the state after import. We also skip all verifications + // because there's no parent state and we trust the sync module to verify + // that the state is correct and finalized. return Ok((block, Default::default())) } @@ -1399,18 +1405,24 @@ where ) -> Result { let hash = block.post_hash(); let number = *block.header.number(); + let info = self.client.info(); - // early exit if block already in chain, otherwise the check for - // epoch changes will error when trying to re-import an epoch change - match self.client.status(hash) { - Ok(sp_blockchain::BlockStatus::InChain) => { - // When re-importing existing block strip away intermediates. - let _ = block.remove_intermediate::>(INTERMEDIATE_KEY); - block.fork_choice = Some(ForkChoiceStrategy::Custom(false)); - return self.inner.import_block(block, new_cache).await.map_err(Into::into) - }, - Ok(sp_blockchain::BlockStatus::Unknown) => {}, - Err(e) => return Err(ConsensusError::ClientImport(e.to_string())), + let block_status = self + .client + .status(hash) + .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; + + // Skip babe logic if block already in chain or importing blocks during initial sync, + // otherwise the check for epoch changes will error because trying to re-import an + // epoch change or because of missing epoch data in the tree, respectivelly. + if info.block_gap.map_or(false, |(s, e)| s <= number && number <= e) || + block_status == BlockStatus::InChain + { + // When re-importing existing block strip away intermediates. + // In case of initial sync intermediates should not be present... + let _ = block.remove_intermediate::>(INTERMEDIATE_KEY); + block.fork_choice = Some(ForkChoiceStrategy::Custom(false)); + return self.inner.import_block(block, new_cache).await.map_err(Into::into) } if block.with_state() { @@ -1506,8 +1518,6 @@ where )), } - let info = self.client.info(); - if let Some(next_epoch_descriptor) = next_epoch_digest { old_epoch_changes = Some((*epoch_changes).clone()); @@ -1701,9 +1711,6 @@ where Client: HeaderBackend + HeaderMetadata, { let info = client.info(); - if info.block_gap.is_none() { - epoch_changes.clear_gap(); - } let finalized_slot = { let finalized_header = client diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index c213a49b8e4e4..7784a99437626 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -320,106 +320,6 @@ impl AsRef for IncrementedEpoch { } } -/// A pair of epochs for the gap block download validation. -/// Block gap is created after the warp sync is complete. Blocks -/// are imported both at the tip of the chain and at the start of the gap. -/// This holds a pair of epochs that are required to validate headers -/// at the start of the gap. Since gap download does not allow forks we don't -/// need to keep a tree of epochs. -#[derive(Clone, Encode, Decode, Debug)] -pub struct GapEpochs { - current: (Hash, Number, PersistedEpoch), - next: Option<(Hash, Number, E)>, -} - -impl GapEpochs -where - Hash: Copy + PartialEq + std::fmt::Debug, - Number: Copy + PartialEq + std::fmt::Debug, - E: Epoch, -{ - /// Check if given slot matches one of the gap epochs. - /// Returns epoch identifier if it does. - fn matches( - &self, - slot: E::Slot, - ) -> Option<(Hash, Number, EpochHeader, EpochIdentifierPosition)> { - match &self.current { - (_, _, PersistedEpoch::Genesis(epoch_0, _)) - if slot >= epoch_0.start_slot() && slot < epoch_0.end_slot() => - return Some(( - self.current.0, - self.current.1, - epoch_0.into(), - EpochIdentifierPosition::Genesis0, - )), - (_, _, PersistedEpoch::Genesis(_, epoch_1)) - if slot >= epoch_1.start_slot() && slot < epoch_1.end_slot() => - return Some(( - self.current.0, - self.current.1, - epoch_1.into(), - EpochIdentifierPosition::Genesis1, - )), - (_, _, PersistedEpoch::Regular(epoch_n)) - if slot >= epoch_n.start_slot() && slot < epoch_n.end_slot() => - return Some(( - self.current.0, - self.current.1, - epoch_n.into(), - EpochIdentifierPosition::Regular, - )), - _ => {}, - }; - match &self.next { - Some((h, n, epoch_n)) if slot >= epoch_n.start_slot() && slot < epoch_n.end_slot() => - Some((*h, *n, epoch_n.into(), EpochIdentifierPosition::Regular)), - _ => None, - } - } - - /// Returns epoch data if it matches given identifier. - pub fn epoch(&self, id: &EpochIdentifier) -> Option<&E> { - match (&self.current, &self.next) { - ((h, n, e), _) if h == &id.hash && n == &id.number => match e { - PersistedEpoch::Genesis(ref epoch_0, _) - if id.position == EpochIdentifierPosition::Genesis0 => - Some(epoch_0), - PersistedEpoch::Genesis(_, ref epoch_1) - if id.position == EpochIdentifierPosition::Genesis1 => - Some(epoch_1), - PersistedEpoch::Regular(ref epoch_n) - if id.position == EpochIdentifierPosition::Regular => - Some(epoch_n), - _ => None, - }, - (_, Some((h, n, e))) - if h == &id.hash && - n == &id.number && id.position == EpochIdentifierPosition::Regular => - Some(e), - _ => None, - } - } - - /// Import a new gap epoch, potentially replacing an old epoch. - fn import(&mut self, slot: E::Slot, hash: Hash, number: Number, epoch: E) -> Result<(), E> { - match (&mut self.current, &mut self.next) { - ((_, _, PersistedEpoch::Genesis(_, epoch_1)), _) if slot == epoch_1.end_slot() => { - self.next = Some((hash, number, epoch)); - Ok(()) - }, - (_, Some((_, _, epoch_n))) if slot == epoch_n.end_slot() => { - let (cur_h, cur_n, cur_epoch) = - self.next.take().expect("Already matched as `Some`"); - self.current = (cur_h, cur_n, PersistedEpoch::Regular(cur_epoch)); - self.next = Some((hash, number, epoch)); - Ok(()) - }, - _ => Err(epoch), - } - } -} - /// Tree of all epoch changes across all *seen* forks. Data stored in tree is /// the hash and block number of the block signaling the epoch change, and the /// epoch that was signalled at that block. @@ -435,14 +335,10 @@ where /// same DAG entry, pinned to a specific block #1. /// /// Further epochs (epoch_2, ..., epoch_n) each get their own entry. -/// -/// Also maintains a pair of epochs for the start of the gap, -/// as long as there's an active gap download after a warp sync. #[derive(Clone, Encode, Decode, Debug)] pub struct EpochChanges { inner: ForkTree>, epochs: BTreeMap<(Hash, Number), PersistedEpoch>, - gap: Option>, } // create a fake header hash which hasn't been included in the chain. @@ -460,7 +356,7 @@ where Number: Ord, { fn default() -> Self { - EpochChanges { inner: ForkTree::new(), epochs: BTreeMap::new(), gap: None } + EpochChanges { inner: ForkTree::new(), epochs: BTreeMap::new() } } } @@ -480,11 +376,6 @@ where self.inner.rebalance() } - /// Clear gap epochs if any. - pub fn clear_gap(&mut self) { - self.gap = None; - } - /// Map the epoch changes from one storing data to a different one. pub fn map(self, mut f: F) -> EpochChanges where @@ -493,10 +384,6 @@ where { EpochChanges { inner: self.inner.map(&mut |_, _, header: PersistedEpochHeader| header.map()), - gap: self.gap.map(|GapEpochs { current: (h, n, header), next }| GapEpochs { - current: (h, n, header.map(&h, &n, &mut f)), - next: next.map(|(h, n, e)| (h, n, f(&h, &n, e))), - }), epochs: self .epochs .into_iter() @@ -536,9 +423,6 @@ where /// Get a reference to an epoch with given identifier. pub fn epoch(&self, id: &EpochIdentifier) -> Option<&E> { - if let Some(e) = &self.gap.as_ref().and_then(|gap| gap.epoch(id)) { - return Some(e) - } self.epochs.get(&(id.hash, id.number)).and_then(|v| match v { PersistedEpoch::Genesis(ref epoch_0, _) if id.position == EpochIdentifierPosition::Genesis0 => @@ -665,15 +549,6 @@ where return Ok(Some(ViableEpochDescriptor::UnimportedGenesis(slot))) } - if let Some(gap) = &self.gap { - if let Some((hash, number, hdr, position)) = gap.matches(slot) { - return Ok(Some(ViableEpochDescriptor::Signaled( - EpochIdentifier { position, hash, number }, - hdr, - ))) - } - } - // 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 @@ -744,28 +619,9 @@ where ) -> Result<(), fork_tree::Error> { let is_descendent_of = descendent_of_builder.build_is_descendent_of(Some((hash, parent_hash))); - let slot = epoch.as_ref().start_slot(); - let IncrementedEpoch(mut epoch) = epoch; + let IncrementedEpoch(epoch) = epoch; let header = PersistedEpochHeader::::from(&epoch); - if let Some(gap) = &mut self.gap { - if let PersistedEpoch::Regular(e) = epoch { - epoch = match gap.import(slot, hash, number, e) { - Ok(()) => return Ok(()), - Err(e) => PersistedEpoch::Regular(e), - } - } - } else if epoch.is_genesis() && - !self.epochs.is_empty() && - !self.epochs.values().any(|e| e.is_genesis()) - { - // There's a genesis epoch imported when we already have an active epoch. - // This happens after the warp sync as the ancient blocks download start. - // We need to start tracking gap epochs here. - self.gap = Some(GapEpochs { current: (hash, number, epoch), next: None }); - return Ok(()) - } - let res = self.inner.import(hash, number, header, &is_descendent_of); match res { @@ -1278,288 +1134,4 @@ mod tests { list.sort(); assert_eq!(list, vec![b"A", b"G", b"L"]); } - - /// Test that ensures that the gap is not enabled when we import multiple genesis blocks. - #[test] - fn gap_is_not_enabled_when_multiple_genesis_epochs_are_imported() { - // X - // / - // 0 - A - // - let is_descendent_of = |base: &Hash, block: &Hash| -> Result { - match (base, *block) { - (b"0", _) => Ok(true), - _ => Ok(false), - } - }; - - let duration = 100; - - let make_genesis = |slot| Epoch { start_slot: slot, duration }; - - let mut epoch_changes = EpochChanges::new(); - let next_descriptor = (); - - // insert genesis epoch for A - { - let genesis_epoch_a_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 100) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&genesis_epoch_a_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"A", 1, *b"0", incremented_epoch) - .unwrap(); - } - - // insert genesis epoch for X - { - let genesis_epoch_x_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 1000) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&genesis_epoch_x_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"X", 1, *b"0", incremented_epoch) - .unwrap(); - } - - // Clearing the gap should be a no-op. - epoch_changes.clear_gap(); - - // Check that both epochs are available. - let epoch_a = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"A", 1, 101, &make_genesis) - .unwrap() - .unwrap(); - - let epoch_x = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"X", 1, 1001, &make_genesis) - .unwrap() - .unwrap(); - - assert!(epoch_a != epoch_x) - } - - #[test] - fn gap_epochs_advance() { - // 0 - 1 - 2 - 3 - .... 42 - 43 - let is_descendent_of = |base: &Hash, block: &Hash| -> Result { - match (base, *block) { - (b"0", _) => Ok(true), - (b"1", b) => Ok(b == *b"0"), - (b"2", b) => Ok(b == *b"1"), - (b"3", b) => Ok(b == *b"2"), - _ => Ok(false), - } - }; - - let duration = 100; - - let make_genesis = |slot| Epoch { start_slot: slot, duration }; - - let mut epoch_changes = EpochChanges::new(); - let next_descriptor = (); - - let epoch42 = Epoch { start_slot: 42, duration: 100 }; - let epoch43 = Epoch { start_slot: 43, duration: 100 }; - epoch_changes.reset(*b"0", *b"1", 4200, epoch42, epoch43); - assert!(epoch_changes.gap.is_none()); - - // Import a new genesis epoch, this should crate the gap. - let genesis_epoch_a_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 100) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&genesis_epoch_a_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"1", 1, *b"0", incremented_epoch) - .unwrap(); - assert!(epoch_changes.gap.is_some()); - - let genesis_epoch = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 100) - .unwrap() - .unwrap(); - - assert_eq!(genesis_epoch, ViableEpochDescriptor::UnimportedGenesis(100)); - - // Import more epochs and check that gap advances. - let import_epoch_1 = - epoch_changes.viable_epoch(&genesis_epoch, &make_genesis).unwrap().increment(()); - - let epoch_1 = import_epoch_1.as_ref().clone(); - epoch_changes - .import(&is_descendent_of, *b"1", 1, *b"0", import_epoch_1) - .unwrap(); - let genesis_epoch_data = epoch_changes.epoch_data(&genesis_epoch, &make_genesis).unwrap(); - let end_slot = genesis_epoch_data.end_slot(); - let x = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"1", 1, end_slot, &make_genesis) - .unwrap() - .unwrap(); - - assert_eq!(x, epoch_1); - assert_eq!(epoch_changes.gap.as_ref().unwrap().current.0, *b"1"); - assert!(epoch_changes.gap.as_ref().unwrap().next.is_none()); - - let epoch_1_desriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"1", 1, end_slot) - .unwrap() - .unwrap(); - let epoch_1 = epoch_changes.epoch_data(&epoch_1_desriptor, &make_genesis).unwrap(); - let import_epoch_2 = epoch_changes - .viable_epoch(&epoch_1_desriptor, &make_genesis) - .unwrap() - .increment(()); - let epoch_2 = import_epoch_2.as_ref().clone(); - epoch_changes - .import(&is_descendent_of, *b"2", 2, *b"1", import_epoch_2) - .unwrap(); - - let end_slot = epoch_1.end_slot(); - let x = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"2", 2, end_slot, &make_genesis) - .unwrap() - .unwrap(); - assert_eq!(epoch_changes.gap.as_ref().unwrap().current.0, *b"1"); - assert_eq!(epoch_changes.gap.as_ref().unwrap().next.as_ref().unwrap().0, *b"2"); - assert_eq!(x, epoch_2); - - let epoch_2_desriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"2", 2, end_slot) - .unwrap() - .unwrap(); - let import_epoch_3 = epoch_changes - .viable_epoch(&epoch_2_desriptor, &make_genesis) - .unwrap() - .increment(()); - epoch_changes - .import(&is_descendent_of, *b"3", 3, *b"2", import_epoch_3) - .unwrap(); - - assert_eq!(epoch_changes.gap.as_ref().unwrap().current.0, *b"2"); - - epoch_changes.clear_gap(); - assert!(epoch_changes.gap.is_none()); - } - - /// Test that ensures that the gap is not enabled when there's still genesis - /// epochs imported, regardless of whether there are already other further - /// epochs imported descending from such genesis epochs. - #[test] - fn gap_is_not_enabled_when_at_least_one_genesis_epoch_is_still_imported() { - // A (#1) - B (#201) - // / - // 0 - C (#1) - // - // The epoch duration is 100 slots, each of these blocks represents - // an epoch change block. block B starts a new epoch at #201 since the - // genesis epoch spans two epochs. - - let is_descendent_of = |base: &Hash, block: &Hash| -> Result { - match (base, block) { - (b"0", _) => Ok(true), - (b"A", b"B") => Ok(true), - _ => Ok(false), - } - }; - - let duration = 100; - let make_genesis = |slot| Epoch { start_slot: slot, duration }; - let mut epoch_changes = EpochChanges::new(); - let next_descriptor = (); - - // insert genesis epoch for A at slot 1 - { - let genesis_epoch_a_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 1) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&genesis_epoch_a_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"A", 1, *b"0", incremented_epoch) - .unwrap(); - } - - // insert regular epoch for B at slot 201, descending from A - { - let epoch_b_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"A", 1, 201) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&epoch_b_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"B", 201, *b"A", incremented_epoch) - .unwrap(); - } - - // insert genesis epoch for C at slot 1000 - { - let genesis_epoch_x_descriptor = epoch_changes - .epoch_descriptor_for_child_of(&is_descendent_of, b"0", 0, 1000) - .unwrap() - .unwrap(); - - let incremented_epoch = epoch_changes - .viable_epoch(&genesis_epoch_x_descriptor, &make_genesis) - .unwrap() - .increment(next_descriptor); - - epoch_changes - .import(&is_descendent_of, *b"C", 1, *b"0", incremented_epoch) - .unwrap(); - } - - // Clearing the gap should be a no-op. - epoch_changes.clear_gap(); - - // Check that all three epochs are available. - let epoch_a = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"A", 1, 10, &make_genesis) - .unwrap() - .unwrap(); - - let epoch_b = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"B", 201, 201, &make_genesis) - .unwrap() - .unwrap(); - - assert!(epoch_a != epoch_b); - - // the genesis epoch A will span slots [1, 200] with epoch B starting at slot 201 - assert_eq!(epoch_b.start_slot(), 201); - - let epoch_c = epoch_changes - .epoch_data_for_child_of(&is_descendent_of, b"C", 1, 1001, &make_genesis) - .unwrap() - .unwrap(); - - assert!(epoch_a != epoch_c); - } } diff --git a/client/consensus/epochs/src/migration.rs b/client/consensus/epochs/src/migration.rs index c4ed47e9c1c05..052d0b7be4a53 100644 --- a/client/consensus/epochs/src/migration.rs +++ b/client/consensus/epochs/src/migration.rs @@ -64,7 +64,7 @@ where header }); - EpochChanges { inner, epochs, gap: None } + EpochChanges { inner, epochs } } } @@ -75,6 +75,6 @@ where { /// Migrate the type into current epoch changes definition. pub fn migrate(self) -> EpochChanges { - EpochChanges { inner: self.inner, epochs: self.epochs, gap: None } + EpochChanges { inner: self.inner, epochs: self.epochs } } }