From 1d5290e7a589e64dc51b1e9937a1eaf1dfb138d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 21 Jun 2022 23:54:13 +0100 Subject: [PATCH 01/14] babe: allow skipping epochs in pallet --- frame/babe/src/lib.rs | 49 ++++++++++++++++------------ primitives/consensus/babe/src/lib.rs | 21 ++++++++++++ 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index 1effc2c1989fa..17ed725be88a6 100644 --- a/frame/babe/src/lib.rs +++ b/frame/babe/src/lib.rs @@ -574,10 +574,19 @@ impl Pallet { // by the session module to be called before this. debug_assert!(Self::initialized().is_some()); - // Update epoch index - let epoch_index = EpochIndex::::get() - .checked_add(1) - .expect("epoch indices will never reach 2^64 before the death of the universe; qed"); + // Update epoch index. + // + // NOTE: we figure out the epoch index from the slot, which may not + // necessarily be contiguous if the chain was offline for more than + // `T::EpochDuration` slots. When skipping from epoch N to e.g. N+4, we + // will be using the randomness and authorities for that epoch that had + // been previously announced for epoch N+1, and the randomness collected + // during the current epoch (N) will be used for epoch N+5. + let epoch_index = sp_consensus_babe::epoch_index( + CurrentSlot::::get(), + GenesisSlot::::get(), + T::EpochDuration::get(), + ); EpochIndex::::put(epoch_index); Authorities::::put(authorities); @@ -624,11 +633,16 @@ impl Pallet { } } - /// Finds the start slot of the current epoch. only guaranteed to - /// give correct results after `initialize` of the first block - /// in the chain (as its result is based off of `GenesisSlot`). + /// Finds the start slot of the current epoch. + /// + /// Only guaranteed to give correct results after `initialize` of the first + /// block in the chain (as its result is based off of `GenesisSlot`). pub fn current_epoch_start() -> Slot { - Self::epoch_start(EpochIndex::::get()) + sp_consensus_babe::epoch_start_slot( + EpochIndex::::get(), + GenesisSlot::::get(), + T::EpochDuration::get(), + ) } /// Produces information about the current epoch. @@ -652,9 +666,15 @@ impl Pallet { if u64 is not enough we should crash for safety; qed.", ); + let start_slot = sp_consensus_babe::epoch_start_slot( + next_epoch_index, + GenesisSlot::::get(), + T::EpochDuration::get(), + ); + Epoch { epoch_index: next_epoch_index, - start_slot: Self::epoch_start(next_epoch_index), + start_slot, duration: T::EpochDuration::get(), authorities: NextAuthorities::::get().to_vec(), randomness: NextRandomness::::get(), @@ -666,17 +686,6 @@ impl Pallet { } } - fn epoch_start(epoch_index: u64) -> Slot { - // (epoch_index * epoch_duration) + genesis_slot - - const PROOF: &str = "slot number is u64; it should relate in some way to wall clock time; \ - if u64 is not enough we should crash for safety; qed."; - - let epoch_start = epoch_index.checked_mul(T::EpochDuration::get()).expect(PROOF); - - epoch_start.checked_add(*GenesisSlot::::get()).expect(PROOF).into() - } - fn deposit_consensus(new: U) { let log = DigestItem::Consensus(BABE_ENGINE_ID, new.encode()); >::deposit_log(log) diff --git a/primitives/consensus/babe/src/lib.rs b/primitives/consensus/babe/src/lib.rs index 492d1a9a7238f..100bdaf9d6f63 100644 --- a/primitives/consensus/babe/src/lib.rs +++ b/primitives/consensus/babe/src/lib.rs @@ -353,6 +353,27 @@ pub struct Epoch { pub config: BabeEpochConfiguration, } +/// Returns the epoch index the given slot belongs to. +pub fn epoch_index(slot: Slot, genesis_slot: Slot, epoch_duration: u64) -> u64 { + if slot < genesis_slot { + return 0 + } + + (*slot - *genesis_slot) / epoch_duration +} + +/// Returns the first slot at the given epoch index. +pub fn epoch_start_slot(epoch_index: u64, genesis_slot: Slot, epoch_duration: u64) -> Slot { + // (epoch_index * epoch_duration) + genesis_slot + + const PROOF: &str = "slot number is u64; it should relate in some way to wall clock time; \ + if u64 is not enough we should crash for safety; qed."; + + let epoch_start = epoch_index.checked_mul(epoch_duration).expect(PROOF); + + epoch_start.checked_add(*genesis_slot).expect(PROOF).into() +} + sp_api::decl_runtime_apis! { /// API necessary for block authorship with BABE. #[api_version(2)] From 5df7281c53c9175b729c3001eca80ce586043046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 22 Jun 2022 00:01:04 +0100 Subject: [PATCH 02/14] babe: detect and skip epochs on client --- client/consensus/babe/src/lib.rs | 62 ++++++++++++++++++++++++++++-- client/consensus/epochs/src/lib.rs | 22 +++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 8478122375319..0a55c5868330f 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -126,7 +126,7 @@ use sp_inherents::{CreateInherentDataProviders, InherentData, InherentDataProvid use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; use sp_runtime::{ generic::{BlockId, OpaqueDigestItemId}, - traits::{Block as BlockT, Header, NumberFor, SaturatedConversion, Saturating, Zero}, + traits::{Block as BlockT, Header, NumberFor, One, SaturatedConversion, Saturating, Zero}, DigestItem, }; @@ -1355,15 +1355,43 @@ pub struct BabeBlockImport { inner: I, client: Arc, epoch_changes: SharedEpochChanges, + genesis_slot: Option, config: Config, } +impl BabeBlockImport +where + Client: HeaderBackend, +{ + fn genesis_slot(&mut self) -> Option { + if self.genesis_slot.is_some() { + return self.genesis_slot.clone() + } + + let genesis_slot = + self.client.header(BlockId::Number(One::one())).ok().flatten().map(|header| { + let pre_digest = find_pre_digest::(&header).expect( + "valid babe headers must contain a predigest; header has been already verified; qed", + ); + + pre_digest.slot() + }); + + if genesis_slot.is_some() { + self.genesis_slot = genesis_slot.clone(); + } + + genesis_slot + } +} + impl Clone for BabeBlockImport { fn clone(&self) -> Self { BabeBlockImport { inner: self.inner.clone(), client: self.client.clone(), epoch_changes: self.epoch_changes.clone(), + genesis_slot: self.genesis_slot.clone(), config: self.config.clone(), } } @@ -1376,7 +1404,7 @@ impl BabeBlockImport { block_import: I, config: Config, ) -> Self { - BabeBlockImport { client, inner: block_import, epoch_changes, config } + BabeBlockImport { client, inner: block_import, epoch_changes, config, genesis_slot: None } } } @@ -1515,6 +1543,8 @@ where )) } + let genesis_slot = self.genesis_slot(); + // if there's a pending epoch we'll save the previous epoch changes here // this way we can revert it if there's any error let mut old_epoch_changes = None; @@ -1581,8 +1611,8 @@ where if let Some(next_epoch_descriptor) = next_epoch_digest { old_epoch_changes = Some((*epoch_changes).clone()); - let viable_epoch = epoch_changes - .viable_epoch(&epoch_descriptor, |slot| { + let mut viable_epoch = epoch_changes + .viable_epoch_mut(&epoch_descriptor, |slot| { Epoch::genesis(&self.config.genesis_config, slot) }) .ok_or_else(|| { @@ -1600,6 +1630,29 @@ where log::Level::Info }; + if let Some(genesis_slot) = genesis_slot { + let epoch_index = sp_consensus_babe::epoch_index( + slot, + genesis_slot, + viable_epoch.as_ref().duration, + ); + + if epoch_index != viable_epoch.as_ref().epoch_index { + warn!(target: "babe", "👶 Epoch(s) skipped: from {} to {}", + viable_epoch.as_ref().epoch_index, epoch_index, + ); + + let epoch_start = sp_consensus_babe::epoch_start_slot( + epoch_index, + genesis_slot, + viable_epoch.as_ref().duration, + ); + + viable_epoch.as_mut().epoch_index = epoch_index; + viable_epoch.as_mut().start_slot = epoch_start; + } + } + log!(target: "babe", log_level, "👶 New epoch {} launching at block {} (block slot {} >= start slot {}).", @@ -1627,6 +1680,7 @@ where let prune_and_import = || { prune_finalized(self.client.clone(), &mut epoch_changes)?; + epoch_changes.sync(); epoch_changes .import( descendent_query(&*self.client), diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index 3a943e4851a4d..c59fcd512851c 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -505,6 +505,28 @@ where } } + pub fn sync(&mut self) { + self.inner = self.inner.clone().map(&mut |hash, number, epoch_header| { + let epoch = self.epochs.get(&(*hash, *number)).unwrap(); + + let epoch_data = match epoch { + PersistedEpoch::Genesis(..) => return epoch_header, + PersistedEpoch::Regular(epoch_data) => epoch_data, + }; + + let epoch_header = match epoch_header { + PersistedEpochHeader::Genesis(..) => epoch_header, + PersistedEpochHeader::Regular(mut epoch_header) => { + epoch_header.start_slot = epoch_data.start_slot(); + epoch_header.end_slot = epoch_data.end_slot(); + PersistedEpochHeader::Regular(epoch_header) + }, + }; + + epoch_header + }); + } + /// Prune out finalized epochs, except for the ancestor of the finalized /// block. The given slot should be the slot number at which the finalized /// block was authored. From 2641a3fa3360b942aace5d4dd08d5403bd018245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:35:06 +0100 Subject: [PATCH 03/14] babe: cleaner epoch util functions --- primitives/consensus/babe/src/lib.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/primitives/consensus/babe/src/lib.rs b/primitives/consensus/babe/src/lib.rs index 295e8ad36bc0d..cb44afcb8d4e4 100644 --- a/primitives/consensus/babe/src/lib.rs +++ b/primitives/consensus/babe/src/lib.rs @@ -362,11 +362,7 @@ pub struct Epoch { /// Returns the epoch index the given slot belongs to. pub fn epoch_index(slot: Slot, genesis_slot: Slot, epoch_duration: u64) -> u64 { - if slot < genesis_slot { - return 0 - } - - (*slot - *genesis_slot) / epoch_duration + *slot.saturating_sub(genesis_slot) / epoch_duration } /// Returns the first slot at the given epoch index. @@ -376,9 +372,11 @@ pub fn epoch_start_slot(epoch_index: u64, genesis_slot: Slot, epoch_duration: u6 const PROOF: &str = "slot number is u64; it should relate in some way to wall clock time; \ if u64 is not enough we should crash for safety; qed."; - let epoch_start = epoch_index.checked_mul(epoch_duration).expect(PROOF); - - epoch_start.checked_add(*genesis_slot).expect(PROOF).into() + epoch_index + .checked_mul(epoch_duration) + .and_then(|slot| slot.checked_add(*genesis_slot)) + .expect(PROOF) + .into() } sp_api::decl_runtime_apis! { From 04a18acaa842319f6faa9b02433da26471e66101 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:35:44 +0100 Subject: [PATCH 04/14] babe: add test for runtime handling of skipped epochs --- frame/babe/src/tests.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs index d4132e6378540..0b8a02547144b 100644 --- a/frame/babe/src/tests.rs +++ b/frame/babe/src/tests.rs @@ -948,3 +948,34 @@ fn generate_equivocation_report_blob() { println!("equivocation_proof.encode(): {:?}", equivocation_proof.encode()); }); } + +#[test] +fn skipping_over_epochs_works() { + let mut ext = new_test_ext(3); + + ext.execute_with(|| { + let epoch_duration: u64 = ::EpochDuration::get(); + + // this sets the genesis slot to 100; + let genesis_slot = 100; + go_to_block(1, genesis_slot); + + // we will author all blocks from epoch #0 and arrive at a point where + // we are in epoch #1. we should already have the randomness ready that + // will be used in epoch #2 + progress_to_block(epoch_duration + 1); + assert_eq!(EpochIndex::::get(), 1); + + // genesis randomness is an array of zeros + let randomness_for_epoch_2 = NextRandomness::::get(); + assert!(randomness_for_epoch_2 != [0; 32]); + + // we will now create a block for a slot that is part of epoch #4. + // we should appropriately increment the epoch index as well as re-use + // the randomness from epoch #2 on epoch #4 + go_to_block(System::block_number() + 1, genesis_slot + epoch_duration * 4); + + assert_eq!(EpochIndex::::get(), 4); + assert_eq!(Randomness::::get(), randomness_for_epoch_2); + }); +} From 402d871d39015233af0f72898ed217657507cb86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:38:12 +0100 Subject: [PATCH 05/14] babe: simpler implementation of client handling of skipped epochs --- client/consensus/babe/src/lib.rs | 63 ++++++------------------------ client/consensus/epochs/src/lib.rs | 22 ----------- 2 files changed, 12 insertions(+), 73 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 55925f87c8fb8..2b658951367ac 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -123,7 +123,7 @@ use sp_inherents::{CreateInherentDataProviders, InherentData, InherentDataProvid use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; use sp_runtime::{ generic::{BlockId, OpaqueDigestItemId}, - traits::{Block as BlockT, Header, NumberFor, One, SaturatedConversion, Saturating, Zero}, + traits::{Block as BlockT, Header, NumberFor, SaturatedConversion, Saturating, Zero}, DigestItem, }; @@ -1299,43 +1299,15 @@ pub struct BabeBlockImport { inner: I, client: Arc, epoch_changes: SharedEpochChanges, - genesis_slot: Option, config: BabeConfiguration, } -impl BabeBlockImport -where - Client: HeaderBackend, -{ - fn genesis_slot(&mut self) -> Option { - if self.genesis_slot.is_some() { - return self.genesis_slot.clone() - } - - let genesis_slot = - self.client.header(BlockId::Number(One::one())).ok().flatten().map(|header| { - let pre_digest = find_pre_digest::(&header).expect( - "valid babe headers must contain a predigest; header has been already verified; qed", - ); - - pre_digest.slot() - }); - - if genesis_slot.is_some() { - self.genesis_slot = genesis_slot.clone(); - } - - genesis_slot - } -} - impl Clone for BabeBlockImport { fn clone(&self) -> Self { BabeBlockImport { inner: self.inner.clone(), client: self.client.clone(), epoch_changes: self.epoch_changes.clone(), - genesis_slot: self.genesis_slot.clone(), config: self.config.clone(), } } @@ -1348,7 +1320,7 @@ impl BabeBlockImport { block_import: I, config: BabeConfiguration, ) -> Self { - BabeBlockImport { client, inner: block_import, epoch_changes, config, genesis_slot: None } + BabeBlockImport { client, inner: block_import, epoch_changes, config } } } @@ -1487,8 +1459,6 @@ where )) } - let genesis_slot = self.genesis_slot(); - // if there's a pending epoch we'll save the previous epoch changes here // this way we can revert it if there's any error let mut old_epoch_changes = None; @@ -1572,27 +1542,19 @@ where log::Level::Info }; - if let Some(genesis_slot) = genesis_slot { - let epoch_index = sp_consensus_babe::epoch_index( - slot, - genesis_slot, - viable_epoch.as_ref().duration, - ); + if viable_epoch.as_ref().end_slot() <= slot { + let mut epoch_data = viable_epoch.as_mut(); + let skipped_epochs = (*slot - *epoch_data.start_slot) / epoch_data.duration; - if epoch_index != viable_epoch.as_ref().epoch_index { - warn!(target: "babe", "👶 Epoch(s) skipped: from {} to {}", - viable_epoch.as_ref().epoch_index, epoch_index, - ); + let original_epoch_index = epoch_data.epoch_index; - let epoch_start = sp_consensus_babe::epoch_start_slot( - epoch_index, - genesis_slot, - viable_epoch.as_ref().duration, - ); + epoch_data.epoch_index += skipped_epochs; + epoch_data.start_slot = + Slot::from(*epoch_data.start_slot + skipped_epochs * epoch_data.duration); - viable_epoch.as_mut().epoch_index = epoch_index; - viable_epoch.as_mut().start_slot = epoch_start; - } + warn!(target: "babe", "👶 Epoch(s) skipped: from {} to {}", + original_epoch_index, epoch_data.epoch_index, + ); } log!(target: "babe", @@ -1622,7 +1584,6 @@ where let prune_and_import = || { prune_finalized(self.client.clone(), &mut epoch_changes)?; - epoch_changes.sync(); epoch_changes .import( descendent_query(&*self.client), diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index c822be196f572..2e0186495db5e 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -505,28 +505,6 @@ where } } - pub fn sync(&mut self) { - self.inner = self.inner.clone().map(&mut |hash, number, epoch_header| { - let epoch = self.epochs.get(&(*hash, *number)).unwrap(); - - let epoch_data = match epoch { - PersistedEpoch::Genesis(..) => return epoch_header, - PersistedEpoch::Regular(epoch_data) => epoch_data, - }; - - let epoch_header = match epoch_header { - PersistedEpochHeader::Genesis(..) => epoch_header, - PersistedEpochHeader::Regular(mut epoch_header) => { - epoch_header.start_slot = epoch_data.start_slot(); - epoch_header.end_slot = epoch_data.end_slot(); - PersistedEpochHeader::Regular(epoch_header) - }, - }; - - epoch_header - }); - } - /// Prune out finalized epochs, except for the ancestor of the finalized /// block. The given slot should be the slot number at which the finalized /// block was authored. From e2a58e9899ca1a4e11a35d03430c988719490b13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:39:09 +0100 Subject: [PATCH 06/14] babe: test client-side handling of skipped epochs --- client/consensus/babe/src/tests.rs | 112 +++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 58f5e7b8eb6d4..98f1c074c16d3 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -27,6 +27,7 @@ use rand_chacha::{rand_core::SeedableRng, ChaChaRng}; use sc_block_builder::{BlockBuilder, BlockBuilderProvider}; use sc_client_api::{backend::TransactionFor, BlockchainEvents, Finalizer}; use sc_consensus::{BoxBlockImport, BoxJustificationImport}; +use sc_consensus_epochs::{EpochIdentifier, EpochIdentifierPosition}; use sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging; use sc_keystore::LocalKeystore; use sc_network_test::{Block as TestBlock, *}; @@ -1069,3 +1070,114 @@ fn obsolete_blocks_aux_data_cleanup() { // Present C4, C5 assert!(aux_data_check(&fork3_hashes, true)); } + +#[test] +fn allows_skipping_epochs() { + let mut net = BabeTestNet::new(1); + + let peer = net.peer(0); + let data = peer.data.as_ref().expect("babe link set up during initialization"); + + let client = peer.client().as_client(); + let mut block_import = data.block_import.lock().take().expect("import set up during init"); + + let mut proposer_factory = DummyFactory { + client: client.clone(), + config: data.link.config.clone(), + epoch_changes: data.link.epoch_changes.clone(), + mutator: Arc::new(|_, _| ()), + }; + + let epoch_changes = data.link.epoch_changes.clone(); + let epoch_length = data.link.config.epoch_length; + + // we create all of the blocks in epoch 0 as well as a block in epoch 1 + let blocks = propose_and_import_blocks( + &client, + &mut proposer_factory, + &mut block_import, + BlockId::Number(0), + epoch_length as usize + 1, + ); + + // the first block in epoch 0 (#1) announces both epoch 0 and 1 (this is a + // special genesis epoch) + let epoch0 = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Genesis0, + hash: blocks[0], + number: 1, + }) + .unwrap() + .clone(); + + assert_eq!(epoch0.epoch_index, 0); + assert_eq!(epoch0.start_slot, Slot::from(1)); + + let epoch1 = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Genesis1, + hash: blocks[0], + number: 1, + }) + .unwrap() + .clone(); + + assert_eq!(epoch1.epoch_index, 1); + assert_eq!(epoch1.start_slot, Slot::from(epoch_length + 1)); + + // the first block in epoch 1 (#7) announces epoch 2. we will be skipping + // this epoch and therefore re-using its data for epoch 3 + let epoch2 = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Regular, + hash: blocks[epoch_length as usize], + number: epoch_length + 1, + }) + .unwrap() + .clone(); + + assert_eq!(epoch2.epoch_index, 2); + assert_eq!(epoch2.start_slot, Slot::from(epoch_length * 2 + 1)); + + // we now author a block that belongs to epoch 3, thereby skipping epoch 2 + let last_block = client.expect_header(BlockId::Hash(*blocks.last().unwrap())).unwrap(); + let block = propose_and_import_block( + &last_block, + Some((epoch_length * 3 + 1).into()), + &mut proposer_factory, + &mut block_import, + ); + + // we check the epoch data that was announced at block #7 (that we checked above) + let epoch3 = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Regular, + hash: blocks[epoch_length as usize], + number: epoch_length + 1, + }) + .unwrap() + .clone(); + + // and it now is updated to start at epoch 3 with an updated slot + assert_eq!(epoch3.epoch_index, 3); + assert_eq!(epoch3.start_slot, Slot::from(epoch_length * 3 + 1)); + + // and the first block in epoch 3 (#8) announces epoch 4 + let epoch = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Regular, + hash: block, + number: epoch_length + 2, + }) + .unwrap() + .clone(); + + assert_eq!(epoch.epoch_index, 4); + assert_eq!(epoch.start_slot, Slot::from(epoch_length * 4 + 1)); +} From 8cc10da59cc170ee69df2b3d554e5ce1004b85de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:50:35 +0100 Subject: [PATCH 07/14] babe: add comments on client-side skipped epochs --- client/consensus/babe/src/lib.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 2b658951367ac..27508a5939cd9 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1543,11 +1543,24 @@ where }; if viable_epoch.as_ref().end_slot() <= slot { + // some epochs must have been skipped as our current slot + // fits outside the current epoch. we will figure out + // which epoch it belongs to and we will re-use the same + // data for that epoch + let mut epoch_data = viable_epoch.as_mut(); let skipped_epochs = (*slot - *epoch_data.start_slot) / epoch_data.duration; let original_epoch_index = epoch_data.epoch_index; + // NOTE: notice that we are only updating the `Epoch` from `EpochChanges` (that + // is stored in the map), and not the `EpochHeader` that is stored in the fork + // tree. next time we search for an epoch for a given slot we will do it + // through the fork tree (which isn't updated), but the reason this works is + // because we will search in-depth in the tree with the predicate + // `epoch.start_slot <= slot` which will still match correctly without the + // updated `start_slot`. the new epoch that will get inserted below (after + // `increment`) will already use a correct `start_slot`. epoch_data.epoch_index += skipped_epochs; epoch_data.start_slot = Slot::from(*epoch_data.start_slot + skipped_epochs * epoch_data.duration); From b81f940b51cedd9f103432d51557453f653d8c83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 18 Oct 2022 22:51:25 +0100 Subject: [PATCH 08/14] babe: remove emptyline --- client/consensus/babe/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 27508a5939cd9..ba83065626b6b 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1547,7 +1547,6 @@ where // fits outside the current epoch. we will figure out // which epoch it belongs to and we will re-use the same // data for that epoch - let mut epoch_data = viable_epoch.as_mut(); let skipped_epochs = (*slot - *epoch_data.start_slot) / epoch_data.duration; From 83332a4a6335440b773cfa279c7c6fb813c822a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 19 Oct 2022 00:40:20 +0100 Subject: [PATCH 09/14] babe: make it resilient to forks --- client/consensus/babe/src/lib.rs | 23 ++++++++------ client/consensus/babe/src/tests.rs | 51 ++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index ba83065626b6b..7b23cd892a8f2 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1526,10 +1526,11 @@ where old_epoch_changes = Some((*epoch_changes).clone()); let mut viable_epoch = epoch_changes - .viable_epoch_mut(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot)) + .viable_epoch(&epoch_descriptor, |slot| Epoch::genesis(&self.config, slot)) .ok_or_else(|| { ConsensusError::ClientImport(Error::::FetchEpoch(parent_hash).into()) - })?; + })? + .into_cloned(); let epoch_config = next_config_digest .map(Into::into) @@ -1552,14 +1553,16 @@ where let original_epoch_index = epoch_data.epoch_index; - // NOTE: notice that we are only updating the `Epoch` from `EpochChanges` (that - // is stored in the map), and not the `EpochHeader` that is stored in the fork - // tree. next time we search for an epoch for a given slot we will do it - // through the fork tree (which isn't updated), but the reason this works is - // because we will search in-depth in the tree with the predicate - // `epoch.start_slot <= slot` which will still match correctly without the - // updated `start_slot`. the new epoch that will get inserted below (after - // `increment`) will already use a correct `start_slot`. + // NOTE: notice that we are only updating a local copy of the `Epoch`, this + // makes it so that when we insert the new epoch into `EpochChanges` below + // (after incrementing it), it will use the correct epoch index and start slot. + // we do not update the original epoch that will be re-used because there might + // be other forks (that we haven't imported) where the epoch isn't skipped, and + // to import those forks we want to keep the original epoch data. not updating + // the original epoch works because when we search the tree for which epoch to + // use for a given slot, we will search in-depth with the predicate + // `epoch.start_slot <= slot` which will still match correctly without updating + // `start_slot` to the correct value as below. epoch_data.epoch_index += skipped_epochs; epoch_data.start_slot = Slot::from(*epoch_data.start_slot + skipped_epochs * epoch_data.duration); diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 98f1c074c16d3..eeccf77a8fd84 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -1152,32 +1152,49 @@ fn allows_skipping_epochs() { &mut block_import, ); - // we check the epoch data that was announced at block #7 (that we checked above) - let epoch3 = epoch_changes + // and the first block in epoch 3 (#8) announces epoch 4 + let epoch4 = epoch_changes .shared_data() .epoch(&EpochIdentifier { position: EpochIdentifierPosition::Regular, - hash: blocks[epoch_length as usize], - number: epoch_length + 1, + hash: block, + number: epoch_length + 2, }) .unwrap() .clone(); - // and it now is updated to start at epoch 3 with an updated slot - assert_eq!(epoch3.epoch_index, 3); - assert_eq!(epoch3.start_slot, Slot::from(epoch_length * 3 + 1)); + assert_eq!(epoch4.epoch_index, 4); + assert_eq!(epoch4.start_slot, Slot::from(epoch_length * 4 + 1)); - // and the first block in epoch 3 (#8) announces epoch 4 - let epoch = epoch_changes + // if we try to get the epoch data for a slot in epoch 3 + let epoch3 = epoch_changes .shared_data() - .epoch(&EpochIdentifier { - position: EpochIdentifierPosition::Regular, - hash: block, - number: epoch_length + 2, - }) + .epoch_data_for_child_of( + descendent_query(&*client), + &block, + epoch_length + 2, + (epoch_length * 3 + 2).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) .unwrap() - .clone(); + .unwrap(); + + // we get back the data for epoch 2 + assert_eq!(epoch3, epoch2); + + // but if we try to get the epoch data for a slot in epoch 4 + let epoch4_ = epoch_changes + .shared_data() + .epoch_data_for_child_of( + descendent_query(&*client), + &block, + epoch_length + 2, + (epoch_length * 4 + 1).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) + .unwrap() + .unwrap(); - assert_eq!(epoch.epoch_index, 4); - assert_eq!(epoch.start_slot, Slot::from(epoch_length * 4 + 1)); + // we get epoch 4 as expected + assert_eq!(epoch4, epoch4_); } From 1ea940738b02cf3ac642b5ee8c1af32d4f2d4be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 19 Oct 2022 00:44:29 +0100 Subject: [PATCH 10/14] babe: typo --- client/consensus/babe/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 7b23cd892a8f2..68b16da847965 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1554,7 +1554,7 @@ where let original_epoch_index = epoch_data.epoch_index; // NOTE: notice that we are only updating a local copy of the `Epoch`, this - // makes it so that when we insert the new epoch into `EpochChanges` below + // makes it so that when we insert the next epoch into `EpochChanges` below // (after incrementing it), it will use the correct epoch index and start slot. // we do not update the original epoch that will be re-used because there might // be other forks (that we haven't imported) where the epoch isn't skipped, and From 5bcb243a8aeb87a10de2286afb309f7e616de35f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 24 Oct 2022 18:03:09 +0100 Subject: [PATCH 11/14] babe: overflow-safe math --- client/consensus/babe/src/lib.rs | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 68b16da847965..abcdd4848407e 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1549,9 +1549,8 @@ where // which epoch it belongs to and we will re-use the same // data for that epoch let mut epoch_data = viable_epoch.as_mut(); - let skipped_epochs = (*slot - *epoch_data.start_slot) / epoch_data.duration; - - let original_epoch_index = epoch_data.epoch_index; + let skipped_epochs = + *slot.saturating_sub(epoch_data.start_slot) / epoch_data.duration; // NOTE: notice that we are only updating a local copy of the `Epoch`, this // makes it so that when we insert the next epoch into `EpochChanges` below @@ -1563,13 +1562,26 @@ where // use for a given slot, we will search in-depth with the predicate // `epoch.start_slot <= slot` which will still match correctly without updating // `start_slot` to the correct value as below. - epoch_data.epoch_index += skipped_epochs; - epoch_data.start_slot = - Slot::from(*epoch_data.start_slot + skipped_epochs * epoch_data.duration); + let epoch_index = epoch_data.epoch_index.checked_add(skipped_epochs).expect( + "epoch number is u64; it should be strictly smaller than number of slots; \ + slots relate in some way to wall clock time; \ + if u64 is not enough we should crash for safety; qed.", + ); + + let start_slot = skipped_epochs + .checked_mul(epoch_data.duration) + .and_then(|skipped_slots| epoch_data.start_slot.checked_add(skipped_slots)) + .expect( + "slot number is u64; it should relate in some way to wall clock time; \ + if u64 is not enough we should crash for safety; qed.", + ); warn!(target: "babe", "👶 Epoch(s) skipped: from {} to {}", - original_epoch_index, epoch_data.epoch_index, + epoch_data.epoch_index, epoch_index, ); + + epoch_data.epoch_index = epoch_index; + epoch_data.start_slot = Slot::from(start_slot); } log!(target: "babe", From 7fde70d96cc9fb8ecd7147bc398ddd07711a2534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 24 Oct 2022 18:20:19 +0100 Subject: [PATCH 12/14] babe: add test for skipping epochs across different forks --- client/consensus/babe/src/tests.rs | 127 +++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index eeccf77a8fd84..976257d3dd5c8 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -1198,3 +1198,130 @@ fn allows_skipping_epochs() { // we get epoch 4 as expected assert_eq!(epoch4, epoch4_); } + +#[test] +fn allows_skipping_epochs_on_some_forks() { + let mut net = BabeTestNet::new(1); + + let peer = net.peer(0); + let data = peer.data.as_ref().expect("babe link set up during initialization"); + + let client = peer.client().as_client(); + let mut block_import = data.block_import.lock().take().expect("import set up during init"); + + let mut proposer_factory = DummyFactory { + client: client.clone(), + config: data.link.config.clone(), + epoch_changes: data.link.epoch_changes.clone(), + mutator: Arc::new(|_, _| ()), + }; + + let epoch_changes = data.link.epoch_changes.clone(); + let epoch_length = data.link.config.epoch_length; + + // we create all of the blocks in epoch 0 as well as two blocks in epoch 1 + let blocks = propose_and_import_blocks( + &client, + &mut proposer_factory, + &mut block_import, + BlockId::Number(0), + epoch_length as usize + 1, + ); + + // we now author a block that belongs to epoch 2, built on top of the last + // authored block in epoch 1. + let last_block = client.expect_header(BlockId::Hash(*blocks.last().unwrap())).unwrap(); + + let epoch2_block = propose_and_import_block( + &last_block, + Some((epoch_length * 2 + 1).into()), + &mut proposer_factory, + &mut block_import, + ); + + // if we try to get the epoch data for a slot in epoch 2, we get the data that + // was previously announced when epoch 1 started + let epoch2 = epoch_changes + .shared_data() + .epoch_data_for_child_of( + descendent_query(&*client), + &epoch2_block, + epoch_length + 2, + (epoch_length * 2 + 2).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) + .unwrap() + .unwrap(); + + // we now author a block that belongs to epoch 3, built on top of the last + // authored block in epoch 1. authoring this block means we're skipping epoch 2 + // entirely on this fork + let epoch3_block = propose_and_import_block( + &last_block, + Some((epoch_length * 3 + 1).into()), + &mut proposer_factory, + &mut block_import, + ); + + // if we try to get the epoch data for a slot in epoch 3 + let epoch3_ = epoch_changes + .shared_data() + .epoch_data_for_child_of( + descendent_query(&*client), + &epoch3_block, + epoch_length + 2, + (epoch_length * 3 + 2).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) + .unwrap() + .unwrap(); + + // we get back the data for epoch 2 + assert_eq!(epoch3_, epoch2); + + // if we try to get the epoch data for a slot in epoch 4 in the fork + // where we skipped epoch 2, we should get the epoch data for epoch 4 + // that was announced at the beginning of epoch 3 + let epoch_data = epoch_changes + .shared_data() + .epoch_data_for_child_of( + descendent_query(&*client), + &epoch3_block, + epoch_length + 2, + (epoch_length * 4 + 1).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) + .unwrap() + .unwrap(); + + assert!(epoch_data != epoch3_); + + // if we try to get the epoch data for a slot in epoch 4 in the fork + // where we didn't skip epoch 2, we should get back the data for epoch 3, + // that was announced when epoch 2 started in that fork + let epoch_data = epoch_changes + .shared_data() + .epoch_data_for_child_of( + descendent_query(&*client), + &epoch2_block, + epoch_length + 2, + (epoch_length * 4 + 1).into(), + |slot| Epoch::genesis(&data.link.config, slot), + ) + .unwrap() + .unwrap(); + + assert!(epoch_data != epoch3_); + + let epoch3 = epoch_changes + .shared_data() + .epoch(&EpochIdentifier { + position: EpochIdentifierPosition::Regular, + hash: epoch2_block, + number: epoch_length + 2, + }) + .unwrap() + .clone(); + + assert_eq!(epoch_data, epoch3); +} From 727a1ea257cdbac68838006690f51711182c1f13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 23 Dec 2022 21:43:29 +0100 Subject: [PATCH 13/14] Fix tests --- client/consensus/babe/src/tests.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 2cd5abac0a8a0..7a2cd75b530e8 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -1111,8 +1111,8 @@ async fn obsolete_blocks_aux_data_cleanup() { assert!(aux_data_check(&fork3_hashes, true)); } -#[test] -fn allows_skipping_epochs() { +#[tokio::test] +async fn allows_skipping_epochs() { let mut net = BabeTestNet::new(1); let peer = net.peer(0); @@ -1136,9 +1136,9 @@ fn allows_skipping_epochs() { &client, &mut proposer_factory, &mut block_import, - BlockId::Number(0), + client.chain_info().genesis_hash, epoch_length as usize + 1, - ); + ).await; // the first block in epoch 0 (#1) announces both epoch 0 and 1 (this is a // special genesis epoch) @@ -1184,13 +1184,13 @@ fn allows_skipping_epochs() { assert_eq!(epoch2.start_slot, Slot::from(epoch_length * 2 + 1)); // we now author a block that belongs to epoch 3, thereby skipping epoch 2 - let last_block = client.expect_header(BlockId::Hash(*blocks.last().unwrap())).unwrap(); + let last_block = client.expect_header(*blocks.last().unwrap()).unwrap(); let block = propose_and_import_block( &last_block, Some((epoch_length * 3 + 1).into()), &mut proposer_factory, &mut block_import, - ); + ).await; // and the first block in epoch 3 (#8) announces epoch 4 let epoch4 = epoch_changes @@ -1239,8 +1239,8 @@ fn allows_skipping_epochs() { assert_eq!(epoch4, epoch4_); } -#[test] -fn allows_skipping_epochs_on_some_forks() { +#[tokio::test] +async fn allows_skipping_epochs_on_some_forks() { let mut net = BabeTestNet::new(1); let peer = net.peer(0); @@ -1264,20 +1264,20 @@ fn allows_skipping_epochs_on_some_forks() { &client, &mut proposer_factory, &mut block_import, - BlockId::Number(0), + client.chain_info().genesis_hash, epoch_length as usize + 1, - ); + ).await; // we now author a block that belongs to epoch 2, built on top of the last // authored block in epoch 1. - let last_block = client.expect_header(BlockId::Hash(*blocks.last().unwrap())).unwrap(); + let last_block = client.expect_header(*blocks.last().unwrap()).unwrap(); let epoch2_block = propose_and_import_block( &last_block, Some((epoch_length * 2 + 1).into()), &mut proposer_factory, &mut block_import, - ); + ).await; // if we try to get the epoch data for a slot in epoch 2, we get the data that // was previously announced when epoch 1 started @@ -1301,7 +1301,7 @@ fn allows_skipping_epochs_on_some_forks() { Some((epoch_length * 3 + 1).into()), &mut proposer_factory, &mut block_import, - ); + ).await; // if we try to get the epoch data for a slot in epoch 3 let epoch3_ = epoch_changes From 792e3158b72a4079ac517b35f5501e537834fb75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 23 Dec 2022 23:24:54 +0100 Subject: [PATCH 14/14] FMT --- client/consensus/babe/src/tests.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 7a2cd75b530e8..f74864a003e2a 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -1138,7 +1138,8 @@ async fn allows_skipping_epochs() { &mut block_import, client.chain_info().genesis_hash, epoch_length as usize + 1, - ).await; + ) + .await; // the first block in epoch 0 (#1) announces both epoch 0 and 1 (this is a // special genesis epoch) @@ -1190,7 +1191,8 @@ async fn allows_skipping_epochs() { Some((epoch_length * 3 + 1).into()), &mut proposer_factory, &mut block_import, - ).await; + ) + .await; // and the first block in epoch 3 (#8) announces epoch 4 let epoch4 = epoch_changes @@ -1266,7 +1268,8 @@ async fn allows_skipping_epochs_on_some_forks() { &mut block_import, client.chain_info().genesis_hash, epoch_length as usize + 1, - ).await; + ) + .await; // we now author a block that belongs to epoch 2, built on top of the last // authored block in epoch 1. @@ -1277,7 +1280,8 @@ async fn allows_skipping_epochs_on_some_forks() { Some((epoch_length * 2 + 1).into()), &mut proposer_factory, &mut block_import, - ).await; + ) + .await; // if we try to get the epoch data for a slot in epoch 2, we get the data that // was previously announced when epoch 1 started @@ -1301,7 +1305,8 @@ async fn allows_skipping_epochs_on_some_forks() { Some((epoch_length * 3 + 1).into()), &mut proposer_factory, &mut block_import, - ).await; + ) + .await; // if we try to get the epoch data for a slot in epoch 3 let epoch3_ = epoch_changes