From e35f11d9766bfa4fe591fc412b3b60b4d1c908aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 7 Feb 2023 22:22:04 +0000 Subject: [PATCH 1/3] babe: account for skipped epochs when handling equivocations --- frame/babe/src/lib.rs | 79 +++++++++++++++++++++++++++++++++++++---- frame/babe/src/tests.rs | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 6 deletions(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index eb87d65b0549f..d91b036237a66 100644 --- a/frame/babe/src/lib.rs +++ b/frame/babe/src/lib.rs @@ -39,6 +39,7 @@ use sp_runtime::{ ConsensusEngineId, KeyTypeId, Permill, }; use sp_session::{GetSessionNumber, GetValidatorCount}; +use sp_staking::SessionIndex; use sp_std::prelude::*; use sp_consensus_babe::{ @@ -102,7 +103,7 @@ impl EpochChangeTrigger for SameAuthoritiesForever { let authorities = >::authorities(); let next_authorities = authorities.clone(); - >::enact_epoch_change(authorities, next_authorities); + >::enact_epoch_change(authorities, next_authorities, None); } } } @@ -316,6 +317,19 @@ pub mod pallet { #[pallet::storage] pub(super) type NextEpochConfig = StorageValue<_, BabeEpochConfiguration>; + /// A list of the last 100 skipped epochs and the corresponding session index + /// when the epoch was skipped. + /// + /// This is only used for validating equivocation proofs. An equivocation proof + /// must contains a key-ownership proof for a given session, therefore we need a + /// way to tie together sessions and epoch indices, i.e. we need to validate that + /// a validator was the owner of a given key on a given session, and what the + /// active epoch index was during that session. + #[pallet::storage] + #[pallet::getter(fn skipped_epochs)] + pub(super) type SkippedEpochs = + StorageValue<_, BoundedVec<(u64, SessionIndex), ConstU32<100>>, ValueQuery>; + #[cfg_attr(feature = "std", derive(Default))] #[pallet::genesis_config] pub struct GenesisConfig { @@ -577,6 +591,7 @@ impl Pallet { pub fn enact_epoch_change( authorities: WeakBoundedVec<(AuthorityId, BabeAuthorityWeight), T::MaxAuthorities>, next_authorities: WeakBoundedVec<(AuthorityId, BabeAuthorityWeight), T::MaxAuthorities>, + session_index: Option, ) { // PRECONDITION: caller has done initialization and is guaranteed // by the session module to be called before this. @@ -602,6 +617,24 @@ impl Pallet { T::EpochDuration::get(), ); + let current_epoch_index = EpochIndex::::get(); + if current_epoch_index.saturating_add(1) != epoch_index { + // we are skipping epochs therefore we need to update the mapping + // of epochs to session + if let Some(session_index) = session_index { + SkippedEpochs::::mutate(|skipped_epochs| { + if skipped_epochs.is_full() { + // NOTE: this is O(n) but we currently don't have a bounded `VecDeque`. + // this vector is bounded to a small number of elements so performance + // shouldn't be an issue. + skipped_epochs.remove(0); + } + + skipped_epochs.force_push((epoch_index, session_index)); + }) + } + } + EpochIndex::::put(epoch_index); Authorities::::put(authorities); @@ -816,6 +849,36 @@ impl Pallet { this_randomness } + /// Returns the session index that was live when the given epoch happened, + /// taking into account any skipped epochs. + /// + /// This function is only well defined for epochs that actually existed, + /// e.g. if we skipped from epoch 10 to 20 then a call for epoch 15 (which + /// didn't exist) will return an incorrect session index. + fn session_index_for_epoch(epoch_index: u64) -> SessionIndex { + let skipped_epochs = SkippedEpochs::::get(); + match skipped_epochs.binary_search_by_key(&epoch_index, |(epoch_index, _)| *epoch_index) { + // we have an exact match so we just return the given session index + Ok(index) => skipped_epochs[index].1, + // we haven't found any skipped epoch before the given the epoch, + // so the epoch index and session index should match + Err(0) => epoch_index.saturated_into::(), + // we have found a skipped epoch before the given epoch + Err(index) => { + // the element before the given index should give us the skipped epoch + // that's closest to the one we're trying to find the session index for + let closest_skipped_epoch = skipped_epochs[index - 1]; + + // calculate the number of skipped epochs at this point by checking the difference + // between the epoch and session indices. epoch index should always be greater or + // equal than session index, this is because epochs can be skipped whereas sessions + // can't + let skipped_epochs = closest_skipped_epoch.0 - closest_skipped_epoch.1 as u64; + epoch_index.saturating_sub(skipped_epochs).saturated_into::() + }, + } + } + fn do_report_equivocation( reporter: Option, equivocation_proof: EquivocationProof, @@ -832,12 +895,11 @@ impl Pallet { let validator_set_count = key_owner_proof.validator_count(); let session_index = key_owner_proof.session(); - let epoch_index = (*slot.saturating_sub(GenesisSlot::::get()) / T::EpochDuration::get()) - .saturated_into::(); + let epoch_index = *slot.saturating_sub(GenesisSlot::::get()) / T::EpochDuration::get(); // check that the slot number is consistent with the session index // in the key ownership proof (i.e. slot is for that epoch) - if epoch_index != session_index { + if Self::session_index_for_epoch(epoch_index) != session_index { return Err(Error::::InvalidKeyOwnershipProof.into()) } @@ -926,7 +988,10 @@ impl sp_runtime::BoundToRuntimeAppPublic for Pallet { type Public = AuthorityId; } -impl OneSessionHandler for Pallet { +impl OneSessionHandler for Pallet +where + T: pallet_session::Config, +{ type Key = AuthorityId; fn on_genesis_session<'a, I: 'a>(validators: I) @@ -959,7 +1024,9 @@ impl OneSessionHandler for Pallet { ), ); - Self::enact_epoch_change(bounded_authorities, next_bounded_authorities) + let session_index = >::current_index(); + + Self::enact_epoch_change(bounded_authorities, next_bounded_authorities, Some(session_index)) } fn on_disabled(i: u32) { diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs index 0b8a02547144b..938269a6cfa62 100644 --- a/frame/babe/src/tests.rs +++ b/frame/babe/src/tests.rs @@ -826,6 +826,56 @@ fn report_equivocation_has_valid_weight() { .all(|w| w[0].ref_time() < w[1].ref_time())); } +#[test] +fn report_equivocation_after_skipped_epochs_works() { + let (pairs, mut ext) = new_test_ext_with_pairs(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); + assert_eq!(EpochIndex::::get(), 0); + + // skip from epoch #0 to epoch #10 + go_to_block(System::block_number() + 1, genesis_slot + epoch_duration * 10); + + assert_eq!(EpochIndex::::get(), 10); + assert_eq!(SkippedEpochs::::get(), vec![(10, 1)]); + + // generate an equivocation proof for validator at index 1 + let authorities = Babe::authorities(); + let offending_validator_index = 1; + let offending_authority_pair = pairs + .into_iter() + .find(|p| p.public() == authorities[offending_validator_index].0) + .unwrap(); + + let equivocation_proof = generate_equivocation_proof( + offending_validator_index as u32, + &offending_authority_pair, + CurrentSlot::::get(), + ); + + // create the key ownership proof + let key = (sp_consensus_babe::KEY_TYPE, &offending_authority_pair.public()); + let key_owner_proof = Historical::prove(key).unwrap(); + + // which is for session index 1 (while current epoch index is 10) + assert_eq!(key_owner_proof.session, 1); + + // report the equivocation, in order for the validation to pass the mapping + // between epoch index and session index must be checked. + assert!(Babe::report_equivocation_unsigned( + RuntimeOrigin::none(), + Box::new(equivocation_proof), + key_owner_proof + ) + .is_ok()); + }) +} + #[test] fn valid_equivocation_reports_dont_pay_fees() { let (pairs, mut ext) = new_test_ext_with_pairs(3); @@ -977,5 +1027,17 @@ fn skipping_over_epochs_works() { assert_eq!(EpochIndex::::get(), 4); assert_eq!(Randomness::::get(), randomness_for_epoch_2); + + // after skipping epochs the information is registered on-chain so that + // we can map epochs to sessions + assert_eq!(SkippedEpochs::::get(), vec![(4, 2)]); + + // before epochs are skipped the mapping should be one to one + assert_eq!(Babe::session_index_for_epoch(0), 0); + assert_eq!(Babe::session_index_for_epoch(1), 1); + + // otherwise the session index is offset by the number of skipped epochs + assert_eq!(Babe::session_index_for_epoch(4), 2); + assert_eq!(Babe::session_index_for_epoch(5), 3); }); } From 30d343996fc5b15a069fa1ea54ac635c8e595d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 7 Feb 2023 22:29:51 +0000 Subject: [PATCH 2/3] typos --- frame/babe/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index d91b036237a66..33671462c72ae 100644 --- a/frame/babe/src/lib.rs +++ b/frame/babe/src/lib.rs @@ -860,7 +860,7 @@ impl Pallet { match skipped_epochs.binary_search_by_key(&epoch_index, |(epoch_index, _)| *epoch_index) { // we have an exact match so we just return the given session index Ok(index) => skipped_epochs[index].1, - // we haven't found any skipped epoch before the given the epoch, + // we haven't found any skipped epoch before the given epoch, // so the epoch index and session index should match Err(0) => epoch_index.saturated_into::(), // we have found a skipped epoch before the given epoch @@ -871,7 +871,7 @@ impl Pallet { // calculate the number of skipped epochs at this point by checking the difference // between the epoch and session indices. epoch index should always be greater or - // equal than session index, this is because epochs can be skipped whereas sessions + // equal to session index, this is because epochs can be skipped whereas sessions // can't let skipped_epochs = closest_skipped_epoch.0 - closest_skipped_epoch.1 as u64; epoch_index.saturating_sub(skipped_epochs).saturated_into::() From 9b61c1c37411a08b70964c5fc8748ff9ade6ff06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 16 Feb 2023 19:42:09 +0000 Subject: [PATCH 3/3] babe: enforce epoch index >= session index --- frame/babe/src/lib.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index 33671462c72ae..69c89fdcea776 100644 --- a/frame/babe/src/lib.rs +++ b/frame/babe/src/lib.rs @@ -623,6 +623,17 @@ impl Pallet { // of epochs to session if let Some(session_index) = session_index { SkippedEpochs::::mutate(|skipped_epochs| { + if epoch_index < session_index as u64 { + log::warn!( + target: LOG_TARGET, + "Current epoch index {} is lower than session index {}.", + epoch_index, + session_index, + ); + + return + } + if skipped_epochs.is_full() { // NOTE: this is O(n) but we currently don't have a bounded `VecDeque`. // this vector is bounded to a small number of elements so performance @@ -872,7 +883,7 @@ impl Pallet { // calculate the number of skipped epochs at this point by checking the difference // between the epoch and session indices. epoch index should always be greater or // equal to session index, this is because epochs can be skipped whereas sessions - // can't + // can't (this is enforced when pushing into `SkippedEpochs`) let skipped_epochs = closest_skipped_epoch.0 - closest_skipped_epoch.1 as u64; epoch_index.saturating_sub(skipped_epochs).saturated_into::() },