babe: account for skipped epochs when handling equivocations (#13335)

* babe: account for skipped epochs when handling equivocations

* typos

* babe: enforce epoch index >= session index

Author: andresilva

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 = <Pallet<T>>::authorities();
 			let next_authorities = authorities.clone();
 
-			<Pallet<T>>::enact_epoch_change(authorities, next_authorities);
+			<Pallet<T>>::enact_epoch_change(authorities, next_authorities, None);
 		}
 	}
 }
@@ -316,6 +317,19 @@ pub mod pallet {
 	#[pallet::storage]
 	pub(super) type NextEpochConfig<T> = 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<T> =
+		StorageValue<_, BoundedVec<(u64, SessionIndex), ConstU32<100>>, ValueQuery>;
+
 	#[cfg_attr(feature = "std", derive(Default))]
 	#[pallet::genesis_config]
 	pub struct GenesisConfig {
@@ -577,6 +591,7 @@ impl<T: Config> Pallet<T> {
 	pub fn enact_epoch_change(
 		authorities: WeakBoundedVec<(AuthorityId, BabeAuthorityWeight), T::MaxAuthorities>,
 		next_authorities: WeakBoundedVec<(AuthorityId, BabeAuthorityWeight), T::MaxAuthorities>,
+		session_index: Option<SessionIndex>,
 	) {
 		// PRECONDITION: caller has done initialization and is guaranteed
 		// by the session module to be called before this.
@@ -602,6 +617,35 @@ impl<T: Config> Pallet<T> {
 			T::EpochDuration::get(),
 		);
 
+		let current_epoch_index = EpochIndex::<T>::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::<T>::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
+						// shouldn't be an issue.
+						skipped_epochs.remove(0);
+					}
+
+					skipped_epochs.force_push((epoch_index, session_index));
+				})
+			}
+		}
+
 		EpochIndex::<T>::put(epoch_index);
 		Authorities::<T>::put(authorities);
 
@@ -816,6 +860,36 @@ impl<T: Config> Pallet<T> {
 		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::<T>::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 epoch,
+			// so the epoch index and session index should match
+			Err(0) => epoch_index.saturated_into::<u32>(),
+			// 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 to session index, this is because epochs can be skipped whereas sessions
+				// 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::<u32>()
+			},
+		}
+	}
+
 	fn do_report_equivocation(
 		reporter: Option<T::AccountId>,
 		equivocation_proof: EquivocationProof<T::Header>,
@@ -832,12 +906,11 @@ impl<T: Config> Pallet<T> {
 		let validator_set_count = key_owner_proof.validator_count();
 		let session_index = key_owner_proof.session();
 
-		let epoch_index = (*slot.saturating_sub(GenesisSlot::<T>::get()) / T::EpochDuration::get())
-			.saturated_into::<u32>();
+		let epoch_index = *slot.saturating_sub(GenesisSlot::<T>::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::<T>::InvalidKeyOwnershipProof.into())
 		}
 
@@ -926,7 +999,10 @@ impl<T: Config> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
 	type Public = AuthorityId;
 }
 
-impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
+impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T>
+where
+	T: pallet_session::Config,
+{
 	type Key = AuthorityId;
 
 	fn on_genesis_session<'a, I: 'a>(validators: I)
@@ -959,7 +1035,9 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
 			),
 		);
 
-		Self::enact_epoch_change(bounded_authorities, next_bounded_authorities)
+		let session_index = <pallet_session::Pallet<T>>::current_index();
+
+		Self::enact_epoch_change(bounded_authorities, next_bounded_authorities, Some(session_index))
 	}
 
 	fn on_disabled(i: u32) {

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 = <Test as Config>::EpochDuration::get();
+
+		// this sets the genesis slot to 100;
+		let genesis_slot = 100;
+		go_to_block(1, genesis_slot);
+		assert_eq!(EpochIndex::<Test>::get(), 0);
+
+		// skip from epoch #0 to epoch #10
+		go_to_block(System::block_number() + 1, genesis_slot + epoch_duration * 10);
+
+		assert_eq!(EpochIndex::<Test>::get(), 10);
+		assert_eq!(SkippedEpochs::<Test>::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::<Test>::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::<Test>::get(), 4);
 		assert_eq!(Randomness::<Test>::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::<Test>::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);
 	});
 }